1294349 - Telemetry to support background video decoder suspend: Recovery time from video-decode-suspended

Status: RESOLVED FIXED

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

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

To help tweak the decoder-suspend functionality, we want to measure how long it takes to recover from the video being suspended.
Telemetry will be keyed by audio presence ('AV' with audio, 'V' video only), hardware acceleration '(hw)', and by resolution ranges (e.g. '721-1080'). An 'All' key will also accumulate all numbers.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1294349 - Histogram for VIDEO_SUSPEND_RECOVERY_TIME_MS -

Telemetry probe, for datareview (using r? in mozreview) by telemetry peer.

This probe records how long it takes to recover from the video being suspended.
This will help with gradually default-enabling video-suspend for "safe" videos.

Also fixed some previous telemetry descriptions that had an incorrect example
resolution (2180 instead of 2160).

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

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1294349 - Report VIDEO_SUSPEND_RECOVERY_TIME_MS -

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

Comment 3

[Tzuhao Kuo [:kaku]]

Comment on attachment 8779989 [details]
Bug 1294349 - Histogram for VIDEO_SUSPEND_RECOVERY_TIME_MS -

https://reviewboard.mozilla.org/r/70850/#review68244

Comment 4

[Tzuhao Kuo [:kaku]]

Comment on attachment 8779990 [details]
Bug 1294349 - Report VIDEO_SUSPEND_RECOVERY_TIME_MS -

https://reviewboard.mozilla.org/r/70852/#review68246

::: dom/media/MediaDecoderStateMachine.cpp:1467
(Diff revision 1)
> +  // Local reference to mInfo, so that it will be copied in the lambda below.
> +  MediaInfo& info = mInfo;
> +
> +  bool hw = mReader->VideoIsHardwareAccelerated();
> +
>    // Do the seek.
>    mSeekTaskRequest.Begin(
> -    mSeekTask->Seek(Duration())->Then(OwnerThread(), __func__, this,
> +    mSeekTask->Seek(Duration())
> +      ->Then(OwnerThread(), __func__, this,
> -                                      &MediaDecoderStateMachine::OnSeekTaskResolved,
> +             &MediaDecoderStateMachine::OnSeekTaskResolved,
> -                                      &MediaDecoderStateMachine::OnSeekTaskRejected));
> +             &MediaDecoderStateMachine::OnSeekTaskRejected)
> +      ->CompletionPromise()
> +      ->Then(AbstractThread::MainThread(), __func__,
> +             [start, info, hw](){ ReportRecoveryTelemetry(start, info, hw); },
> +             [](){}));

bug 1294352 merged InitiateDecodeRecoverySeek() with InitiateSeek(). You might encounter merging conflict while landing.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8779990 [details]
Bug 1294349 - Report VIDEO_SUSPEND_RECOVERY_TIME_MS -

https://reviewboard.mozilla.org/r/70852/#review68246

> bug 1294352 merged InitiateDecodeRecoverySeek() with InitiateSeek(). You might encounter merging conflict while landing.

Yes I saw that bug pop up on my radar, I'll follow up when it lands...

Thank you for the reviews.

Comment 6

[François Marier [:francois]]

Comment on attachment 8779989 [details]
Bug 1294349 - Histogram for VIDEO_SUSPEND_RECOVERY_TIME_MS -

https://reviewboard.mozilla.org/r/70850/#review68860

datareview+

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8779989 [details]
Bug 1294349 - Histogram for VIDEO_SUSPEND_RECOVERY_TIME_MS -

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

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8779990 [details]
Bug 1294349 - Report VIDEO_SUSPEND_RECOVERY_TIME_MS -

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

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8779989 [details]
Bug 1294349 - Histogram for VIDEO_SUSPEND_RECOVERY_TIME_MS -

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

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8779990 [details]
Bug 1294349 - Report VIDEO_SUSPEND_RECOVERY_TIME_MS -

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

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8779989 [details]
Bug 1294349 - Histogram for VIDEO_SUSPEND_RECOVERY_TIME_MS -

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

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8779990 [details]
Bug 1294349 - Report VIDEO_SUSPEND_RECOVERY_TIME_MS -

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17306a183ba3
Histogram for VIDEO_SUSPEND_RECOVERY_TIME_MS - r=francois,kaku
https://hg.mozilla.org/integration/autoland/rev/3599d88d2b24
Report VIDEO_SUSPEND_RECOVERY_TIME_MS - r=kaku

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/17306a183ba3
https://hg.mozilla.org/mozilla-central/rev/3599d88d2b24

Comment 15

[Tzuhao Kuo [:kaku]]

Comment on attachment 8779990 [details]
Bug 1294349 - Report VIDEO_SUSPEND_RECOVERY_TIME_MS -

https://reviewboard.mozilla.org/r/70852/#review71944

::: dom/media/MediaDecoderStateMachine.cpp:1413
(Diff revision 4)
>    mVideoDecodeSuspendTimer.Reset();
>  
>    if (mVideoDecodeSuspended) {
>      mVideoDecodeSuspended = false;
>      mOnPlaybackEvent.Notify(MediaEventType::ExitVideoSuspend);
>      mReader->SetVideoBlankDecode(false);

Sorry, Gerald, I went thorugh this patch again while I was studying the telemetry data and I think that we should start counting the time from this line on so that it includes (exactlly) the time used to switch decoder. Althought the telemetry looks sensable now and I think it is unlikely that the switching-decoder-task been ran between this line and the line we intialize the seek. WDYT?