Skip to content

Conversation

fpertl
Copy link
Contributor

@fpertl fpertl commented May 11, 2020

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 44 files at r1.
Reviewable status: 2 of 44 files reviewed, 3 unresolved discussions (waiting on @fpertl)


cpp/OcvSsdFaceDetection/LICENSE, line 17 at r1 (raw file):

 ******************************************************************************/

 This project contains content developed by The MITRE Corporation. If this code

As previously discussed:

When your component is distributed or executed with opencv_face_detector_uint8.pb then we are not sure what the usage restrictions are. There's nothing we can do but make the users aware of this in our LICENSE file and README by linking them to the opencv_face_detector_uint8.pb file directly in the OpenCV repo. We need a similar disclaimer like the one we have in the OALPR license file. In your case, it would be something like

When this software is packaged and/or distributed with the opencv_face_detector_uint8.pb model (link), the resulting product inherits the license and usage restrictions of that model, which are not explicitly stated by the original author. Please contact the author before using this component for commercial purposes.

Note that in the Dockerfile for your component you should state org.label-schema.license=Mixed.


cpp/OcvSsdFaceDetection/README.md, line 1 at r1 (raw file):

# Overview

Add a Trained Models section where you describe the opencv_face_detector_uint8.pb model, similar to that in the OcvDnnDetection component's README.md.


Where do these files come from?

  string  sp_model_path = plugin_path + "/data/shape_predictor_5_face_landmarks.dat";
  string  tr_model_path = plugin_path + "/data/nn4.small2.v1.t7";

Should we list them in Trained Models too?


cpp/OcvSsdFaceDetection/.devcontainer/Dockerfile, line 79 at r1 (raw file):

RUN ln -s /usr/bin/cmake3 /usr/bin/cmake;  

LABEL org.label-schema.license="Apache 2.0" \

Please use Mixed as the license here.

Copy link
Contributor Author

@fpertl fpertl left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 44 files reviewed, 3 unresolved discussions (waiting on @jrobble)


cpp/OcvSsdFaceDetection/LICENSE, line 17 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

As previously discussed:

When your component is distributed or executed with opencv_face_detector_uint8.pb then we are not sure what the usage restrictions are. There's nothing we can do but make the users aware of this in our LICENSE file and README by linking them to the opencv_face_detector_uint8.pb file directly in the OpenCV repo. We need a similar disclaimer like the one we have in the OALPR license file. In your case, it would be something like

When this software is packaged and/or distributed with the opencv_face_detector_uint8.pb model (link), the resulting product inherits the license and usage restrictions of that model, which are not explicitly stated by the original author. Please contact the author before using this component for commercial purposes.

Note that in the Dockerfile for your component you should state org.label-schema.license=Mixed.

Done.


cpp/OcvSsdFaceDetection/README.md, line 1 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Add a Trained Models section where you describe the opencv_face_detector_uint8.pb model, similar to that in the OcvDnnDetection component's README.md.


Where do these files come from?

  string  sp_model_path = plugin_path + "/data/shape_predictor_5_face_landmarks.dat";
  string  tr_model_path = plugin_path + "/data/nn4.small2.v1.t7";

Should we list them in Trained Models too?

These files should have been listed in the plugin-files/data/NOTICE. I also added the requested section in the README.


cpp/OcvSsdFaceDetection/.devcontainer/Dockerfile, line 79 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Please use Mixed as the license here.

Done.

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 4 files at r2.
Reviewable status: 4 of 44 files reviewed, 4 unresolved discussions (waiting on @fpertl)


cpp/OcvSsdFaceDetection/README.md, line 20 at r2 (raw file):

