Reduce buffering wait for MSE

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bobbyholley, Assigned: bobbyholley)

Tracking

(Blocks: 1 bug)

unspecified
mozilla36
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

In buffering mode, we spin the state machine until we get a "sufficient" amount of undecoded data, or until we time out (currently set to 30 seconds), at which point we just start playing with whatever we have.

This may make sense for network streams, but I don't think it does for MSE, where the availability of data may depend on JS, which may in turn be waiting for us to reach some certain point in the timeline. For robustness, I think we should set this value somewhere between 0.5 and 5 seconds for MSE.

Thoughts?
Hm, I think we should basically just turn off the buffering wait for MSE. It's effectively a streaming heuristic, and when we're doing MSE heuristics are supposed to live in JS.

I've got a patch and tests, which I'll upload momentarily.
Posted patch Part 2 - Tests. v1 (obsolete) — Splinter Review
Attachment #8516027 - Flags: review?(cpearce)
Comment on attachment 8516027 [details] [diff] [review]
Part 2 - Tests. v1

Review of attachment 8516027 [details] [diff] [review]:
-----------------------------------------------------------------

Probably better for someone who has more clues about mediasource to review this patch.

::: dom/media/mediasource/test/test_BufferingWait.html
@@ +20,5 @@
> +    var sb = ms.addSourceBuffer("video/webm");
> +    ok(sb, "Create a SourceBuffer");
> +
> +    function once(target, name, cb) {
> +      target.addEventListener(name, cb, function() {

Doesn't addEventListener have signature:

EventTarget.addEventListener(type, listener[, useCapture, wantsUntrusted  ]);

?

i.e. you're passing (name, function, function), not (name, function, [bool, [bool]]) ?

I'd guess this is evaluating to addEventListener(name, cb, true), and you're not actually never running the second function being passed in?
Attachment #8516027 - Flags: review?(cpearce) → review?(karlt)
Comment on attachment 8516026 [details] [diff] [review]
Part 1 - Don't use a buffering wait for MSE. v1

Review of attachment 8516026 [details] [diff] [review]:
-----------------------------------------------------------------

Do we need a way to signal from MSE that data has been appended, and that we can begin decoding again? It seems to me there is potential for a 1s latency when

::: dom/media/MediaDecoderStateMachine.cpp
@@ +217,5 @@
>  
>    mAmpleVideoFrames =
>      std::max<uint32_t>(Preferences::GetUint("media.video-queue.default-size", 10), 3);
>  
> +  mBufferingWait = mScheduler->IsRealTime() ? 0 : mReader->GetBufferingWait();

I don't think we want to do it this way for MSE (and RealTime is probably broken too, it was always an ill-conceived hack).

I think what we should do is make the buffering code for MSE and non-MSE different. For MSE, we (should!) have perfect knowledge of what's been appended to the stream, and we should know when script appends more segments to the stream. Whereas in the non-MSE case, we're dealing with the uncertainty of data directly streaming in over the network.

So what I think we should do is for the MSE case only enter buffering state when we've run out of segments to decode, and only exit buffering when script appends new segments to all active streams. We should keep the existing buffering logic for the non-MSE case (we can come back and fix it later) but I think we need newer, simpler logic for the MSE case. We don't need the mBufferingWait timeout or HasLowUndecodedData checks, we can just assume that when playback reaches the EOS of all active subdecoders of the MSEReader, we should hit buffering.
Attachment #8516026 - Flags: review?(cpearce) → review-
(In reply to Chris Pearce (:cpearce) from comment #5)
> Do we need a way to signal from MSE that data has been appended, and that we
> can begin decoding again? It seems to me there is potential for a 1s latency
> when

This comment was half baked; it's superseded by my other comments, I just neglected to delete it, so please ignore it.
(In reply to Chris Pearce (:cpearce) from comment #4)
> I'd guess this is evaluating to addEventListener(name, cb, true), and you're
> not actually never running the second function being passed in?

Oh yeah, yuck - stupid typo. I never noticed the lack of removeEventListener because it was only being used for promise resolution (which is idempotent). Thanks for catching that - I've updated the tests here and in bug 1091008.
Attachment #8516027 - Attachment is obsolete: true
Attachment #8516027 - Flags: review?(karlt)
Attachment #8516590 - Flags: review?(karlt)
Comment on attachment 8516026 [details] [diff] [review]
Part 1 - Don't use a buffering wait for MSE. v1

(In reply to Chris Pearce (:cpearce) from comment #5)
> I think what we should do is make the buffering code for MSE and non-MSE
> different.

I didn't have time to look into this today. Can I convince you to r+ this patch for now and do this as a followup?

To avoid the youtube buffering trap from bug 1091008 comment 19, we need either this patch or the one from bug 1091008 comment 29. I can understand why the latter is a regression, but this patch seems like a strict improvement over the current setup, and would allow:
* Landing all of the patches in bug 1091008 before they bitrot (technically they all have r+ already and pass CI, but I don't want to regress youtube).
* Getting the tests from bug 1091008 in the tree before they regress.
* Getting the tests from this bug in the tree before they regress.

If that's alright with you, I'll try to hack up a prototype for comment 5 this week, and then we can discuss it next week when our timezone offset is better.
Attachment #8516026 - Flags: review- → review?(cpearce)
Comment on attachment 8516590 [details] [diff] [review]
Part 2 - Tests. v2

The spec doesn't seem very clear on how MediaSource influences the network effects on the resource fetch algorithm, but I agree we need to work out something sensible.  Might be worth looking at what Blink does.  Similar issues in bug 975782.
Attachment #8516590 - Flags: review?(karlt) → review+
Comment on attachment 8516026 [details] [diff] [review]
Part 1 - Don't use a buffering wait for MSE. v1

Review of attachment 8516026 [details] [diff] [review]:
-----------------------------------------------------------------

OK, r+ provided we fix this properly ASAP.
Attachment #8516026 - Flags: review?(cpearce) → review+
Blocks: 778617
https://hg.mozilla.org/mozilla-central/rev/c16da52a422a
https://hg.mozilla.org/mozilla-central/rev/dc17724b71ef
Assignee: nobody → bobbyholley
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1097723
Blocks: 1109437
You need to log in before you can comment on or make changes to this bug.