1299718 - Telemetry to support background video decoder suspend: the usage of a hidden video element as a content source

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Tzuhao Kuo [:kaku]]

To see how often a HIDDEN video element is used as the source of {drawImage(), captureStream() and createImageBitmap()} APIs.

Comment 1

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1299718 part 1 - Histogram for VIDEO_AS_CONTENT_SOURCE;

Review commit: https://reviewboard.mozilla.org/r/76294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76294/

Comment 2

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1299718 part 2 - implement the MarkAsContentSource() API;

Review commit: https://reviewboard.mozilla.org/r/76296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76296/

Comment 3

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1299718 part 3 - call MarkAsContentSource() at where using video element as a source;

Review commit: https://reviewboard.mozilla.org/r/76298/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76298/

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8787551 [details]
Bug 1299718 part 1 - Histogram for VIDEO_AS_CONTENT_SOURCE;

https://reviewboard.mozilla.org/r/76294/#review74380

r+ after you fix the typo:

::: toolkit/components/telemetry/Histograms.json:8506
(Diff revision 1)
>      "bug_numbers": [1294349]
>    },
> +  "VIDEO_AS_CONTENT_SOURCE" : {
> +    "alert_emails": ["[e-mail]", "[e-mail]"],
> +    "expires_in_version": "56",
> +    "description": "Usage of a {visible / invsible} video element as the source of {drawImage(), createPattern(), createImageBitmap() and captureStream()} APIs. 0 = invisible, 1 = visible.",

Typo: 'invsible' -> 'invisible'

Also, from my previous experience, you'll probably need to explicitly say what the keys are (in this case: "drawImage", etc., right?)
But I'll let Francois comment further about the description.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8787552 [details]
Bug 1299718 part 2 - implement the MarkAsContentSource() API;

https://reviewboard.mozilla.org/r/76296/#review74382

r+ with typo and suggestion:

::: dom/html/HTMLMediaElement.h:744
(Diff revision 1)
>    bool ComputedMuted() const;
>    nsSuspendedTypes ComputedSuspended() const;
>  
>    void SetMediaInfo(const MediaInfo aInfo);
>  
> +  // Telemetry: to record the usage of a {visible / invsible} video element as

Typo: 'invsible' -> 'invisible'

::: dom/html/HTMLMediaElement.cpp:6623
(Diff revision 1)
> +      key.AppendASCII("captureStream");
> +      break;
> +    }
> +  }
> +
> +  const bool isVisible = mVisibilityState != Visibility::NONVISIBLE;

Accumulate() takes a uint32_t, so I think you should make 'isVisible' of that type, and explicitly assign the value 0 or 1, as you have described in the histogram.
If you agree, you'll need to change the LOG below to match.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8787553 [details]
Bug 1299718 part 3 - call MarkAsContentSource() at where using video element as a source;

https://reviewboard.mozilla.org/r/76298/#review74386

Comment 7

[u480271]

Comment on attachment 8787552 [details]
Bug 1299718 part 2 - implement the MarkAsContentSource() API;

https://reviewboard.mozilla.org/r/76296/#review74388

Comment 8

[u480271]

Comment on attachment 8787553 [details]
Bug 1299718 part 3 - call MarkAsContentSource() at where using video element as a source;

https://reviewboard.mozilla.org/r/76298/#review74390

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8787553 [details]
Bug 1299718 part 3 - call MarkAsContentSource() at where using video element as a source;

https://reviewboard.mozilla.org/r/76298/#review74694

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8787551 [details]
Bug 1299718 part 1 - Histogram for VIDEO_AS_CONTENT_SOURCE;

https://reviewboard.mozilla.org/r/76294/#review74380

> Typo: 'invsible' -> 'invisible'
> 
> Also, from my previous experience, you'll probably need to explicitly say what the keys are (in this case: "drawImage", etc., right?)
> But I'll let Francois comment further about the description.

Will add the key information, thank you!

Comment 11

[Tzuhao Kuo [:kaku]]

Comment on attachment 8787552 [details]
Bug 1299718 part 2 - implement the MarkAsContentSource() API;

https://reviewboard.mozilla.org/r/76296/#review74382

> Typo: 'invsible' -> 'invisible'

Sorry and thanks!

> Accumulate() takes a uint32_t, so I think you should make 'isVisible' of that type, and explicitly assign the value 0 or 1, as you have described in the histogram.
> If you agree, you'll need to change the LOG below to match.

Okay, will then modify it, thanks!

Comment 12

[Tzuhao Kuo [:kaku]]

Comment on attachment 8787551 [details]
Bug 1299718 part 1 - Histogram for VIDEO_AS_CONTENT_SOURCE;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76294/diff/1-2/

Comment 13

[Tzuhao Kuo [:kaku]]

Comment on attachment 8787552 [details]
Bug 1299718 part 2 - implement the MarkAsContentSource() API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76296/diff/1-2/

Comment 14

[Tzuhao Kuo [:kaku]]

Comment on attachment 8787553 [details]
Bug 1299718 part 3 - call MarkAsContentSource() at where using video element as a source;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76298/diff/1-2/

Comment 15

[Benjamin Smedberg]

Comment on attachment 8787551 [details]
Bug 1299718 part 1 - Histogram for VIDEO_AS_CONTENT_SOURCE;

https://reviewboard.mozilla.org/r/76294/#review75166

