Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

- [PP] Addded ability to preprocess inputs into plugin desired format #857

Merged

Conversation

anton-potapov
Copy link
Contributor

@anton-potapov anton-potapov commented Jun 10, 2020

[PP] Addded ability to preprocess inputs into plugin
desired format

changed InferRequestInternal:

  • added _deviceInputs member to store plugin desired perprocessing
    targets
  • added default argument to preProcessingRequired to describe plugin
    specific desired preprocessing target
  • SetBlob and GetBlob to deal with plugin desired preprocessing targets
    (_deviceInputs)
  • added addInputPreProcessingFor helper method to avoid code
    duplication

changed TEMPLATE plugin to use new functionality:

  • removed explicit presicion conversion (to use built-in one of
    InferRequestInternal)
  • _networkInputBlobs to use InferRequestInternal::_deviceInputs

@anton-potapov anton-potapov requested a review from a team June 10, 2020 11:40
@anton-potapov anton-potapov force-pushed the preproc_streamlining branch 2 times, most recently from fe8d75c to be11fd8 Compare June 11, 2020 08:11
@ilya-lavrenov ilya-lavrenov added the category: preprocessing (deprecated) Inference Engine Preprocessing library label Jul 21, 2020
@anton-potapov anton-potapov force-pushed the preproc_streamlining branch from be11fd8 to 7e5b7eb Compare July 24, 2020 09:47
@openvino-pushbot openvino-pushbot added the category: inference OpenVINO Runtime library - Inference label Aug 11, 2020
@ilya-lavrenov ilya-lavrenov self-assigned this Aug 15, 2020
for (auto& pp_result : inputs) {
auto const& input_name = pp_result.first;
auto & result_blob = pp_result.second;
auto const& ir_input_blob = _inputs[input_name];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if result_blob (inputs[input_name]) and ir_input_blob(_inputs[input_name]) point to the one blob?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's handled in the device_specific_preproc_req flag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not relevant any more - patch is reworked

execute(_roiBlob, outBlob, info, serial, batchSize);
}
void PreProcessData::execute(const Blob::Ptr& inBlob, Blob::Ptr &outBlob, const PreProcessInfo& info, bool serial, int batchSize) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: redundant line

Copy link
Contributor Author

@anton-potapov anton-potapov Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not relevant any more - patch is reworked

