Implement Media Source Extension Buffer monitoring as per spec

RESOLVED FIXED in Firefox 45

Status

()

Core
Audio/Video: Playback
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox44 affected, firefox45 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
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

2 years ago
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
(Assignee)

Comment 3

2 years ago
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
Summary: DASH-IF live stream do not autostart. → DASH-IF streams do not autostart.
Assignee: nobody → jyavenard
Priority: -- → P1
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.
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.
(Assignee)

Comment 6

2 years ago
CCing JW; he would be the one to know about this...
(Assignee)

Updated

2 years ago
Flags: needinfo?(jwwang)
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?
Flags: needinfo?(jwwang) → needinfo?(jyavenard)
(Assignee)

Comment 8

2 years ago
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
Flags: needinfo?(jyavenard)
Flags: needinfo?(gsquelart)
Flags: needinfo?(giles)
(Assignee)

Updated

2 years ago
Depends on: 1205179
Thank you Jean-Yves for opening bug 1205179.
Flags: needinfo?(gsquelart)
Flags: needinfo?(giles)
(Assignee)

Comment 10

2 years ago
JW, apply patch from bug 1205179.

need to uplift that ASAP
Ok, I can start debugging now.
The video element doesn't have the 'autoplay' attribute. After adding 'autoplay' using Inspector, playback will start automatically.
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.
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'.
(Assignee)

Comment 15

2 years ago
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?
Flags: needinfo?(bbcrddave)

Comment 16

2 years ago
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).
Flags: needinfo?(bbcrddave)
Can you show me the spec quotes?
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

2 years ago
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?
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.
(Assignee)

Comment 21

2 years ago
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

Updated

2 years ago

Comment 22

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

Comment 23

2 years ago
See also https://bugzilla.mozilla.org/show_bug.cgi?id=1211752
Blocks: 1221329
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
This Firefox problem was also reported in the Clapper HLS player:

https://github.com/clappr/clappr/issues/696
status-firefox44: --- → affected
status-firefox45: --- → affected
(Assignee)

Updated

2 years ago
Summary: DASH-IF streams do not autostart. → Implement Media Source Extension Buffer monitoring as per spec
(Assignee)

Updated

2 years ago
Depends on: 1229256
(Assignee)

Updated

2 years ago
Depends on: 1229299
(Assignee)

Updated

2 years ago
Blocks: 1229631
(Assignee)

Comment 26

2 years ago
Created attachment 8694548 [details] [diff] [review]
[MSE] P1. Move definition of EOS_FUZZ to be public.
Attachment #8694548 - Flags: review?(gsquelart)
(Assignee)

Comment 27

2 years ago
Created attachment 8694549 [details] [diff] [review]
[MSE] P2. Prevent crash should buffered range be read while shutting down.
Attachment #8694549 - Flags: review?(gsquelart)
(Assignee)

Comment 28

2 years ago
Created attachment 8694550 [details] [diff] [review]
[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.
Attachment #8694550 - Flags: review?(jwwang)
(Assignee)

Comment 29

2 years ago
Created attachment 8694551 [details] [diff] [review]
[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.
Attachment #8694551 - Flags: review?(jwwang)
Attachment #8694548 - Flags: review?(gsquelart) → review+
Attachment #8694549 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 30

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

wrong title.
Attachment #8694551 - Attachment is obsolete: true
Attachment #8694551 - Flags: review?(jwwang)
(Assignee)

Comment 31

2 years ago
Created attachment 8694555 [details] [diff] [review]
[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.
Attachment #8694555 - Flags: review?(jwwang)
Attachment #8694550 - Flags: review?(jwwang) → review+
Attachment #8694555 - Flags: review?(jwwang) → review+
(Assignee)

Comment 32

2 years ago
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.
(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?
(Assignee)

Comment 34

2 years ago
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
(Assignee)

Comment 35

2 years ago
Created attachment 8694647 [details] [diff] [review]
[MSE] P5. Add mochitest testing new behaviour.

Ensure that the canplay/canplaythrough and readyState is properly updated according to buffering condition.
Attachment #8694647 - Flags: review?(jwwang)
(Assignee)

Comment 36

2 years ago
Created attachment 8694663 [details] [diff] [review]
[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.
Attachment #8694663 - Flags: review?(jwwang)
(Assignee)

Updated

2 years ago
Attachment #8694647 - Attachment is obsolete: true
Attachment #8694647 - Flags: review?(jwwang)
(Assignee)

Comment 37

2 years ago
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e37fc86ea157
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?
(Assignee)

Comment 39

2 years ago
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
(Assignee)

Comment 40

2 years ago
Created attachment 8695040 [details] [diff] [review]
[MSE] P4. Add mochitest testing new behaviour.

Ensure that the canplay/canplaythrough and readyState is properly updated according to buffering condition.
Attachment #8695040 - Flags: review?(jwwang)
(Assignee)

Updated

2 years ago
Attachment #8694663 - Attachment is obsolete: true
Attachment #8694663 - Flags: review?(jwwang)
(Assignee)

Comment 41

2 years ago
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?
Flags: needinfo?(cpearce)
Yes, regretfully.
Flags: needinfo?(cpearce)
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.
Attachment #8695040 - Flags: review?(jwwang) → review+

Comment 44

2 years ago
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

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1231780

Comment 47

2 years ago
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?
(Assignee)

Comment 48

2 years ago
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
You need to log in before you can comment on or make changes to this bug.