::: toolkit/components/telemetry/Histograms.json:8506
(Diff revision 2)
>      "bug_numbers": [1294349]
>    },
> +  "VIDEO_AS_CONTENT_SOURCE" : {
> +    "alert_emails": ["[e-mail]", "[e-mail]"],
> +    "expires_in_version": "56",
> +    "description": "Usage of a {visible / invisible} video element as the source of {drawImage(), createPattern(), createImageBitmap() and captureStream()} APIs. 0 = invisible, 1 = visible. Keyed by different APIs and 'All' will accumulate all occurances",

If this has to be keyed, then I need you to document the key values more exactly. It seems that you can/should use a boolean instead of an enumeration in this case.

However, we should avoid keyed histograms when we don't need them, and I really think we can cover this with a single enumerated histogram:

kind: enumerated
n_values: 10
values: [
  # Recorded every time a visible video is used as a source
  "ALL_VISIBLE",
  # Recorded every time an invisible video is used as a source
  "ALL_INVISIBLE",
  # Recorded when videos are used as a drawImage source
  "drawImage_VISIBLE", "drawImage_INVISIBLE",
  # Recorded when videos are used as a createPattern source
  "createPattern_VISIBLE", "createPattern_INVISIBLE",
  # Recorded when videos are used as a createImageBitmap source
  ...etc
}

Comment 16

[Tzuhao Kuo [:kaku]]

Comment on attachment 8787551 [details]
Bug 1299718 part 1 - Histogram for VIDEO_AS_CONTENT_SOURCE;

https://reviewboard.mozilla.org/r/76294/#review75166

> If this has to be keyed, then I need you to document the key values more exactly. It seems that you can/should use a boolean instead of an enumeration in this case.
> 
> However, we should avoid keyed histograms when we don't need them, and I really think we can cover this with a single enumerated histogram:
> 
> kind: enumerated
> n_values: 10
> values: [
>   # Recorded every time a visible video is used as a source
>   "ALL_VISIBLE",
>   # Recorded every time an invisible video is used as a source
>   "ALL_INVISIBLE",
>   # Recorded when videos are used as a drawImage source
>   "drawImage_VISIBLE", "drawImage_INVISIBLE",
>   # Recorded when videos are used as a createPattern source
>   "createPattern_VISIBLE", "createPattern_INVISIBLE",
>   # Recorded when videos are used as a createImageBitmap source
>   ...etc
> }

Thanks for your suggestion and I will modify the patch accordingly.
However, I would like to make sure the usage of "values" field here since I cannot find it in the MDN document (https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe).

Is the "values" field to the "enumerated" as the "labels" field is to the "enumerated"? If so, how to choose between enumerated and enumerated?

Comment 17

[Tzuhao Kuo [:kaku]]

Comment on attachment 8787551 [details]
Bug 1299718 part 1 - Histogram for VIDEO_AS_CONTENT_SOURCE;

https://reviewboard.mozilla.org/r/76294/#review75166

> Thanks for your suggestion and I will modify the patch accordingly.
> However, I would like to make sure the usage of "values" field here since I cannot find it in the MDN document (https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe).
> 
> Is the "values" field to the "enumerated" as the "labels" field is to the "enumerated"? If so, how to choose between enumerated and enumerated?

Correct typo:

> Is the "values" field to the "enumerated" as the "labels" field is to the "enumerated"? If so, how to choose between enumerated and enumerated?

Should be: ...... as the "labels" field is to the "categorical"? ...


However, the compiler complains that "values" is not permitted with the "enumerated" kind, so I guess that you're just describing the meaning of each enumerated value; or are you suggesting using "categorical" with "labels"?

Comment 18

[Benjamin Smedberg]

I didn't mean strictly JSON "values". I meant that these are the possible values that can be recorded, and you'd note that in the enumerated histogram description.

categorical histograms aren't yet supported on telemetry.mozilla.org (and may not be in the longitudinal dataset), so they aren't ready to use.

Comment 19

[Tzuhao Kuo [:kaku]]

Comment on attachment 8787551 [details]
Bug 1299718 part 1 - Histogram for VIDEO_AS_CONTENT_SOURCE;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76294/diff/2-3/

Comment 20

[Tzuhao Kuo [:kaku]]

Comment on attachment 8787552 [details]
Bug 1299718 part 2 - implement the MarkAsContentSource() API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76296/diff/2-3/

Comment 21

[Tzuhao Kuo [:kaku]]

Comment on attachment 8787553 [details]
Bug 1299718 part 3 - call MarkAsContentSource() at where using video element as a source;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76298/diff/2-3/

Comment 22

[Tzuhao Kuo [:kaku]]

Note that bug 1284350 might have conflict with this bug.

Comment 23

[Tzuhao Kuo [:kaku]]

(In reply to Tzuhao Kuo [:kaku] from comment #19)
> Comment on attachment 8787551 [details]
> Bug 1299718 part 1 - Histogram for VIDEO_AS_CONTENT_SOURCE;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/76294/diff/2-3/
Hello, Benjamin, please help to review this patch, thanks!

Comment 24

[Tzuhao Kuo [:kaku]]

@Dan, thanks for taking care of this bug!

Comment 25

[Benjamin Smedberg]

Comment on attachment 8787551 [details]
Bug 1299718 part 1 - Histogram for VIDEO_AS_CONTENT_SOURCE;

https://reviewboard.mozilla.org/r/76294/#review77064

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa3b9f12a721
part 1 - Histogram for VIDEO_AS_CONTENT_SOURCE; r=bsmedberg,gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/85b3e5ee142b
part 2 - implement the MarkAsContentSource() API; r=gerald,kamidphish
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1cdea819863
part 3 - call MarkAsContentSource() at where using video element as a source; r=gerald,kamidphish,mtseng

Comment 27

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Clearing Benjamin's NI, as he responded in comment 25 -- Thank you.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa3b9f12a721
https://hg.mozilla.org/mozilla-central/rev/85b3e5ee142b
https://hg.mozilla.org/mozilla-central/rev/b1cdea819863