Add option for NMS for boxes with different labels by CowKeyMan · Pull Request #18891 · opencv/opencv

CowKeyMan · 2020-11-22T00:14:14Z

In the detect function in modules/dnn/include/opencv2/dnn/dnn.hpp, whose implementation can be found at modules/dnn/src/model.cpp, the Non Max Suppression (NMS) is applied only for objects of the same label. Thus, a flag
was added with the purpose to allow developers to choose if they want to keep the default implementation or wether they would like NMS to be applied to all the boxes, regardless of label.

The flag is called nmsDifferentLabels, and is given a default value of false, which applies the current default implementation, thus allowing existing projects to update opencv without disruption

resolves #18832

The following image is from the MSCOCO dataset and the predictions were generated using YoloV4 trained on the MSCOCO dataset (weights here: https://github.com/AlexeyAB/darknet/releases/download/darknet_yolo_v3_optimal/yolov4.weights) . The numer '20' is the label for 'elephant' while the label '0' is the label for 'person'. You can see that the person and elephant have a very close bounding box, however they are not being suppressed when nmsDifferentLabels is False, but they are being suppressed when nmsDifferentLabels is True. You can also see that the box chosen was that with the highest confidence.

This is useful when an object is determined as 2 objects at once. In my case, I had some cars which were being classified as both a car and a truck at the same time, and I could not NMS them properly without adding more code. It was also difficult to find out why NMS was not behaving 'properly', leaving me to have to actually dig into the source code. This fixes this issue and in my opinion makes the code clearer without taking assumptions.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

force_builders=Custom,Custom Win,Custom Mac
build_image:Custom=ubuntu-openvino-2021.1.0:20.04
build_image:Custom Win=openvino-2021.1.0
build_image:Custom Mac=openvino-2021.1.0

test_modules:Custom=dnn,python2,python3,java
test_modules:Custom Win=dnn,python2,python3,java
test_modules:Custom Mac=dnn,python2,python3,java

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

asmorkalov · 2020-11-22T11:44:20Z

@sl-sergei Please take a look.

asmorkalov · 2020-11-22T11:44:55Z

@CowKeyMan Thanks for the patch. Could you add test for the new feature?

asmorkalov · 2020-11-22T11:45:48Z

@alalek Is it reasonable to rebase the solution to 3.4 or master branches?

alalek · 2020-11-22T15:27:18Z

@CowKeyMan Thank you for contribution!

I believe "master" branch should be OK (there is no such classes on 3.4 branch).

Main question here is algorithm properties vs detection parameters.

Good API should be abstract and simple (from User perspective).
In general it should not require extra input parameters in its ".detect()" method.
All parameters which are not supposed to be changed between frames/images should go as algorithm properties through .setProperty()/.getProperty():

void main()
{
    DetectionModel detector(...)
    // configure detection properties once (usually specific for used DL model)
    detector.setProp1(...)
            .setProp2(...)
            .setProp3(...)
    ;

    // call some processing algorithm which uses "abstract"
    process(detector, ...);
}

void process(const DetectionModel& detector, ...)
{
    ...
    // we should not know here which exact model is used and which input parameters are good for that
    detector.detect(frame, where_to_store_detection_result_only)
    ...
}

These control methods are expected:

// * - (bool) useGlobalNMS - ignores class ID during NMS process. Default is `false`.

CV_WRAP DetectionModel& setGlobalNMS(bool useGlobalNMS);
CV_WRAP float getGlobalNMS() const;

@dkurt @l-bat Please check naming.

See #17570 for implementation details.

P.S. Even confThreshold/nmsThreshold should go as properties (these thresholds are specific for used DL models) but it is another task (not a scope of this PR).

CowKeyMan · 2020-11-22T16:16:27Z

@asmorkalov I have updated my PR with a test case

I tried to put this into 'master' earlier. However, since it changes a function definiton (adding a new parameter, therefore changing the api), the Linux x64 build was failing

CowKeyMan · 2020-11-22T17:23:19Z

The last build for Win64 OpenCL failed when I do not think it should have. Looking at the output it says that it was forcefully terminated with a SIGINT, is this of any concern?

Should I do the setters and getters in this PR or should this be a separate PR after this one?

modules/dnn/include/opencv2/dnn/dnn.hpp

modules/dnn/src/model.cpp

modules/dnn/test/test_model.cpp

dkurt · 2020-11-23T10:41:01Z

modules/dnn/include/opencv2/dnn/dnn.hpp

                             CV_OUT std::vector<float>& confidences, CV_OUT std::vector<Rect>& boxes,
-                             float confThreshold = 0.5f, float nmsThreshold = 0.0f);
+                             float confThreshold = 0.5f, float nmsThreshold = 0.0f,
+                             bool nmsAcrossClasses = false);


