Closed Bug 1204401 Opened 4 years ago Closed 4 years ago

Change the buffering criteria in MDSM::UpdateRenderedVideoFrames

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(1 file)

https://hg.mozilla.org/mozilla-central/file/68718290640b/dom/media/MediaDecoderStateMachine.cpp#l2576

If next video frame is 10 mins later, remainingTime will be as long as 10 mins. It doesn't make sense to ask for so much data in order not to enter buffering state.
(See https://hg.mozilla.org/mozilla-central/file/68718290640b/dom/media/MediaDecoderStateMachine.cpp#l2605)

We should just pass a configurable constant (that is EXHAUSTED_DATA_MARGIN_USECS) to HasLowDecodedData().
Blocks: 1172830
Since |remainingTime + EXHAUSTED_DATA_MARGIN_USECS| is exactly 100000 for a audio-only file, I will just change EXHAUSTED_DATA_MARGIN_USECS to 100000.
Bug 1204401 - Change the buffering criteria in MDSM::UpdateRenderedVideoFrames.
Attachment #8660537 - Flags: review?(jyavenard)
See comment 0 and comment 1 for the rationale.
Assignee: nobody → jwwang
Comment on attachment 8660537 [details]
MozReview Request: Bug 1204401 - Change the buffering criteria in MDSM::UpdateRenderedVideoFrames.

https://reviewboard.mozilla.org/r/19133/#review17129

::: dom/media/MediaDecoderStateMachine.cpp:2623
(Diff revision 1)
> -      shouldBuffer = HasLowDecodedData(remainingTime + EXHAUSTED_DATA_MARGIN_USECS) &&
> +      shouldBuffer = HasLowDecodedData(EXHAUSTED_DATA_MARGIN_USECS) &&

I don't think that's right.

If we want to avoid the issue of remainingTime being too long due to an invalid frame ; or too many frames dropped etc.
Then doing std::min(40000, remainingTime) should be used instead.

Now if remainingTime is < AUDIO_DURATION_USECS we will attempt to buffer more than we used it.
Attachment #8660537 - Flags: review?(jyavenard)
https://hg.mozilla.org/mozilla-central/file/68718290640b/dom/media/MediaDecoderStateMachine.cpp#l2605
The value passed to HasLowDecodedData() is used to determine whether we have enough decoded audio data. We will enter buffering state if HasLowDecodedData() returns false.

For example, HasLowDecodedData(40000) returns false if we have only 30000us audio data. HasLowDecodedData(20) returns true if we have more than 20us audio data. However, HasLowDecodedData(100000000) will return false even if we have 5s audio. The smaller the value is, the less decoded audio data we need in order not to enter buffering state.

It doesn't make sense to me that we take into account how far away the next video frame is. The value passed to HasLowDecodedData is about audio only. We should always enter buffering state if decoded audio is below some threshold (which I choose to be EXHAUSTED_DATA_MARGIN_USECS == 100ms).
Flags: needinfo?(jyavenard)
Comment on attachment 8660537 [details]
MozReview Request: Bug 1204401 - Change the buffering criteria in MDSM::UpdateRenderedVideoFrames.

https://reviewboard.mozilla.org/r/19133/#review17215

Following discussion on IRC ; this is to make the threshold as a constant 100ms
Attachment #8660537 - Flags: review+
Thanks for the review!
Flags: needinfo?(jyavenard)
https://hg.mozilla.org/mozilla-central/rev/dcaf4cb39929
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.