Blob ROI API for passing ROI through IE by alpkn · Pull Request #879 · openvinotoolkit/openvino

alpkn · 2020-06-10T21:32:31Z

Enable workflow of transmitting ROI through IE API.
Added methods to IE::Blob class;
Updated tests, added new tests.

Internal validation expected to fail since an update of closed repos required

inference-engine/src/inference_engine/ie_blob_common.cpp

inference-engine/tests/unit/inference_engine/ie_blob_test.cpp

alpkn · 2020-06-16T08:18:25Z

@anton-potapov @dmatveev @dkurt @gladilov-gleb @rgarnov @slyalin @vladimir-paramuzov @yury-gorbachev

Guys, please state your attitude to the solution.

Brief outcut from the earlier talks and its distinctive features:

Plugin has an ability to distinguish whether blob is ROI blob or not, and get ROI info;
After client supplied ROI and created a blob, he is able to get the ROI back from the blob;
Designed in object oriented way: the task of creating ROI blob is decomposed into several steps; overriding the methods is available when necessary, but is not required;
Current behavior of IE core is kept intact;
Memory shared between the blob and ROI blob is preserved until the last of them gets out of the scope;

ggladilov

LGTM in general, some questions

Thanks for your proposal!

inference-engine/include/ie_blob.h

inference-engine/include/ie_compound_blob.h

inference-engine/src/cldnn_engine/cldnn_remote_context.h

inference-engine/src/inference_engine/ie_blob.cpp

inference-engine/tests/unit/inference_engine/ie_blob_test.cpp

ArtemySkrebkov

Are you going to add a sample how to use API in a real use case?
I think it is worth adding to see how API looks in a possible user's code.

alpkn · 2020-06-17T14:18:49Z

Yes.
The wish for having a sample was mentioned on arch forum.

inference-engine/include/ie_blob.h

alpkn · 2020-07-13T13:39:20Z

inference-engine/samples/classification_of_detected_roi/main.cpp

+            detInferRequest.SetBlob(detInputName, imageBlob);
+            detInferRequest.Infer();
+
+            const std::vector<Blob::Ptr> ROIs = getDetectionROIs(as<MemoryBlob>(detInferRequest.GetBlob(detOutputName)), imageBlob);


@anton-potapov @ilyachur
getROI() usage:
Client has the convenience of not dragging detected boxes through the call stack;
Semantics is expressed in a few lines with no mandatory deep-dive;

Compare to cryptic sequence of functions when ROI data is not available for the client: https://github.com/openvinotoolkit/openvino/pull/882/files#diff-b1b9024f2bb9a523502fcb4e2fb3c9d1R505

upd in case of link above isn't functional: it's file https://github.com/openvinotoolkit/openvino/pull/882/files#diff-b1b9024f2bb9a523502fcb4e2fb3c9d1 , line 505

Looks like in alternative solution we should just use vector of blobs in order to save all ROI... It doesn't look like a big impact.

Also I have a question:

Why in this solution should we write more lines of code (551 vs 522)? Both files are new and it is hard to understand what are differen, just code style or alternative solution requires less code lines.

we should just use vector of blobs...

That's the kind of aux data which causes unnecessary inconvenience for the client. This impacts any user code related to multiple ROI blobs.

There are additional handling and informational messages added, after the sample was tested on the image set. They add lines.
Readability is worth more than amount of lines anyway.

From application view, getROI() is helpful, it can simply logical of application to manage ROI list and ROIBlob.
BTW, is it better if put ROI list into ROIblob rather than put one ROI into one ROIBlob?

The container of Blobs each having one ROI can be passed through IE API, and then list of ROI for the single inference will be composed by the plugin-specific code.

Ok, what does getROI() return for usual blob?

nullptr.
Two (at least) useful appliances:

single check whether blob is ROI: if (blob->getROI()) { ...

break the loop over the chain of nested ROI blobs: for (; blob->getROI(); blob = blob->getROI()->original)

alpkn · 2020-07-13T14:01:19Z

@ilya-lavrenov
Would you please explain the cons of having the set of decomposed methods (i.e., shallowClone, getROI/setROI) plus an aggregate createROI (which makes use of them) as an opposite to having the single createROI methods?

alpkn · 2020-07-16T06:57:02Z

Please note there is client's will for having getROI() available for the application.

rzubarev · 2020-07-21T08:54:51Z

👍

ArtemySkrebkov · 2020-07-22T08:31:49Z

👍

zhangcong2019 · 2020-07-23T06:02:07Z

👍

alpkn · 2020-07-24T07:59:22Z

To guys, who voted in favor of the single-function approach:
Any information used for instantiating the blobs can be read back from the blob by the client currently. There is an obvious requirement to request the ROI info used for creating the blob as well. Clients of the new API confirmed their wish to have this ability by voting up. Lack of the getter causes client code to maintain the ROI info structures and drag them along with the blobs through the execution.

@yury-gorbachev
I am certain that alternate proposal is less customer oriented than the current one and serves client's needs poorly; there is clear difference within the client code for these two approaches and there is client preference for the convenient one.

@ilya-lavrenov
I still have not got any response from you on the question above.

vinograd47 · 2020-07-24T09:18:03Z

Two step approach has another drawback - potential incorrect createROI implementation. If some plugin implemented RemoteBlob before and added clone override (which is not complex task) - such RemoteBlob will get default createROI implementation, while ROI is not supported by the plugin, since it will be a new field in the Blob class and plugin code was not updated yet.

Single function implementation is atomic in this point of view. Either it is overridden and implemented, which means that ROI is supported for particular kind of Blob or it is not implemented at all and throws exception (default behavior). There will be no middle state.

alpkn · 2020-07-24T10:13:30Z

... potential incorrect ... will get default ...

That is too artificial thought and might said just about any programming interface. There are tests which make sure that implementation works as expected.

ilya-lavrenov · 2020-07-29T11:17:46Z

Approach which introduce less changes in public API was merged:

It does not include clone, setROI as proposed in this PR.
It does not include getROI which is questionable and we don't have clean understanding why is it needed in IE public API.

alpkn · 2020-07-29T15:22:14Z

@ilya-lavrenov
I remind the question above was as following:

Would you please explain the cons of having the set of decomposed methods (i.e., shallowClone, getROI/setROI) plus an aggregate createROI (which makes use of them) as an opposite to having the single createROI methods?

The comment above is not answering the question: you just stated the actions you did. The first item is the pure fact; second was already proved wrong by the customer's opinion and by the code in the sample which is part of this PR.

alpkn added this to the 2020.4 milestone Jun 10, 2020

vinograd47 reviewed Jun 11, 2020

View reviewed changes

inference-engine/src/inference_engine/ie_blob_common.cpp Show resolved Hide resolved

vinograd47 reviewed Jun 11, 2020

View reviewed changes

inference-engine/tests/unit/inference_engine/ie_blob_test.cpp Show resolved Hide resolved

alpkn mentioned this pull request Jun 11, 2020

[IE] Another proposal for Blob ROI API #882

Merged

alpkn requested review from ArtemySkrebkov, dkurt, dmatveev, ilya-lavrenov, ilyachur and rzubarev June 11, 2020 09:06

alpkn force-pushed the blob-roi-api branch from 2fbbd8b to b6a91dd Compare June 11, 2020 15:45

alpkn changed the title ~~Blob ROI API~~ Blob ROI API for passing ROI through IE Jun 11, 2020

alpkn marked this pull request as ready for review June 11, 2020 15:55

alpkn requested review from a team as code owners June 11, 2020 15:55

alpkn requested review from a team June 11, 2020 15:55

alpkn force-pushed the blob-roi-api branch 4 times, most recently from 83475bd to 9bc4cb5 Compare June 15, 2020 09:52

ggladilov reviewed Jun 16, 2020

View reviewed changes

ArtemySkrebkov reviewed Jun 17, 2020

View reviewed changes

alpkn force-pushed the blob-roi-api branch 3 times, most recently from 7e579f1 to a44bc9b Compare June 24, 2020 18:41

vinograd47 reviewed Jun 25, 2020

View reviewed changes

inference-engine/include/ie_blob.h Show resolved Hide resolved

alpkn added 6 commits July 7, 2020 14:10

update tests; add tests for new methods

440771c

Allow chain of consequent ROI blobs to be constructed

5cca77d

nv12/i420 roi methods impl; tests

45b8518

classification_of_detected_roi sample; README is yet to come

49a41ff

review

d4b9ef4

clone/shallowClone

d71ac2f

ilya-lavrenov modified the milestones: 2020.4, 2021.1 Jul 8, 2020

vinograd47 approved these changes Jul 9, 2020

View reviewed changes

alpkn commented Jul 13, 2020

View reviewed changes

alpkn assigned alpkn and ilya-lavrenov and unassigned alpkn Jul 16, 2020

ArtemySkrebkov self-requested a review July 20, 2020 14:41

ArtemySkrebkov approved these changes Jul 20, 2020

View reviewed changes

ggladilov self-requested a review July 20, 2020 14:42

ggladilov approved these changes Jul 20, 2020

View reviewed changes

ilya-lavrenov added the category: inference OpenVINO Runtime library - Inference label Jul 21, 2020

ilya-lavrenov closed this Jul 29, 2020

Conversation

alpkn commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alpkn commented Jun 16, 2020

Uh oh!

ggladilov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ArtemySkrebkov left a comment

Choose a reason for hiding this comment

Uh oh!

alpkn commented Jun 17, 2020

Uh oh!

Uh oh!

alpkn Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilyachur Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

alpkn Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

riverlijunjie Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

alpkn Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

ilyachur Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

alpkn Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

alpkn commented Jul 13, 2020

Uh oh!

alpkn commented Jul 16, 2020

Uh oh!

rzubarev commented Jul 21, 2020

Uh oh!

ArtemySkrebkov commented Jul 22, 2020

Uh oh!

zhangcong2019 commented Jul 23, 2020

Uh oh!

alpkn commented Jul 24, 2020

Uh oh!

vinograd47 commented Jul 24, 2020

Uh oh!

alpkn commented Jul 24, 2020

Uh oh!

ilya-lavrenov commented Jul 29, 2020

Uh oh!

alpkn commented Jul 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

alpkn commented Jun 10, 2020 •

edited

Loading

alpkn Jul 13, 2020 •

edited

Loading