@ilyachur ilyachur self-assigned this Aug 17, 2020
if (_roiBlob == nullptr) {
THROW_IE_EXCEPTION << "Input pre-processing is called without ROI blob set";
if (inBlob == nullptr) {
THROW_IE_EXCEPTION << "Input pre-processing is called with null inBlob";
}

batchSize = PreprocEngine::getCorrectBatchSize(batchSize, _roiBlob);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose _roiBlob should be replaced with inBlob everywhere in the function body (including its sub-routines).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not relevant any more - patch is reworked

auto pp_it = _preProcData.find(input_name);
const bool preprocDataExists = (pp_it != _preProcData.end());

Blob::Ptr roi_blob = (preprocDataExists) ? (*pp_it).second ->getRoiBlob() : Blob::Ptr{nullptr};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does getRoiBlob always returns non-NULL? Just from its name I would expect that this is an optional crop pre-processing, which might not be enabled by user (for example, only color space conversion preprocessing).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes, if preprocDataExists is true, it means that preporoc engine is created for particular input and setRoiBlob is called in SetBlob

Copy link
Contributor Author

@anton-potapov anton-potapov Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilya-lavrenov is correct.

getRoiBlob is a bad name, and now should be something like getUserBlob. I have added a FIXME to rename it (and accompanying one seRoiBlob )

Blob::Ptr roi_blob = (preprocDataExists) ? (*pp_it).second ->getRoiBlob() : Blob::Ptr{nullptr};

const bool device_specific_preproc_req = (ir_input_blob != result_blob);
const bool user_specific_preproc_req = (roi_blob != nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the previous comment - I would expect that user_specific_preproc_req == preprocDataExists. And roi_blob is just an optional pre-processing configuration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roi_blob is not empty, if preprocDataExists is true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not relevant any more - patch is reworked

@@ -779,6 +779,7 @@ class PreProcessData : public IPreProcessData {
Blob::Ptr getRoiBlob() const override;

void execute(Blob::Ptr &outBlob, const PreProcessInfo& info, bool serial, int batchSize = -1) override;
void execute(const Blob::Ptr& inBlob, Blob::Ptr &outBlob, const PreProcessInfo& info, bool serial, int batchSize = -1) override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to rename outBlob to preprocessedBlobs as inference requests also has some outBlobs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inBlob to userBlob?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed outBlob to preprocessedBlob and _roiBlob to _userBlob. (though renaming of according methods is postponed)

@apankratovantonp
Copy link
Contributor

@anton-potapov Please add usage example with explanations into openvino/docs/template_plugin.

if (device_specific_preproc_req && !preprocDataExists) {
pp_it = _preProcData.emplace_hint(_preProcData.end(), input_name, CreatePreprocDataHelper());
}
auto& pp = (*pp_it).second;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(*pp_it).second -> pp_it->second

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not relevant any more - patch is reworked

@@ -779,6 +779,7 @@ class PreProcessData : public IPreProcessData {
Blob::Ptr getRoiBlob() const override;

void execute(Blob::Ptr &outBlob, const PreProcessInfo& info, bool serial, int batchSize = -1) override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need a method with a single blob argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not relevant any more - patch is reworked

}
auto& pp = (*pp_it).second;

auto src_blob = (roi_blob) ? roi_blob : ir_input_blob;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src_blob -> user_blob?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not relevant any more - patch is reworked

@@ -228,13 +228,29 @@ class InferRequestInternal : virtual public IInferRequestInternal {
* @param serial Whether to use multiple threads to execute the step
*/
void execDataPreprocessing(InferenceEngine::BlobMap& inputs, bool serial = false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why inputs? I suppose they are outputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed to preprocessedBlobs


Blob::Ptr roi_blob = (preprocDataExists) ? (*pp_it).second ->getRoiBlob() : Blob::Ptr{nullptr};

const bool device_specific_preproc_req = (ir_input_blob != result_blob);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to see the usage of this function when device_specific_preproc_req is true, because currently all plugins call this function with _inputs and device_specific_preproc_req is always false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not relevant any more - patch is reworked

@ilya-lavrenov
Copy link
Contributor

@anton-potapov when are we going to proceed with this PR?

@anton-potapov anton-potapov requested a review from a team as a code owner November 9, 2020 09:26
@anton-potapov anton-potapov requested a review from a team November 9, 2020 09:26
@anton-potapov anton-potapov force-pushed the preproc_streamlining branch 2 times, most recently from c9cff58 to e8946d2 Compare November 24, 2020 09:38
@anton-potapov anton-potapov force-pushed the preproc_streamlining branch 2 times, most recently from d62ba8e to b63d411 Compare December 1, 2020 14:31
@anton-potapov anton-potapov changed the title - [PrepProcessing] Addded ability to preprocess inputs into plugin - [PrepProcessing] Addded ability to preprocess inputs into plugin desired format Dec 1, 2020
@anton-potapov
Copy link
Contributor Author

@anton-potapov Please add usage example with explanations into openvino/docs/template_plugin.

added

@anton-potapov
Copy link
Contributor Author

@ilya-lavrenov ping

@anton-potapov anton-potapov force-pushed the preproc_streamlining branch 2 times, most recently from d90f60b to f8ca497 Compare December 7, 2020 08:52
@ilya-lavrenov ilya-lavrenov added this to the 2021.3 milestone Dec 8, 2020
@anton-potapov anton-potapov force-pushed the preproc_streamlining branch 2 times, most recently from 929a08f to 1223f07 Compare December 9, 2020 10:37
@ilya-lavrenov
Copy link
Contributor

@ilyachur @vinograd47 please, have a look as well.

Copy link
Contributor

@ilyachur ilyachur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general LGTM.
But I have a question. Do we have any plugins (excludes the template plugin) which use it?
Because I have found when added the tests for pre-processing that majority of plugins use the own code for pre-processing.

@anton-potapov
Copy link
Contributor Author

anton-potapov commented Dec 9, 2020

... Do we have any plugins (excludes the template plugin) which use it?

Not yet.

Because I have found when added the tests for pre-processing that majority of plugins use the own code for pre-processing.

This is correct, test were added in advance to make sure the behaviour is not broken during the refactoring.

Copy link
Contributor

@vinograd47 vinograd47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me

@anton-potapov anton-potapov changed the title - [PrepProcessing] Addded ability to preprocess inputs into plugin desired format - [PP] Addded ability to preprocess inputs into plugin desired format Dec 10, 2020
@anton-potapov anton-potapov force-pushed the preproc_streamlining branch 2 times, most recently from 3fcc2e1 to 71f443f Compare December 11, 2020 09:28
desired format

changed InferRequestInternal:
 - added _deviceInputs member to store plugin desired perprocessing
   targets
 - added default argument to preProcessingRequired to describe plugin
   specific desired preprocessing target
 - SetBlob and GetBlob to deal with plugin desired preprocessing targets
   (_deviceInputs)
 - added addInputPreProcessingFor helper method to avoid code
   duplication

changed TEMPLATE plugin to use new functionality:
 - removed explicit presicion conversion (to use built-in one of
   InferRequestInternal)
 - _networkInputBlobs to use InferRequestInternal::_deviceInputs
@ilya-lavrenov ilya-lavrenov merged commit 2495eaf into openvinotoolkit:master Dec 11, 2020
vzinovie pushed a commit to vzinovie/openvino that referenced this pull request Dec 15, 2020
…#857)

desired format

changed InferRequestInternal:
 - added _deviceInputs member to store plugin desired perprocessing
   targets
 - added default argument to preProcessingRequired to describe plugin
   specific desired preprocessing target
 - SetBlob and GetBlob to deal with plugin desired preprocessing targets
   (_deviceInputs)
 - added addInputPreProcessingFor helper method to avoid code
   duplication

changed TEMPLATE plugin to use new functionality:
 - removed explicit presicion conversion (to use built-in one of
   InferRequestInternal)
 - _networkInputBlobs to use InferRequestInternal::_deviceInputs
mryzhov pushed a commit to mryzhov/openvino that referenced this pull request Dec 16, 2020
…#857)

desired format

changed InferRequestInternal:
 - added _deviceInputs member to store plugin desired perprocessing
   targets
 - added default argument to preProcessingRequired to describe plugin
   specific desired preprocessing target
 - SetBlob and GetBlob to deal with plugin desired preprocessing targets
   (_deviceInputs)
 - added addInputPreProcessingFor helper method to avoid code
   duplication

changed TEMPLATE plugin to use new functionality:
 - removed explicit presicion conversion (to use built-in one of
   InferRequestInternal)
 - _networkInputBlobs to use InferRequestInternal::_deviceInputs
mryzhov pushed a commit to mryzhov/openvino that referenced this pull request Jan 14, 2021
…#857)

desired format

changed InferRequestInternal:
 - added _deviceInputs member to store plugin desired perprocessing
   targets
 - added default argument to preProcessingRequired to describe plugin
   specific desired preprocessing target
 - SetBlob and GetBlob to deal with plugin desired preprocessing targets
   (_deviceInputs)
 - added addInputPreProcessingFor helper method to avoid code
   duplication

changed TEMPLATE plugin to use new functionality:
 - removed explicit presicion conversion (to use built-in one of
   InferRequestInternal)
 - _networkInputBlobs to use InferRequestInternal::_deviceInputs
jiwaszki pushed a commit to akuporos/openvino that referenced this pull request Jan 15, 2021
…#857)

desired format

changed InferRequestInternal:
 - added _deviceInputs member to store plugin desired perprocessing
   targets
 - added default argument to preProcessingRequired to describe plugin
   specific desired preprocessing target
 - SetBlob and GetBlob to deal with plugin desired preprocessing targets
   (_deviceInputs)
 - added addInputPreProcessingFor helper method to avoid code
   duplication

changed TEMPLATE plugin to use new functionality:
 - removed explicit presicion conversion (to use built-in one of
   InferRequestInternal)
 - _networkInputBlobs to use InferRequestInternal::_deviceInputs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: inference OpenVINO Runtime library - Inference category: preprocessing (deprecated) Inference Engine Preprocessing library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants