1093020 - Reduce buffering wait for MSE

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

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?

Comment 1

[Bobby Holley (:bholley)]

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.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Don't use a buffering wait for MSE. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Tests. v1

Comment 4

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

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?

Comment 5

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

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.

Comment 6

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

(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.

Comment 7

[Bobby Holley (:bholley)]

(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.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 2 - Tests. v2

Comment 9

[Bobby Holley (:bholley)]

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.

Comment 10

[Karl Tomlinson (:karlt)]

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.

Comment 11

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

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.

Comment 12

[Bobby Holley (:bholley)]

Final try push with all of this stuff: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dcaf8dfcafe0

Comment 13

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fe9e11333c7d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/21ddb8a58fea

Comment 14

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=43a51201545a since this push caused windows m2 permanent test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3612062&repo=mozilla-inbound

Comment 15

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d057f1ed36eb
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=dc17724b71ef

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c16da52a422a
https://hg.mozilla.org/mozilla-central/rev/dc17724b71ef