<br>
* A Minimum Output Sum of Squared Error (MOSSE) tracker implemented in OpenCV's tracking API based on [Bolme et al.](https://ieeexplore.ieee.org/document/5539960), *"Visual object tracking using adaptive correlation filters."*, 2010 
* A linear assignment cost solver, a DLIB implementation of the Hungarian algorithm (also know as the Kuhn-Munkres algorithm) to solve detection to track assignment. See [Kuhn](https://en.wikipedia.org/wiki/Naval_Research_Logistics_Quarterly), *"The Hungarian Method for the assignment problem"*, 1955 for original method.

Is this link correct? I would expect the link to be called "Naval Research Logistics Quarterly" instead of "Kuhn" based on where it took me on Wikipedia.

Overall though, 👍 on the content here.


cpp/OcvSsdFaceDetection/README.md, line 23 at r2 (raw file):

## Tracking implementation
Detected faces in videos are aggregated into tracks as frames are processed. This is done using multiple stages and linear assignment cost solver:  While processing each frame, tracks that have not had any detections of some number of frames are terminated, then detections are assigned to the remaing tracks using Intersection Over Union (IoU) as a cost, any unassigned detections are attempted to be assigned to tracks using the cos distance with the OpenFace features. Any tracks that did not receive a detection are then continued using a correlation tracker prediction.<br>

If you're expecting a blank line here, you need to use <br><br> to have the desired effect; however, it's more common to just space the paragraphs out directly in the Markdown and add your own blank line between them.


cpp/OcvSsdFaceDetection/plugin-files/data/NOTICE, line 12 at r2 (raw file):

Single shot multi box face detector for 300x300 images 
derived from res10_300x300_ssd_iter_140000_fp16.caffemodel by quantization and conversion to tensorflow 
downloaded form https://github.com/opencv/opencv_3rdparty/raw/8033c2bc31b3256f0d461c919ecc01c2428ca03b/opencv_face_detector_uint8.pb

Typo: form. I think you want from.


cpp/OcvSsdFaceDetection/plugin-files/data/NOTICE, line 14 at r2 (raw file):

downloaded form https://github.com/opencv/opencv_3rdparty/raw/8033c2bc31b3256f0d461c919ecc01c2428ca03b/opencv_face_detector_uint8.pb
and https://github.com/opencv/opencv_extra/tree/master/testdata/dnn/opencv_face_detector.pbtxt
The training data used to create this model is not specified, the procedure use is detailed here

I think you want procedure used.

Copy link
Contributor Author

@fpertl fpertl left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 44 files reviewed, 4 unresolved discussions (waiting on @jrobble)


cpp/OcvSsdFaceDetection/README.md, line 20 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Is this link correct? I would expect the link to be called "Naval Research Logistics Quarterly" instead of "Kuhn" based on where it took me on Wikipedia.

Overall though, 👍 on the content here.

replaced link


cpp/OcvSsdFaceDetection/README.md, line 23 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

If you're expecting a blank line here, you need to use <br><br> to have the desired effect; however, it's more common to just space the paragraphs out directly in the Markdown and add your own blank line between them.

removed
in favor of spaces


cpp/OcvSsdFaceDetection/plugin-files/data/NOTICE, line 12 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Typo: form. I think you want from.

Done.


cpp/OcvSsdFaceDetection/plugin-files/data/NOTICE, line 14 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

I think you want procedure used.

Done.

@jrobble
Copy link
Member

jrobble commented May 18, 2020

Assigning @hhuangMITRE to do an initial round of review.

Copy link
Contributor

@hhuangMITRE hhuangMITRE left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 44 files reviewed, 61 unresolved discussions (waiting on @fpertl, @hhuangMITRE, and @jrobble)


cpp/OcvSsdFaceDetection/CMakeLists.txt, line 32 at r3 (raw file):

set(CMAKE_CXX_STANDARD 11)
#set(CMAKE_BUILD_TYPE RelWithDebInfo)

This line was commented out.
Is it something users may find they should enable?
If so, add a comment here. Otherwise, this should be removed before we push to develop, thanks!


cpp/OcvSsdFaceDetection/DetectionLocation.h, line 64 at r3 (raw file):

      const size_t            frameIdx;               ///< frame index frame where detection is located (for videos)

      static bool Init(log4cxx::LoggerPtr log, const string plugin_path);    ///< setup class shared memebers

Likely typo: memebers -> members


cpp/OcvSsdFaceDetection/DetectionLocation.h, line 82 at r3 (raw file):

    private:

      static log4cxx::LoggerPtr                _log;                ///< shared log opbject

Typo opbject->object


cpp/OcvSsdFaceDetection/DetectionLocation.h, line 99 at r3 (raw file):

  };
 
  /** **************************************************************************

The comment blocks look great, but for some reason, all of the comment blocks written here and across other cpp files appear to all have a random space after the second asterick (\** *...).

At the end of the comment block, another space seems to be randomly added right before the last asterisk (** *\).

I didn't notice this until I saw it near the end of DetectionLocation.cpp (then suddenly it was kind of noticeable). I'm not sure if this was intentional or following some C++ style so I'm leaving this as an info comment.


cpp/OcvSsdFaceDetection/DetectionLocation.h, line 105 at r3 (raw file):

  ostream& operator<< (ostream& out, const DetectionLocation& d) {
    out  << "[" << (MPFImageLocation)d 
              // << " L:" << d.landmarks << " F["    

Is this line no longer needed or would users like to view d.landmarks occasionally for debug purposes?
If that's the case, we can include a comment, or maybe even a toggle setting.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 90 at r3 (raw file):

/** **************************************************************************
* Compute 1 - Intersection Over Union metric for two detections comprised of 
* 1 - the ratio of the area of the intersection of the detection rectangels

Typo: rectangels->rectangles


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 103 at r3 (raw file):

	int lry = min(y_left_upper + height, d.y_left_upper + d.height);

	float inter_area = max(0, lrx - ulx) * max(0, lry - uly);	                   

There's some trailing spaces and switching between tabs and double spaces for indents in this cpp file.
It's somewhat visible in Reviewable but I was able to spot it in my editor (CLion) more easily.

The trailing spaces should be removed, and tabs in this function should be swapped to spaces for consistency.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 113 at r3 (raw file):

*
* \param   d second detection 
* \returns   absolute difference in frame indecies

Typo indecies -> indices


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 125 at r3 (raw file):

/** **************************************************************************
* Compute euclidian center to distance center distance from normalized centers

Typo euclidian->euclidean


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 197 at r3 (raw file):

    const cv::Size THUMB_SIZE(THUMBNAIL_SIZE_X,THUMBNAIL_SIZE_Y);
 
    // Landmark indecies for OpenFace nn4.v2, nn4.small1.v1, nn4.small2.v1

Typo indecies -> indices


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 226 at r3 (raw file):

/** **************************************************************************
* Lazy accessor method to get/compute feature fector based on thumbnail 

Typo fector->vector


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 243 at r3 (raw file):

      const cv::Scalar meanVal(0.0, 0.0, 0.0);  // BGR mean pixel color 
      cv::Mat inputBlob = cv::dnn::blobFromImage(getThumbnail(), // BGR image
                                                inScaleFactor,   // no pixel value scaline (e.g. 1.0/255.0)

Possible typo: scaline -> scaling


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 262 at r3 (raw file):

* Detect objects using SSD DNN opencv face detector network   
* 
* \param  cfg job configuration setting including iamge frame

Both image frame and requirements appear to have typos.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 278 at r3 (raw file):

  cv::Mat inputBlob = cv::dnn::blobFromImage(cfg.bgrFrame,  // BGR image
                                            inScaleFactor, // no pixel value scaline (e.g. 1.0/255.0)

Possible typo: scaline -> scaling


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 317 at r3 (raw file):

* \param plugin_path plugin directory so configs and models can be loaded
*
* \return true if eveything was properly initialized, flase otherwise

Typo for everything and false


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 365 at r3 (raw file):

  if(_trackerPtr.empty()){   // initialize a new tracker if we don't have one already
    //_trackerPtr = cv::TrackerKCF::create(); 

Do we still occasionally need to initialize the tracker via cv::TrackerKCF::create() instead of cv::TrackerMOSSE::create()?
If so, we may need a toggle setting/job parameter for controlling job behavior.
If only for debug purposes, a comment is needed.
Otherwise, the line should be removed.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 385 at r3 (raw file):

      detPtr->_trackerStartFrameIdx = _trackerStartFrameIdx;
      _trackerPtr.release();
      detPtr->_feature = getFeature();  // clone feature of prior detection                

Trailing spaces after this comment.


cpp/OcvSsdFaceDetection/Dockerfile, line 26 at r3 (raw file):

# limitations under the License.                                            #
#############################################################################

Note to self: double check docker build.


cpp/OcvSsdFaceDetection/JobConfig.h, line 45 at r3 (raw file):

  /** ****************************************************************************
  *   get MPF properies of various types

Typo: properies -> properties


cpp/OcvSsdFaceDetection/JobConfig.cpp, line 74 at r3 (raw file):

  lastError(MPF_DETECTION_SUCCESS),
  _imreaderPtr(unique_ptr<MPFImageReader>()),
  _videocapPtr(unique_ptr<MPFVideoCapture>()){}                    

Trailing whitespace.


cpp/OcvSsdFaceDetection/JobConfig.cpp, line 83 at r3 (raw file):

                                                                               LOG4CXX_DEBUG(_log, "[" << job.job_name << "Data URI = " << job.data_uri);
  _parse(job);
  

Trailing whitespace.


cpp/OcvSsdFaceDetection/JobConfig.cpp, line 98 at r3 (raw file):

/* **************************************************************************
*  Constructor that parses parameters from MPFVideoJob and initializes 
*  video caputure / reader

Typo caputure -> capture


cpp/OcvSsdFaceDetection/JobConfig.cpp, line 115 at r3 (raw file):

    cv::Size fs = _videocapPtr->GetFrameSize();
    float diag  = sqrt(fs.width*fs.width + fs.height*fs.height); 
    widthOdiag  = fs.width  / diag; 

Appears to be a single trailing whitespace after float diag = sqrt(fs.width*fs.width + fs.height*fs.height); and widthOdiag = fs.width / diag;. CLion and other editors can help autoclear this since they are hard to spot outside of Reviewable.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.h, line 49 at r3 (raw file):

    public:
      bool Init() override;      

Trailing whitespace.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.h, line 61 at r3 (raw file):

      template<DetectionLocationCostFunc COST_FUNC>
      vector<long> _calcAssignemntVector(const TrackPtrList            &tracks,

Should this be renamed: _calcAssignemntVector-> _calcAssignmentVector?
The Assignemnt keyword in the middle seems to a typo of Assignment.
The corresponding function in .cpp would need to be corrected too.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.code-workspace, line 1 at r3 (raw file):

{

I vaguely remember this file format, so I double checked online.
This appears to be a workspace settings file for reviewing code under Microsoft Visual Studio Code editor.

Is it required for the component outside of code development? If not, we'll need to remove this file from the repo with git rm


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 58 at r3 (raw file):

 *  print out opencv matrix on a single line
 * 
 * \param   m matric to serialize to single line string

Typo: matric -> matrix


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 73 at r3 (raw file):

 *  print out dlib matrix on a single line
 * 
 * \param   m matric to serialize to single line string

Typo: matric -> matrix


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 106 at r3 (raw file):

    log4cxx::xml::DOMConfigurator::configure(config_path + "/Log4cxxConfig.xml");
    _log = log4cxx::Logger::getLogger("OcvSsdFaceDetection");                  LOG4CXX_DEBUG(_log,"Initializing OcvSSDFaceDetector");
    JobConfig::_log = _log;                                                  //LOG4CXX_TRACE(_log, cv::getBuildInformation() << std::endl);

If this logger message is no longer needed, the commented out line should be removed.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 115 at r3 (raw file):

    }                                                                          LOG4CXX_TRACE(_log,"read config file:" << config_params_path);
    for(auto p = params.begin(); p != params.end(); p++){
      const string key = p.key().toStdString();                                   

There is a lot of trailing whitespace in this file. I recommend using an editor to clean up all excess whitespace.

Also leaving behind comments for whitespace that are harder to spot or might be missed by editor.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 117 at r3 (raw file):

      const string key = p.key().toStdString();                                   
      const string val = p.value().toStdString();                              LOG4CXX_TRACE(_log,"Config    Vars:" << key << "=" << val);
      const char* env_val = getenv(key.c_str());                               LOG4CXX_TRACE(_log,"Veryfying ENVs:" << key << "=" << env_val);

Typo:"Veryfying -> "Verifying


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 148 at r3 (raw file):

* \param TrackPtrList  list of existing tracks to consider for assignment
* \param detections    vector of detections that need assigned to tracks
* \param maxCost       maximum assignment cost, if exeeded the particular 

Typo: exeeded -> exceeded


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 149 at r3 (raw file):

* \param detections    vector of detections that need assigned to tracks
* \param maxCost       maximum assignment cost, if exeeded the particular 
*                      detection to track assignemnt will be removed from result

Typo: assignemnt -> assignment


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 152 at r3 (raw file):

* \returns assignment vector am[track,detection] with dim (# tracks x # detections)
*          if am[x,y]==0 then detection[y] should be assigned to track[x]
* 

Single trailing whitespace after this comment asterisk.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 155 at r3 (raw file):

_calcAssignemntVector

Has a typo in the middle: Assignemnt -> Assignment


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 159 at r3 (raw file):

                                                         const float                    maxCost = INT_MAX){
  vector<long> av; //assignment vector to return
  

Trailing whitespace.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 213 at r3 (raw file):

    if(assignmentVector[t] != -1){                                              LOG4CXX_TRACE(_log,"assigning det: " << "f" << detections[assignmentVector[t]]->frameIdx << " " <<  ((MPFImageLocation)*(detections[assignmentVector[t]])) << "to track " << *track);
      track->back()->releaseBGRFrame();                                        // no longer need previous frame data.
      track->push_back(move(detections[assignmentVector[t]]));              

Trailing whitespace.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 245 at r3 (raw file):

    for(auto &det:detections){
      MPFImageLocation loc = *det;
      det.reset();                                                             // release frame object                                                      

Trailing whitespace in this comment.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 268 at r3 (raw file):

*
* \param[in,out]  tracks  Tracks collection to which detections will be added
*                         

Trailing whitespace in this asterisk.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 270 at r3 (raw file):

*                         
* \returns MPFVideoTrack object resulting from conversion
* 

Extra space after this asterisk.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 278 at r3 (raw file):

  MPFVideoTrack mpf_track;
  

Trailing whitespace before and after these two lines of code.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 311 at r3 (raw file):

                                                     MPFVideoTrackVec  &mpf_tracks) {

  try{ 

Extra trailing whitespace right after try{


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 323 at r3 (raw file):

      // remove any tracks too far in the past
      trackPtrs.remove_if([&](unique_ptr<Track>& tPtr){                      
        if(cfg.frameIdx - tPtr->back()->frameIdx > cfg.maxFrameGap){           LOG4CXX_TRACE(_log,"droping old track: " << *tPtr);

Typo: droping -> dropping


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 338 at r3 (raw file):

            // intersection over union tracking and assignment
            vector<long> av = _calcAssignemntVector<&DetectionLocation::iouDist>(trackPtrs,detections,cfg.maxIOUDist);
            _assignDetections2Tracks(trackPtrs, detections, av);               LOG4CXX_TRACE(_log,"IOU assignment complete"); 

Extra trailing whitespace after the semicolon.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 346 at r3 (raw file):

            }

            // center-to-center distance tracking and assignemnt

Typo: assignemnt -> assignment


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 403 at r3 (raw file):

/* **************************************************************************
*  Perform histogram equalization 
*************************************************************************** */ 

