Mercurial > hg > piper-cpp
changeset 186:52322dde68ea
Fix erroneous logic for handling step and block size in prior commit
The earlier change had a logical misconception. If PluginStub is
receiving the correct step and block size back from the configure call,
the plugin on the server side must have already been successfully
initialised, as the step and block size are only returned in a
successful configure response. This means the test for a failed
initialise and redo with the correct parameters must be done on the
server side (in LoaderRequests) not the client. The client has a more
complicated job, which is to notice that a *successful* configure had
returned different framing parameters from those passed to the
initialise call, and to pretend that it had actually failed until the
host called again with the correct parameters. We definitely need tests
for this!
| author | Chris Cannam <cannam@all-day-breakfast.com> |
|---|---|
| date | Mon, 06 Feb 2017 16:44:33 +0000 |
| parents | 3eb00e5c76c4 |
| children | ad6025dc0b04 458766b73e71 |
| files | vamp-client/PluginStub.h vamp-client/qt/AutoPlugin.h vamp-server/simple-server.cpp vamp-support/LoaderRequests.h |
| diffstat | 4 files changed, 125 insertions(+), 21 deletions(-) [+] |
line wrap: on
line diff
--- a/vamp-client/PluginStub.h Fri Feb 03 16:23:21 2017 +0000 +++ b/vamp-client/PluginStub.h Mon Feb 06 16:44:33 2017 +0000 @@ -53,7 +53,48 @@ class PluginStub : public Vamp::Plugin { enum State { - Loaded, Configured, Finished, Failed + /** + * The plugin's corresponding Piper feature extractor has been + * loaded but no subsequent state change has happened. This is + * the initial state of PluginStub on construction, since it + * is associated with a pre-loaded handle. + */ + Loaded, + + /** + * The plugin has been configured, and the step and block size + * received from the host in its last call to initialise() + * match those that were returned in the configuration + * response (i.e. the server's desired step and block + * size). Our m_config record reflects these correct + * values. The plugin is ready to process. + */ + Configured, + + /** + * The plugin has been configured, but the step and block size + * received from the host in its last call to initialise() + * differ from those returned by the server in the + * configuration response. Our initialise() call therefore + * returned false, and the plugin cannot be used until the + * host calls initialise() again with the "correct" step and + * block size. Our m_config record reflects these correct + * values, so the host can retrieve them through + * getPreferredStepSize and getPreferredBlockSize. + */ + Misconfigured, + + /** + * The finish() function has been called and the plugin + * unloaded. No further plugin activity is possible. + */ + Finished, + + /** + * A call has failed unrecoverably. No further plugin activity + * is possible. + */ + Failed }; public: @@ -157,6 +198,17 @@ if (m_state == Failed) { throw std::logic_error("Plugin is in failed state"); } + + if (m_state == Misconfigured) { + if (int(stepSize) == m_config.framing.stepSize && + int(blockSize) == m_config.framing.blockSize) { + m_state = Configured; + return true; + } else { + return false; + } + } + if (m_state != Loaded) { m_state = Failed; throw std::logic_error("Plugin has already been initialised"); @@ -169,7 +221,7 @@ try { auto response = m_client->configure(this, m_config); m_outputs = response.outputs; - + // Update with the new preferred step and block size now // that the plugin has taken into account its parameter // settings. If the values passed in to initialise() @@ -177,6 +229,16 @@ // call to getPreferredStepSize/BlockSize on this plugin // object will at least get acceptable values from now on m_config.framing = response.framing; + + // And if they didn't match up with the passed-in ones, + // lodge ourselves in Misconfigured state and report + // failure so as to provoke the host to call initialise() + // again before any processing. + if (m_config.framing.stepSize != int(stepSize) || + m_config.framing.blockSize != int(blockSize)) { + m_state = Misconfigured; + return false; + } } catch (const std::exception &e) { m_state = Failed; @@ -196,7 +258,7 @@ if (m_state == Failed) { throw std::logic_error("Plugin is in failed state"); } - if (m_state == Loaded) { + if (m_state == Loaded || m_state == Misconfigured) { // reset is a no-op if the plugin hasn't been initialised yet return; } @@ -268,7 +330,7 @@ if (m_state == Failed) { throw std::logic_error("Plugin is in failed state"); } - if (m_state == Loaded) { + if (m_state == Loaded || m_state == Misconfigured) { m_state = Failed; throw std::logic_error("Plugin has not been initialised"); } @@ -297,7 +359,7 @@ if (m_state == Failed) { throw std::logic_error("Plugin is in failed state"); } - if (m_state == Loaded) { + if (m_state == Loaded || m_state == Misconfigured) { m_state = Failed; throw std::logic_error("Plugin has not been configured"); }
--- a/vamp-client/qt/AutoPlugin.h Fri Feb 03 16:23:21 2017 +0000 +++ b/vamp-client/qt/AutoPlugin.h Mon Feb 06 16:44:33 2017 +0000 @@ -138,14 +138,7 @@ virtual bool initialise(size_t inputChannels, size_t stepSize, size_t blockSize) { - try { - return getPlugin()->initialise(inputChannels, stepSize, blockSize); - } catch (const ServiceError &e) { - // Sadly, the Vamp API has taught hosts to try to divine - // initialisation problems from a bool return value alone - log(std::string("AutoPlugin: initialise failed: ") + e.what()); - return false; - } + return getPlugin()->initialise(inputChannels, stepSize, blockSize); } virtual void reset() {
--- a/vamp-server/simple-server.cpp Fri Feb 03 16:23:21 2017 +0000 +++ b/vamp-server/simple-server.cpp Mon Feb 06 16:44:33 2017 +0000 @@ -495,7 +495,7 @@ mapper.markConfigured (h, creq.configuration.channelCount, - creq.configuration.framing.blockSize); + response.configurationResponse.framing.blockSize); response.success = true; } break; @@ -521,8 +521,12 @@ const float **fbuffers = new const float *[channels]; for (int i = 0; i < channels; ++i) { if (int(preq.inputBuffers[i].size()) != mapper.getBlockSize(h)) { + ostringstream os; + os << "wrong block size supplied to process (" + << preq.inputBuffers[i].size() + << ", expecting " << mapper.getBlockSize(h) << ")" << ends; delete[] fbuffers; - throw runtime_error("wrong block size supplied to process"); + throw runtime_error(os.str()); } fbuffers[i] = preq.inputBuffers[i].data(); }
--- a/vamp-support/LoaderRequests.h Fri Feb 03 16:23:21 2017 +0000 +++ b/vamp-support/LoaderRequests.h Mon Feb 06 16:44:33 2017 +0000 @@ -136,6 +136,10 @@ return response; } + Framing pluginPreferredFraming; + pluginPreferredFraming.stepSize = req.plugin->getPreferredStepSize(); + pluginPreferredFraming.blockSize = req.plugin->getPreferredBlockSize(); + if (req.plugin->initialise(req.configuration.channelCount, req.configuration.framing.stepSize, req.configuration.framing.blockSize)) { @@ -148,13 +152,54 @@ response.framing = req.configuration.framing; } else { + + // If initialise() fails, one reason could be that it + // didn't like the passed-in framing (step and block + // size). + // + // Vamp and Piper have quite different mechanisms for + // negotiating step and block size: + // + // - If a Vamp plugin doesn't like the step and block size + // passed to initialise(), it fails the initialise() call, + // returning false from it. The host is expected to have + // called getPreferredStepSize()/BlockSize() after it made + // any parameter changes that might have affected these + // preferences (but before calling initialise). + // + // - If a Piper server doesn't like the step and block + // size passed in a configure request, but if everything + // else about the configure request is OK, then it returns + // a successful configure response including its preferred + // step and block sizes in the response (which the host + // must then use). The important thing to note is that + // this is still a successful response, something we do + // not yet have here. + // + // We need to check whether the passed-in framing differs + // from the plugin's preferences; if so, then we form a + // working supposition that initialise() failed because of + // this. Vamp contains nothing to allow us to test this, + // except to try initialise() again with different + // values. So we try again with the values the plugin told + // us it would prefer and, if that succeeds, return them + // in a successful response in the Piper manner. + // + // Note that if the "other side" (i.e. the client) wants + // to interpret this as if it were dealing with a Vamp + // plugin, then it's going to need some equal-but-opposite + // acrobatics. - // If initialise() fails, one reason could be that it - // didn't like the passed-in step and block size. If we - // return its current preferred values here, the - // host/client can retry with these (if they differ) - response.framing.stepSize = req.plugin->getPreferredStepSize(); - response.framing.blockSize = req.plugin->getPreferredBlockSize(); + if (req.plugin->initialise(req.configuration.channelCount, + pluginPreferredFraming.stepSize, + pluginPreferredFraming.blockSize)) { + + response.outputs = req.plugin->getOutputDescriptors(); + response.framing = pluginPreferredFraming; + + } // ... else we return no outputs, which is the error + // case (presumably to be converted to Piper error + // response). } return response;