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

RESOLVED FIXED in Firefox 51

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: kaku, Assigned: kamidphish)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

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

Updated

a year ago
Assignee: nobody → kaku
Blocks: 1295921, 1276556
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
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": ["ajones@mozilla.com", "kaku@mozilla.com"],
> +    "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.
Attachment #8787551 - Flags: review?(gsquelart) → review+

Comment 5

a year ago
mozreview-review
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.
Attachment #8787552 - Flags: review?(gsquelart) → review+

Comment 6

a year ago
mozreview-review
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
Attachment #8787553 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 7

a year ago
mozreview-review
Comment on attachment 8787552 [details]
Bug 1299718 part 2 - implement the MarkAsContentSource() API;

https://reviewboard.mozilla.org/r/76296/#review74388
Attachment #8787552 - Flags: review?(dglastonbury) → review+
(Assignee)

Comment 8

a year ago
mozreview-review
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
Attachment #8787553 - Flags: review?(dglastonbury) → review+

Comment 9

a year ago
mozreview-review
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
Attachment #8787553 - Flags: review?(mtseng) → review+
(Reporter)

Comment 10

a year ago
mozreview-review-reply
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!
(Reporter)

Comment 11

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

a year ago
mozreview-review
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": ["ajones@mozilla.com", "kaku@mozilla.com"],
> +    "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
}

Updated

a year ago
Attachment #8787551 - Flags: review?(benjamin) → review-
(Reporter)

Comment 16

a year ago
mozreview-review-reply
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?
(Reporter)

Comment 17

a year ago
mozreview-review-reply
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

a year ago
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.
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 22

a year ago
Note that bug 1284350 might have conflict with this bug.
Depends on: 1284350
(Reporter)

Comment 23

a year ago
(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!
Flags: needinfo?(benjamin)
(Reporter)

Comment 24

a year ago
@Dan, thanks for taking care of this bug!
Assignee: kaku → dglastonbury

Comment 25

a year ago
mozreview-review
Comment on attachment 8787551 [details]
Bug 1299718 part 1 - Histogram for VIDEO_AS_CONTENT_SOURCE;

https://reviewboard.mozilla.org/r/76294/#review77064
Attachment #8787551 - Flags: review?(benjamin) → review+

Comment 26

a year ago
Pushed by dglastonbury@mozilla.com:
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
Clearing Benjamin's NI, as he responded in comment 25 -- Thank you.
Flags: needinfo?(benjamin)

Comment 28

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aa3b9f12a721
https://hg.mozilla.org/mozilla-central/rev/85b3e5ee142b
https://hg.mozilla.org/mozilla-central/rev/b1cdea819863
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Reporter)

Updated

10 months ago
Blocks: 1337301
You need to log in before you can comment on or make changes to this bug.