1421179 - Improve the accuracy of ChannelMediaDecoder::mPlaybackStatistics

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Description

[JW Wang [:jwwang] [:jw_wang]]

My tests (playing dom/media/test/gizmo.webm) show that the WEMB demuxer needs to read bytes near the end of the stream when decoding metadata. This is bad for ChannelMediaDecoder::NotifyBytesConsumed() which makes an assumption that mDecoderPosition should progress steadily without jumping back and forth which is exactly what happens during seek and is avoided by the mIgnoreProgressData flag.

We should start playback statistics only after loading metadata so we can get more accurate and stable statistics.

https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/dom/media/ChannelMediaDecoder.cpp#315-329

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/dom/media/ChannelMediaDecoder.cpp#312,324

NotifyBytesConsumed() is called when data is consumed by the decoder. Therefore mPlaybackStatistics estimates the data rate consumed by the decoder instead of the player. Usually, the 2 rates won't differ too much for we throttle the decoder so it won't get ahead of the player too much. However, for small files, the difference might be noticeable since the decoder might be able to decode all frames even before playback starts.

Ideally, we should count the data rate of the player instead of the decoder and pass it to BaseMediaResource::SetPlaybackRate() so the MediaCache can make better decisions on data eviction algorithm.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1421179. P1 - associate data with playback events published by MDSM.

This is required for we want to associate playback offset with the
PlaybackStarted/PlaybackStopped events.

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

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1421179. P2 - mPlaybackStatistics should accumulate bytes as playback position progresses.

The original code accumulates bytes as the underlying decoder moves on which is wrong.

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

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1421179. P3 - ComputePlaybackRate() should call mPlaybackStatistics.GetRate() instead of mPlaybackStatistics.GetRateAtLastStop().

Otherwise mPlaybackBytesPerSecond will always be zero before playback is
stopped for the 1st time. This is important for a live stream where playabck
rate can't be calculated from the duration and resource length (-1).

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

Comment 5

[Benjamin Chen [:bechen]]

Comment on attachment 8933561 [details]
Bug 1421179. P1 - associate data with playback events published by MDSM.

https://reviewboard.mozilla.org/r/204498/#review210056

Comment 6

[Benjamin Chen [:bechen]]

Comment on attachment 8933562 [details]
Bug 1421179. P2 - mPlaybackStatistics should accumulate bytes as playback position progresses.

https://reviewboard.mozilla.org/r/204500/#review210060

::: dom/media/MediaDecoderStateMachine.cpp:3321
(Diff revision 1)
>          &MediaDecoderStateMachine::OnMediaSinkVideoComplete,
>          &MediaDecoderStateMachine::OnMediaSinkVideoError)
>        ->Track(mMediaSinkVideoPromise);
>      }
>    }
> +  if (!isStarted) {

There is also a 'if (!isStarted)' at line 3299.
How about early return 'if (isStarted)'?

Comment 7

[Benjamin Chen [:bechen]]

Comment on attachment 8933563 [details]
Bug 1421179. P3 - ComputePlaybackRate() should call mPlaybackStatistics.GetRate() instead of mPlaybackStatistics.GetRateAtLastStop().

https://reviewboard.mozilla.org/r/204502/#review210062

::: dom/media/ChannelMediaDecoder.cpp:414
(Diff revision 1)
>      mPlaybackBytesPerSecond = length / mDuration;
>      return;
>    }
>  
>    bool reliable = false;
> -  mPlaybackBytesPerSecond = mPlaybackStatistics.GetRateAtLastStop(&reliable);
> +  mPlaybackBytesPerSecond = mPlaybackStatistics.GetRate(&reliable);

Should we remove the GetRateAtLastStop() function entirely?

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8933562 [details]
Bug 1421179. P2 - mPlaybackStatistics should accumulate bytes as playback position progresses.

https://reviewboard.mozilla.org/r/204500/#review210060

> There is also a 'if (!isStarted)' at line 3299.
> How about early return 'if (isStarted)'?

Somehow, I should combine 2 if blocks.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8933563 [details]
Bug 1421179. P3 - ComputePlaybackRate() should call mPlaybackStatistics.GetRate() instead of mPlaybackStatistics.GetRateAtLastStop().

https://reviewboard.mozilla.org/r/204502/#review210062

> Should we remove the GetRateAtLastStop() function entirely?

Let's discuss it in another bug.

Comment 10

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

Comment on attachment 8933561 [details]
Bug 1421179. P1 - associate data with playback events published by MDSM.

https://reviewboard.mozilla.org/r/204498/#review210096

Comment 11

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

Comment on attachment 8933562 [details]
Bug 1421179. P2 - mPlaybackStatistics should accumulate bytes as playback position progresses.

https://reviewboard.mozilla.org/r/204500/#review210106

Comment 12

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

Comment on attachment 8933563 [details]
Bug 1421179. P3 - ComputePlaybackRate() should call mPlaybackStatistics.GetRate() instead of mPlaybackStatistics.GetRateAtLastStop().

https://reviewboard.mozilla.org/r/204502/#review210108

::: commit-message-77d8a:4
(Diff revision 1)
> +Bug 1421179. P3 - ComputePlaybackRate() should call mPlaybackStatistics.GetRate() instead of mPlaybackStatistics.GetRateAtLastStop().
> +
> +Otherwise mPlaybackBytesPerSecond will always be zero before playback is
> +stopped for the 1st time. This is important for a live stream where playabck

'playabck' -> 'playback'

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8933562 [details]
Bug 1421179. P2 - mPlaybackStatistics should accumulate bytes as playback position progresses.

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

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8933563 [details]
Bug 1421179. P3 - ComputePlaybackRate() should call mPlaybackStatistics.GetRate() instead of mPlaybackStatistics.GetRateAtLastStop().

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

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the reviews!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6b0da49aaf7
P1 - associate data with playback events published by MDSM. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/43a77e985bfe
P2 - mPlaybackStatistics should accumulate bytes as playback position progresses. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/0132d8fe0d55
P3 - ComputePlaybackRate() should call mPlaybackStatistics.GetRate() instead of mPlaybackStatistics.GetRateAtLastStop(). r=bechen,gerald

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d6b0da49aaf7
https://hg.mozilla.org/mozilla-central/rev/43a77e985bfe
https://hg.mozilla.org/mozilla-central/rev/0132d8fe0d55