G-API: Add standalone fix for graph empty input by sivanov-work · Pull Request #20184 · opencv/opencv

sivanov-work · 2021-05-31T06:22:45Z

STANDALONE version of the fix

G-API graph invocation may fail in case of user passed empty cv::Mat object and this issue detected in core part of G-API on compute step.
The main idea is move pretty-check into compile phase and separate if from actual invocation (note: compile doesn't mean C++ compile but rather means phase of graph pipeline execution )

Added UTs:

for empty cv::Mat compilation

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_gapi_standalone:Linux x64=ade-0.1.1f
build_gapi_standalone:Win64=ade-0.1.1f
build_gapi_standalone:Mac=ade-0.1.1f
build_gapi_standalone:Linux x64 Debug=ade-0.1.1f

Xbuild_image:Custom=centos:7
Xbuildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

build_image:Custom=ubuntu-openvino-2021.3.0:20.04
build_image:Custom Win=openvino-2021.2.0
build_image:Custom Mac=openvino-2021.2.0

test_modules:Custom=gapi,python2,python3,java
test_modules:Custom Win=gapi,python2,python3,java
test_modules:Custom Mac=gapi,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=*

dmatveev · 2021-06-02T11:15:45Z

modules/gapi/src/api/gproto.cpp

    {
        const auto desc = cv::descr_of(util::get<cv::Mat>(arg));
-        GAPI_Assert(desc.size.height != 0 && desc.size.width != 0 && "incorrect dimensions of Mat!"); break;
+        GAPI_Assert(desc.size.height != 0 && desc.size.width != 0 && "incorrect dimensions of Mat!");


I believe the problem was in using != intead of >, and at some point we get -1 there which passed through the test, isnt it?

yes, it is.
But, i've decided that empty_gmat_descr comparison is more clearest here, isnt it ?

I don't know, does it actually works for the case?

modules/gapi/src/api/gproto.cpp

dmatveev · 2021-06-02T12:34:06Z

modules/gapi/src/compiler/gcompiler.cpp

+                if(cv::empty_gmat_desc() == cv::util::get<cv::GMatDesc>(meta))
+                {
+                    is_valid = false;
+                    tracer << "empty cv::Mat is not allowed as compile argument";


Empty cv::Mat (as the cv::Mat in general) is not a compile argument, so this message may be misleading

Also - does this check handle the case when an empty cv::Mat is passed to .apply() ?

I've checked the code and found:

this parts is called from compile()

compile() can be called as standalone method
or

be part of apply(), when arguments set was changed (recompile)

So, message looks like really misleading in direct apply() case.
And empty cv::Mat cannot be passed to .apply() because recompile would be triggered in first and invoke this compile()

@dmatveev What do you thinks about the message?
empty cv::Mat is not allowed as graph input argument

It is more correct but again, we don't deal with cv::Mat at all when compiling a graph, so referring to a structure which may not exist at the time can be misleading.

modules/gapi/src/compiler/gcompiler.cpp

modules/gapi/test/gapi_gcompiled_tests.cpp

modules/gapi/include/opencv2/gapi/gproto.hpp

dmatveev · 2021-06-03T19:34:22Z

modules/gapi/include/opencv2/gapi/gproto.hpp

+bool validate_input_meta_arg(const GMetaArg& meta, std::ostream* tracer = nullptr);
+bool validate_input_meta(const GMatDesc& meta, std::ostream* tracer = nullptr);


Why do we need this here? It is a public header - it is exposed in the installed library package. Is it supposed to be used somehow by end users? I believe it is not

it's actually the common part between gproto.cpp & gcompiled.cpp -which uses the same validation fucntionality

I've added it similarly in the same way with validate_input_args because i got the impression after file inspection that entire file exposes a set of auxiliary functions - additionally, i didn't face with usage other function in this file as public interface. This overall assumption score told me that file is utility file
But maybe I wrong and this utility-like function should be implemented in other place

moved to src/ai/gmeta_chec.hpp/cpp

gproto.cpp and gproto_priv.hpp are the right ones.

dmatveev · 2021-06-03T19:38:15Z

modules/gapi/src/api/gproto.cpp

+                    "incorrect description of cv::UMat! "
+                    "Dimensions, channel and depth must be positive");


I believe the check itself and the error message here mix levels of abstractions.

Looking at this code, you don't know what is actually checked by this function, but the assert text complains about some specific fields.

I am not sure if it needs to be an assert at all. If you want provide details, provide it in the exception text you throw from the validate_input_meta() function (so it becomes void not bool).

It make sense, and "incorrect description of cv::*Mat" is enough

about exception i think it is a bad idea:

void function with the only reason in throwing exception looks ugly and unsafe and inconvenient in usage and considered as bad practice

GAPI_Assert here means that this place is performance critical in Release build, so exception is not good here

It is not performance critical, in the performance-critical code we use DbgAssert.

dmatveev · 2021-06-03T19:40:01Z

modules/gapi/src/api/gproto.cpp

+bool cv::validate_input_meta(const cv::GMatDesc& meta, std::ostream* tracer)
+{
+    if (meta.dims.empty())
+    {
+        if (!(meta.size.height > 0 && meta.size.width > 0))
+        {
+            if (tracer)
+            {
+                *tracer << "Image format is invalid. Size must contain positive values, got width: "
+                        << meta.size.width << ", height: " << meta.size.height;
+            }
+            return false;
+        }
+
+        if (!(meta.chan > 0))
+        {
+            if (tracer)
+            {
+                *tracer << "Image format is invalid. Channel mustn't be negative value, got channel: "
+                        << meta.chan;
+            }
+            return false;
+        }
+    }
+
+    if (!(meta.depth >= 0))
+    {
+        if (tracer)
+        {
+            *tracer << "Image format is invalid. Depth must be positive value, got depth: "
+                    << meta.depth;
+        }
+        return false;
    }
+    return true;
 }


Why bool here again? I believe the code simplifies a lot if we drop this tracer thing at all and simply throw.

yes, it simplifed here, but after it become more complicated in caller context. It is a bad idea, as for me, to wrap function around try-catch every time

if you dislike optional tracer, then it is possible in the following approach

std::tuple<bool, std::string> validate_(); ... auto res = validate_(...) if(!res.first) { printf(res.second) }

what do you think?

Mainly we don't need try/catch in/with our own code.

modules/gapi/test/gapi_gcompiled_tests.cpp

dmatveev · 2021-06-07T12:26:11Z

modules/gapi/CMakeLists.txt

    src/api/ginfer.cpp
    src/api/media.cpp
    src/api/rmat.cpp
+    src/api/gmeta_check.cpp


a separate file may be too much for this simple function, consider gproto.cpp & its _priv.hpp.

yeah, i thought about _priv.hpp, but I was confused with comment // FIXME: why it is here?

https://github.com/opencv/opencv/pull/20184/files#diff-dfc809caba75402a8e78080f173d484de3b67c66af4b57c81c680acf8275b6e4L22

And i realized that this include should be removed is in future
@dmatveev What do you think about that - do you still suggest to move it in _priv.hpp ?

I believe in 2021 nobody remembers the answer to

// FIXME: why it is here?

:)

modules/gapi/src/api/gmeta_check.cpp

modules/gapi/src/compiler/gcompiler.cpp

dmatveev · 2021-06-08T20:08:21Z

modules/gapi/src/api/gproto.cpp

+    {
+        if (!(meta.size.height > 0 && meta.size.width > 0))
+        {
+            cv::util::throw_error(std::logic_error(std::string("Image format is invalid. Size must contain positive values, got width: ") +


This line is 139 characters long; I believe the OpenCV's coding style allows something up to 110 but we're trying to follow ~80. I believe the std::string() thing is not needed here as operator+(const char*, std::string) should work things out. throw_error itself is terrible (was done wrong initially and nobody has fixed it then), so we usually write this stuff like

cv::util::throw_error (std::logic_error (...);

Also, I believe a simple assert could work fine here (except the failed value won't be printed).

dmatveev · 2021-06-08T20:10:27Z

modules/gapi/src/compiler/gcompiler.cpp

+        try
+        {
+            gimpl::proto::validate_input_meta_arg(meta); //may throw
+        }
+        catch(const std::exception& ex)
+        {
+            const auto index = ade::util::index(meta_arg_idx);
+            util::throw_error(std::logic_error
+                        ("GComputation metadata incorrect value "
+                         "(argument " + std::to_string(index) + "), error: " +
+                         ex.what()));


I believe this try/catch is redundant - the code throws anyway, the only thing it adds is the error message - you can use the LOG() trick instead to provide the context.

sivanov-work · 2021-06-09T08:04:44Z

modules/gapi/src/compiler/gcompiler.cpp

        const auto &proto = std::get<1>(ade::util::value(meta_arg_idx));

+        const auto index = ade::util::index(meta_arg_idx);
+        GAPI_LOG_DEBUG(nullptr, "Process index: " << index);


[DEBUG:0] global C:\Users\sivanov\github\opencv\modules\gapi\src\compiler\gcompiler.cpp (346) cv::gimpl::GCompiler::validateInputMeta Total count: 1 [DEBUG:0] global C:\Users\sivanov\github\opencv\modules\gapi\src\compiler\gcompiler.cpp (353) cv::gimpl::GCompiler::validateInputMeta Process index: 0

dmatveev

Fantastic, thanks!

G-API: Add standalone fix for graph empty input * Add sandalone fix for graph empty input * Apply some review comments * Fix whitespace * Apply review comment: make Mat check more deeper * Apply some comments * Remove tracer apply exception throwing * Apply comments: move validatio into gproto_priv.hpp * Apply minor text correction * Fix alignment, remove try-catch

Add sandalone fix for graph empty input

c881ccd

sivanov-work added the category: g-api / gapi label May 31, 2021

sivanov-work requested a review from dmatveev May 31, 2021 06:22

sivanov-work changed the title ~~Add standalone fix for graph empty input~~ G-API: Add standalone fix for graph empty input May 31, 2021

dmatveev self-assigned this Jun 2, 2021

dmatveev added this to the 4.5.3 milestone Jun 2, 2021

dmatveev reviewed Jun 2, 2021

View reviewed changes

sivanov-work added 3 commits June 3, 2021 08:02

Apply some review comments

c9b2837

Fix whitespace

afed486

Apply review comment: make Mat check more deeper

823d793

dmatveev reviewed Jun 3, 2021

View reviewed changes

sivanov-work added 2 commits June 4, 2021 10:34

Apply some comments

63d33f8

Remove tracer apply exception throwing

1103134

dmatveev reviewed Jun 7, 2021

View reviewed changes

sivanov-work added 2 commits June 8, 2021 14:46

Apply comments: move validatio into gproto_priv.hpp

f654999

Apply minor text correction

726c4cf

dmatveev reviewed Jun 8, 2021

View reviewed changes

Fix alignment, remove try-catch

beae814

sivanov-work commented Jun 9, 2021

View reviewed changes

sivanov-work requested a review from dmatveev June 9, 2021 08:05

dmatveev approved these changes Jun 10, 2021

View reviewed changes

alalek merged commit e461031 into opencv:master Jun 10, 2021

alalek mentioned this pull request Jun 13, 2021

(5.x) Merge 4.x #20266

Merged

alalek mentioned this pull request Oct 15, 2021

(5.x) Merge 4.x #20886

Merged

sivanov-work deleted the fix_gapi_empty_input branch March 5, 2022 05:50

		bool validate_input_meta_arg(const GMetaArg& meta, std::ostream* tracer = nullptr);
		bool validate_input_meta(const GMatDesc& meta, std::ostream* tracer = nullptr);

		"incorrect description of cv::UMat! "
		"Dimensions, channel and depth must be positive");

Uh oh!

Conversation

sivanov-work commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sivanov-work commented May 31, 2021 •

edited

Loading