1194624 - Implement Media Source Extension Buffer monitoring as per spec

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

This is a recent regression, used to start automatically.

STR:
1: go to http://dashif.org/reference/players/javascript/1.4.0/samples/dash-if-reference-player/index.html
2: load http://dash.bidi.int.bbc.co.uk/e/pseudolive/bbb/client_manifest.mpd

Expected result: video start playing after a short time

Actual result: video doesn't start until you press play.

Comment 1

[Jean-Yves Avenard [:jya]]

I wonder if this is due to the dash-if recent change related to seeking at the start. They use to wait for the seeking event and start playback.

Comment 2

[u514836]

This regression occurred before dash.js behaviour changed. See http://dashif.org/reference/players/javascript/1.4.0/samples/dash-if-reference-player/index.html?mpd=http://rdmedia.bbc.co.uk/dash/ondemand/testcard/1/client_manifest-events.mpd

Comment 3

[Jean-Yves Avenard [:jya]]

Actually, this doesn't be a problem with just live stream. It's the same with this one:
http://rdmedia.bbc.co.uk/dash/ondemand/testcard/1/client_manifest-events.mpd

Comment 4

[Yury Delendik (:yury)]

Looking at the logs difference (with Chrome) starts after loadedmetadata event. At the https://github.com/Dash-Industry-Forum/dash.js/blob/development/src/streaming/controllers/PlaybackController.js#L110 in the initStart, the firstAppended[streamInfo.id] evaluated to true (but expected false to start playback), means media buffer was updated before loadedmetadata.

Comment 5

[Yury Delendik (:yury)]