There's an extra trailing whitespace on this line and the line above it.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 413 at r3 (raw file):

  
  cv::equalizeHist(hsvComponents[2],hsvComponents[2]);
  //_equalizerPtr->apply(hsvComponents[2],hsvComponents[2]); // local histogram equalization

I assume this line has been commented out as it's no longer needed. If so, it should be removed before the PR is complete, thanks!


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 420 at r3 (raw file):

/* **************************************************************************
*  Perform image normalization 
*************************************************************************** */ 

Extra trailing whitespace after /


cpp/OcvSsdFaceDetection/README.md, line 6 at r3 (raw file):

the OpenCV Single Shot Detector.

   

Trailing whitespace in the above line.
It's hard to spot even in the editor so I thought I'd leave a notice here.


cpp/OcvSsdFaceDetection/README.md, line 12 at r3 (raw file):

The OpenCV SSD face detection component comes with three trained models:

* A discretized tensorflow version of OpenCV's Single Shot Detector (SSD) for faces, [opencv_face_detector_uint8.pbtxt](https://github.com/opencv/opencv_extra/tree/master/testdata/dnn/opencv_face_detector.pbtxt) and [opencv_face_detector_uint8.pb](https://github.com/opencv/opencv_3rdparty/raw/8033c2bc31b3256f0d461c919ecc01c2428ca03b/opencv_face_detector_uint8.pb). This model was discretized from an existing caffee model, [opencv_face_detector.caffemodel](https://github.com/opencv/opencv_3rdparty/raw/dnn_samples_face_detector_20170830/res10_300x300_ssd_iter_140000.caffemodel). More details about how this model was trained can be found [here](https://github.com/opencv/opencv/blob/3.4.3/samples/dnn/face_detector/how_to_train_face_detector.txt). Details of the SSD model architecture are presented in [Liu et al.](https://arxiv.org/abs/1512.02325), *"SSD: Single Shot MultiBox Detector"*, 2016 

