1302045 - MDSM might enter buffering state prematurely

Status: RESOLVED FIXED

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

Description

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

http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/dom/media/MediaDecoderStateMachine.cpp#2170

HasLowUndecodedData() returns false when MDSM doesn't have [endOfDecodedData, endOfDecodedData + aUsecs] in the buffer ranges and causes MDSM to enter buffering.

The more decoded data we have, the more undecoded data we need in order not to enter buffering. This will cause MDSM  to enter buffering prematurely even when it can play without stopping for buffering. This also kinda defeat the purpose of prerolling where we decode a certain amount of data to a smooth playback at the beginning.

HasLowUndecodedData() should check if it has [GetMediaTime, GetMediaTime + aUsecs] in the buffer ranges.

Comment 1

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

Attachment: Bug 1302045. Check if we have [GetMediaTime(), GetMediaTime() + aUsecs] in the buffer ranges to decide whether we are in low buffered data.

Note we consider [endOfDecodedData, GetMediaTime()] is always in the buffer ranges.

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

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8792374 [details]
Bug 1302045. Check if we have [GetMediaTime(), GetMediaTime() + aUsecs] in the buffer ranges to decide whether we are in low buffered data.

https://reviewboard.mozilla.org/r/79444/#review78314

::: dom/media/MediaDecoderStateMachine.cpp:2308
(Diff revision 1)
>    if (Duration().ToMicroseconds() < endOfDecodedData) {
>      // Our duration is not up to date. No point buffering.
>      return false;
>    }
> -  media::TimeInterval interval(media::TimeUnit::FromMicroseconds(endOfDecodedData),
> -                               media::TimeUnit::FromMicroseconds(std::min(endOfDecodedData + aUsecs, Duration().ToMicroseconds())));
> +
> +  // Have decoded all samples. No point buffering.

Put the comment inside the branch, so it's clear that the comment applies to the branch bein taken, and not to the branch *not* being taken.

::: dom/media/MediaDecoderStateMachine.cpp:2317
(Diff revision 1)
> +
> +  int64_t start = endOfDecodedData;
> +  int64_t end = std::min(GetMediaTime() + aUsecs, Duration().ToMicroseconds());
> +  media::TimeInterval interval(media::TimeUnit::FromMicroseconds(start),
> +                               media::TimeUnit::FromMicroseconds(end));
> +  return !mBuffered.Ref().Contains(interval);

Maybe rename HasLowUndecodedData() to HasLowBufferedData(), since it doesn't check if we've got low undecoded data anymore, it checks against buffered data..

Comment 3

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

Comment on attachment 8792374 [details]
Bug 1302045. Check if we have [GetMediaTime(), GetMediaTime() + aUsecs] in the buffer ranges to decide whether we are in low buffered data.

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

Comment 4

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

Thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/324bb746e8c7
Check if we have [GetMediaTime(), GetMediaTime() + aUsecs] in the buffer ranges to decide whether we are in low buffered data. r=cpearce

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/324bb746e8c7