After digging more, the 'canplay' event is not generated for the video element (handler at https://github.com/Dash-Industry-Forum/dash.js/blob/development/src/streaming/controllers/PlaybackController.js#L165), which really causes the video not to autoplay.

Comment 6

[Jean-Yves Avenard [:jya]]

CCing JW; he would be the one to know about this...

Comment 7

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

Got such errors and can't play at all:

E/MPEG4Extractor(  468): short avcC chunk (7 bytes)
W/MPEG4Extractor(  468): Error -1007 parsing chuck at offset 580 looking for first track
2015-09-16 06:06:12.369908 UTC - -89876544[7f17f9426a00]: Decoder=7f17c0119800 ChangeState LOADING => SHUTDOWN

Did I forget to turn on some prefs?

Comment 8

[Jean-Yves Avenard [:jya]]

Ah ! that's a regression due to bug 1202677 ; BBC uses AVC3 ; they would use avcC with no SPS/PPS in it.

The test to ignore short avcC shouldn't be < 7 not <= 7

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you Jean-Yves for opening bug 1205179.

Comment 10

[Jean-Yves Avenard [:jya]]

JW, apply patch from bug 1205179.

need to uplift that ASAP

Comment 11

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

Ok, I can start debugging now.

Comment 12

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

The video element doesn't have the 'autoplay' attribute. After adding 'autoplay' using Inspector, playback will start automatically.

Comment 13

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

Now I realized the 'autoplay' in comment 5 doesn't refer to the attribute of the media element. I will investigate why 'canplay' is not fired.

Comment 14

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

Ok, I see why.

The default value of 'preload' is 'metadata'. Therefore MDSM stops decoding video frames after loading metadata and it just doesn't have enough video frames for ready state to change to HAVE_FUTURE_DATA.

I think the site should specify |preload=auto| explicitly to ensure enough media data is fetched and decoded to fire 'canplay'.

Comment 15

[Jean-Yves Avenard [:jya]]

Could you raise a bug on dash-if then if you think the problem is on their end?

David is this something you could fix?

Comment 16

[u514836]

Testing the initial value of preload returns "" - the spec says that this maps to the Automatic state, so setting preload=auto should have no effect (though it clearly does).

Comment 17

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

Can you show me the spec quotes?

Comment 18

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

Ok, I found it

The empty string is also a valid keyword, and maps to the Automatic state. The attribute's missing value default is user-agent defined

The point is, preload="" is different preload not specified at all. In the DASH player, it is not specified instead of being assigned an empty string.

Comment 19

[u514836]

In Nightly (on Windows at least), I see that the first frame is decoded and displayed which implies that frames are obtained, and not just the metadata. Only that frame and the next frame should be required to transition to HAVE_FUTURE_DATA and dispatch canplay?

Comment 20

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

The first frame only give you HAVE_CURRENT_DATA. It takes at least 2 more frames (which is implementation dependent) to reach HAVE_FUTURE_DATA.

Comment 21

[Jean-Yves Avenard [:jya]]

After discussing this with JW on IRC, it seems that our implementation doesn't really follow https://w3c.github.io/media-source/#buffer-monitoring 

It something that is rather extensive and can't be fixed overnight. Having a workaround in the dash player may be preferred for the time being

Comment 22

[u514836]

A possibly relevant bug has been raised for discussion in MSE. See https://github.com/w3c/media-source/issues/11.

Comment 23

[u514836]

See also https://bugzilla.mozilla.org/show_bug.cgi?id=1211752

Comment 24

[Chris Peterson [:cpeterson]]

dash.js 1.5.1 works around this issue by forcing preload = "auto" for Firefox:

https://github.com/Dash-Industry-Forum/dash.js/issues/759#issuecomment-143976651
https://github.com/Dash-Industry-Forum/dash.js/pull/818

Comment 25

[Chris Peterson [:cpeterson]]

This Firefox problem was also reported in the Clapper HLS player:

https://github.com/clappr/clappr/issues/696

Comment 26

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P1. Move definition of EOS_FUZZ to be public.

Comment 27

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P2. Prevent crash should buffered range be read while shutting down.

Comment 28

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P3. Enable NextFrameBufferedStatus() for MediaSourceDecoder.

We require a slightly modification to the default implementation as mediasource allows for gaps of up to 125ms between samples and often videos do not start with a time of 0.

Comment 29

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P3. Determine when canplaythrough can be fired.

We assume that if we have 30s of data buffered ahead of the current position or up to the element's duration that we can play ahead without interruption.

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 8694551 [details] [diff] [review]
[MSE] P3. Determine when canplaythrough can be fired.

wrong title.

Comment 31

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P4. Determine when canplaythrough can be fired.

We assume that if we have 30s of data buffered ahead of the current position or up to the element's duration that we can play ahead without interruption.

Comment 32

[Jean-Yves Avenard [:jya]]

JW, I'm getting an intermittent assertion that video.duration is <= video.currentTime

how could this happen?

the mochitest dom/media/mediasource/test/test_TruncatedDuration.html also trigger the situation, where we truncate the mediasource duration so it's prior current time. Fairly certain that this used to cause to seek to the new duration if currentTime was > duration.

But this doesn't appear in the spec anymore, nor do I see that in the code anymore.

Going to simply return true when that happens.

Comment 33

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

(In reply to Jean-Yves Avenard [:jya] from comment #32)
> JW, I'm getting an intermittent assertion that video.duration is <=
> video.currentTime
Where is the assertion?

Comment 34

[Jean-Yves Avenard [:jya]]

when I create the TimeInterval that start <= end: 
Assertion failure: aStart <= aEnd, at c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\obj-firefox\dist\include\Intervals.h:65

Comment 35

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P5. Add mochitest testing new behaviour.

Ensure that the canplay/canplaythrough and readyState is properly updated according to buffering condition.

Comment 36

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P5. Add mochitest testing new behaviour.

Ensure that the canplay/canplaythrough and readyState is properly updated according to buffering condition.

Updated as per IRC discussion.

Comment 37

[Jean-Yves Avenard [:jya]]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e37fc86ea157

Comment 38

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

Comment on attachment 8694663 [details] [diff] [review]
[MSE] P5. Add mochitest testing new behaviour.

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

::: dom/media/mediasource/test/test_PlayEvents.html
@@ +42,5 @@
> +    .then(function() {
> +      ok(true, "got canplay event");
> +      // set element duration to 3s, this ensure that the following step will always have at least
> +      // loaded sufficient data.
> +      ms.duration = 3;

Can you add comments like "buffered=0-0.801666" so it is easier t track buffer ranges after each time we append data to the buffer?

@@ +85,5 @@
> +       return Promise.all(promises);
> +    })
> +    .then(function() {
> +       ok(true, "got canplaythrough event");
> +      // set element duration to 20s, so we have space with no data.

25s?

@@ +92,5 @@
> +      return once(el, 'durationchange');
> +    })
> +    .then(function() {
> +       ok(true, "got durationchange event");
> +       el.currentTime = 20;

The buffer ranges should be [0-6.406666,15-21.406666] now. Seeking to 20s will not fire 'waiting'. Is that a mistake?

Comment 39

[Jean-Yves Avenard [:jya]]

The buffered ranges, where I seek is taken very lightly really.
It's really only designed to test three different environment. I could have recreated a new mediasource from zero; but I felt that simply using a different time range would do (as it's much simpler).

First one happening between 0-10
Second between 15-20
And then between 20-25

Were you looking at the latest patch? I only ever wait on "waiting" on the last go. Which would happen around 25s

In regards to the first duration being set to 3; as I wanted to check that canplaythrough got fired (which would happen because we have the entire video buffered). I set the duration as 3 like I could have set it to 0. The duration will be updated whenever a new buffer is appended. That way I'm guaranteed that the end of the buffered range match with the duration.

I'll add a note on what range we're expecting to get.

Here is a try run:
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e37fc86ea157

Comment 40

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P4. Add mochitest testing new behaviour.

Ensure that the canplay/canplaythrough and readyState is properly updated according to buffering condition.

Comment 41

[Jean-Yves Avenard [:jya]]

Upon further thinking.

We can't at this stage be 100% compliant with the MSE buffer monitoring spec due to the underlying decoders which may hold frames.

The MSE specs define the status of readyState entirely based on the buffered range.
http://w3c.github.io/media-source/index.html#buffer-monitoring

so readyState should be HAVE_CURRENT_DATA if our source buffer contained a single frame at position 0.
this is covered by the case "If HTMLMediaElement.buffered contains a TimeRange that ends at the current playback position and does not have a range covering the time immediately after the current position"

However, decoders hold frame, the WMF h264 decoder won't output anything until at least 25 frames have been input.
So the only way for us to have readyState == HAVE_CURRENT_DATA, is by having a buffered range containing at least 25 frames.

Same with HAVE_FUTURE_DATA, the spec require that we only have one single frame *buffered* past the currentTime to have readyState = HAVE_FUTURE_DATA. But on windows this will never occur until we have at least 26 frames buffered.

All this indicates that to be compliant with the spec, the buffered range must represent frames we *can* play.

The only way to achieve this is by draining the decoders when we reach any gap in the media data range. So that every single frame contained in the buffered range is playable.
Unfortunately this is something we have considered several times in the past but decided that we wouldn't do it.

Chris, do you concur with this analysis?

Comment 42

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

Yes, regretfully.

Comment 43

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

Comment on attachment 8695040 [details] [diff] [review]
[MSE] P4. Add mochitest testing new behaviour.

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

::: dom/media/mediasource/test/test_PlayEvents.html
@@ +40,5 @@
> +       // Load [0, 1.601666). We must ensure that we load over 25 frames as the
> +       // windows H264 decoder will not produce a sample until then
> +       // (bug 1191138).
> +       promises.push(fetchAndLoad(videosb, 'bipbop/bipbop_video', range(1, 3), '.m4s'));
> +       return Promise.all(promises);

3-spaces indention.

@@ +43,5 @@
> +       promises.push(fetchAndLoad(videosb, 'bipbop/bipbop_video', range(1, 3), '.m4s'));
> +       return Promise.all(promises);
> +    })
> +    .then(function() {
> +      ok(true, "got canplay event");

2-spaces indention. Not consistent.

@@ +59,5 @@
> +       promises.push(fetchAndLoad(videosb, 'bipbop/bipbop_video', range(3, 5), '.m4s'));
> +       return Promise.all(promises);
> +    })
> +    .then(function() {
> +       ok(true, "got canplaythrough event");

one extra space indention.

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb2251cbb83
https://hg.mozilla.org/integration/mozilla-inbound/rev/e41d7d513d94
https://hg.mozilla.org/integration/mozilla-inbound/rev/c97bb3fecd89
https://hg.mozilla.org/integration/mozilla-inbound/rev/518d76ae72e6

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9fb2251cbb83
https://hg.mozilla.org/mozilla-central/rev/e41d7d513d94
https://hg.mozilla.org/mozilla-central/rev/c97bb3fecd89
https://hg.mozilla.org/mozilla-central/rev/518d76ae72e6

Comment 47

[Jeremy Noring]

I don't believe this fix made it into Firefox 44; it's affecting DASH playback for many of my users.  Is there any intention to uplift this to 44?

Comment 48

[Jean-Yves Avenard [:jya]]

44 won't be received any changes like this.

the change was consequential and relied on a lot of other work done in 45. so uplifting to 44 isn't trivial