Should This model was discretized from an existing caffee model be This model was discretized from an existing Caffe model?

I think caffee is a typo of Caffe, the deep learning framework.


cpp/OcvSsdFaceDetection/README.md, line 14 at r3 (raw file):

This model was tranied with

Typo: tranied with -> trained with


cpp/OcvSsdFaceDetection/README.md, line 23 at r3 (raw file):

assigned to the remaing tracks

Typo: remaing tracks -> remaining tracks


cpp/OcvSsdFaceDetection/README.md, line 25 at r3 (raw file):

they can be perforemd only

Typo: perforemd -> performed


cpp/OcvSsdFaceDetection/README.md, line 25 at r3 (raw file):

and more track fragmantation.

Typo: fragmantation -> fragmentation


cpp/OcvSsdFaceDetection/types.h, line 41 at r3 (raw file):

  typedef vector<MPFVideoTrack>    MPFVideoTrackVec;     ///< vector of MPFVideoTracks
  typedef vector<MPFImageLocation> MPFImageLocationVec;  ///< vector of MPFImageLocations
  typedef vector<cv::Mat>          cvMatVec;             ///< vector of OpenCV matricie/images

Typo: OpenCV matricie/images -> OpenCV matrices/images


cpp/OcvSsdFaceDetection/types.h, line 43 at r3 (raw file):

  typedef vector<cv::Mat>          cvMatVec;             ///< vector of OpenCV matricie/images
  typedef vector<cv::Rect>         cvRectVec;            ///< vector of OpenCV rectangles
  typedef vector<cv::Point>        cvPointVec;           ///< vector of OpenCV points     

Trailing whitespace.


cpp/OcvSsdFaceDetection/types.h, line 55 at r3 (raw file):

//<< l.detection_properties.at("LANDMARKS")

Is this an optional line for debug purposes? I wonder if users might want to view LANDMARKS detection outputs too.


cpp/OcvSsdFaceDetection/.devcontainer/Dockerfile, line 28 at r3 (raw file):

from openmpf/openmpf_cpp_component_build

COPY fsroot/ /

Is everything under.devcontainer for development purposes only?

I see that the contents from the local fsroot/ directory are copied over and assume that its only present in the developer's directory as it's not included with the rest of this component nor in my CentOS os. Online search suggests its inherent in macOS.

If it's for development purposes, @jrobble should we keep this here for reference or remove it from the repo?

Everything under .devcontainer appears to be designed to work with a specific OS and environment (included VS Code condig json too). Since the regular Dockerfile included with the component should work with the OpenMPF docker deployment, I assume we can just keep this directory out of the repo.


cpp/OcvSsdFaceDetection/.devcontainer/devcontainer.json, line 4 at r3 (raw file):

    "name": "OcvSsdFaceDetection",
  
    // The order of the files is important since later files override previous ones

CLion throws an error and says comments are not allowed in JSON standard. Since this appears to be a config file for VS Code development or another development service, I assumed this other format didn't mind the comments.

@jrobble Also not sure if we should include developer-specific config files here if that's the case.
@fpertl Let me know if any .devcontainer files are needed with the component itself or if these files are optional, thanks!

Copy link
Contributor Author

@fpertl fpertl left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 44 files reviewed, 60 unresolved discussions (waiting on @fpertl, @hhuangMITRE, and @jrobble)


cpp/OcvSsdFaceDetection/CMakeLists.txt, line 32 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

This line was commented out.
Is it something users may find they should enable?
If so, add a comment here. Otherwise, this should be removed before we push to develop, thanks!

uncommented RelWithDevInfo, which builds release version of the code but adds debugging symbols, and commented Debug build type with added comments. Not sure why this is an issue?


cpp/OcvSsdFaceDetection/DetectionLocation.h, line 64 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Likely typo: memebers -> members

Done.


