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

RESOLVED FIXED in Firefox 51

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kaku, Assigned: kamidphish)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

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

Updated

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

Comment 4

2 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

2 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

2 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+
(Assignee)

Comment 7

2 years 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

2 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/#review74390
Attachment #8787553 - Flags: review?(dglastonbury) → review+

Comment 9

2 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

2 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

2 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

2 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

2 years ago
Attachment #8787551 - Flags: review?(benjamin) → review-
(Reporter)

Comment 16

2 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

2 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

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

Comment 22

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

Comment 23

2 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

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

Comment 25

2 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

2 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

2 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
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Reporter)

Updated

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