The facial landmarks via IE sample is added for dnn module by OrestChura · Pull Request #15137 · opencv/opencv

OrestChura · 2019-07-24T17:17:39Z

This pullrequest changes

adds new sample that uses two models from IE: face detector (face-detection-adas-0001) and facial landmarks detector (facial-landmarks-35-adas-0002). The goal of the sample is to show how to inference IE models via dnn API. The sample detects described features on the source and then shows the result.
added IE build flag check in samples/dnn/CMakeLists.txt as the sample requires IE.

OrestChura · 2019-07-24T17:30:13Z

@dmatveev, @AsyaPronina, @andrey-golubev, @rgarnov, @dkurt Hello! Please, review my PR

AsyaPronina · 2019-07-25T17:13:22Z

samples/dnn/facial_landmarks_ie.cpp

+using namespace cv::dnn;
+
+#include <iostream>
+//#include "common.hpp"


Please remove

AsyaPronina · 2019-07-25T17:14:43Z

samples/dnn/facial_landmarks_ie.cpp

+     "{ b                    | |     The color to draw face rectangle and landmarks in format \"(b, g, r)\".}"
+     "{ g                    | |     The color to draw face rectangle and landmarks in format \"(b, g, r)\".}"
+     "{ r                    | |     The color to draw face rectangle and landmarks in format \"(b, g, r)\".}"
+     "{ faceframework        | |     Optional name of an origin framework of the facedetector. Detect it automatically if it does not set. }"


AsyaPronina · 2019-07-25T17:15:50Z

samples/dnn/facial_landmarks_ie.cpp

+                                         "3: VPU }"
+        );
+
+    parser.about("Use this script to run classification deep learning networks using OpenCV.");


About message need to be fixed

dkurt · 2019-07-25T18:54:34Z

samples/dnn/facial_landmarks_ie.cpp

+    //Models' definition & initialization
+    Net faceNet = readNet(facexmlPath, facebinPath);
+    faceNet.setPreferableTarget(faceTarget);
+    faceNet.setPreferableBackend(faceBackend);


There is only DNN_INFERENCE_ENGINE_BACKEND is supported fo IR models so you can emit faceBackend argument

Have I got it right that it has no sense even to ask the user about backends as there is only one of them available, so I can delete parser keys too?

dmatveev · 2019-07-25T20:53:11Z

@AsyaPronina I asked to make an MR in our internal Gitlab, not here. Not sure if it makes sense to start merging it right now in any form. Please make an integration branch in Gitlab and move the MRs there (open against that integration branch).

dmatveev · 2019-07-25T20:59:07Z

Still, if you think it is more useful and productive to continue here (and while a build is green), please continue here.

Not sure if any builder actually tests this code now, though (some custom builder configurations is required AFAIK).

- In addition to the Dmitry's issue, framework parser key is cut; - Work with color changed, choosing by user is cut; - Some fixes about Point() declarations; - Comment fixes

AsyaPronina · 2019-07-26T08:51:47Z

samples/dnn/facial_landmarks_ie.cpp

+    const unsigned int landmrows = 60;
+    const float bb_enlarge_coefficient = 1.0f;
+
+    const std::string win1 = "FaceLandmarkDetector";


win1 is not a good name in my opinion, why do you use 1?

Because there was another window some time ago.. My mistake, thank you

AsyaPronina · 2019-07-26T09:18:38Z

samples/dnn/facial_landmarks_ie.cpp

@@ -0,0 +1,195 @@
+// This file is part of OpenCV project.


Sorry for confuse but license header is not need in the samples folder

The strings 1-7 should be removed?

There is the license header in, for example, colorization sample..

AsyaPronina · 2019-07-26T09:21:29Z

samples/dnn/facial_landmarks_ie.cpp

+    }
+
+    //Parsing input arguments
+    const std::string facexmlPath = parser.get<std::string>("facestruct");


If you use Camel case for variable naming please follow standard rules for this:
CamelCase Notation
CamelCase (sometimes called "CamelBack") notation is the practice of writing compound words or phrases in which the parts are joined without spaces, with each part's initial letter capitalized and the first letter in either upper or lower case. For example: SampleDataTable, computeExpr

AsyaPronina · 2019-07-26T09:21:49Z

samples/dnn/facial_landmarks_ie.cpp

+    if (parser.has("input"))
+        cap.open(parser.get<String>("input"));
+    else if (!cap.open(0))
+            {


Please align braces

AsyaPronina · 2019-07-26T09:22:22Z

samples/dnn/facial_landmarks_ie.cpp

+           break;
+        }
+
+        Mat shwimg;         //Image to show


I suggest to stick to one naming style

AsyaPronina · 2019-07-26T09:22:31Z

samples/dnn/facial_landmarks_ie.cpp

+        Mat inputImg;
+        resize(img, inputImg, Size(facecols, facerows));
+
+        //Infering Facedetector


Face detector or FaceDetector

AsyaPronina · 2019-07-26T09:23:16Z

samples/dnn/facial_landmarks_ie.cpp

+        Mat outFaces = faceNet.forward();
+
+        float* faceData = (float*)(outFaces.data);
+        for (size_t i = 0; i < outFaces.total(); i += faceObjectSize)


Very minor suggestion: if you use size_t it is better to use 0ul

AsyaPronina · 2019-07-26T09:27:16Z

samples/dnn/facial_landmarks_ie.cpp

+                int top = int(faceData[i + 4] * img.rows);
+                top = std::max(top, 0);
+                int right  = int(faceData[i + 5] * img.cols);
+                right = std::min(right, img.cols-2);