Please make as a method:

CV_WRAP DetectionModel& setNMSAcrossClasses (bool value);

Otherwise this method is too complicated.

Hi, I'm doing this, however for when the return type is DetectionModel&, I get an error in the python binding that no default contructor for DetectionModel exists. It works fine when the type is void for the setter though.

Yes, please take a look at Model class definition: https://github.com/opencv/opencv/blob/master/modules/dnn/include/opencv2/dnn/dnn.hpp

Right, just saw the 'need to fix bindings first' comment. Does that mean I should leave it as void for now or create a default constructor for DetectionModel as well?

modules/dnn/include/opencv2/dnn/dnn.hpp

In the detect function in modules/dnn/include/opencv2/dnn/dnn.hpp, whose implementation can be found at modules/dnn/src/model.cpp, the Non Max Suppression (NMS) is applied only for objects of the same label. Thus, a flag was added with the purpose to allow developers to choose if they want to keep the default implementation or wether they would like NMS to be applied to all the boxes, regardless of label. The flag is called nmsDifferentLabels, and is given a default value of false, which applies the current default implementation, thus allowing existing projects to update opencv without disruption Solves issue #18832

dkurt

👍 Thank you!

CowKeyMan · 2020-11-29T22:06:00Z

@dkurt Thank you for your patience and guidance

alalek

Well done!

alalek · 2020-11-29T22:44:10Z

modules/dnn/include/opencv2/dnn/dnn.hpp

+          * This function allows you to toggle this behaviour.
+          * @param[in] value The new value for nmsAcrossClasses
+          */
+         CV_WRAP void setNmsAcrossClasses(bool value);


Please change return type:
void => DetectionModel& (return *this;)
to allow chains of .set*() calls.

(+ default dummy ctor DetectionModel() for bindings (see mentioned PR), + checks for nullptr impl from this ctor)

Right, forgot about this. Would you be able to elaborate further what you mean by "checks for nullptr impl", please? In other words,

