Closed Bug 1294349 Opened 5 years ago Closed 5 years ago

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

Categories

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

50 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

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 on attachment 8779989 [details]
Bug 1294349 - Histogram for VIDEO_SUSPEND_RECOVERY_TIME_MS -

https://reviewboard.mozilla.org/r/70850/#review68244
Attachment #8779989 - Flags: review?(kaku) → review+
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.
Attachment #8779990 - Flags: review?(kaku) → review+
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.
Depends on: 1294352
Comment on attachment 8779989 [details]
Bug 1294349 - Histogram for VIDEO_SUSPEND_RECOVERY_TIME_MS -

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

datareview+
Attachment #8779989 - Flags: review?(francois) → review+
Pushed by gsquelart@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/17306a183ba3
https://hg.mozilla.org/mozilla-central/rev/3599d88d2b24
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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?
Attachment #8779990 - Flags: review+
You need to log in before you can comment on or make changes to this bug.