All processing in graph demo by mpashchenkov · Pull Request #3 · mpashchenkov/open_model_zoo

mpashchenkov · 2021-03-11T09:14:34Z

No description provided.

mpashchenkov · 2021-03-11T09:25:45Z

@aDanPin main.cpp kernels.hpp kernels.cpp

aDanPin · 2021-03-12T13:23:59Z

+    std::istringstream stream_to_split(in_str);
+
+    while (std::getline(stream_to_split, label, ',')) {
+      labels.push_back(label);


Two whitespace instead four

aDanPin · 2021-03-12T13:33:55Z

+
+        slog::info << "Device info: " << slog::endl;
+
+        for (const auto &device : devices) {


Is this cycle needed if you use gapi?

Print CPU, GPU info. M B need to cut or change.

Part of сode carved out

aDanPin · 2021-03-12T14:14:44Z

+        }
+
+        // Create tracker for reid
+        TrackerParams tracker_reid_params;


Could it be initialize using initializer-list?

Set of numbers in brackets will create questions.

aDanPin · 2021-03-12T14:15:10Z

+        tracker_reid_params.objects_type = "face";
+
+        // Create Tracker for action recognition
+        TrackerParams tracker_action_params;


Set of numbers in brackets will create questions.

aDanPin · 2021-03-12T14:18:12Z

+                if (!fd_model_path.empty()) {
+                    cv::GMat detections =
+                        cv::gapi::infer<nets::FaceDetector>(in);
+                    faces = custom::FaceDetectorPostProc::on( in


Is using some::gapi::stuff::on() not in kernel valid?

aDanPin · 2021-03-16T14:37:13Z

+namespace cv {
+namespace gapi {
+namespace wip {
+class GCaptureSource : public IStreamSource


We already have same class in out G-API framework, isn`t?

Another heir with the same name (GCaptureSource)

For TOP K rising hands case

This is purely a conflict, please rename

aDanPin · 2021-03-16T14:44:40Z

+    if (!no_show_) {
+        cv::imshow(top_window ? top_window_name_ : main_window_name_, frame);
+    }
+}


Ooooooooouuuuu
Sir, one Enter please!!

mpashchenkov · 2021-03-18T12:34:28Z

@@ -0,0 +1,7 @@
+# This file can be used with the --list option of the model downloader.
+face-detection-adas-????
+face-reidentification-retail-????


dmatveev · 2021-03-19T18:30:10Z

@@ -1,4 +1,4 @@
-#!/usr/bin/env python3


This may be risky

dmatveev · 2021-03-19T18:30:42Z

@@ -0,0 +1,135 @@
+# Smart Classroom C++ Demo


I believe it is the old file?

dmatveev · 2021-03-19T18:31:35Z

@@ -0,0 +1,44 @@
+#!/usr/bin/env python


Do we need our special script here or the standard smart classroom's one can be reused?

dmatveev · 2021-03-19T18:32:43Z

@@ -0,0 +1,188 @@
+// Copyright (C) 2021 Intel Corporation


For every file coming from the original demo, can you please mention it in the file's header comment?
It'd simplify the review a lot.

dmatveev · 2021-03-19T18:33:16Z

@@ -0,0 +1,50 @@
+// Copyright (C) 2021 Intel Corporation
+// SPDX-License-Identifier: Apache-2.0


Same question here

dmatveev · 2021-03-19T19:42:15Z

+
+        ActionDetectorConfig action_config;
+        ActionDetectionKernelInput ad_kernel_input;
+        if (!ad_model_path.empty()) {


I'd instead introduce operator+= for a network package like @aDanPin did for graph arguments.

Hmm. But it doesn't solve this problem.

Actually it will, see on the right

dmatveev · 2021-03-19T19:43:26Z

+                GetBinPath(ad_model_path),
+                FLAGS_d_act,
+            }.cfgOutputLayers(outputBlobList);
+        }


I'd move all of this out of main(). It makes reading and understanding the main() flow really hard when it is that long.

Moved to .hpp. Added comments for main steps.

Not sure what is moved where but the code producing the network packages based on options is still there.

May not be a problem now, though.

dmatveev · 2021-03-19T19:44:45Z

+    }
+    slog::info << "Execution successful" << slog::endl;
+    return 0;
+}


Please decompose main() into pieces. What I'd expect to see there is only building GComputation object. Everything else (e.g. preparing compile arguments) should be moved out.

dmatveev · 2021-03-19T19:46:07Z

@@ -0,0 +1,404 @@
+// Copyright (C) 2021 Intel Corporation
+// SPDX-License-Identifier: Apache-2.0


Can all these kernels be thin wrappers over the functionality written in the original smart classroom?

Such approach would better illustrate the idea how cheap is moving from "traditional" code to G-API execution model.

No. Some code depends on IE. Drawing depends on KuhnMunkers ....

dmatveev · 2021-03-19T19:46:48Z

@@ -0,0 +1,580 @@
+// Copyright (C) 2021 Intel Corporation
+// SPDX-License-Identifier: Apache-2.0


Is it inherited or not?

dmatveev

👍

Please complete the clean-up and let's move it to OMZ repo.

dmatveev · 2021-04-07T12:33:09Z

+    config::createFaceRecPtr(frec_kernel_input, rec_config);
+    face_id_to_label_map = frec_kernel_input.ptr->GetIDToLabelMap();
+}
+} // namespace preparation


Newline is missing

dmatveev · 2021-04-07T12:35:38Z

+
+namespace cv {
+namespace gapi {
+namespace wip {


This is not a cv::gapi thing so no need to put these fake namespaces here. It is a custom input source written to handle the app specifics.

dmatveev · 2021-04-07T12:40:42Z


-}  // namespace
+std::string GetBinPath(const std::string &pathXML) {
+    CV_Assert(pathXML.substr(pathXML.size() - 4, pathXML.size()) == ".xml");


will fail with .XML. Not sure if it is the case though

dmatveev · 2021-04-07T12:41:24Z

+std::string GetBinPath(const std::string &pathXML) {
+    CV_Assert(pathXML.substr(pathXML.size() - 4, pathXML.size()) == ".xml");
+    std::string pathBIN(pathXML);
+    return pathBIN.replace(pathBIN.size() - 3, 3, "bin");


may not work with .BIN in a case-sensitive OS/FS. Still, not sure if it is the case

dmatveev · 2021-04-07T12:42:27Z

-            loadedDevices.insert(device);
-        }
+        /** Create default net's parameters **/
+        cv::gapi::ie::Params<nets::FaceDetector> det_net({}, {}, {});


Can't say I like it.

So do we have operator+= for GNetworkPackage?

dmatveev · 2021-04-07T12:46:50Z

+            cv::GArray<cv::GMat> embeddings(std::vector<cv::Mat>{});
+            cv::GArray<DetectedAction> persons_with_actions(std::vector<DetectedAction>{});
+            cv::GArray<detection::DetectedObject> faces(std::vector<detection::DetectedObject>{});


Does it needs to be value-initialized like this?

I've got the idea but please put a comment since it may be not very obvious for the reader. :)

dmatveev · 2021-04-07T12:48:39Z

+                outs += GOut(top_k);
+            }
+            /** Draw ROI and labels **/
+            auto rendered = cv::gapi::wip::draw::render3ch(frame,


so if you do rendering, do you need the original frame to be among the inputs?

If the rendering is optional, I'd either produce the original input image intact OR a rendered one as the same graph output.

dmatveev · 2021-04-07T12:49:24Z

+                                                        networks,
+                                                        TrackerParamsPack{ tracker_reid_params, tracker_action_params },
+                                                        logger_params));


Probably this block should be indented deeper here, but it may change with the OMZ guidelines.

dmatveev · 2021-04-07T12:49:49Z

-            face_detector->enqueue(frame);
-            face_detector->submitRequest();
-        }
+        cc.setSource(cv::gin(cv::gapi::wip::make_src<cv::gapi::wip::CustomCapSource>(cap)));


Please use setSource<>() version since it is less scary (no make_src is required)

dmatveev · 2021-04-07T12:56:06Z

+                    /** TOP_K part. SPACE_KEY is pushed, monitoring enabled
+                     *  Compile and start graph **/
+                    if (!cap.grab()) break;
+                    cc.setSource(cv::gin(cv::gapi::wip::make_src<cv::gapi::wip::CustomCapSource>(cap)));


Use setSource<>()

dmatveev · 2021-04-19T09:03:41Z

@mpashchenkov is gallery/infer2 thing already pushed here? I don't see any updates since my last comment on this PR.

Is it done at the OMZ repo already? Should this PR be closed then?

…se_channels)

dmatveev

Please go ahead with the OMZ integration.

dmatveev · 2021-04-22T05:30:40Z

+bool fileExists(const std::string& name) {
+    std::ifstream f(name.c_str());
+    return f.good();
+}
+
+std::string folderName(const std::string& path) {
+    size_t found_pos;
+    found_pos = path.find_last_of(separator());
+    if (found_pos != std::string::npos)
+        return path.substr(0, found_pos);
+    return std::string(".") + separator();
+}


I see too much code for .hpp here, and the functions are neither static nor inline

May cause problems if the header is included more than once

Also, don't see a point in having so much code in .hpp if it is not a template.

dmatveev · 2021-04-22T05:34:51Z

-            slog::err << "Cannot find target action: " << FLAGS_top_id << slog::endl;
-            return 1;
-        }
+        cv::VideoCapture cap(video_path != "cam" ? video_path : 0);


not sure how this ?: works but barely remember that both true- and false- branch should have the same expression type. MAYBE it results into a proper constructor overload being called but I don't really know.

dmatveev · 2021-04-22T05:38:17Z

+                    const std::vector<cv::Mat>& landmarks,
+                    const std::vector<cv::Rect>& face_rois,
+                          std::vector<cv::Mat>& out_images) {
+        cv::Mat out_image = in.clone();


Do you really need this clone?
Does AlignFaces works really inplace or reallocate its every face image?

[49783] Fix wrong path

mpashchenkov closed this Mar 11, 2021

mpashchenkov reopened this Mar 11, 2021

mpashchenkov force-pushed the mp/internal_scd branch from e4806bb to 45101a5 Compare March 15, 2021 09:12

aDanPin reviewed Mar 15, 2021

View reviewed changes

aDanPin reviewed Mar 16, 2021

View reviewed changes

mpashchenkov force-pushed the mp/internal_scd branch from ad76ace to 0f45376 Compare March 18, 2021 11:53

mpashchenkov commented Mar 18, 2021

View reviewed changes

mpashchenkov force-pushed the mp/internal_scd branch from ca51988 to 7f02574 Compare March 18, 2021 12:47

dmatveev reviewed Mar 19, 2021

View reviewed changes

mpashchenkov force-pushed the mp/internal_scd branch 2 times, most recently from bbba898 to 8835cdf Compare April 1, 2021 10:59

dmatveev approved these changes Apr 7, 2021

View reviewed changes

Maxim Pashchenkov added 2 commits April 19, 2021 15:47

infer2 Gallery + up graph in main

166f7c1

Removed lambda

967e17c

mpashchenkov force-pushed the mp/internal_scd branch from a3f5cb4 to 967e17c Compare April 19, 2021 13:51

Maxim Pashchenkov added 5 commits April 19, 2021 18:46

4

a11b8e2

Removed lambda for gallery, mv correct preproc (for net without rever…

a5ad85b

…se_channels)

Universal output

584cbad

Added ParseSSD

ff69f55

GNetPackage +=, one more end line

6a80959

dmatveev reviewed Apr 22, 2021

View reviewed changes

mpashchenkov pushed a commit that referenced this pull request Apr 27, 2021

Merge pull request #3 from aalborov/aalborov/fix-49783

6618cd7

[49783] Fix wrong path

mpashchenkov pushed a commit that referenced this pull request Oct 1, 2021

End #3

ab872f2


		slog::info << "Device info: " << slog::endl;

		for (const auto &device : devices) {

		@@ -0,0 +1,50 @@
		// Copyright (C) 2021 Intel Corporation
		// SPDX-License-Identifier: Apache-2.0

		@@ -0,0 +1,404 @@
		// Copyright (C) 2021 Intel Corporation
		// SPDX-License-Identifier: Apache-2.0

		@@ -0,0 +1,580 @@
		// Copyright (C) 2021 Intel Corporation
		// SPDX-License-Identifier: Apache-2.0

Conversation

mpashchenkov commented Mar 11, 2021

Uh oh!

mpashchenkov commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mpashchenkov commented Mar 11, 2021 •

edited

Loading