1354090 - DASH stream doesn't play

Status: VERIFIED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Playback of http://dash.edgesuite.net/envivio/Envivio-dash2/manifest.mpd doesn't start.

This bug is about tracking on why (initial assumption is that the content is invalid and not per spec)

But Chrome and Edge plays it (Safari doesn't either)

Comment 1

[Jean-Yves Avenard [:jya]]

in this content, each media content starts with a [ftyp] box.

a [ftyp] box per ISOBMFF spec, defines an init segment starts (ftyp + moov)

https://w3c.github.io/media-source/isobmff-byte-stream-format.html#iso-media-segments
"Valid top-level boxes defined in ISO/IEC 14496-12 other than ftyp, moov, styp, moof, and mdat are allowed to appear between the end of an initialization segment or media segment and before the beginning of a new media segment. These boxes MUST be accepted and ignored by the user agent and are not considered part of the media segment in this specification. "

so a ftyp box between an init segment and the media segment isn't to be ignored.

In the MSE spec:
Segment Parser Loop:
https://w3c.github.io/media-source/index.html#sourcebuffer-segment-parser-loop

So after processing the first init segment, we go back to append state == WAITING_FOR_SEGMENT

Now we start processing the ftyp box:

1. If the beginning of the input buffer indicates the start of an initialization segment, set the append state to PARSING_INIT_SEGMENT.

so append state is now PARSING_INIT_SEGMENT.

We go to:
5.1 "If the input buffer does not contain a complete initialization segment yet, then jump to the need more data step below."

So we continue parsing the data, or append state is always PARSING_INIT_SEGMENT, the moov is now processed, we do not have a complete initialization segment, we never return to append sate -- WAITING_FOR_SEGMENT

and as such the media segment is never processed, the data will never be added to the track buffer, and the buffered range will stay empty.

Or behaviour of this data is correct.

Though seeing that two browsers do play it, we could maybe handle that case better and only assume we start a new init segment if immediately followed with a moov box.
I don't like it though. This content is clearly invalid

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1354090: P1. Fix coding style.

Review commit: https://reviewboard.mozilla.org/r/127684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127684/

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1354090: P2. Add operator+(val).

Such that:
[x, y] + a = [x+a, y+a]

Review commit: https://reviewboard.mozilla.org/r/127686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127686/

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1354090: P3. Properly offset the media and init byte ranges.

The init and media segment byte ranged where not offset by the amount of bytes currently parsed. Whenever a new init segment signature was seen we would recreate a container parser.
This would lead to invalid offsets later.

Review commit: https://reviewboard.mozilla.org/r/127688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127688/

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1354090: P4. Only assume we have an init segment with moov box.

By ISOBMFF spec, an init segment is made of an ftyp and a moov box. However the ftyp box serve little purpose as such and only the moov atom contains essential information.

Some streams incorrectly add ftyp box all accross the content, despite the ISOBMFF spec stating (4.3.1):
Box Type: `ftyp’
Container: File
Mandatory: Yes
Quantity: Exactly one (but see below)

Additionally, with this change the ftyp box may not be present in content following earlier spec. So we will now be able to play them.

Review commit: https://reviewboard.mozilla.org/r/127690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127690/

Comment 6

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

Comment on attachment 8855787 [details]
Bug 1354090: P1. Fix coding style.

https://reviewboard.mozilla.org/r/127684/#review130670

Comment 7

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

Comment on attachment 8855788 [details]
Bug 1354090: P2. Add operator+(val).

https://reviewboard.mozilla.org/r/127686/#review130672

Comment 8

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

Comment on attachment 8855789 [details]
Bug 1354090: P3. Properly offset the media and init byte ranges.

https://reviewboard.mozilla.org/r/127688/#review130674

::: commit-message-b72d4:1
(Diff revision 1)
> +Bug 1354090: P3. Properly offset the media and init byte range. r?gerald

'range' -> 'ranges'

::: commit-message-b72d4:3
(Diff revision 1)
> +Bug 1354090: P3. Properly offset the media and init byte range. r?gerald
> +
> +The init and media segment byte ranged where not offset by the amount of bytes currently parsed. Whenever a new init segment signature was seen we would recreate a container parser.

'ranged where' -> 'ranges were'

::: dom/media/mediasource/ContainerParser.cpp:309
(Diff revision 1)
> -        mParser.EndSegmentOffset(mapping[endIdx].mClusterEndOffset));
> +          mParser.EndSegmentOffset(mapping[endIdx].mClusterEndOffset)) +
> +        mGlobalOffset;

Pedantic style: '+' should be at the start of a new line. (Only '||' and '&&' go at the end.)

Comment 9

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

Comment on attachment 8855790 [details]
Bug 1354090: P4. Only assume we have an init segment with moov box.

https://reviewboard.mozilla.org/r/127690/#review130678

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8855787 [details]
Bug 1354090: P1. Fix coding style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127684/diff/1-2/

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8855788 [details]
Bug 1354090: P2. Add operator+(val).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127686/diff/1-2/

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8855789 [details]
Bug 1354090: P3. Properly offset the media and init byte ranges.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127688/diff/1-2/

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8855790 [details]
Bug 1354090: P4. Only assume we have an init segment with moov box.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127690/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eaeef79b2bbf
P1. Fix coding style. r=gerald
https://hg.mozilla.org/integration/autoland/rev/d8be88ae3e46
P2. Add operator+(val). r=gerald
https://hg.mozilla.org/integration/autoland/rev/fb6652b226ae
P3. Properly offset the media and init byte ranges. r=gerald
https://hg.mozilla.org/integration/autoland/rev/f66c473ef308
P4. Only assume we have an init segment with moov box. r=gerald

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8855787 [details]
Bug 1354090: P1. Fix coding style.

Approval Request Comment
[Feature/Bug causing the regression]: MSE
[User impact if declined]: Some content won't play, and parsing corrupted data, this was the real reason behind bug 1353088
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: STR provided in bug description
[List of other uplifts needed for the feature/fix]: All 4 patches, though just Part 4 will do
[Is the change risky?]: I don't believe so.
[Why is the change risky/not risky?]: We look for a different signature in a file, one that must be there.
[String changes made/needed]: none

I understand that this it too late for beta, but I submit it just in case.

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/eaeef79b2bbf
https://hg.mozilla.org/mozilla-central/rev/d8be88ae3e46
https://hg.mozilla.org/mozilla-central/rev/fb6652b226ae
https://hg.mozilla.org/mozilla-central/rev/f66c473ef308

Comment 17

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 18

[Jean-Yves Avenard [:jya]]

Actually, after reconsideration.. Part 1 to Part 3 are the most important part in that it prevents bug 1353088 completely...

P4 allows to play the invalid stream, but it's invalid. We may or may not want to support it.

But P1 to P3 must go in.

Comment 19

[Brindusa Tot, Desktop QA]

Attachment: screenshotBug1354090.png

I tried verifying the bug on latest Nightly build 55.0a1, build ID 20170411030208 with Window 10 x64bit, but the playback of http://dash.edgesuite.net/envivio/Envivio-dash2/manifest.mpd doesn't start. File content is opened instead of playing the video. See attachment.

Am I missing something?

Note that it plays for me only Edge, but not on Chrome.

Comment 20

[Jean-Yves Avenard [:jya]]

Ah yes.. sorry.. This is a DASH file, you need to use a DASH player such as:
http://dashif.org/reference/players/javascript/v2.4.1/samples/dash-if-reference-player/index.html

STR:
go to: http://dashif.org/reference/players/javascript/v2.4.1/samples/dash-if-reference-player/index.html

In the STREAM field copy the mpd url: http://dash.edgesuite.net/envivio/Envivio-dash2/manifest.mpd
Press Load

Video should start.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Some questions. Is DASH in common use? Very common case for our users? When did the regression happen (when did this format work and then when did it break?)

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

In other words did this work in 52 but won't in 53?

Comment 23

[Jean-Yves Avenard [:jya]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21)
> Some questions. Is DASH in common use? Very common case for our users? When
> did the regression happen (when did this format work and then when did it
> break?)

Yes very...
It's used by YouTube, BBC, Ted to mention a few...

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

So was it broken in 52? What changed?

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Used by Youtube for every single video or just for some users? I'm just having a hard time believing that video for all of YouTube has been broken (since Firefox 41?) and no one noticed till now. Can we add some tests... manual testing even for this class of problem?   

And, can we live with this problem until 54 release in June?  If not, please make it really clear that it is a problem with broad user impact (telemetry data?) and this can get into the RC2 build tomorrow. 

If this is related to bug 1353088 it looks like we already have a fix in that bug landed on all branches.

In short I still feel like this missed the boat for 53 unless the situation is serious to the point of being a release blocker or a future dot-release driver.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8855787 [details]
Bug 1354090: P1. Fix coding style.

After some more discussion it sounds like this should be a fairly rare issue and we can hold off for 53. Gerry, up to you for aurora 54 to take this, i'll email you about it.

Comment 27

[Gerry Chang [:gchang]]

Comment on attachment 8855787 [details]
Bug 1354090: P1. Fix coding style.

Given some popular website users will be impacted, let's take this in aurora. Aurora54+.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b8d1179d5660
https://hg.mozilla.org/releases/mozilla-aurora/rev/e4322515d022
https://hg.mozilla.org/releases/mozilla-aurora/rev/e7959171bacf
https://hg.mozilla.org/releases/mozilla-aurora/rev/91b44f180afa

Comment 29

[Brindusa Tot, Desktop QA]

Verified as fixed on the latest Nightly build 55.0a1 Build ID 20170412030252 and video plays on Windows 10x64 and Mac OS X 10.10, but does not play on Ubuntu 16.04 x64 bit.

Jean, should this work on Ubuntu?

Comment 30

[Jean-Yves Avenard [:jya]]

(In reply to Brindusa Tot[:brindusat] from comment #29)
> Verified as fixed on the latest Nightly build 55.0a1 Build ID 20170412030252
> and video plays on Windows 10x64 and Mac OS X 10.10, but does not play on
> Ubuntu 16.04 x64 bit.
> 
> Jean, should this work on Ubuntu?

It certainly should... Do you have libavcodec installed?

Does this stream http://pearce.org.nz/video/h264.html play on the same machine?

Comment 31

[Brindusa Tot, Desktop QA]

(In reply to Jean-Yves Avenard [:jya] from comment #30)
> It certainly should... Do you have libavcodec installed?
> 
> Does this stream http://pearce.org.nz/video/h264.html play on the same
> machine?

The stream http://pearce.org.nz/video/h264.html plays on Ubuntu. Also, the stream http://dash.edgesuite.net/envivio/Envivio-dash2/manifest.mpd (that does not play on Firefox) plays correctly on Chromium(on the same Ubuntu machine)

Related to libavcodec, I tried to install it, but get some error: 
sudo apt-get install libavcodec
Reading package lists... Done
Building dependency tree       
Reading state information... Done
E: Unable to locate package libavcodec

Is there something else I can try?

Comment 32

[Jean-Yves Avenard [:jya]]

if the h264.html video plays on your system, then there's nothing more to do.

On ubuntu, the package to install is libavcodec-ffmpeg56 ; on 16.10 it's libavcodec57.

I can confirm that it doesn't play in 16.10 either... interesting
Chromium uses their own ffmpeg copy and doesn't rely on the system libavocdec

Comment 33

[Jean-Yves Avenard [:jya]]

hmmm.. it plays fine in my central build' but not with the nightly build of the day..

Comment 34

[Jean-Yves Avenard [:jya]]

hmmm...

This is using official mozilla nightly build:
[4687] Caught pending play exception - continuing (NotSupportedError:
The media resource indicated by the src attribute or assigned media
provider object was not suitable.)
dash.all.debug.js:3069:13
[4688] Video Element Error: MEDIA_ERR_SRC_NOT_SUPPORTED
(NS_ERROR_DOM_MEDIA_DEMUXER_ERR (0x806e000c) - virtual
RefPtr<mozilla::MozPromise<mozilla::MediaResult, mozilla::MediaResult,
true> > mozilla::MP4Demuxer::Init(): No MP4 audio () or video ()
tracks)

Working just fine using a central build (by myself)

:rillian, could it be something to do with nightly build and the rust demuxer on linux?

The init segment passed now is only made of the moov box.

Comment 35

[Ralph Giles (:rillian)]

I can reproduce the failure on linux but not mac. It looks like this is a result of switching to the rust mp4parse output for h264 files on that platform in bug 1354963. If I set `media.rust.mp4parser = false` in about:config the stream at http://dashif.org/reference/players/javascript/nightly/dash.js/samples/dash-if-reference-player/index.html?mpd=http://dash.edgesuite.net/envivio/Envivio-dash2/manifest.mpd plays for me.

Comment 36

[Ralph Giles (:rillian)]

I filed bug 1356291 for follow-up of the linux demuxer issue.

Comment 37

[Jean-Yves Avenard [:jya]]

So compiled with gcc 6.2.0 -> demuxer doesn't work
compiled with clang 3.8.1 -> work.

Will open a new bug about it

Comment 38

[Alfredo Yang (:alfredo)]

I found the problem, I'll have a fix later today. https://github.com/mozilla/mp4parse-rust/issues/90

Comment 39

[Brindusa Tot, Desktop QA]

The Linux demuxer issue is tracked on bug 1356291. On Windows and Mac, this bug is fixed, per see comment 29.
Mark this bug verified on FF 55.

Comment 40

[Ciprian Georgiu, Desktop QA]

I've reproduced the bug with an old Nightly from 2017-04-06.

This issue is also verified on 54 beta 1 (20170420011827) using Windows 10 x64 and Mac OS X 10.11.6.