Closed
Bug 1299718
Opened 8 years ago
Closed 8 years ago
Telemetry to support background video decoder suspend: the usage of a hidden video element as a content source
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: kaku, Assigned: u480271)
References
(Blocks 2 open bugs)
Details
Attachments
(3 files)
To see how often a HIDDEN video element is used as the source of {drawImage(), captureStream() and createImageBitmap()} APIs.
Reporter | ||
Updated•8 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•8 years 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•8 years 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•8 years 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+
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+
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•8 years 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•8 years 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•8 years 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•8 years 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•8 years ago
|
Attachment #8787551 -
Flags: review?(benjamin) → review-
Reporter | ||
Comment 16•8 years 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•8 years 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•8 years 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.
Updated•8 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 22•8 years ago
|
||
Note that bug 1284350 might have conflict with this bug.
Depends on: 1284350
Reporter | ||
Comment 23•8 years 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•8 years ago
|
||
@Dan, thanks for taking care of this bug!
Assignee: kaku → dglastonbury
Comment 25•8 years 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•8 years 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•8 years 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
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•