Fix TFImporter diagnostic mode segfault by rogday · Pull Request #20402 · opencv/opencv

rogday · 2021-07-13T10:22:30Z

This PR adds DummyLayer and replaces all failed to initialize layers with it. This solves the issue of memory corruption in getLayerShapes() because DummyLayer just throws an exception in every function.

Test data is at opencv/opencv_extra#882

opencv_extra=tf_diag_dummy

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

alalek · 2021-07-13T19:37:30Z

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

+         *
+         * Intended for internal use only in conjunction with DNN_DIAGNOSTIC_RUN.
+         */
+        void replaceLayer(const String &name, const String &type, LayerParams &params);


IMHO, we don't need this in public API (User visible). The same comment is about DummyLayer.
use dnn/src/dnn_common.hpp instead.

Moved NotImplemented layer to dnn_common.hpp. replaceLayer became a part of addLayer under if (DNN_DIAGNOSTICS_RUN && type == "NotImplemented") which will otherwise throw an exception as before.

alalek · 2021-07-13T19:37:33Z

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

        static Ptr<Layer> create(const LayerParams &params);
    };

+    class CV_EXPORTS DummyLayer : public Layer


DummyLayer -> NotImplementedLayer?

alalek · 2021-07-13T19:38:37Z

modules/dnn/src/layers/dummy_layer.cpp

+// Copyright (C) 2021, Intel Corporation, all rights reserved.
+// Third party copyrights are property of their respective owners.
+


Please remove this unrelated copyright. The first 3 lines from Wiki page is enough.

alalek

Is it possible to add some simple test case for that? (to ensure that this would not crash anymore)

alalek · 2021-07-14T13:22:20Z

modules/dnn/src/layers/not_implemented_layer.cpp

+
+Ptr<Layer> NotImplemented::create(const LayerParams& params)
+{
+    return Ptr<Layer>(new NotImplementedImpl(params));


Ptr<Layer>(new

makePtr should be used instead (similar to std::make_shared)

alalek · 2021-07-14T13:24:42Z

modules/dnn/src/tensorflow/tf_importer.cpp

+    if (DNN_DIAGNOSTICS_RUN)
+    {
+        LayerFactory::registerLayer("NotImplemented", notImplementedRegisterer);
+    }


We have other importers too with similar problems.
What would happen with multiple registration attempts?

IMHO, register should go into NotImplemented as static method.

alalek · 2021-07-15T11:51:55Z

modules/dnn/test/test_tf_importer.cpp

+        Net net = readNetFromTensorflow(netPath, netConfig);
+    }
+private:
+    cv::utils::logging::LogLevel logLevel;


is it used?
should we restore log level in destructor?

I deleted it already. Originally, yes, in constructor I was silencing logs and in the destructor the appropriate level was restored, but in event of test failure we would loose logs. Report.xml shouldn't contain stderr output and I decided to leave logs be.

alalek · 2021-07-15T20:38:03Z

modules/dnn/src/dnn_common.hpp

+Ptr<Layer> notImplementedRegisterer(LayerParams &params);
+


we don't need that in dnn_common.hpp anymore.

Implementation can be placed near NotImplemented::Register()

It is used in TFimporter::parseNode.

Why is not NotImplemented::Register()? (or nothing because it is handled in enableModelDiagnostics())

Let's imagine TFImporter doesn't know some layer. It will be registered in order to not flood the output with errors about it.

If diagnostic mode is not enabled then strategy must be "fail fast". We should not eat problems.

I am indeed talking about the behavior of this function with diagnostic mode turned on.

Got it. It is registered under a new name.

There is another problem:

"NotImplemented" is unregistered when we call enableModelDiagnostics(false);

these unknown layers don't.

Perhaps we should disallow turning off diagnostic mode if it is already enabled. This would be OK for tool, but this would cause problems with tests.

BTW, NotImplemented::Register() can accept the "name" string parameter.

The last problem can be resolved through singleton for registered NotImplemented layers. It should provide two methods:

registerLayerType(const std::string& name) - register and save used name

unregisterLayerTypes() (called by enableModelDiagnostics(false)) - cleanup all previously registered layer's types saved on registration

I will push changes in a few hours. I added LayerHandler class which manages these layers and doesn't need to call registerLayer.

alalek · 2021-07-15T20:40:19Z

modules/dnn/src/dnn_common.hpp

+    static Ptr<Layer> create(const LayerParams &params);
+
+    static void Register();
+    static void unRegister();


Register -> register

https://github.com/opencv/opencv/wiki/Coding_Style_Guide#naming-conventions

This is a reserved keyword.

alalek · 2021-07-15T20:40:30Z

modules/dnn/src/layers/not_implemented_layer.cpp

+    CV_DEPRECATED std::vector<Mat> finalize(const std::vector<Mat> &inputs)
+    {
+        CV_Error(Error::StsNotImplemented, msg);
+        return {};


CV_Error() doesn't return (it throws or abort()).
It is properly marked as [[noreturn]] for popular compilers.
All statements below should be removed.

alalek

Look good to me 👍

rogday force-pushed the tf_diag_dummy branch 2 times, most recently from 55aee70 to d7b19eb Compare July 13, 2021 12:11

rogday requested a review from dkurt July 13, 2021 12:55

alalek reviewed Jul 13, 2021

View reviewed changes

rogday force-pushed the tf_diag_dummy branch 2 times, most recently from 3ca3586 to c052152 Compare July 14, 2021 08:24

alalek reviewed Jul 14, 2021

View reviewed changes

rogday force-pushed the tf_diag_dummy branch from c052152 to 6c5cdd1 Compare July 15, 2021 11:13

rogday mentioned this pull request Jul 15, 2021

Add TF diagnostic tests opencv/opencv_extra#882

Merged

alalek reviewed Jul 15, 2021

View reviewed changes

rogday force-pushed the tf_diag_dummy branch from 6c5cdd1 to 1d0a786 Compare July 15, 2021 11:52

alalek reviewed Jul 15, 2021

View reviewed changes

rogday force-pushed the tf_diag_dummy branch from 1d0a786 to 7474535 Compare July 16, 2021 12:29

rogday removed the request for review from dkurt July 16, 2021 12:34

add NotImplemented layer

c30078c

rogday force-pushed the tf_diag_dummy branch from 7474535 to c30078c Compare July 16, 2021 12:40

alalek approved these changes Jul 16, 2021

View reviewed changes

asmorkalov approved these changes Jul 16, 2021

View reviewed changes

alalek assigned asmorkalov Jul 16, 2021

opencv-pushbot merged commit b61a55e into opencv:master Jul 16, 2021

rogday deleted the tf_diag_dummy branch July 16, 2021 17:59

alalek mentioned this pull request Oct 15, 2021

(5.x) Merge 4.x #20886

Merged

		// Copyright (C) 2021, Intel Corporation, all rights reserved.
		// Third party copyrights are property of their respective owners.

Uh oh!

Conversation

rogday commented Jul 13, 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!

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!

alalek left a comment

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!

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!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rogday commented Jul 13, 2021 •

edited

Loading