cpp/OcvSsdFaceDetection/DetectionLocation.h, line 82 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo opbject->object

Done.


cpp/OcvSsdFaceDetection/DetectionLocation.h, line 99 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

The comment blocks look great, but for some reason, all of the comment blocks written here and across other cpp files appear to all have a random space after the second asterick (\** *...).

At the end of the comment block, another space seems to be randomly added right before the last asterisk (** *\).

I didn't notice this until I saw it near the end of DetectionLocation.cpp (then suddenly it was kind of noticeable). I'm not sure if this was intentional or following some C++ style so I'm leaving this as an info comment.

This is to support auto generation of documents by Doxygen, a popular doc generator.


cpp/OcvSsdFaceDetection/DetectionLocation.h, line 105 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Is this line no longer needed or would users like to view d.landmarks occasionally for debug purposes?
If that's the case, we can include a comment, or maybe even a toggle setting.

removed, if they want it they can type it back in.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 90 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo: rectangels->rectangles

Done.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 103 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

There's some trailing spaces and switching between tabs and double spaces for indents in this cpp file.
It's somewhat visible in Reviewable but I was able to spot it in my editor (CLion) more easily.

The trailing spaces should be removed, and tabs in this function should be swapped to spaces for consistency.

Done.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 113 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo indecies -> indices

Done.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 125 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo euclidian->euclidean

Done.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 197 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo indecies -> indices

Done.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 226 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo fector->vector

Done.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 243 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Possible typo: scaline -> scaling

Done.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 262 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Both image frame and requirements appear to have typos.

Done.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 278 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Possible typo: scaline -> scaling

Done.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 317 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo for everything and false

Done.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 365 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Do we still occasionally need to initialize the tracker via cv::TrackerKCF::create() instead of cv::TrackerMOSSE::create()?
If so, we may need a toggle setting/job parameter for controlling job behavior.
If only for debug purposes, a comment is needed.
Otherwise, the line should be removed.

moved alternative KCF tracker create() to comment for MOSSE tracker


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 385 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing spaces after this comment.

Done.


cpp/OcvSsdFaceDetection/JobConfig.h, line 45 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo: properies -> properties

Done.


cpp/OcvSsdFaceDetection/JobConfig.cpp, line 74 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing whitespace.

Done.


cpp/OcvSsdFaceDetection/JobConfig.cpp, line 83 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing whitespace.

Done.


cpp/OcvSsdFaceDetection/JobConfig.cpp, line 98 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo caputure -> capture

Done.


cpp/OcvSsdFaceDetection/JobConfig.cpp, line 115 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Appears to be a single trailing whitespace after float diag = sqrt(fs.width*fs.width + fs.height*fs.height); and widthOdiag = fs.width / diag;. CLion and other editors can help autoclear this since they are hard to spot outside of Reviewable.

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.h, line 49 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing whitespace.

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.h, line 61 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Should this be renamed: _calcAssignemntVector-> _calcAssignmentVector?
The Assignemnt keyword in the middle seems to a typo of Assignment.
The corresponding function in .cpp would need to be corrected too.

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.code-workspace, line 1 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

I vaguely remember this file format, so I double checked online.
This appears to be a workspace settings file for reviewing code under Microsoft Visual Studio Code editor.

Is it required for the component outside of code development? If not, we'll need to remove this file from the repo with git rm

I discussed this with @jrobble, and was going to leave the dev environment stuff in there for using vscode, see bottom of README.md. Leaving the settings for the open-source development tool from the same company that now owns github.com in there may help others to get up developing quickly and should not harm anything otherwise. I'm happy to remove it and the associated directories .devcontainer .vscode if there is a good reason.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 58 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo: matric -> matrix

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 73 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo: matric -> matrix

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 106 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

If this logger message is no longer needed, the commented out line should be removed.

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 115 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

There is a lot of trailing whitespace in this file. I recommend using an editor to clean up all excess whitespace.

Also leaving behind comments for whitespace that are harder to spot or might be missed by editor.

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 117 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo:"Veryfying -> "Verifying

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 148 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo: exeeded -> exceeded

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 149 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo: assignemnt -> assignment

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 152 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Single trailing whitespace after this comment asterisk.

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 155 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…
_calcAssignemntVector