Please stick to the one convention about using or not the spaces between operators and operands

AsyaPronina · 2019-07-26T09:27:37Z

samples/dnn/facial_landmarks_ie.cpp

+
+                //Postprocessing for landmarks
+                int max_of_sizes = std::max(width, height);
+                int add_width = int(max_of_sizes*bb_enlarge_coefficient) - width;


max_of_sizes and bb_enlarge_coefficient namings differ from Camel Case code convention you use in other code

AsyaPronina · 2019-07-26T09:29:29Z

@AsyaPronina I asked to make an MR in our internal Gitlab, not here. Not sure if it makes sense to start merging it right now in any form. Please make an integration branch in Gitlab and move the MRs there (open against that integration branch).

Okay will sort out with this today

bhack · 2019-07-26T11:26:26Z

IMHO it is better to re-edit this with High Level API #14780
@dvd42 is going to extend high level API proposal also for landmarks quite soon with a new PR.

dmatveev · 2019-07-26T13:35:55Z

IMHO it is better to re-edit this with High Level API #14780
@dvd42 is going to extend high level API proposal also for landmarks quite soon with a new PR.

@bhack thanks for the pointer, but the idea of this is to trial #15090

bhack · 2019-07-26T14:52:44Z

So what it means? Why high level API is not using G-API?

dmatveev · 2019-07-26T15:57:00Z

So what it means? Why high level API is not using G-API?

To be fair, I am not sure what the "high-level API" is. G-API has its own programming model so we're adding inference there now, and this one should be among the early samples.

dkurt · 2019-07-26T16:05:55Z

@bhack, @dmatveev, It's just an abstraction over dnn::Net which lets to emit some preprocessing or postprocessing stuff. dnn::Model could as be as a node of G-API as use G-API internally so there is no conflict between two approaches.

dmatveev · 2019-07-26T16:12:35Z

@dkurt thanks! Actually, efficient pipelining of preprocessing, inference, postprocessing, and other stuff is the reason why we add inference & streaming extensions in G-API. So what's the point and value-add of High-level API?

@bhack meanwhile, if you're interested, here's some sample code and some more general info on G-API.

dkurt · 2019-07-26T16:15:23Z

@dmatveev, Due with this high-level API user do not use blobFromImage, we can use G-API as an optional dependency. It'll be transparent for user.

- Some small issues

dvd42 · 2019-07-30T10:49:42Z

I created 1 PR for the DNN high_level api, adding a keypoints module. Maybe this chain of detection->landmarking could be implemented as part of said module by chaining the detection and keypointing module?

OrestChura · 2019-07-30T16:19:00Z

I created 1 PR for the DNN high_level api, adding a keypoints module. Maybe this chain of detection->landmarking could be implemented as part of said module by chaining the detection and keypointing module?

Thanks a lot! Have seen your PR, it looks reasonable to integrate our sample into your module, but is there any sense to add the InferenceEngine dependence into it? In my opinion, the sample is more intented to reflect the IE models usage in DNN API.

dvd42 · 2019-07-30T16:24:19Z

It seems that this won't be done yet as per dkurts's comment.

OrestChura · 2019-08-29T10:16:45Z

The PR is closed as a similar sample exists already, so there's no need in this sample.

OrestChura added 2 commits July 24, 2019 19:02

The facial landmarks via IE sample is added for dnn module

a564585

Merge branch 'master' into my_sample

ca9e4ac

Fixes whitespaces

b7114a0

AsyaPronina requested review from AsyaPronina, andrey-golubev, dkurt, mshabunin and rgarnov July 25, 2019 17:09

AsyaPronina reviewed Jul 25, 2019

View reviewed changes

dkurt reviewed Jul 25, 2019

View reviewed changes

AsyaPronina requested a review from dmatveev July 25, 2019 20:57

- Fixes issues mentioned by @AsyaPronina and @dkurt, thanks!

ff72d60

- In addition to the Dmitry's issue, framework parser key is cut; - Work with color changed, choosing by user is cut; - Some fixes about Point() declarations; - Comment fixes

AsyaPronina reviewed Jul 26, 2019

View reviewed changes

- Variables names refactored

71af829

- Some small issues

dvd42 mentioned this pull request Jul 30, 2019

Add high level API #14780

Merged

OrestChura closed this Aug 29, 2019

OrestChura deleted the my_sample branch August 29, 2019 10:17

Uh oh!

Conversation

OrestChura commented Jul 24, 2019

This pullrequest changes

Uh oh!

OrestChura commented Jul 24, 2019

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 commented Jul 25, 2019

Uh oh!

dmatveev commented Jul 25, 2019

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!

AsyaPronina Jul 26, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AsyaPronina commented Jul 26, 2019

Uh oh!

bhack commented Jul 26, 2019

Uh oh!

dmatveev commented Jul 26, 2019

Uh oh!

bhack commented Jul 26, 2019

Uh oh!

dmatveev commented Jul 26, 2019

Uh oh!

dkurt commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmatveev commented Jul 26, 2019

Uh oh!

dkurt commented Jul 26, 2019

Uh oh!

dvd42 commented Jul 30, 2019

Uh oh!

OrestChura commented Jul 30, 2019

Uh oh!

dvd42 commented Jul 30, 2019

Uh oh!

OrestChura commented Aug 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AsyaPronina Jul 26, 2019 •

edited

Loading

dkurt commented Jul 26, 2019 •

edited

Loading