ContainerParser doesn't recognize a chunk beginning with a "Segment Type" box as a media segment

RESOLVED FIXED in Firefox 36

Status

()

Core
Audio/Video
P5
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

unspecified
mozilla37
x86
Mac OS X
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(1 attachment)

Per [1], a media segment starts with a moof _or_ an styp. Currently we only support the former. This is necessary for jya's gizmo.mp4 chunks [2] to be recognized as media segments. 

[1] http://www.w3.org/2013/12/byte-stream-format-registry/isobmff-byte-stream-format.html#iso-media-segments
[2] http://people.mozilla.org/~jyavenard/mediatest/fragmented/
Created attachment 8543086 [details] [diff] [review]
Allow segment type box at the beginning of a media segment per spec. v1
Attachment #8543086 - Flags: review?(karlt)
> 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.

We also need to implement this at some point.
Per spec, I think we should test for either moof , or styp followed by moof seeing that the styp is optional but the moof isn't.
(In reply to Jean-Yves Avenard [:jya] from comment #3)
> Per spec

FWIW, there are lots of things that we currently accept that we should reject per spec.

> , I think we should test for either moof , or styp followed by moof
> seeing that the styp is optional but the moof isn't.

Are styp chunks fixed-size? I agree that this fix isn't complete, but it gets us closer to where we need to be, so I don't want to spike it if the alternative isn't easy.
They are like any other atoms: | size | atom_type |
They aren't fixed size: in that gizmo sample, most are 24 bytes long but the last segment is 28 bytes long.

It's debatable that gpac's MP4Box (used to generate gizmo2.m4s) doesn't follow the spec, after a styp atom, we have a sidx atom and only then a moof.
That's not the "An ISO BMFF media segment is defined in this specification as one optional Segment Type Box (styp) followed by a single Movie Fragment Box (moof)" 

I think it would be better to check if we have a moof followed by a mdat. And ignore anything in between, rather than explicitly check for a given atom type.

I actually had a similar change to the check of the init segment in IsInitSegmentPresent (as one VOD provider doesn't always have their init segment starting with a moov atom and not a ftyp)
(In reply to Jean-Yves Avenard [:jya] from comment #5)
> It's debatable that gpac's MP4Box (used to generate gizmo2.m4s) doesn't
> follow the spec, after a styp atom, we have a sidx atom and only then a moof.
> That's not the "An ISO BMFF media segment is defined in this specification
> as one optional Segment Type Box (styp) followed by a single Movie Fragment
> Box (moof)" 

The spec says we should ignore sidx boxes. The DASHIF player parses them but doesn't actually append them to the SourceBuffer. I don't think we should be checking for the mdat unless is required by the spec. The parser uses offsets relative to the moof to point to the data and ignores the mdat structure itself.
Priority: -- → P5
Comment on attachment 8543086 [details] [diff] [review]
Allow segment type box at the beginning of a media segment per spec. v1

IsMediaSegmentPresent was added in http://hg.mozilla.org/mozilla-central/rev/57db771b7b9d .  It seems to be added as a way to be conservative about where the stream can be split into a new decoder.  Currently it is overly conservative, and a Segment Type Box seems another suitable place to start a new decoder.

I guess the moof test might have some false positives because we shouldn't split between styp and moof, but I think a more thorough implementation can be done later.  I don't imagine this change to cause any new problems.
Attachment #8543086 - Flags: review?(karlt) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a909e2281ed1
https://hg.mozilla.org/mozilla-central/rev/a909e2281ed1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8543086 [details] [diff] [review]
Allow segment type box at the beginning of a media segment per spec. v1

Landed on Aurora with a=mse pre-approval.

https://hg.mozilla.org/releases/mozilla-aurora/rev/0300bc3a8ad6

Risk is low, this should mostly affect MSE file handling.
Attachment #8543086 - Flags: approval-mozilla-aurora?
status-firefox36: --- → fixed
status-firefox37: --- → fixed
Attachment #8543086 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.