1261955 - Measure video playback usage

Status: RESOLVED FIXED

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

Description

[Ralph Giles (:rillian)]

This is a tracking bug for thinking about and implementing telemetry as proxies for success or failure for media playback code changes.

Comment 1

[Ralph Giles (:rillian)]

Anthony says we have some expired telemetry, but it needs review before we decide what the measure next.

Comment 2

[Ralph Giles (:rillian)]

Attachment: MozReview Request: Bug 1261955 - Remove VIDEO_MSE_JOIN_LATENCY_MS telemetry probe. r=SingingTree

This is no longer necessary.

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

Comment 3

[Ralph Giles (:rillian)]

Attachment: MozReview Request: Bug 1261955 - Remove VIDEO_MSE_BUFFERING_COUNT telemetry probe. r=SingingTree

This is no longer necessary.

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

Comment 4

[Ralph Giles (:rillian)]

Attachment: MozReview Request: Bug 1261955 - Re-activate VIDEO_PLAY_TIME_MS telemetry probe. r=SingingTree,bsmedberg

Rename the VIDEO_MSE_PLAY_TIME_MS telemetry probe to just
VIDEO_PLAY_TIME_MS and make it active for all video playback.

We were using this to track MSE deployment success. Now we'd
like to do something similar for video playback in general,
regardless of the origin. This allows us to simplify the
collection code somewhat.

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

Comment 5

[Ralph Giles (:rillian)]

Attachment: MozReview Request: Bug 1261955 - Re-activate the VIDEO_UNLOAD_STATE telemetry probe. r=SingingTree,bsmedberg

Rename the VIDEO_MSE_UNLOAD_STATE telemetry probe to just
VIDEO_UNLOAD STATE and reactivate it. We were using this
to track MSE deployment success and would now like to
generalize it to all media playback.

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

Comment 6

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8744091 [details]
MozReview Request: Bug 1261955 - Remove VIDEO_MSE_JOIN_LATENCY_MS telemetry probe. r=SingingTree

https://reviewboard.mozilla.org/r/48267/#review45059

Comment 7

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8744092 [details]
MozReview Request: Bug 1261955 - Remove VIDEO_MSE_BUFFERING_COUNT telemetry probe. r=SingingTree

https://reviewboard.mozilla.org/r/48269/#review45061

Comment 8

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8744093 [details]
MozReview Request: Bug 1261955 - Re-activate VIDEO_PLAY_TIME_MS telemetry probe. r=SingingTree,bsmedberg

https://reviewboard.mozilla.org/r/48271/#review45069

Comment 9

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8744094 [details]
MozReview Request: Bug 1261955 - Re-activate the VIDEO_UNLOAD_STATE telemetry probe. r=SingingTree,bsmedberg

https://reviewboard.mozilla.org/r/48273/#review45073

::: dom/html/HTMLMediaElement.cpp:2848
(Diff revision 1)
>      if (stalled) {
>        state = STALLED;
>      }
>    }
>  
> +  Telemetry::Accumulate(Telemetry::VIDEO_UNLOAD_STATE, state);

Out of interest, is there a reason this was moved above the if block?

Comment 10

[Ralph Giles (:rillian)]