DetectionModel::DetectionModel() : Model() { // What goes here? }

I understand the implementation field impl will be null in this constructor, but I am not sure what exactly to put here. I just updated this PR with a new commit to show the differences I made. I will change and squash it later once we rectify this.

For now, I made the constructor as CV_DEPRECATED_EXTERNAL, which is what the default constructor for Model is using at the moment and left the body empty, also to be like Model's, but am not sure if this is what you intended.

Constructor is fine.
Need to add checks (CV_Assert()) into other methods (where impl is nullptr and impl.dynamicCast<DetectionModel_Impl>() is nullptr too).

Added another commit with the assertions, is this what you meant? Also added the comments next to them, let me know if I should remove or reword

alalek

Well done 👍

CowKeyMan · 2020-12-01T15:34:35Z

Awesome, thank you once again @alalek and @dkurt, as well as @asmorkalov

…nt_labels Add option for NMS for boxes with different labels * DetectionModel impl * Add option for NMS for boxes with different labels In the detect function in modules/dnn/include/opencv2/dnn/dnn.hpp, whose implementation can be found at modules/dnn/src/model.cpp, the Non Max Suppression (NMS) is applied only for objects of the same label. Thus, a flag was added with the purpose to allow developers to choose if they want to keep the default implementation or wether they would like NMS to be applied to all the boxes, regardless of label. The flag is called nmsDifferentLabels, and is given a default value of false, which applies the current default implementation, thus allowing existing projects to update opencv without disruption Solves issue opencv#18832 * Change return type of set & Add default constr * Add assertions due to default constructor

asmorkalov requested a review from sl-sergei November 22, 2020 11:43

asmorkalov added the pr: needs test New functionality requires minimal tests set label Nov 22, 2020

dkurt reviewed Nov 22, 2020

View reviewed changes

modules/dnn/include/opencv2/dnn/dnn.hpp Outdated Show resolved Hide resolved

dkurt reviewed Nov 22, 2020

View reviewed changes

modules/dnn/src/model.cpp Outdated Show resolved Hide resolved

dkurt reviewed Nov 22, 2020

View reviewed changes

modules/dnn/test/test_model.cpp Show resolved Hide resolved

dkurt reviewed Nov 23, 2020

View reviewed changes

modules/dnn/test/test_model.cpp Outdated Show resolved Hide resolved

dkurt reviewed Nov 23, 2020

View reviewed changes

modules/dnn/test/test_model.cpp Outdated Show resolved Hide resolved

asmorkalov removed the request for review from sl-sergei November 23, 2020 07:50

dkurt reviewed Nov 23, 2020

View reviewed changes

dkurt reviewed Nov 24, 2020

View reviewed changes

modules/dnn/include/opencv2/dnn/dnn.hpp Outdated Show resolved Hide resolved

CowKeyMan changed the base branch from next to master November 29, 2020 15:47

dkurt and others added 2 commits November 29, 2020 16:52

DetectionModel impl

ba13141

dkurt approved these changes Nov 29, 2020

View reviewed changes

alalek reviewed Nov 29, 2020

View reviewed changes

asmorkalov added category: dnn feature and removed pr: needs test New functionality requires minimal tests set labels Nov 30, 2020

CowKeyMan added 2 commits November 30, 2020 21:51

Change return type of set & Add default constr

9010cb2

Add assertions due to default constructor

776ff24

alalek approved these changes Dec 1, 2020

View reviewed changes

alalek assigned dkurt Dec 1, 2020

alalek merged commit 9d37cda into opencv:master Dec 1, 2020

CowKeyMan deleted the NMS_boxes_with_different_labels branch December 1, 2020 21:35

alalek mentioned this pull request Apr 9, 2021

(5.x) Merge 4.x #19885

Merged

Uh oh!

Conversation

CowKeyMan commented Nov 22, 2020 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

asmorkalov commented Nov 22, 2020

Uh oh!

asmorkalov commented Nov 22, 2020

Uh oh!

asmorkalov commented Nov 22, 2020

Uh oh!

alalek commented Nov 22, 2020

Uh oh!

CowKeyMan commented Nov 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CowKeyMan commented Nov 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dkurt Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

CowKeyMan Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

dkurt Nov 24, 2020

Choose a reason for hiding this comment

Uh oh!

CowKeyMan Nov 24, 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!

Uh oh!

dkurt left a comment

Choose a reason for hiding this comment

Uh oh!

CowKeyMan commented Nov 29, 2020

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

alalek Nov 29, 2020

Choose a reason for hiding this comment

Uh oh!

CowKeyMan Nov 30, 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!

alalek Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

CowKeyMan Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

CowKeyMan commented Dec 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CowKeyMan commented Nov 22, 2020 •

edited by alalek

Loading

CowKeyMan commented Nov 22, 2020 •

edited

Loading

CowKeyMan Nov 24, 2020 •

edited

Loading

CowKeyMan Nov 30, 2020 •

edited

Loading