Has a typo in the middle: Assignemnt -> Assignment

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 213 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing whitespace.

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 245 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing whitespace in this comment.

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 268 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing whitespace in this asterisk.

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 270 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Extra space after this asterisk.

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 278 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing whitespace before and after these two lines of code.

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 311 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Extra trailing whitespace right after try{

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 323 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo: droping -> dropping

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 338 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Extra trailing whitespace after the semicolon.

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 346 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo: assignemnt -> assignment

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 413 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

I assume this line has been commented out as it's no longer needed. If so, it should be removed before the PR is complete, thanks!

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 420 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Extra trailing whitespace after /

Done.


cpp/OcvSsdFaceDetection/README.md, line 20 at r2 (raw file):

Previously, fpertl wrote…

replaced link

Done.


cpp/OcvSsdFaceDetection/README.md, line 6 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing whitespace in the above line.
It's hard to spot even in the editor so I thought I'd leave a notice here.

Done.


cpp/OcvSsdFaceDetection/README.md, line 12 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Should This model was discretized from an existing caffee model be This model was discretized from an existing Caffe model?

I think caffee is a typo of Caffe, the deep learning framework.

Done.


cpp/OcvSsdFaceDetection/README.md, line 14 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…
This model was tranied with

Typo: tranied with -> trained with

Done.


cpp/OcvSsdFaceDetection/README.md, line 23 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…
assigned to the remaing tracks

Typo: remaing tracks -> remaining tracks

Done.


cpp/OcvSsdFaceDetection/README.md, line 25 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…
they can be perforemd only

Typo: perforemd -> performed

Done.


cpp/OcvSsdFaceDetection/README.md, line 25 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…
and more track fragmantation.

Typo: fragmantation -> fragmentation

Done.


cpp/OcvSsdFaceDetection/types.h, line 41 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo: OpenCV matricie/images -> OpenCV matrices/images

Done.


cpp/OcvSsdFaceDetection/types.h, line 43 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing whitespace.

Done.


cpp/OcvSsdFaceDetection/types.h, line 55 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…
//<< l.detection_properties.at("LANDMARKS")

Is this an optional line for debug purposes? I wonder if users might want to view LANDMARKS detection outputs too.

removed comment


cpp/OcvSsdFaceDetection/.devcontainer/Dockerfile, line 28 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Is everything under.devcontainer for development purposes only?

I see that the contents from the local fsroot/ directory are copied over and assume that its only present in the developer's directory as it's not included with the rest of this component nor in my CentOS os. Online search suggests its inherent in macOS.

If it's for development purposes, @jrobble should we keep this here for reference or remove it from the repo?

Everything under .devcontainer appears to be designed to work with a specific OS and environment (included VS Code condig json too). Since the regular Dockerfile included with the component should work with the OpenMPF docker deployment, I assume we can just keep this directory out of the repo.

The fsroot directory is just an optional local cache for the opencv and dlib install code used further down in the docker file. You can stash those installs in fsroot\tmp\ and the somewhat time consuming downloads will be skipped at build. I only reinstalled opencv since the openMPF component image was not including the contrib modules at the time, or being compiled for performance using linear algebra acceleration library (s.a openBLAS).

All this is for use from inside the CentOS component-builder used for development of openMPF components (i.e. openmpf/openmpf_cpp_component_build) . There is nothing specific to the host os running this container. Vscode, which is cross-platform, launches an instance of openmpf/openmpf_cpp_component_build` with the extras installed and you then get to develop with a nice IDE directly inside the openMPF build container see. I thought this was a nice cross-platform way to setup an open source IDE that would allow developers to work/develop from inside the openMPF build container, since we don't really have a "development" container image. If we don't like it, it is easy to remove.


cpp/OcvSsdFaceDetection/.devcontainer/devcontainer.json, line 4 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

CLion throws an error and says comments are not allowed in JSON standard. Since this appears to be a config file for VS Code development or another development service, I assumed this other format didn't mind the comments.

@jrobble Also not sure if we should include developer-specific config files here if that's the case.
@fpertl Let me know if any .devcontainer files are needed with the component itself or if these files are optional, thanks!

completely optional, but potentially useful to other developers.

Copy link
Contributor

@hhuangMITRE hhuangMITRE left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 44 files reviewed, 50 unresolved discussions (waiting on @fpertl, @hhuangMITRE, and @jrobble)


cpp/OcvSsdFaceDetection/CMakeLists.txt, line 32 at r3 (raw file):

Previously, fpertl wrote…

uncommented RelWithDevInfo, which builds release version of the code but adds debugging symbols, and commented Debug build type with added comments. Not sure why this is an issue?

We usually clean up commented code or leave a note behind if it's for others. I was also wondering about which version (release, release w/ dev, debug) was appropriate for deployment, but I assume release w/ dev is fine.


cpp/OcvSsdFaceDetection/DetectionLocation.h, line 99 at r3 (raw file):

Previously, fpertl wrote…

This is to support auto generation of documents by Doxygen, a popular doc generator.

Got it, thank you.


cpp/OcvSsdFaceDetection/DetectionLocation.h, line 105 at r3 (raw file):

Previously, fpertl wrote…

removed, if they want it they can type it back in.

I see, I was wondering if the information was something users might want to retrieve on occasion but I agree that this is fine too.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 103 at r3 (raw file):

Previously, fpertl wrote…

Done.

Sounds good, I still see some whitespace here and in DetectionLocation.h, but I'll resolve this discussion for now. I remember VS code has an option to autoclean trailing whitespace upon saves, I found and left some steps here if it helps.


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 365 at r3 (raw file):

Previously, fpertl wrote…

moved alternative KCF tracker create() to comment for MOSSE tracker

Looks great, thanks!


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.code-workspace, line 1 at r3 (raw file):

Previously, fpertl wrote…

I discussed this with @jrobble, and was going to leave the dev environment stuff in there for using vscode, see bottom of README.md. Leaving the settings for the open-source development tool from the same company that now owns github.com in there may help others to get up developing quickly and should not harm anything otherwise. I'm happy to remove it and the associated directories .devcontainer .vscode if there is a good reason.

I see, thanks! I agree leaving it behind for other developers would be great, so keeping it here is fine.


cpp/OcvSsdFaceDetection/.devcontainer/Dockerfile, line 28 at r3 (raw file):

Previously, fpertl wrote…

The fsroot directory is just an optional local cache for the opencv and dlib install code used further down in the docker file. You can stash those installs in fsroot\tmp\ and the somewhat time consuming downloads will be skipped at build. I only reinstalled opencv since the openMPF component image was not including the contrib modules at the time, or being compiled for performance using linear algebra acceleration library (s.a openBLAS).

All this is for use from inside the CentOS component-builder used for development of openMPF components (i.e. openmpf/openmpf_cpp_component_build) . There is nothing specific to the host os running this container. Vscode, which is cross-platform, launches an instance of openmpf/openmpf_cpp_component_build` with the extras installed and you then get to develop with a nice IDE directly inside the openMPF build container see. I thought this was a nice cross-platform way to setup an open source IDE that would allow developers to work/develop from inside the openMPF build container, since we don't really have a "development" container image. If we don't like it, it is easy to remove.

I see, thanks for the explanation! I'm fine with leaving this here then.


cpp/OcvSsdFaceDetection/.devcontainer/devcontainer.json, line 4 at r3 (raw file):

Previously, fpertl wrote…

completely optional, but potentially useful to other developers.

Sure thing, I think it's nice to have in that case. I'm fine with leaving this here then.

Copy link
Contributor

@hhuangMITRE hhuangMITRE left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 44 files reviewed, 63 unresolved discussions (waiting on @fpertl, @hhuangMITRE, and @jrobble)


cpp/OcvSsdFaceDetection/DetectionLocation.cpp, line 103 at r4 (raw file):

  int lry = min(y_left_upper + height, d.y_left_upper + d.height);

  float inter_area = max(0, lrx - ulx) * max(0, lry - uly);	                   

I still see some whitespace here and in several other files, but I'll resolve this discussion for now.

I remember VS code has an option to autoclean trailing whitespace upon saves (most editors have a similar feature). This can help make formatting files a bit easier and avoid manual cleanup. https://stackoverflow.com/questions/30884131/remove-trailing-spaces-automatically-or-with-a-shortcut


cpp/OcvSsdFaceDetection/plugin-files/config/mpfOcvSsdFaceDetection.ini, line 39 at r4 (raw file):

TRACKING_MAX_FRAME_GAP: 4

# threshold for 1-iou distance below which detection will be conidered part of same track

Typo: conidered -> considered. Typo occurs two more times below.


cpp/OcvSsdFaceDetection/plugin-files/data/NOTICE, line 6 at r4 (raw file):


# shape_predictor_5_face_landmarks.dat
origianlly created by DLIB author downloaded from DLIB https://github.com/davisking/dlib-models/blob/master/shape_predictor_5_face_landmarks.dat.bz2

Typo: origianlly ->originally


cpp/OcvSsdFaceDetection/plugin-files/descriptor/descriptor.json, line 44 at r4 (raw file):

          "type": "INT",
          "defaultValue": "0"
        },        

