Closed Bug 1299718 Opened 3 years ago Closed 3 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)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: kaku, Assigned: kamidphish)

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.
Assignee: nobody → kaku
Blocks: 1295921, 1276556
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 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 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 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+
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 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 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
}
Attachment #8787551 - Flags: review?(benjamin) → review-
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 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"?
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.
Note that bug 1284350 might have conflict with this bug.
Depends on: 1284350
(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)
@Dan, thanks for taking care of this bug!
Assignee: kaku → dglastonbury
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+
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)
Blocks: 1337301
You need to log in before you can comment on or make changes to this bug.