Skip to content

DNN: More stable DB text detection API#22614

Merged
opencv-pushbot merged 1 commit intoopencv:4.xfrom
zihaomu:add_std2DB_API
May 1, 2023
Merged

DNN: More stable DB text detection API#22614
opencv-pushbot merged 1 commit intoopencv:4.xfrom
zihaomu:add_std2DB_API

Conversation

@zihaomu
Copy link
Copy Markdown
Member

@zihaomu zihaomu commented Oct 9, 2022 •
edited
Loading

Merge with extra: opencv/opencv_extra#1011

The followings are the purpose of the PR:

  1. Let blobFromImage support Scalar scale to feed the requirement of Standard Deviation value.
  2. more stable DB text detection API, the old post-processing will get assert errors when length == 0.;. The related code part.
  3. Output the correct confidence or score instead of always being 1..
  4. Support PP-OCR-v2 and PP-OCR-v3 DB models. The original inference code can be found here.

Related regression test model.

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 another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the 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

alalek
alalek reviewed Oct 9, 2022
View reviewed changes
@asmorkalov asmorkalov requested a review from rogday October 10, 2022 12:09
@zihaomu zihaomu force-pushed the add_std2DB_API branch 4 times, most recently from c87eb1f to 1f3c63e Compare October 12, 2022 10:53
@zihaomu
Copy link
Copy Markdown
Member Author

zihaomu commented Oct 12, 2022 •
edited
Loading

Hi, I need your help @alalek, @asmorkalov @rogday. It looks like there is a problem with the Objective-C binding of blobFromImage after overloading, is there a solution?
CI error message

@zihaomu zihaomu mentioned this pull request Oct 13, 2022
Closed
2 tasks
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Oct 14, 2022

@zihaomu Please take a look on https://stackoverflow.com/questions/2286312/method-overloading-in-objective-c. Objective-C does not support methods overload as it's designed in C++. It adds parameters names to methods signature to resolve methods with the same name correctly. Actually the same issue should happen with Python bindings. Bindings generator should generate bindings for one of the overloaded methods, not all of the variants.
My suggestion:

  • Add test for thre new overloads for Python and Java to be sure that several variants are available and correct overload is called.
  • Use CV_WRAP_AS with alternative function name in bindings.

@zihaomu
Copy link
Copy Markdown
Member Author

zihaomu commented Oct 17, 2022 •
edited
Loading

Hi @vpisarev, please check if this PR really needs to overload the blobFromImage.

Actually, there are 3 different ways to support DB API with stddev value:

  1. overload the blobFromImage, let the blobFromImage support Scalar scale so that we can support the stddev,
  2. overload the Model::setInputScale and let the Model::impl::processFrame support the stddev value,
  3. overload the TextDetectionModel_DB_Impl::processFrame and let this function support DB with stddev.

@asmorkalov asmorkalov requested a review from vpisarev October 17, 2022 06:13
@zihaomu
Copy link
Copy Markdown
Member Author

zihaomu commented Oct 20, 2022

Update offline discussion results:
Extend the blobFromImages to support more common requirements, including stddev, letterbox and data layout (NCHW and NHWC).
My plan is to create another PR for this extension, and after that, we can merge this PR.

@zihaomu zihaomu force-pushed the add_std2DB_API branch 2 times, most recently from 6464bbf to 7895efa Compare April 25, 2023 07:16
zihaomu
zihaomu commented Apr 25, 2023
View reviewed changes
modules/dnn/src/model.cpp Outdated
Comment on lines +179 to +185
// Because 0 is meaningless in scalefactor.
// If the scalefactor is (x, 0, 0, 0), we convert it to (x, x, x, x).
if (scale[1] == 0 && scale[2] == 0 && scale[3] == 0)
{
CV_Assert(scale[0] != 0 && "Scalefactor of 0 is meaningless.");
scale = Scalar::all(scale[0]);
}
Copy link
Copy Markdown
Member Author

@zihaomu zihaomu Apr 25, 2023

Choose a reason for hiding this comment

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

Because we have backward compatibility, I think this hack is necessary.

zihaomu
zihaomu commented Apr 25, 2023
View reviewed changes
modules/dnn/src/model.cpp
}
/*virtual*/
void setInputScale(double scale_)
void setInputScale(const Scalar& scale_)
Copy link
Copy Markdown
Member Author

@zihaomu zihaomu Apr 25, 2023

Choose a reason for hiding this comment

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

Change Module API from double to Scalar.

@zihaomu zihaomu force-pushed the add_std2DB_API branch 2 times, most recently from 29f5018 to eb3d550 Compare April 28, 2023 06:03
opencv-alalek
opencv-alalek approved these changes Apr 28, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@opencv-alalek opencv-alalek left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank you!

@opencv-alalek opencv-alalek added this to the 4.8.0 milestone Apr 28, 2023
opencv-alalek
opencv-alalek reviewed Apr 28, 2023
View reviewed changes
modules/dnn/src/dnn_common.hpp Outdated
// Scalefactor is a common parameter used for data scaling. In OpenCV, we often use Scalar to represent it.
// Because 0 is meaningless in scalefactor.
// If the scalefactor is (x, 0, 0, 0), we convert it to (x, x, x, x). The following func will do this hack.
Scalar hackScalar(const Scalar& _scale)
Copy link
Copy Markdown
Contributor

@opencv-alalek opencv-alalek Apr 28, 2023

Choose a reason for hiding this comment

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

Should be static / static inline or moved to .cpp

Copy link
Copy Markdown
Contributor

@vpisarev vpisarev Apr 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

can we use some better name? say, broadcastRealScalar? Also, I agree that for now it should be put into some internal header and made inline function

Copy link
Copy Markdown
Member Author

@zihaomu zihaomu Apr 28, 2023

Choose a reason for hiding this comment

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

got it

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented May 1, 2023

@opencv-alalek Could you, please, update ABI compliance checker options?

@opencv-pushbot opencv-pushbot merged commit 3c76b33 into opencv:4.x May 1, 2023
@asmorkalov asmorkalov mentioned this pull request May 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalek alalek alalek left review comments

@opencv-alalek opencv-alalek opencv-alalek approved these changes

@rogday rogday Awaiting requested review from rogday

@vpisarev vpisarev Awaiting requested review from vpisarev

Assignees

@opencv-alalek opencv-alalek

Labels

category: dnn feature

Projects

None yet

Milestone

4.8.0

Development

Successfully merging this pull request may close these issues.

6 participants

@zihaomu @asmorkalov @vpisarev @alalek @opencv-alalek @opencv-pushbot