Closed Bug 1302045 Opened 6 years ago Closed 6 years ago

MDSM might enter buffering state prematurely

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → jwwang
Blocks: 1295892, 1300711
Priority: -- → P3
Attachment #8792374 - Flags: review?(cpearce)
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..
Attachment #8792374 - Flags: review?(cpearce) → review+
Thanks!
Pushed by jwwang@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/324bb746e8c7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1305672
You need to log in before you can comment on or make changes to this bug.