(In reply to Bryce Van Dyk (:SingingTree) from comment #9)

> > +  Telemetry::Accumulate(Telemetry::VIDEO_UNLOAD_STATE, state);
> 
> Out of interest, is there a reason this was moved above the if block?

I moved it to right after `state` is calculated, which is a more logical grouping. The if block is about calculating and reporting VIDEO_DROPPED_FRAMES_PROPORTION and doesn't have anything to do with the unload state.

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8744093 [details]
MozReview Request: Bug 1261955 - Re-activate VIDEO_PLAY_TIME_MS telemetry probe. r=SingingTree,bsmedberg

Adding bsmedberg for data collection review.

Comment 12

[Benjamin Smedberg]

Comment on attachment 8744094 [details]
MozReview Request: Bug 1261955 - Re-activate the VIDEO_UNLOAD_STATE telemetry probe. r=SingingTree,bsmedberg

https://reviewboard.mozilla.org/r/48273/#review47331

::: toolkit/components/telemetry/histogram-whitelists.json:1112
(Diff revision 1)
>      "VIDEO_DECODED_H264_SPS_LEVEL",
>      "VIDEO_DECODED_H264_SPS_PROFILE",
>      "VIDEO_EME_PLAY_SUCCESS",
>      "VIDEO_H264_SPS_MAX_NUM_REF_FRAMES",
>      "VIDEO_PLAY_TIME_MS",
> -    "VIDEO_MSE_UNLOAD_STATE",
> +    "VIDEO_UNLOAD_STATE",

Because you added alert_emails and bug_numbers, it should not be necessary for this to be in histogram-whitelists and more. Please remove it.

Comment 13

[Ralph Giles (:rillian)]

Will do. Thanks for the review!

Comment 14

[Ralph Giles (:rillian)]

Comment on attachment 8744091 [details]
MozReview Request: Bug 1261955 - Remove VIDEO_MSE_JOIN_LATENCY_MS telemetry probe. r=SingingTree

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

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 8744092 [details]
MozReview Request: Bug 1261955 - Remove VIDEO_MSE_BUFFERING_COUNT telemetry probe. r=SingingTree

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

Comment 16

[Ralph Giles (:rillian)]

Comment on attachment 8744093 [details]
MozReview Request: Bug 1261955 - Re-activate VIDEO_PLAY_TIME_MS telemetry probe. r=SingingTree,bsmedberg

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

Comment 17

[Ralph Giles (:rillian)]

Comment on attachment 8744094 [details]
MozReview Request: Bug 1261955 - Re-activate the VIDEO_UNLOAD_STATE telemetry probe. r=SingingTree,bsmedberg

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

Comment 18

[Ralph Giles (:rillian)]

https://reviewboard.mozilla.org/r/48273/#review47367

Comment 19

[Benjamin Smedberg]

Comment on attachment 8744093 [details]
MozReview Request: Bug 1261955 - Re-activate VIDEO_PLAY_TIME_MS telemetry probe. r=SingingTree,bsmedberg

https://reviewboard.mozilla.org/r/48271/#review47529

::: toolkit/components/telemetry/Histograms.json:8954
(Diff revision 2)
>      "description": "EME video playback success or failure"
>    },
> -  "VIDEO_MSE_PLAY_TIME_MS" : {
> -    "expires_in_version": "45",
> -    "description": "Total time spent playing MSE video",
> +  "VIDEO_PLAY_TIME_MS" : {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "55",
> +    "description": "Total time spent playing video",

I know this predates this commit, but it would be helpful to specify more about how this is recorded. Most important, what unit (seconds or milliseconds). Also, is this recorded once per <video> when the video is unloaded? That gives you the advantage of being able to count the number of videos that existed by seeing how many values were recorded.

data-review=me in any case, but improving this would be really nice

Comment 20

[Ralph Giles (:rillian)]

Comment on attachment 8744091 [details]
MozReview Request: Bug 1261955 - Remove VIDEO_MSE_JOIN_LATENCY_MS telemetry probe. r=SingingTree

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

Comment 21

[Ralph Giles (:rillian)]

Comment on attachment 8744092 [details]
MozReview Request: Bug 1261955 - Remove VIDEO_MSE_BUFFERING_COUNT telemetry probe. r=SingingTree

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

Comment 22

[Ralph Giles (:rillian)]

Comment on attachment 8744093 [details]
MozReview Request: Bug 1261955 - Re-activate VIDEO_PLAY_TIME_MS telemetry probe. r=SingingTree,bsmedberg

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

Comment 23

[Ralph Giles (:rillian)]

Comment on attachment 8744094 [details]
MozReview Request: Bug 1261955 - Re-activate the VIDEO_UNLOAD_STATE telemetry probe. r=SingingTree,bsmedberg

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

Comment 24

[Ralph Giles (:rillian)]

https://reviewboard.mozilla.org/r/48271/#review47561

Ok, thanks. I've updated the description with more information.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44b85a2e805e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c901bd6c5a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/e702c0b87823
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6c715ede6e

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/44b85a2e805e
https://hg.mozilla.org/mozilla-central/rev/9c901bd6c5a5
https://hg.mozilla.org/mozilla-central/rev/e702c0b87823
https://hg.mozilla.org/mozilla-central/rev/cf6c715ede6e