feat: face and person detection samples by telpirion · Pull Request #362 · googleapis/nodejs-video-intelligence

telpirion · 2020-02-06T05:31:57Z

These code samples add the following region tags:

video_detect_person_beta
video_detect_person_gcs_beta
video_detect_faces_beta
video_detect_faces_gcs_beta

These code samples will be the canonicals.

codecov · 2020-02-06T05:51:02Z

Codecov Report

Merging #362 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #362   +/-   ##
=======================================
  Coverage   25.65%   25.65%           
=======================================
  Files          38       38           
  Lines        9135     9135           
=======================================
  Hits         2344     2344           
  Misses       6791     6791

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c96b179...2dbd81c. Read the comment docs.

bcoe

this is looking reasonable to me, a few stylistic nits.

bcoe · 2020-02-06T17:59:21Z

samples/analyze.v1p3beta1.js

+  // const path = 'Local file to analyze, e.g. ./my-file.mp4';
+
+  // Reads a local video file and converts it to base64
+  const file = await util.promisify(fs.readFile)(path);


if we're blocking any ways, I might be tempted to use fs.readFileSync, avoid the promisify step.

bcoe · 2020-02-06T18:03:10Z

samples/analyze.v1p3beta1.js

+  const personAnnotations =
+    results[0].annotationResults[0].personDetectionAnnotations;
+
+  personAnnotations.forEach(personAnnotation => {


to avoid the forEach, and an extra layer of callbacks, you could do this:

for (const {segment, timestampedObjects} of tracks) { console.info(segment, timestampedObjects); }

I've been tending to use this syntax rather than forEach these days, it's also nice because you can break out of the loop with break.

Done, here and elsewhere.

Thank you for showing me this syntax!

telpirion · 2020-02-06T21:08:38Z

Thank you for the review!

nnegrey

Region tag fixes

nnegrey · 2020-02-12T17:56:57Z

samples/analyze.v1p3beta1.js

+'use strict';
+
+async function detectPerson(path) {
+  //[START video_detect_person_beta]


// [START

... in this pull request.

nnegrey · 2020-02-12T17:57:21Z

samples/analyze.v1p3beta1.js

+  // [END video_detect_person_beta]
+}
+async function detectPersonGCS(gcsUri) {
+  //[START video_detect_person_gcs_beta]


Add space // [START

Done in this pull request.

nnegrey · 2020-02-12T17:57:49Z

samples/analyze.v1p3beta1.js

+      }
+    }
+  }
+  // [END video_detect_person_beta]


video_detect_person_gcs_beta

Acknowledged.

It's already fixed :).

nnegrey · 2020-02-12T17:58:23Z

samples/analyze.v1p3beta1.js

+        console.log(`\tAttribute: ${name}; `);
+      }
+    }
+  }


// [END video_detect_faces_beta]

Done in this pull request.

nnegrey · 2020-02-12T17:58:43Z

samples/analyze.v1p3beta1.js

+        console.log(`\tAttribute: ${name}; `);
+      }
+    }
+  }


// [END video_detect_faces_gcs_beta

Done in this pull request.

nnegrey · 2020-02-12T18:01:13Z

Couple style things:

Each sample should have its own file
async needs to be inside the region tag:
example: https://github.com/googleapis/nodejs-automl/blob/master/samples/delete_model.js

nnegrey · 2020-02-12T18:02:36Z

samples/analyze.v1p3beta1.js

+  const results = await operation.promise();
+  console.log('Waiting for operation to complete...');
+
+  // Gets annotations for video


Add a note that we get the first result because only 1 video is processed.

Done in this pull request.

nnegrey · 2020-02-12T18:03:01Z

samples/analyze.v1p3beta1.js

+    videoContext: {
+      personDetectionConfig: {
+        // Must set includeBoundingBoxes to true to get poses and attributes.
+        includeBoundingBoxes: true,


You don't use the bounding boxes in the output, is it worth addding?

The bounding boxes are necessary to get the person and face detection segments but don't need to be read.

Reading bounding boxes is a pretty common task for the Video Intelligence API. On one hand, developers should be familiar with it already and this sample is really more about the attributes. On the other hand, it is a common task for using the API and maybe a good thing to include anyway.

I don't have a strong feeling one way or the other about bounding boxes, besides that adding them makes this sample longer. What is your preference?

nnegrey · 2020-02-12T18:07:28Z

samples/analyze.v1p3beta1.js

+      if (segment.startTimeOffset.seconds === undefined) {
+        segment.startTimeOffset.seconds = 0;
+      }
+      if (segment.startTimeOffset.nanos === undefined) {
+        segment.startTimeOffset.nanos = 0;
+      }
+      if (segment.endTimeOffset.seconds === undefined) {
+        segment.endTimeOffset.seconds = 0;
+      }
+      if (segment.endTimeOffset.nanos === undefined) {
+        segment.endTimeOffset.nanos = 0;
+      }


@telpirion, does this ever happen?
If so, that seems like an issue we need to raise.

It can happen, yes. Sometimes the result from the API simply doesn't return a value and thus the property is undefined.

TBH, I'm following the pattern demonstrated here (among other places):
https://github.com/googleapis/nodejs-video-intelligence/blob/master/samples/analyze.v1p2beta1.js#L42-L61

telpirion · 2020-02-12T19:18:58Z

Thank you for the review!

A couple follow-up questions:

Couple style things:

Each sample should have its own file

The existing beta samples use just "analyze.v1p2beta1.js.". The file includes multiple samples (and it's what I used as an example).

Does that need to be refactored?

telpirion added 2 commits January 31, 2020 11:32

Adds method stubs for face and person detection

efb6453

Adds Video face and people detection samples

8ea1e1f

telpirion requested a review from bcoe February 6, 2020 05:31

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 6, 2020

telpirion added 2 commits February 5, 2020 21:32

Fixes license header

82a500c

Fixes some linter errors

4162583

bcoe approved these changes Feb 6, 2020

View reviewed changes

telpirion added 2 commits February 6, 2020 13:01

Makes changes per reviewer feedback.

00c23cb

Additional small changes

9d9c5a1

telpirion added 2 commits February 6, 2020 13:20

Fixes more linter issues

c96b179

Merge branch 'master' into face-person-samples

2dbd81c

telpirion merged commit cff2f36 into master Feb 7, 2020

telpirion deleted the face-person-samples branch February 7, 2020 23:47

yoshi-automation mentioned this pull request Feb 8, 2020

docs: update auto-generated README #364

Merged

nnegrey reviewed Feb 12, 2020

View reviewed changes

This was referenced Feb 12, 2020

Face and Person detection samples googleapis/java-video-intelligence#76

Closed

feat: face and person detection samples GoogleCloudPlatform/java-docs-samples#2066

Merged

Conversation

telpirion commented Feb 6, 2020

Uh oh!

codecov bot commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bcoe 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!

telpirion commented Feb 6, 2020

Uh oh!

nnegrey 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!

nnegrey commented Feb 12, 2020

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!

telpirion commented Feb 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Feb 6, 2020 •

edited

Loading