Adding MoveNet to Pose Detection API #627

ardoerlemans · 2021-03-31T21:43:53Z

This change is

Line lengths will be updated in an upcoming commit.

lina128

Hi Ard, thank you for the PR! I've made a pass on the PR, please take a look.

Reviewable status: 0 of 1 approvals obtained (waiting on @ardoerlemans and @pyu10055)

pose-detection/src/movenet/detector.ts, line 83 at r1 (raw file):

   *
   * This does standard ImageNet pre-processing before inferring through the
   * model. The image should pixels should have values [0-255]. It returns a

Is this description accurate?

pose-detection/src/movenet/detector.ts, line 131 at r1 (raw file):

    imageTensor3D.dispose();

    const [keypoints, ,] = await this.runInference(imageTensor4D, estimationConfig.minimumKeypointScore);

There are multiple runInferences in this code, the other two actually run inference on a model. This one also includes preprocessing code and postprocessing code, so for a reader, it's hard to understand what's happening here. Can we move those code logic to this method? In general, it is better to group small chunks of code for better readability.

pose-detection/src/movenet/detector_utils.ts, line 23 at r1 (raw file):

export function validateModelConfig(modelConfig: MoveNetModelConfig):
    MoveNetModelConfig {
  const config = modelConfig || MOVENET_CONFIG;

This will modify user input config, can you change to

const config = modelConfig == null ? MOVENET_CONFIG : {...modelConfig};

pose-detection/src/movenet/detector_utils.ts, line 26 at r1 (raw file):

  if (config.inputResolution == null) {
    config.inputResolution = {height: 192, width: 192};

Store 192 in a constant variable.

pose-detection/src/movenet/detector_utils.ts, line 34 at r1 (raw file):

export function validateEstimationConfig(
    estimationConfig: MoveNetEstimationConfig): MoveNetEstimationConfig {
  const config = estimationConfig;

Same. Use {...estimationConfig}, also check null case.

pose-detection/src/movenet/detector_utils.ts, line 49 at r1 (raw file):

  }

  if (!config.minimumKeypointScore) {

if (config.minimumKeypointScore == null)

This will check null and undefined, but won't affect when the value is 0.

pose-detection/src/movenet/index.ts, line 3 at r1 (raw file):

/**
 * @license
 * Copyright 2019 Google LLC. All Rights Reserved.

2021

pose-detection/src/movenet/keypoint_model.ts, line 26 at r1 (raw file):

 * Encapsulates a TensorFlow person keypoint model.
 */
export class KeypointModel extends Model {

Why do you need a Model class and a KeypointModel class?

pose-detection/src/movenet/keypoint_model.ts, line 55 at r1 (raw file):

    for (let i = 0; i < numKeypoints; ++i) {
      keypoints[i] = {
        'y': instanceValues[i][0],

It can just be

{
   y: instanceValues[i][0],
   x: instanceValues[i][1]

pose-detection/src/movenet/model.ts, line 18 at r1 (raw file):

 */

import '@tensorflow/tfjs-backend-cpu';

You don't need these two lines.

pose-detection/src/movenet/model.ts, line 42 at r1 (raw file):

  private processingTime: number;
  private numInferences: number;
  private model?: tf.GraphModel;

Why model is optional? Also should it be private const model?

pose-detection/src/movenet/model.ts, line 45 at r1 (raw file):

  constructor() {
    this.processingTime = 0;

You can initialize these in variable declaration.

pose-detection/src/movenet/model.ts, line 60 at r1 (raw file):

   * Runs inference on an image.
   * @param inputImage 4D tensor containing the input image. Should be of size
   *     [1, modelHeight, modelWidth, 3]. The tensor will be disposed.

Remove 'The tensor will be disposed', because it is implementation detail.

pose-detection/src/movenet/model.ts, line 61 at r1 (raw file):

   * @param inputImage 4D tensor containing the input image. Should be of size
   *     [1, modelHeight, modelWidth, 3]. The tensor will be disposed.
   * @param executeSync Whether to execute the model synchronously. A model with

Why do you need this parameter?

pose-detection/src/movenet/model.ts, line 64 at r1 (raw file):

   *     control flow ops can only be executed asynchronously. See:
   *     https://js.tensorflow.org/api/latest/#tf.GraphModel.executeAsync
   * @return An multidimensional array containing the values of the first output

Why not return the tensor as output?

pose-detection/src/movenet/model.ts, line 67 at r1 (raw file):

   *     tensor or 'null' if the model was not initialized yet.
   */
  async runInference(inputImage: tfc.Tensor, executeSync: boolean):

tfc.Tensor4D

pose-detection/src/movenet/model.ts, line 76 at r1 (raw file):

    let outputTensor;
    if (executeSync) {
      outputTensor = this.model.execute(inputImage) as tfc.Tensor;

Add some description about the output tensor shape and meaning.

pose-detection/src/movenet/model.ts, line 92 at r1 (raw file):

    // Skip first few inference in statistics because those usually take a lot
    // longer while the model 'warms up'.
    const inferencesToSkip = 5;

I believe this is useful for benchmarking, but not so for real application. Can we remove this skip logic?

pose-detection/src/movenet/model.ts, line 100 at r1 (raw file):

        // Use a moving average to make the visualization of the FPS less jumpy.
        // TensorFlow.js inference times can vary quite a bit.
        const newSampleWeight = 0.02;

Do you need the processing time for visualizing FPS or do you need it for the model? If the model doesn't need it, I suggest remove it, because the demo page has already setup the fps visualization that is going to be used for all models.

pose-detection/src/movenet/one_euro_filter.ts, line 53 at r1 (raw file):

  constructor(
      updateRateOffset = 40.0, updateRateSlope = 4e3, speedUpdateRate = 0.5,

I think this is essentially doing similar thing as the one_euro_filter in the shared calculators folder: https://github.com/tensorflow/tfjs-models/blob/master/pose-detection/src/calculators/one_euro_filter.ts

Can we use the existing one_euro_filter? Or is there key difference between this implementation and the existing one?

pose-detection/src/movenet/types.ts, line 20 at r1 (raw file):

/**
 * Additional MoveNet model loading config.

Please add description for each field, so that users know how to set them.

pose-detection/src/movenet/types.ts, line 23 at r1 (raw file):

 */
export interface MoveNetModelConfig extends ModelConfig {
  inputResolution: InputResolution;

This should be optional from user standpoint, right?

pose-detection/src/movenet/types.ts, line 28 at r1 (raw file):

/**
 * MoveNet Specific Inference Config

Please add description for minimumKeypointScore.

ronnyvotel

first round of comments

ronnyvotel · 2021-04-01T15:44:42Z

pose-detection/src/movenet/detector.ts

+    super();
+    this.model = moveNetModel;
+    // Should we retrieve these values from the config? Or the URL maybe?
+    this.modelWidth = 192;


should the dimensions be hard coded like this, or put in the config?

ronnyvotel · 2021-04-01T15:50:27Z

pose-detection/src/movenet/detector.ts

+    };
+
+    for (const key of Object.keys(targetKeypoints)) {
+      targetKeypoints[key] = [


apologies in advance for my complete lack of typescript. why do we mark targetKeypoints as const, and then update it in this loop?

ronnyvotel · 2021-04-01T16:10:52Z

pose-detection/src/movenet/detector.ts

+  determineCropRegion(
+      keypoints: Keypoint[], webcamHeight: number, webcamWidth: number,
+      minimumKeypointScore: number) {
+    const keypointIndices: {[index: string]: number} = {


i noticed that constants.ts also defines all the coco keypoints (although with different names). does it make sense to use that, or do you think it's better to keep it here in case we eventually want to use a different set of keypoints?

ardoerlemans

Thank you both for the thorough review! I have addressed all comments and I have also added the logic to allow the user to choose between the two MoveNet models.

Reviewable status: 0 of 1 approvals obtained (waiting on @lina128, @pyu10055, and @ronnyvotel)

pose-detection/src/movenet/detector.ts, line 50 at r1 (raw file):

Previously, ronnyvotel wrote…

should the dimensions be hard coded like this, or put in the config?

I have moved them to the constants. The selected model type will determine these.

pose-detection/src/movenet/detector.ts, line 83 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Is this description accurate?

The ImageNet part is correct, but the number of poses isn't. I have updated the comment to reflect that we currently only handle the single pose case.

pose-detection/src/movenet/detector.ts, line 131 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

There are multiple runInferences in this code, the other two actually run inference on a model. This one also includes preprocessing code and postprocessing code, so for a reader, it's hard to understand what's happening here. Can we move those code logic to this method? In general, it is better to group small chunks of code for better readability.

Makes sense. I have moved the code from runInference to this method.

pose-detection/src/movenet/detector.ts, line 147 at r1 (raw file):

Previously, ronnyvotel wrote…

i noticed that constants.ts also defines all the coco keypoints (although with different names). does it make sense to use that, or do you think it's better to keep it here in case we eventually want to use a different set of keypoints?

We could probably use the names as they are defined in COCO_KEYPOINTS, but this is actually a map from name to keypoint index, so I think we still need this here.

pose-detection/src/movenet/detector.ts, line 188 at r1 (raw file):

Previously, ronnyvotel wrote…

apologies in advance for my complete lack of typescript. why do we mark targetKeypoints as const, and then update it in this loop?

The 'const' means that you cannot assign a new value to the variable targetKeypoints, but it doesn't mean that the values that it's referring to are immutable:

https://www.typescriptlang.org/docs/handbook/variable-declarations.html#const-declarations

pose-detection/src/movenet/detector_utils.ts, line 23 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

This will modify user input config, can you change to
const config = modelConfig == null ? MOVENET_CONFIG : {...modelConfig};

Done. Please note that this should probably also change in posenet/detector_utils.ts.

pose-detection/src/movenet/detector_utils.ts, line 26 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Store 192 in a constant variable.

Done.

pose-detection/src/movenet/detector_utils.ts, line 34 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Same. Use {...estimationConfig}, also check null case.

Done. This should probably also be updated in posenet/detector_utils.ts.

pose-detection/src/movenet/detector_utils.ts, line 49 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

if (config.minimumKeypointScore == null)
This will check null and undefined, but won't affect when the value is 0.

Updated this to (!config.miniumKeypointScore)

pose-detection/src/movenet/index.ts, line 3 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

2021

Done.

pose-detection/src/movenet/keypoint_model.ts, line 26 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Why do you need a Model class and a KeypointModel class?

A Model can execute any model and just returns the raw tensor outputs, a KeypointModel interprets the Model output and converts it into 17 keypoints.

pose-detection/src/movenet/keypoint_model.ts, line 55 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

It can just be

{
   y: instanceValues[i][0],
   x: instanceValues[i][1]
</blockquote></details>

Done.
___
*[pose-detection/src/movenet/model.ts, line 18 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBKCHe3H0fB39ST5gb:-MXDPiT1AxxyU9y5XBCF:b-896fix) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/model.ts#L18)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

You don't need these two lines.
</blockquote></details>

Done.
___
*[pose-detection/src/movenet/model.ts, line 42 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBK0Bf1dZ5zrRktT_z:-MXDQ4lK81mGbeDW2MXd:b-4yhjww) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/model.ts#L42)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

Why model is optional? Also should it be `private const model`?
</blockquote></details>

Good point, made it not optional. Adding 'const' apparently results in 'A class member cannot have the 'const' keyword.'?
___
*[pose-detection/src/movenet/model.ts, line 45 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBJv175QkXgQFeIHKz:-MXDQjgSDazcTwBnM0Z4:b-896fix) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/model.ts#L45)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

You can initialize these in variable declaration.
</blockquote></details>

Done.
___
*[pose-detection/src/movenet/model.ts, line 60 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBKR05DT9eyyFX4yIQ:-MXDRa5n19RRwoF8MTTh:b8sa3wa) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/model.ts#L60)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

Remove 'The tensor will be disposed', because it is implementation detail.
</blockquote></details>

It seems to me that his is actually quite important for users of this class: your tensor will not be available to be used for anything else after calling runInference.
___
*[pose-detection/src/movenet/model.ts, line 61 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBKaho-lamZvz7a_FD:-MXDRyzV2eqBbLla9z4B:bceutd0) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/model.ts#L61)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

Why do you need this parameter?
</blockquote></details>

As the comment explains: some models have control ops and you can't execute them synchronously. MoveNet used to have control ops and may have them when we update the model to multi-person use cases.
___
*[pose-detection/src/movenet/model.ts, line 64 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBKi3LAP0xJiRyXUQz:-MXDTV1R8tMOhWryHD_Y:b74ktgo) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/model.ts#L64)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

Why not return the tensor as output?
</blockquote></details>

Good question. It seemed easier to return values from this call that you don't have to call dispose() on.
___
*[pose-detection/src/movenet/model.ts, line 67 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBKL8C6JEt06ppRDKP:-MXDUF4rDoGmrbnlOW-R:b-896fix) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/model.ts#L67)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

tfc.Tensor4D
</blockquote></details>

Done.
___
*[pose-detection/src/movenet/model.ts, line 76 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBL1OCDys9XNHQI9xL:-MXDUITc4wzVgNcG2eR5:bulxfbz) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/model.ts#L76)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

Add some description about the output tensor shape and meaning.
</blockquote></details>

The Model class can run any model, so there's no assumption here about shape and meaning of the model's output.
___
*[pose-detection/src/movenet/model.ts, line 92 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBLE1f8_TOdzctwtYz:-MXDUVejDIFibZUNPs_o:b-896fix) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/model.ts#L92)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

I believe this is useful for benchmarking, but not so for real application. Can we remove this skip logic?
</blockquote></details>

Done.
___
*[pose-detection/src/movenet/model.ts, line 100 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBLVwd5gel4uKgl5r6:-MXDV7sv5cQ8yYCryTSd:b-bv0la7) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/model.ts#L100)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

Do you need the processing time for visualizing FPS or do you need it for the model? If the model doesn't need it, I suggest remove it, because the demo page has already setup the fps visualization that is going to be used for all models.
</blockquote></details>

We don't need it anymore. Done.
___
*[pose-detection/src/movenet/one_euro_filter.ts, line 53 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBNlkl0uIQTSa9CaDX:-MXDWO-cEczQKssWBAVp:b-64h430) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/one_euro_filter.ts#L53)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

I think this is essentially doing similar thing as the one_euro_filter in the shared calculators folder: https://github.com/tensorflow/tfjs-models/blob/master/pose-detection/src/calculators/one_euro_filter.ts

Can we use the existing one_euro_filter? Or is there key difference between this implementation and the existing one?
</blockquote></details>

One difference that I see is that our version handles arrays of numbers instead of single numbers, which makes it a bit easier to use on arrays of keypoints. Our version is also more robust against keypoint prediction errors.
___
*[pose-detection/src/movenet/types.ts, line 20 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBOGyKCsxipQGJKDGG:-MXDllW0CKHnWjeCTV1E:b-896fix) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/types.ts#L20)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

Please add description for each field, so that users know how to set them.
</blockquote></details>

Done.
___
*[pose-detection/src/movenet/types.ts, line 23 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBODYTDb-tWDub5vuW:-MXDlmPj8luz2xR3vFha:b-21ih6k) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/types.ts#L23)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

This should be optional from user standpoint, right?
</blockquote></details>

This parameter is now removed. We actually don't support setting custom input resolutions to the models.
___
*[pose-detection/src/movenet/types.ts, line 28 at r1](https://reviewable.io/reviews/tensorflow/tfjs-models/627#-MXBON-W0vF8W8bHdmnV:-MXDlvtg4b7SsSocyZ5U:b-896fix) ([raw file](https://github.com/tensorflow/tfjs-models/blob/548cbed8cef28f5fa861f576cb0fff6e66c1cf60/pose-detection/src/movenet/types.ts#L28)):*
<details><summary><i>Previously, lina128 (Na Li) wrote…</i></summary><blockquote>

Please add description for minimumKeypointScore.
</blockquote></details>

Done.


<!-- Sent from Reviewable.io -->

pyu10055

Reviewable status: 0 of 1 approvals obtained (waiting on @ardoerlemans, @lina128, and @ronnyvotel)

pose-detection/src/movenet/constants.ts, line 21 at r3 (raw file):

export const VALID_ARCHITECTURES =
    ['SinglePose.Lightning', 'SinglePose.Thunder'];

should this two values be constant, since they are referenced directly in many other places.

pose-detection/src/movenet/detector.ts, line 60 at r3 (raw file):

    this.previousFrameTime = 0;
    // Assume 30 fps.
    this.frameTimeDiff = 0.0333;

is this related to the camera frame rate? should this be a config param?

pose-detection/src/movenet/detector.ts, line 68 at r3 (raw file):

ModelConfig

any code snippet or block in the jsdoc should be surrounded with `` like ModelConfig.

pose-detection/src/movenet/detector.ts, line 241 at r3 (raw file):

  }

  determineCropRegion(

it will be good to divide this method into functional blocks and as Ronny suggested, that declare the static variable as constant outside of the method.
In general, keep methods within 50 lines.

pose-detection/src/movenet/one_euro_filter.ts, line 53 at r1 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

One difference that I see is that our version handles arrays of numbers instead of single numbers, which makes it a bit easier to use on arrays of keypoints. Our version is also more robust against keypoint prediction errors.

probably better to move this to the common filter folder and name it differently.

MoveNet determineCropRegion refactored and moved OneEuroFilter to common filters directory

ardoerlemans

Reviewable status: 0 of 1 approvals obtained (waiting on @lina128, @pyu10055, and @ronnyvotel)

pose-detection/src/movenet/constants.ts, line 21 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

should this two values be constant, since they are referenced directly in many other places.

Done.

pose-detection/src/movenet/detector.ts, line 60 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

is this related to the camera frame rate? should this be a config param?

This value is dynamically updated in this class to determine the actual camera frame rate. I have updated the comment to clarify that.

pose-detection/src/movenet/detector.ts, line 68 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

ModelConfig
any code snippet or block in the jsdoc should be surrounded with `` like ModelConfig.

Done.

pose-detection/src/movenet/detector.ts, line 241 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

it will be good to divide this method into functional blocks and as Ronny suggested, that declare the static variable as constant outside of the method.
In general, keep methods within 50 lines.

Done.

pose-detection/src/movenet/one_euro_filter.ts, line 53 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

probably better to move this to the common filter folder and name it differently.

Done.

lina128

Reviewable status: 0 of 1 approvals obtained (waiting on @ardoerlemans, @lina128, @pyu10055, and @ronnyvotel)

pose-detection/demo/src/camera.js, line 131 at r4 (raw file):

  drawKeypoint(keypoint, scaleY, scaleX) {
    // If score is null, just show the keypoint.
    const score = !keypoint ? 0 : keypoint.score != null ? keypoint.score : 1;

It's better to just skip when keypoint == null

pose-detection/src/util.ts, line 27 at r4 (raw file):

    case SupportedModels.PoseNet:
      return constants.COCO_KEYPOINTS_BY_SIDE;
    case SupportedModels.MoveNet:

case SupportedModels.PoseNet:
         SupportedModels.MoveNet:

pose-detection/src/util.ts, line 39 at r4 (raw file):

    case SupportedModels.PoseNet:
      return constants.COCO_CONNECTED_KEYPOINTS_PAIRS;
    case SupportedModels.MoveNet:

Same as above.

pose-detection/src/movenet/constants.ts, line 21 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

should this two values be constant, since they are referenced directly in many other places.

+1

pose-detection/src/movenet/detector.ts, line 83 at r1 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

The ImageNet part is correct, but the number of poses isn't. I have updated the comment to reflect that we currently only handle the single pose case.

What about the pixel values?

pose-detection/src/movenet/detector.ts, line 60 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

is this related to the camera frame rate? should this be a config param?

Also, does it work for image?

pose-detection/src/movenet/detector.ts, line 51 at r4 (raw file):

    if (config.modelType === 'SinglePose.Lightning') {
      this.modelWidth = MOVENET_SINGLEPOSE_LIGHTNING_RESOLUTION;

Can you store the variable in a InputResolution type? See src/types.ts

pose-detection/src/movenet/detector.ts, line 84 at r4 (raw file):

      if (config.modelType === 'SinglePose.Lightning') {
        modelUrl =
            'https://tfhub.dev/google/tfjs-model/movenet/singlepose/lightning/1';

This should be in a constant.

pose-detection/src/movenet/keypoint_model.ts, line 26 at r1 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

A Model can execute any model and just returns the raw tensor outputs, a KeypointModel interprets the Model output and converts it into 17 keypoints.

But there's only one model class in this case, right? If there will be other model class, can we refactor it later?

pose-detection/src/movenet/model.ts, line 42 at r1 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

Good point, made it not optional. Adding 'const' apparently results in 'A class member cannot have the 'const' keyword.'?

Can you make it private readonly?

pose-detection/src/movenet/model.ts, line 64 at r1 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

Good question. It seemed easier to return values from this call that you don't have to call dispose() on.

Can you return tensor? It is common practice that application code should dispose tensor result from inference.

pose-detection/src/movenet/model.ts, line 76 at r1 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

The Model class can run any model, so there's no assumption here about shape and meaning of the model's output.

I think we are over-designing here, I don't think we need a generic Model class, given that there's only one subclass.

pose-detection/src/movenet/one_euro_filter.ts, line 53 at r1 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

Done.

Actually this wrapper method handles arrays of keypoints, which in turn calls OneEuroFilter for each x and y for each keypoint. https://github.com/tensorflow/tfjs-models/blob/master/pose-detection/src/blazepose/calculators/landmarks_one_euro_filter.ts

We can move this method to the common filter folder if this is generic enough.

Also, about more robust against keyoint prediction errors, that sounds great, can you update the one_euro_filter? We want to share code as much as possible, this helps control the bundle size.

This threshold was mostly used for internal cropping logic.

ronnyvotel

Reviewed 2 of 14 files at r1, 8 of 9 files at r2, 2 of 3 files at r3, 2 of 2 files at r4.
Reviewable status: 0 of 1 approvals obtained (waiting on @ardoerlemans, @lina128, and @pyu10055)

pose-detection/src/create_detector.ts, line 35 at r4 (raw file):

    model: SupportedModels,
    modelConfig: PosenetModelConfig|
    BlazeposeModelConfig): Promise<PoseDetector> {

Do we need to add MoveNetModelConfig as an option up here?

pose-detection/src/movenet/detector.ts, line 50 at r1 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

I have moved them to the constants. The selected model type will determine these.

thanks!

pose-detection/src/movenet/detector.ts, line 188 at r1 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

The 'const' means that you cannot assign a new value to the variable targetKeypoints, but it doesn't mean that the values that it's referring to are immutable:

https://www.typescriptlang.org/docs/handbook/variable-declarations.html#const-declarations

ahh, so it's like a top level const in C++. thanks.

pose-detection/src/movenet/detector.ts, line 125 at r4 (raw file):

this.maxPoses = config.maxPoses;

if we don't use maxPoses, any reason why we are storing it as a member?

pose-detection/src/movenet/detector.ts, line 135 at r4 (raw file):

    if (this.previousFrameTime !== 0) {
      const newSampleWeight = 0.02;
      this.frameTimeDiff = newSampleWeight * this.frameTimeDiff +

nit: isn't newSampleWeight being applied to the previous sample?

pose-detection/src/movenet/detector.ts, line 185 at r4 (raw file):

      // Determine next crop region based on detected keypoints and if a crop
      // region is not detected, this will trigger the full model to run.

trigger the model to run on the full image

pose-detection/src/movenet/detector.ts, line 200 at r4 (raw file):

    }

    // If no cropRegion was available from a previous run, or if no new

I wonder if we should separate the cases where no cropRegion is available in a previous run vs no cropRegion could be determined after the inference above. Now there is a chance where we can run inference twice per estimatePoses, perhaps causing highly variable latency. Does it make sense to run on the full image in an else statement above, and avoid this double-inference case? It would also simplify the code and avoid a lot of duplication.

pose-detection/src/movenet/keypoint_model.ts, line 26 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

But there's only one model class in this case, right? If there will be other model class, can we refactor it later?

Yeah to me it requires some focus to understand why we have two parallel class structures:
BasePoseDetector -> MoveNetDetection
Model -> KeypointModel

Do other models (PoseNet / BlazePose) use this same kind of delegation for running model inference?

ardoerlemans

Reviewable status: 0 of 1 approvals obtained (waiting on @lina128, @pyu10055, and @ronnyvotel)

pose-detection/demo/src/camera.js, line 131 at r4 (raw file):

Previously, lina128 (Na Li) wrote…

It's better to just skip when keypoint == null

I have actually just removed that change because our model does not return null keypoints anymore.

pose-detection/src/create_detector.ts, line 35 at r4 (raw file):

Previously, ronnyvotel wrote…

Do we need to add MoveNetModelConfig as an option up here?

Nice catch! Yes. Done.

pose-detection/src/util.ts, line 27 at r4 (raw file):

Previously, lina128 (Na Li) wrote…

case SupportedModels.PoseNet:
         SupportedModels.MoveNet:

Done.

pose-detection/src/util.ts, line 39 at r4 (raw file):

Previously, lina128 (Na Li) wrote…

Same as above.

Done.

pose-detection/src/movenet/constants.ts, line 21 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

+1

Done.

pose-detection/src/movenet/detector.ts, line 50 at r1 (raw file):

Previously, ronnyvotel wrote…

thanks!

Acknowledged.

pose-detection/src/movenet/detector.ts, line 83 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

What about the pixel values?

That's accurate as well.

pose-detection/src/movenet/detector.ts, line 188 at r1 (raw file):

Previously, ronnyvotel wrote…

ahh, so it's like a top level const in C++. thanks.

Done.

pose-detection/src/movenet/detector.ts, line 60 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

Also, does it work for image?

We use performance.now() to determine the time between frames. The filter was designed for streams of keypoints, but do you think this will be used on individual images as well? In that case I should probably add a parameter that allows users to disable the filter?

pose-detection/src/movenet/detector.ts, line 51 at r4 (raw file):

Previously, lina128 (Na Li) wrote…

Can you store the variable in a InputResolution type? See src/types.ts

Done.

pose-detection/src/movenet/detector.ts, line 84 at r4 (raw file):

Previously, lina128 (Na Li) wrote…

This should be in a constant.

Done.

pose-detection/src/movenet/detector.ts, line 125 at r4 (raw file):

Previously, ronnyvotel wrote…

this.maxPoses = config.maxPoses;
if we don't use maxPoses, any reason why we are storing it as a member?

Done.

pose-detection/src/movenet/detector.ts, line 135 at r4 (raw file):

Previously, ronnyvotel wrote…

nit: isn't newSampleWeight being applied to the previous sample?

Again a good find! This should be reversed.

pose-detection/src/movenet/detector.ts, line 185 at r4 (raw file):

Previously, ronnyvotel wrote…

trigger the model to run on the full image

Done.

pose-detection/src/movenet/detector.ts, line 200 at r4 (raw file):

Previously, ronnyvotel wrote…

I wonder if we should separate the cases where no cropRegion is available in a previous run vs no cropRegion could be determined after the inference above. Now there is a chance where we can run inference twice per estimatePoses, perhaps causing highly variable latency. Does it make sense to run on the full image in an else statement above, and avoid this double-inference case? It would also simplify the code and avoid a lot of duplication.

Yes, it does seem better to run the full image model in an 'else' so that we don't run inference twice in a single estimatePoses call. I'll send out the simplification as a separate PR.

pose-detection/src/movenet/keypoint_model.ts, line 26 at r1 (raw file):

Previously, ronnyvotel wrote…

Yeah to me it requires some focus to understand why we have two parallel class structures:
BasePoseDetector -> MoveNetDetection
Model -> KeypointModel

Do other models (PoseNet / BlazePose) use this same kind of delegation for running model inference?

I have removed the Model class.

pose-detection/src/movenet/model.ts, line 42 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Can you make it private readonly?

That wouldn't work because of the assignments in load().

pose-detection/src/movenet/model.ts, line 64 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Can you return tensor? It is common practice that application code should dispose tensor result from inference.

I have removed the Model class.

pose-detection/src/movenet/model.ts, line 76 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

I think we are over-designing here, I don't think we need a generic Model class, given that there's only one subclass.

Done.

pose-detection/src/movenet/one_euro_filter.ts, line 53 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Actually this wrapper method handles arrays of keypoints, which in turn calls OneEuroFilter for each x and y for each keypoint. https://github.com/tensorflow/tfjs-models/blob/master/pose-detection/src/blazepose/calculators/landmarks_one_euro_filter.ts

We can move this method to the common filter folder if this is generic enough.

Also, about more robust against keyoint prediction errors, that sounds great, can you update the one_euro_filter? We want to share code as much as possible, this helps control the bundle size.

As discussed offline, we would definitely like to use the existing filter, but we want to make sure we understand the differences and have a bit more time to test with the existing filter. I can send a follow-up PR next week.

ardoerlemans

Thanks again for all the comments! Hopefully we're another step closer now. Let me know if there's anything else you'd like me to look at.

Reviewable status: 0 of 1 approvals obtained (waiting on @lina128, @pyu10055, and @ronnyvotel)

lina128

Reviewable status: 0 of 1 approvals obtained (waiting on @ardoerlemans, @lina128, @pyu10055, and @ronnyvotel)

pose-detection/demo/src/camera.js, line 131 at r4 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

I have actually just removed that change because our model does not return null keypoints anymore.

Great.

pose-detection/demo/src/index.js, line 45 at r6 (raw file):

    case posedetection.SupportedModels.MoveNet:
      return posedetection.createDetector(
          STATE.model.model, {modelType: 'SinglePose.Lightning'});

Should it use the constant value from posedetection package?

pose-detection/src/util.ts, line 27 at r4 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

Done.

I can't see the change, it's still case1: return foo; case2: return foo; insteadof case1:case2: return foo;

pose-detection/src/calculators/one_euro_filter_array.ts, line 22 at r6 (raw file):

 * https://en.wikipedia.org/wiki/Moving_average
 */
class EWMA {

Please use the shared one_euro_filter once this PR is merged: #632
Also as discussed offline, please remove the smoothing/filter part from this PR to unblock it and also keep the size of the PR smaller.

pyu10055

Reviewable status: 0 of 1 approvals obtained (waiting on @ardoerlemans, @lina128, @pyu10055, and @ronnyvotel)

pose-detection/demo/src/index.js, line 45 at r6 (raw file):

    case posedetection.SupportedModels.MoveNet:
      return posedetection.createDetector(
          STATE.model.model, {modelType: 'SinglePose.Lightning'});

Is the model type constants exposed by the library, if so use the constant value.

pose-detection/src/movenet/detector.ts, line 272 at r6 (raw file):

  determineCropRegion(
      keypoints: Keypoint[], webcamHeight: number, webcamWidth: number) {
    const targetKeypoints: {[index: string]: number[]} = {

I am wondering if you need to initialize the values at declaration, since you are assigning the values in the loop, the keys could come from your defined constant.

ardoerlemans

Reviewable status: 0 of 1 approvals obtained (waiting on @lina128, @pyu10055, and @ronnyvotel)

pose-detection/demo/src/camera.js, line 131 at r4 (raw file):

Previously, lina128 (Na Li) wrote…

Great.

Done.

pose-detection/demo/src/index.js, line 45 at r6 (raw file):

Previously, lina128 (Na Li) wrote…

Should it use the constant value from posedetection package?

Yes! Updated.

pose-detection/demo/src/index.js, line 45 at r6 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

Is the model type constants exposed by the library, if so use the constant value.

Done.

pose-detection/src/util.ts, line 27 at r4 (raw file):

Previously, lina128 (Na Li) wrote…

I can't see the change, it's still case1: return foo; case2: return foo; insteadof case1:case2: return foo;

Hmm, looks like I didn't commit that file, sorry! It should be updated now.

pose-detection/src/movenet/detector.ts, line 272 at r6 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

I am wondering if you need to initialize the values at declaration, since you are assigning the values in the loop, the keys could come from your defined constant.

You're right, the initialization wasn't necessary. I'm not sure how to use the new constant though, as it's a map from string to int?

pose-detection/src/calculators/one_euro_filter_array.ts, line 22 at r6 (raw file):

Previously, lina128 (Na Li) wrote…

Please use the shared one_euro_filter once this PR is merged: #632
Also as discussed offline, please remove the smoothing/filter part from this PR to unblock it and also keep the size of the PR smaller.

Added a TODO to merge our filter with the existing one as soon as possible. I have moved the filter file back to the MoveNet source.

ronnyvotel

Reviewed 1 of 14 files at r1, 5 of 8 files at r6, 5 of 5 files at r7, 2 of 2 files at r8.
Reviewable status: 0 of 1 approvals obtained (waiting on @ardoerlemans, @lina128, and @pyu10055)

pose-detection/demo/src/params.js, line 29 at r7 (raw file):

  camera: {targetFPS: 60, sizeOption: '640 X 480'},
  model: {
    model: posedetection.SupportedModels.PoseNet,

Can you replace this with MoveNet? Perhaps also re-order in the GUI console.

ardoerlemans

Reviewable status: 0 of 1 approvals obtained (waiting on @lina128, @pyu10055, and @ronnyvotel)

pose-detection/demo/src/params.js, line 29 at r7 (raw file):

Previously, ronnyvotel wrote…

Can you replace this with MoveNet? Perhaps also re-order in the GUI console.

Done.

pyu10055

Reviewable status: 0 of 1 approvals obtained (waiting on @ardoerlemans, @lina128, @pyu10055, and @ronnyvotel)

pose-detection/src/movenet/detector.ts, line 272 at r6 (raw file):

Quoted 6 lines of code…

    for (const key of Object.keys(targetKeypoints)) {
      targetKeypoints[key] = [
        keypoints[KPT_INDICES[key]].y * webcamHeight,
        keypoints[KPT_INDICES[key]].x * webcamWidth
      ];
    }

Will following work?

    const targetKeypoints: {[index: string]: number[]} = {};
    for (const key of Object.keys(KPT_INDICES)) {
      targetKeypoints[key] = [
        keypoints[KPT_INDICES[key]].y * webcamHeight,
        keypoints[KPT_INDICES[key]].x * webcamWidth
      ];
    }

ronnyvotel

Thanks

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @ardoerlemans, @lina128, and @pyu10055)

ardoerlemans

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128 and @pyu10055)

pose-detection/src/movenet/detector.ts, line 272 at r6 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

    for (const key of Object.keys(targetKeypoints)) {
      targetKeypoints[key] = [
        keypoints[KPT_INDICES[key]].y * webcamHeight,
        keypoints[KPT_INDICES[key]].x * webcamWidth
      ];
    }

Will following work?

    const targetKeypoints: {[index: string]: number[]} = {};
    for (const key of Object.keys(KPT_INDICES)) {
      targetKeypoints[key] = [
        keypoints[KPT_INDICES[key]].y * webcamHeight,
        keypoints[KPT_INDICES[key]].x * webcamWidth
      ];
    }

Yes, that works! Thanks! Updated the code accordingly.

pyu10055

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @ardoerlemans, @lina128, and @pyu10055)

pose-detection/src/types.ts, line 20 at r10 (raw file):

export enum SupportedModels {
  // Ordered by speed/accuracy.

Please remove this comment, TFJS is not imposing opinions on the models.

ardoerlemans

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128 and @pyu10055)

pose-detection/src/types.ts, line 20 at r10 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

Please remove this comment, TFJS is not imposing opinions on the models.

Done.

pyu10055

Thank you for the great work!

lina128

Reviewable status: complete! 2 of 1 approvals obtained (waiting on @ardoerlemans, @lina128, and @pyu10055)

pose-detection/demo/src/index.js, line 23 at r10 (raw file):

import * as tf from '@tensorflow/tfjs-core';

import {ARCHITECTURE_SINGLEPOSE_LIGHTNING, ARCHITECTURE_SINGLEPOSE_THUNDER} from '../../src/movenet/constants';

The demo shouldn't import from any src code, export the constants and use from posedetection api, something like posedetection.movenet.modelType.SINGLEPOSE_LIGHTNING

pose-detection/demo/src/index.js, line 49 at r10 (raw file):

      if (STATE.model[STATE.model.model].modelType == 'Lightning') {
        modelType = ARCHITECTURE_SINGLEPOSE_LIGHTNING;
      }

Can you shorten this three lines as a ternary?

pose-detection/demo/src/option_panel.js, line 70 at r10 (raw file):

  const moveNetTypeController = moveNetFolder.add(
      STATE.model[posedetection.SupportedModels.MoveNet], 'modelType',
      ['Thunder', 'Lightning']);

Let's simply this logic. So right now, the logic is nested, you first choose a model in the Model folder, then you can also choose a model in the MoveNet Config folder. Can we just list all possible models in the top level Model folder?

pose-detection/demo/src/option_panel.js, line 72 at r10 (raw file):

      ['Thunder', 'Lightning']);
  moveNetTypeController.onChange(type => {
    STATE.changeToModel = STATE.model;

Shouldn't it be

STATE.changeToModel = type;

pose-detection/src/types.ts, line 20 at r10 (raw file):

export enum SupportedModels {
  // Ordered by speed/accuracy.

Please remove.

pose-detection/src/movenet/constants.ts, line 20 at r10 (raw file):

import {MoveNetEstimationConfig, MoveNetModelConfig} from './types';

export const ARCHITECTURE_SINGLEPOSE_LIGHTNING = 'SinglePose.Lightning';

Can we shorten the variable names so that users don't need to type a long name? Can I suggest SINGLEPOSE_LIGHTNING?

pose-detection/src/movenet/constants.ts, line 23 at r10 (raw file):

export const ARCHITECTURE_SINGLEPOSE_THUNDER = 'SinglePose.Thunder';

export const VALID_ARCHITECTURES =

Do you need this list to be a constant? Is it used in more than one place? And can it just be ARCHITECTURES? Or even better MODELS. I think users are not very familiar with architectures.

pose-detection/src/movenet/constants.ts, line 45 at r10 (raw file):

export const MIN_CROP_KEYPOINT_SCORE = 0.3;

export const KPT_INDICES: {[index: string]: number} = {

Can you move this to the top level constants and rename it COCO_KEYPOINTS_NAMED_MAP

pose-detection/src/movenet/detector.ts, line 60 at r3 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

We use performance.now() to determine the time between frames. The filter was designed for streams of keypoints, but do you think this will be used on individual images as well? In that case I should probably add a parameter that allows users to disable the filter?

Yes, it will be great to add a parameter to disable filter. This can enable use cases that use pose detection on static images.

pose-detection/src/movenet/detector.ts, line 57 at r10 (raw file):

    }

    this.previousFrameTime = 0;

This can be set during member declaration.

pose-detection/src/movenet/detector.ts, line 60 at r10 (raw file):

    // This will be used to calculate the actual camera fps. Starts with 30 fps
    // as an assumption.
    this.frameTimeDiff = 0.0333;

The initial frequency can be a constant, right?

pose-detection/src/movenet/detector.ts, line 119 at r10 (raw file):

    }

    // Keep track of fps for one euro filter. The 'window' check is to make sure

Why not use video.currentTime for time tracking? Isn't it more accurate? A video can pause, right?

pose-detection/src/movenet/detector.ts, line 122 at r10 (raw file):

    // the detector can run in NodeJS, that doesn't have performance.now()
    // without require('perf_hooks').
    if (typeof window !== 'undefined') {

You can use tf.util.now(). https://js.tensorflow.org/api/latest/#util.now
This allows the model to run in Node.js environment.

pose-detection/src/movenet/detector.ts, line 133 at r10 (raw file):

    const imageTensor3D = toImageTensor(image);
    const imageHeight = imageTensor3D.shape[0];

Use getImageSize()

pose-detection/src/movenet/detector.ts, line 157 at r10 (raw file):

            [this.modelInputResolution.height, this.modelInputResolution.width];
        return tf.cast(
            tf.image.cropAndResize(

Just want to make sure, your image will lose the original aspect ratio, is this how the model is trained too? If you need to keep aspect ratio, please use convert_image_to_tensor.ts in the shared calculator. Pass the this.cropRegion to the normRect parameter.

pose-detection/src/movenet/detector.ts, line 163 at r10 (raw file):

      });

      // Run cropModel. Model will dispose croppedImage.

Why not dispose the croppedImage in this method? Otherwise, the dispose is all over the place. It will be hard to track where tensor is disposed.

pose-detection/src/movenet/detector.ts, line 167 at r10 (raw file):

      // Convert keypoints to image coordinates.
      const cropHeight = this.cropRegion[2] - this.cropRegion[0];

Can you add an annotation about what cropRegion[0]-cropRegion[4] mean?

pose-detection/src/movenet/detector.ts, line 189 at r10 (raw file):

      // Use exponential filter on the cropping region to make it less jittery.
      if (newCropRegion != null) {
        newCropRegion = newCropRegion.map(x => x * 0.9);

Create a constant for 0.9

pose-detection/src/movenet/detector.ts, line 190 at r10 (raw file):

      if (newCropRegion != null) {
        newCropRegion = newCropRegion.map(x => x * 0.9);
        this.cropRegion = this.cropRegion.map(x => x * 0.1);

Create a constant for 0.1

pose-detection/src/movenet/detector.ts, line 191 at r10 (raw file):

        newCropRegion = newCropRegion.map(x => x * 0.9);
        this.cropRegion = this.cropRegion.map(x => x * 0.1);
        this.cropRegion = this.cropRegion.map((e, i) => e + newCropRegion[i]);

This is basically a low pass filter, right? Can you use the low_pass_filter in the shared calculator? This can be a TODO and actual change can be in a follow up PR.

pose-detection/src/movenet/detector.ts, line 204 at r10 (raw file):

      resizedImage.dispose();

      // Model will dispose resizedImageInt.

Same, dispose here, instead of in another method.

pose-detection/src/movenet/detector.ts, line 214 at r10 (raw file):

      // Determine crop region based on detected keypoints.
      this.cropRegion = this.determineCropRegion(
          keypoints, imageTensor4D.shape[1], imageTensor4D.shape[2]);

Is shape[1] and shape[2] corresponding to the original image size? If so, can you use imageSize = getImageSize(image); imageSize.y; imageSize.x; This is more readable.

pose-detection/src/movenet/detector.ts, line 220 at r10 (raw file):

    // Convert keypoint coordinates from normalized coordinates to image space
    // and leave out keypoints that don't have a high enough confidence.

Remove the second line.

pose-detection/src/movenet/detector.ts, line 227 at r10 (raw file):

    const poses: Pose[] = [];
    poses[0] = {'keypoints': keypoints};

Is there an overall pose score?

pose-detection/src/movenet/detector.ts, line 232 at r10 (raw file):

  }

  torsoVisible(keypoints: Keypoint[]) {

Add return type. Also this is a pure function, can you move it to movenet/calculators/torso_visible.ts

pose-detection/src/movenet/detector.ts, line 243 at r10 (raw file):

  determineTorsoAndBodyRange(
      keypoints: Keypoint[], targetKeypoints: {[index: string]: number[]},

Maybe add some annotation to this method? What is targetKeypoints?

pose-detection/src/movenet/detector.ts, line 244 at r10 (raw file):

  determineTorsoAndBodyRange(
      keypoints: Keypoint[], targetKeypoints: {[index: string]: number[]},
      centerY: number, centerX: number) {

Add return type

pose-detection/src/movenet/detector.ts, line 246 at r10 (raw file):

      centerY: number, centerX: number) {
    const torsoJoints =
        ['left_shoulder', 'right_shoulder', 'left_hip', 'right_hip'];

Can we move this to constants.ts?

pose-detection/src/movenet/detector.ts, line 279 at r11 (raw file):

  determineCropRegion(
      keypoints: Keypoint[], webcamHeight: number, webcamWidth: number) {

Can this be imageHeight?

pose-detection/src/movenet/detector.ts, line 289 at r11 (raw file):

    }

    if (this.torsoVisible(keypoints)) {

Can you add some annotation here about the algorithm? Like how does it calculate the cropped region using the center point between two hip points and torso and body range?

pose-detection/src/movenet/keypoint_model.ts, line 33 at r11 (raw file):

   * @param fromTfHub Indicates whether the model is hosted on TF Hub.
   */
  async load(url: string, fromTfHub = false) {

Can we move this load function to the load function in MoveNetDetection class? I think you don't need this model any more, you can move the detectKeypoints to the MoveNetDetection class too.

ardoerlemans

Reviewable status: complete! 2 of 1 approvals obtained (waiting on @lina128 and @pyu10055)

pose-detection/demo/src/index.js, line 23 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

The demo shouldn't import from any src code, export the constants and use from posedetection api, something like posedetection.movenet.modelType.SINGLEPOSE_LIGHTNING

Done.

pose-detection/demo/src/index.js, line 49 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Can you shorten this three lines as a ternary?

Done.

pose-detection/demo/src/option_panel.js, line 70 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Let's simply this logic. So right now, the logic is nested, you first choose a model in the Model folder, then you can also choose a model in the MoveNet Config folder. Can we just list all possible models in the top level Model folder?

This is similar to choosing the quantization or depth multiplier with PoseNet: based on those settings, PoseNet will load a different model. We could also have specified 'Resolution 256x256, dm1.75' instead of Thunder, but it's the same sort of setting.

pose-detection/demo/src/option_panel.js, line 72 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Shouldn't it be
STATE.changeToModel = type;

In checkGuiUpdate the value of STATE.changeToModel is actually not used, but I agree that it makes more sense to use 'type' here.

pose-detection/src/types.ts, line 20 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Please remove.

Done.

pose-detection/src/movenet/constants.ts, line 20 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Can we shorten the variable names so that users don't need to type a long name? Can I suggest SINGLEPOSE_LIGHTNING?

Done.

pose-detection/src/movenet/constants.ts, line 23 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Do you need this list to be a constant? Is it used in more than one place? And can it just be ARCHITECTURES? Or even better MODELS. I think users are not very familiar with architectures.

I copied the terminology from PoseNet, where 'architecture' is used. I'll update this to MODELS.

pose-detection/src/movenet/constants.ts, line 45 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Can you move this to the top level constants and rename it COCO_KEYPOINTS_NAMED_MAP

Done.

pose-detection/src/movenet/detector.ts, line 60 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

Yes, it will be great to add a parameter to disable filter. This can enable use cases that use pose detection on static images.

I'll send you a follow up PR for that.

pose-detection/src/movenet/detector.ts, line 57 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

This can be set during member declaration.

Done.

pose-detection/src/movenet/detector.ts, line 60 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

The initial frequency can be a constant, right?

This variable is actually updated for every frame.

pose-detection/src/movenet/detector.ts, line 119 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Why not use video.currentTime for time tracking? Isn't it more accurate? A video can pause, right?

The input to estimatePoses is not always a video, but can also be a tensor and in that case we don't have access to the currentTime.

pose-detection/src/movenet/detector.ts, line 122 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

You can use tf.util.now(). https://js.tensorflow.org/api/latest/#util.now
This allows the model to run in Node.js environment.

Great suggestion! Updated.

pose-detection/src/movenet/detector.ts, line 133 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Use getImageSize()

Done.

pose-detection/src/movenet/detector.ts, line 157 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Just want to make sure, your image will lose the original aspect ratio, is this how the model is trained too? If you need to keep aspect ratio, please use convert_image_to_tensor.ts in the shared calculator. Pass the this.cropRegion to the normRect parameter.

The crop will actually be a square (see cropSize above) so this will not change the aspect ratio.

pose-detection/src/movenet/detector.ts, line 163 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Why not dispose the croppedImage in this method? Otherwise, the dispose is all over the place. It will be hard to track where tensor is disposed.

Done.

pose-detection/src/movenet/detector.ts, line 167 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Can you add an annotation about what cropRegion[0]-cropRegion[4] mean?

Done.

pose-detection/src/movenet/detector.ts, line 189 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Create a constant for 0.9

Done.

pose-detection/src/movenet/detector.ts, line 190 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Create a constant for 0.1

Done.

pose-detection/src/movenet/detector.ts, line 191 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

This is basically a low pass filter, right? Can you use the low_pass_filter in the shared calculator? This can be a TODO and actual change can be in a follow up PR.

It is. I'll add the TODO.

pose-detection/src/movenet/detector.ts, line 204 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Same, dispose here, instead of in another method.

Done.

pose-detection/src/movenet/detector.ts, line 214 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Is shape[1] and shape[2] corresponding to the original image size? If so, can you use imageSize = getImageSize(image); imageSize.y; imageSize.x; This is more readable.

Done.

pose-detection/src/movenet/detector.ts, line 220 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Remove the second line.

Done.

pose-detection/src/movenet/detector.ts, line 227 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Is there an overall pose score?

There isn't.

pose-detection/src/movenet/detector.ts, line 232 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Add return type. Also this is a pure function, can you move it to movenet/calculators/torso_visible.ts

Added a return type, but I think it will make it harder to understand for people reading determineCropRegion if they have to go to another file to read what this function does.

pose-detection/src/movenet/detector.ts, line 243 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Maybe add some annotation to this method? What is targetKeypoints?

Done.

pose-detection/src/movenet/detector.ts, line 244 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Add return type

Done.

pose-detection/src/movenet/detector.ts, line 246 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Can we move this to constants.ts?

This is only used in this method, so for a reader it will make it more difficult if they have to go to another file to see what's in the array.

pose-detection/src/movenet/detector.ts, line 279 at r11 (raw file):

Previously, lina128 (Na Li) wrote…

Can this be imageHeight?

Done.

pose-detection/src/movenet/detector.ts, line 289 at r11 (raw file):

Previously, lina128 (Na Li) wrote…

Can you add some annotation here about the algorithm? Like how does it calculate the cropped region using the center point between two hip points and torso and body range?

Done.

pose-detection/src/movenet/keypoint_model.ts, line 33 at r11 (raw file):

Previously, lina128 (Na Li) wrote…

Can we move this load function to the load function in MoveNetDetection class? I think you don't need this model any more, you can move the detectKeypoints to the MoveNetDetection class too.

Done.

lina128

Great work, thank you Ard!

Reviewed 1 of 4 files at r9, 1 of 3 files at r10, 1 of 1 files at r11, 4 of 9 files at r12.
Reviewable status: complete! 3 of 1 approvals obtained (waiting on @ardoerlemans, @lina128, and @pyu10055)

pose-detection/demo/src/option_panel.js, line 70 at r10 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

This is similar to choosing the quantization or depth multiplier with PoseNet: based on those settings, PoseNet will load a different model. We could also have specified 'Resolution 256x256, dm1.75' instead of Thunder, but it's the same sort of setting.

Got it.

pose-detection/demo/src/option_panel.js, line 72 at r10 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

In checkGuiUpdate the value of STATE.changeToModel is actually not used, but I agree that it makes more sense to use 'type' here.

It is used in checkGuiUpdate: https://github.com/tensorflow/tfjs-models/blob/master/pose-detection/demo/src/index.js#L53

pose-detection/src/movenet/detector.ts, line 119 at r10 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

The input to estimatePoses is not always a video, but can also be a tensor and in that case we don't have access to the currentTime.

That's a good point! But what if a video is paused for a long time, won't the large duration skew your filter? Do you think it's possible to expose a parameter to allow user pass in time? This way, they can control where the time comes from. If no time is passed, we use the default video.currentTime. We have an isVideo() method to check whether the input is an HTMLVideoElement.

pose-detection/src/movenet/detector.ts, line 157 at r10 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

The crop will actually be a square (see cropSize above) so this will not change the aspect ratio.

Ah, I see, got it.

pose-detection/src/movenet/detector.ts, line 227 at r10 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

There isn't.

This can just be poses[0] = {keypoints}

pose-detection/src/movenet/detector.ts, line 232 at r10 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

Added a return type, but I think it will make it harder to understand for people reading determineCropRegion if they have to go to another file to read what this function does.

It makes this file smaller, which is also important for readability.

pose-detection/src/movenet/detector.ts, line 289 at r11 (raw file):

Previously, ardoerlemans (Ard Oerlemans) wrote…

Done.

I can't see the annotation.

pose-detection/src/movenet/detector.ts, line 98 at r13 (raw file):

   *     not initialized yet) or if it produced an unexpected tensor size.
   */
  async detectKeypoints(inputImage: tf.Tensor4D, executeSync = true):

nit: I don't think we need executeSync, because when you write the pipeline, you already know whether you can should execute or executeAsync. But this can be changed in a follow up PR.

pose-detection/src/movenet/detector.ts, line 120 at r13 (raw file):

        outputTensor.shape[2] !== numKeypoints || outputTensor.shape[3] !== 3) {
      outputTensor.dispose();
      return null;

I think it's better to not early return, because the filter is stateful. In the case there's no pose in the frame, the filter should be reset. So it's better to let the result propagate. But this can be in a follow-up PR.

pose-detection/src/movenet/detector.ts, line 130 at r13 (raw file):

    for (let i = 0; i < numKeypoints; ++i) {
      keypoints[i] = {
        y: inferenceResult[i * 3],

Better to have some annotation about the shape of the outputTensor, so that reader can understand where the number 3 comes from here.

pose-detection/src/movenet/detector.ts, line 298 at r13 (raw file):

   * @param targetKeypoints Maps from joint names to coordinates.
   */
  determineTorsoAndBodyRange(

I realize this is also a pure function, I recommend move it to another file to make the class file shorter. It is easier to read through.

pose-detection/src/movenet/detector.ts, line 344 at r13 (raw file):

   * function returns a default crop which is the full image padded to square.
   */
  determineCropRegion(

This can also be moved a separate file, in general, a file that's too long is hard to read. It's good practice to write pure functions and wrap them separately, this also has the benefit of easier to write unit test.

pose-detection/src/movenet/detector.ts, line 384 at r13 (raw file):

      const cropLength = cropLengthHalf * 2;
      const cropRegion = [

Can you use the BoundingBox type from src/calculators/interfaces/shape_interfaces? It's easier to read. Can be in a follow-up PR.

pose-detection/src/movenet/detector.ts, line 395 at r13 (raw file):

  }

  keypointsToArray(keypoints: Keypoint[]) {

Move this to another file.

pose-detection/src/movenet/detector.ts, line 404 at r13 (raw file):

  }

  arrayToKeypoints(values: number[], keypoints: Keypoint[]) {

Move this to another file.

ardoerlemans

Thank you all for spending all this time on the review and for providing so many great suggestions! There will be more PRs coming your way soon to fix the remaining concerns.

Reviewable status: complete! 3 of 1 approvals obtained (waiting on @lina128 and @pyu10055)

pose-detection/demo/src/option_panel.js, line 70 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Got it.

Done.

pose-detection/demo/src/option_panel.js, line 72 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

It is used in checkGuiUpdate: https://github.com/tensorflow/tfjs-models/blob/master/pose-detection/demo/src/index.js#L53

There's only a check for not null, but the actual value doesn't matter.

pose-detection/src/movenet/detector.ts, line 119 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

That's a good point! But what if a video is paused for a long time, won't the large duration skew your filter? Do you think it's possible to expose a parameter to allow user pass in time? This way, they can control where the time comes from. If no time is passed, we use the default video.currentTime. We have an isVideo() method to check whether the input is an HTMLVideoElement.

Yeah, I can definitely add that! I'll send you a separate PR. You're right that long pauses will skew the filter. (We could also detect those though and reset the filter.)

pose-detection/src/movenet/detector.ts, line 157 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

Ah, I see, got it.

Done.

pose-detection/src/movenet/detector.ts, line 227 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

This can just be poses[0] = {keypoints}

Done.

pose-detection/src/movenet/detector.ts, line 232 at r10 (raw file):

Previously, lina128 (Na Li) wrote…

It makes this file smaller, which is also important for readability.

I don't think I agree in this case. :) In general, I do also prefer smaller files though.

pose-detection/src/movenet/detector.ts, line 289 at r11 (raw file):

Previously, lina128 (Na Li) wrote…

I can't see the annotation.

There's a comment above the method: https://github.com/ardoerlemans/tfjs-models/blob/cb5ad55a1f9657f095f3a705e855e9d5b93b887f/pose-detection/src/movenet/detector.ts#L335

I hope that works for you?

pose-detection/src/movenet/detector.ts, line 98 at r13 (raw file):

Previously, lina128 (Na Li) wrote…

nit: I don't think we need executeSync, because when you write the pipeline, you already know whether you can should execute or executeAsync. But this can be changed in a follow up PR.

It really depends on the model that we run. If we go multi-person and it contains a control flow op, we need to have the option to run synchronously. But I also agree that we don't currently need it.

pose-detection/src/movenet/detector.ts, line 120 at r13 (raw file):

Previously, lina128 (Na Li) wrote…

I think it's better to not early return, because the filter is stateful. In the case there's no pose in the frame, the filter should be reset. So it's better to let the result propagate. But this can be in a follow-up PR.

Good point. I'll take a note and fix that next week.

pose-detection/src/movenet/detector.ts, line 130 at r13 (raw file):

Previously, lina128 (Na Li) wrote…

Better to have some annotation about the shape of the outputTensor, so that reader can understand where the number 3 comes from here.

That explanation is actually a few lines above it: https://github.com/ardoerlemans/tfjs-models/blob/cb5ad55a1f9657f095f3a705e855e9d5b93b887f/pose-detection/src/movenet/detector.ts#L114

pose-detection/src/movenet/detector.ts, line 298 at r13 (raw file):

Previously, lina128 (Na Li) wrote…

I realize this is also a pure function, I recommend move it to another file to make the class file shorter. It is easier to read through.

Will include this in the cropping refactor.

pose-detection/src/movenet/detector.ts, line 344 at r13 (raw file):

Previously, lina128 (Na Li) wrote…

This can also be moved a separate file, in general, a file that's too long is hard to read. It's good practice to write pure functions and wrap them separately, this also has the benefit of easier to write unit test.

I think I'm starting to see your point here. Ok, I'll make a note of this too and move all the cropping code to a separate file, with a unit test in a follow up PR.

pose-detection/src/movenet/detector.ts, line 384 at r13 (raw file):

Previously, lina128 (Na Li) wrote…

Can you use the BoundingBox type from src/calculators/interfaces/shape_interfaces? It's easier to read. Can be in a follow-up PR.

Ok, noted as well.

pose-detection/src/movenet/detector.ts, line 395 at r13 (raw file):

Previously, lina128 (Na Li) wrote…

Move this to another file.

Will include this in the cropping code refactor.

pose-detection/src/movenet/detector.ts, line 404 at r13 (raw file):

Previously, lina128 (Na Li) wrote…

Move this to another file.

Will include this in the cropping code refactor.

ardoerlemans added 5 commits March 30, 2021 20:10

Adding MoveNet skeleton and support files

f446450

Adding MoveNet implementation and supporting files

f7d3f93

Lint MoveNet files (except for line length)

1a30af9

Line lengths will be updated in an upcoming commit.

Use correct formatting, which includes line lengths

feff3eb

Remove test URL

57e4385

ardoerlemans requested a review from lina128 March 31, 2021 21:43

google-cla bot added the cla: yes label Mar 31, 2021

Merge branch 'master' into master

548cbed

lina128 requested a review from pyu10055 April 1, 2021 07:02

lina128 requested changes Apr 1, 2021

View reviewed changes

ronnyvotel reviewed Apr 1, 2021

View reviewed changes

ardoerlemans and others added 4 commits April 1, 2021 13:37

Addressing code review comments

454f4cb

Merge branch 'master' of https://github.com/ardoerlemans/tfjs-models

8414cb5

Merge branch 'master' into master

9745b64

Fix MoveNet visualization in live demo

fc9a3ac

ardoerlemans commented Apr 1, 2021

View reviewed changes

pyu10055 requested changes Apr 1, 2021

View reviewed changes

Updates after review comments

ac2e5f0

MoveNet determineCropRegion refactored and moved OneEuroFilter to common filters directory

ardoerlemans commented Apr 1, 2021

View reviewed changes

lina128 requested changes Apr 1, 2021

View reviewed changes

Remove configurable keypoint threshold

6dc2d52

This threshold was mostly used for internal cropping logic.

ronnyvotel suggested changes Apr 1, 2021

View reviewed changes

ardoerlemans and others added 2 commits April 1, 2021 16:44

Merge Model and KeypointModel, plus additional updates after review

266d0f6

Merge branch 'master' into master

0364b06

ardoerlemans commented Apr 1, 2021

View reviewed changes

ardoerlemans commented Apr 2, 2021

View reviewed changes

lina128 requested changes Apr 2, 2021

View reviewed changes

pyu10055 requested changes Apr 2, 2021

View reviewed changes

Move one euro filter back to MoveNet with TODO

7634af3

ardoerlemans commented Apr 2, 2021

View reviewed changes

Add test for MoveNet

b4dff3c

ronnyvotel suggested changes Apr 2, 2021

View reviewed changes

ardoerlemans added 2 commits April 2, 2021 12:04

Use MoveNet as default model in pose demo

30d536b

Add MoveNet model type selection to pose demo

44770d8

ardoerlemans commented Apr 2, 2021

View reviewed changes

pyu10055 requested changes Apr 2, 2021

View reviewed changes

Run models at highest possible speed in pose demo

b483506

ronnyvotel approved these changes Apr 2, 2021

View reviewed changes

ardoerlemans commented Apr 2, 2021

View reviewed changes

Fix error in cropping code

e54dd2f

pyu10055 requested changes Apr 2, 2021

View reviewed changes

Remove comment

68e136d

ardoerlemans commented Apr 2, 2021

View reviewed changes

pyu10055 approved these changes Apr 2, 2021

View reviewed changes

lina128 requested changes Apr 2, 2021

View reviewed changes

Updates to resolve review comments

fd718f3

ardoerlemans commented Apr 3, 2021

View reviewed changes

ardoerlemans and others added 2 commits April 2, 2021 17:47

Merge branch 'master' into master

6468bc3

Fix CI test

cb5ad55

lina128 approved these changes Apr 3, 2021

View reviewed changes

ardoerlemans commented Apr 3, 2021

View reviewed changes

Simplify pose array creation

c2d959a

ardoerlemans merged commit 8717622 into tensorflow:master Apr 3, 2021

Adding MoveNet to Pose Detection API #627

Adding MoveNet to Pose Detection API #627

Uh oh!

Conversation

ardoerlemans commented Mar 31, 2021 • edited by nsthorat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lina128 left a comment

Choose a reason for hiding this comment

Uh oh!

ronnyvotel left a comment

Choose a reason for hiding this comment

Uh oh!

ronnyvotel Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

ronnyvotel Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

ronnyvotel Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

ardoerlemans left a comment

Choose a reason for hiding this comment

Uh oh!

pyu10055 left a comment

Choose a reason for hiding this comment

Uh oh!

ardoerlemans left a comment

Choose a reason for hiding this comment

Uh oh!

lina128 left a comment

Choose a reason for hiding this comment

Uh oh!

ronnyvotel left a comment

Choose a reason for hiding this comment

Uh oh!

ardoerlemans left a comment

Choose a reason for hiding this comment

Uh oh!

ardoerlemans left a comment

Choose a reason for hiding this comment

Uh oh!

lina128 left a comment

Choose a reason for hiding this comment

Uh oh!

pyu10055 left a comment

Choose a reason for hiding this comment

Uh oh!

ardoerlemans left a comment

Choose a reason for hiding this comment

Uh oh!

ronnyvotel left a comment

Choose a reason for hiding this comment

Uh oh!

ardoerlemans left a comment

Choose a reason for hiding this comment

Uh oh!

pyu10055 left a comment

Choose a reason for hiding this comment

Uh oh!

ronnyvotel left a comment

Choose a reason for hiding this comment

Uh oh!

ardoerlemans left a comment

Choose a reason for hiding this comment

Uh oh!

pyu10055 left a comment

Choose a reason for hiding this comment

Uh oh!

ardoerlemans left a comment

Choose a reason for hiding this comment

Uh oh!

pyu10055 left a comment

Choose a reason for hiding this comment

Uh oh!

lina128 left a comment

Choose a reason for hiding this comment

Uh oh!

ardoerlemans left a comment

ardoerlemans commented Mar 31, 2021 •

edited by nsthorat

Loading