Trailing whitespace.


cpp/OcvSsdFaceDetection/plugin-files/descriptor/descriptor.json, line 47 at r4 (raw file):

        {
          "name": "TRACKING_MAX_IOU_DIST",
          "description": "Maximum intersection over union distance (1.0 - iou) =[0..1] between detections below which detection will be conidered part of same track",

Typo: conidered -> considered


cpp/OcvSsdFaceDetection/plugin-files/descriptor/descriptor.json, line 53 at r4 (raw file):

        {
          "name": "TRACKING_MAX_FEATURE_DIST",
          "description": "Maximum distance in feature space (1.0 - cos similarity) = [0..1] between detections below which detection will be conidered part of same track",

Typo: conidered -> considered


cpp/OcvSsdFaceDetection/test/test_ocv_ssd_face_detection.cpp, line 26 at r4 (raw file):

 * limitations under the License.                                             *
 ******************************************************************************/

Note to self: Report back to make sure tests passed during docker build.


cpp/OcvSsdFaceDetection/test/test_ocv_ssd_face_detection.cpp, line 350 at r4 (raw file):

      ASSERT_TRUE(detections.front()->featureDist(*detections.front()) < 1e-6);
      GOUT("cross feature dist: " << detections.front()->featureDist(*detections.back()));
      

Trailing whitespace.


cpp/OcvSsdFaceDetection/test/test_ocv_ssd_face_detection.cpp, line 357 at r4 (raw file):

        //cv::Mat_<int> ass = ssd->_calcAssignemntMatrix<&DetectionLocation::iouDist>(tracks,detections,cfg.maxIOUDist);
        //ssd->_assignDetections2Tracks(tracks,detections, ass);

Are these two lines no longer needed? If so they should be cleaned up.
If it's still useful to keep this around for debug purposes please check my comment below, thanks!

Also if we do keep this, I remember_calcAssignemntMatrix (or a similar function) has a typo correction in Assignemnt->Assignment, so that line would also need to be updated.


cpp/OcvSsdFaceDetection/test/test_ocv_ssd_face_detection.cpp, line 372 at r4 (raw file):

"Writing tumbnail

Typo: tumbnail -> thumbnail


cpp/OcvSsdFaceDetection/test/test_ocv_ssd_face_detection.cpp, line 418 at r4 (raw file):

    ocv_ssd_face_detection->SetRunDirectory(current_working_dir + "/../plugin");
    ASSERT_TRUE(ocv_ssd_face_detection->Init());
    

Trailing whitespace.


cpp/OcvSsdFaceDetection/test/test_ocv_ssd_face_detection.cpp, line 424 at r4 (raw file):

Quoted 5 lines of code…
    // create output kown video to view ground truth
    // GOUT("\tWriting ground truth video and test tracks to files.");
    // VideoGeneration video_generation_gt;
    // video_generation_gt.WriteTrackOutputVideo(inVideoFile, known_tracks, (test_output_dir + "/ground_truth.avi"));
    // WriteDetectionsToFile::WriteVideoTracks((test_output_dir + "/ground_truth.txt"), known_tracks);

Typo in kown video to view ground truth.

I'm not opposed to leaving behind a way to view results if it's helpful for debug purposes.

If it's still useful to keep this around, maybe we could leave a toggle (bool debug = true;) and place this code under an if-statement (if (debug) { \\ Create output to view ground truth. ...).

I know we typically clean u commented blocks of code otherwise.


cpp/OcvSsdFaceDetection/test/test_ocv_ssd_face_detection.cpp, line 456 at r4 (raw file):

    // don't forgetx

Is forgetx a typo? Otherwise I assume this is a comment to clean up resources after the tests are complete.

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 44 files reviewed, 62 unresolved discussions (waiting on @fpertl, @hhuangMITRE, and @jrobble)


cpp/OcvSsdFaceDetection/.devcontainer/devcontainer.json, line 4 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Sure thing, I think it's nice to have in that case. I'm fine with leaving this here then.

@hhuangMITRE , yeah, you probably didn't see our discussion on the old PR. Maybe one day another developer with give VS Code a try. It looks promising. (To be clear: I'm not asking you to be that person.) For now, this VS Code stuff is useful to @fpertl , and may be useful to future developers, so I see no harm in keeping it since he made it clear in the README that it's optional: "no way required to use or build the project". Initially though, it did stand out as being a little strange since this is the only component (so far) that has it.

Copy link
Contributor Author

@fpertl fpertl left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 44 files reviewed, 60 unresolved discussions (waiting on @fpertl, @hhuangMITRE, and @jrobble)


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 159 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing whitespace.

Done.


cpp/OcvSsdFaceDetection/OcvSsdFaceDetection.cpp, line 403 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

There's an extra trailing whitespace on this line and the line above it.

Done.


cpp/OcvSsdFaceDetection/plugin-files/config/mpfOcvSsdFaceDetection.ini, line 39 at r4 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo: conidered -> considered. Typo occurs two more times below.

Done.


cpp/OcvSsdFaceDetection/plugin-files/data/NOTICE, line 6 at r4 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo: origianlly ->originally

Done.


cpp/OcvSsdFaceDetection/plugin-files/descriptor/descriptor.json, line 44 at r4 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing whitespace.

Done.


cpp/OcvSsdFaceDetection/plugin-files/descriptor/descriptor.json, line 47 at r4 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo: conidered -> considered

Done.


cpp/OcvSsdFaceDetection/plugin-files/descriptor/descriptor.json, line 53 at r4 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Typo: conidered -> considered

Done.


cpp/OcvSsdFaceDetection/test/test_ocv_ssd_face_detection.cpp, line 350 at r4 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing whitespace.

Done.


cpp/OcvSsdFaceDetection/test/test_ocv_ssd_face_detection.cpp, line 357 at r4 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…
        //cv::Mat_<int> ass = ssd->_calcAssignemntMatrix<&DetectionLocation::iouDist>(tracks,detections,cfg.maxIOUDist);
        //ssd->_assignDetections2Tracks(tracks,detections, ass);

Are these two lines no longer needed? If so they should be cleaned up.
If it's still useful to keep this around for debug purposes please check my comment below, thanks!

Also if we do keep this, I remember_calcAssignemntMatrix (or a similar function) has a typo correction in Assignemnt->Assignment, so that line would also need to be updated.

Done.


cpp/OcvSsdFaceDetection/test/test_ocv_ssd_face_detection.cpp, line 372 at r4 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…
"Writing tumbnail

Typo: tumbnail -> thumbnail

Done.


cpp/OcvSsdFaceDetection/test/test_ocv_ssd_face_detection.cpp, line 418 at r4 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Trailing whitespace.

Done.


cpp/OcvSsdFaceDetection/test/test_ocv_ssd_face_detection.cpp, line 424 at r4 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…
    // create output kown video to view ground truth
    // GOUT("\tWriting ground truth video and test tracks to files.");
    // VideoGeneration video_generation_gt;
    // video_generation_gt.WriteTrackOutputVideo(inVideoFile, known_tracks, (test_output_dir + "/ground_truth.avi"));
    // WriteDetectionsToFile::WriteVideoTracks((test_output_dir + "/ground_truth.txt"), known_tracks);

Typo in kown video to view ground truth.

I'm not opposed to leaving behind a way to view results if it's helpful for debug purposes.

If it's still useful to keep this around, maybe we could leave a toggle (bool debug = true;) and place this code under an if-statement (if (debug) { \\ Create output to view ground truth. ...).

I know we typically clean u commented blocks of code otherwise.

Done.

@jrobble
Copy link
Member

jrobble commented May 23, 2020


cpp/OcvSsdFaceDetection/test/test_ocv_ssd_face_detection.cpp, line 424 at r4 (raw file):

Previously, fpertl wrote…

Done.

Let's just leave this code commented out. I can see how it would be useful for a developer to debug this code, but also it doesn't really make sense to generate these files every time the test is run because most of a time a human is not going to look at them.

Copy link
Contributor Author

@fpertl fpertl left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 44 files reviewed, 2 unresolved discussions (waiting on @fpertl and @jrobble)


cpp/OcvSsdFaceDetection/CMakeLists.txt, line 47 at r7 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Hi all, I've updated the CMakeLists.txt to follow the DlibFaceDetection component's CMakeLists.txt when installing the dlib dependency.

I've tried to install dlib in the docker build and then use find_package(dlib), but then I ran into an issue where the compiler complained of missing dlib source files.

Based on davisking/dlib#34, it seems that this behavior is intentional. Concerning debug builds at least, we'll need to include dlib's cmake file directly during installation within the docker/Centos7 environment.

Since the other dlib component uses the same build steps, I assumed this change would be acceptable, and left a few notes here as well.

That post is from 2015, and probably refers to an outdated version or dlib. The version I used was http://dlib.net/files/dlib-19.19.tar.bz2 . If possible, I am in favor of using current version of dependencies. I used to following to install the current version of dlib:

RUN mkdir -p /tmp/dlib; \
    cd /tmp/dlib; \
    curl --location 'http://dlib.net/files/dlib-19.19.tar.bz2' | tar --extract --bzip2; \
    cd dlib-19.19; \
    mkdir build; \
    cd build; \
    cmake3 -DBUILD_SHARED_LIBS:BOOL="1" ..; \
    cmake3 --build . --config Release; \
    make --jobs "$(nproc)" install; \
    ldconfig;

cpp/OcvSsdFaceDetection/Dockerfile, line 26 at r3 (raw file):

Previously, hhuangMITRE (Howard W Huang) wrote…

Docker build works correctly after changes (tests, component integration with workflow manager, job runs in workflow manager).

Is there any reason to not use the most current version e.g. http://dlib.net/files/dlib-19.19.tar.bz2 ?

Copy link
Contributor

@hhuangMITRE hhuangMITRE left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 44 files reviewed, 2 unresolved discussions (waiting on @fpertl and @jrobble)


cpp/OcvSsdFaceDetection/CMakeLists.txt, line 47 at r7 (raw file):

Previously, fpertl wrote…

That post is from 2015, and probably refers to an outdated version or dlib. The version I used was http://dlib.net/files/dlib-19.19.tar.bz2 . If possible, I am in favor of using current version of dependencies. I used to following to install the current version of dlib:

RUN mkdir -p /tmp/dlib; \
    cd /tmp/dlib; \
    curl --location 'http://dlib.net/files/dlib-19.19.tar.bz2' | tar --extract --bzip2; \
    cd dlib-19.19; \
    mkdir build; \
    cd build; \
    cmake3 -DBUILD_SHARED_LIBS:BOOL="1" ..; \
    cmake3 --build . --config Release; \
    make --jobs "$(nproc)" install; \
    ldconfig;

@fpertl Thank you! I had used the version of dlib that was assigned in the MPF docker build as the Dockerfile seemed to rely on the default dlib dependency.

I've updated the dlib version to 19.19 in the Dockerfile following your suggestion. After updating dlib, the find_package(dlib) installation works correctly now. As a result I've updated/reverted the CMakeLists.txt back to using find_package(dlib). The compiler recommended that I link the libraries as target_link_libraries(mpfOcvSsdFaceDetection ... dlib:dlib) so that's the only change left in CMakeLists.txt after these updates.

@jrobble because we've now updated dlib to 19.19 for this component, we may need to set this to a docker only build as OpenMPF uses dlib 18.18 instead.

Let me know if you have any more comments or suggestions, thanks!


cpp/OcvSsdFaceDetection/Dockerfile, line 26 at r3 (raw file):

Previously, fpertl wrote…

Is there any reason to not use the most current version e.g. http://dlib.net/files/dlib-19.19.tar.bz2 ?

I was using the MPF assigned version previously. I've updated to dlib-19.19 following your install steps, thanks!

@jrobble jrobble changed the title Ssd face Add OcvSsdFaceDetection component Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants