Closed
Bug 1116883
Opened 9 years ago
Closed 9 years ago
ContainerParser doesn't recognize a chunk beginning with a "Segment Type" box as a media segment
Categories
(Core :: Audio/Video, defect, P5)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(1 file)
1.18 KB,
patch
|
karlt
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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/
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8543086 -
Flags: review?(karlt)
Assignee | ||
Comment 2•9 years ago
|
||
> 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.
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
(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.
Comment 5•9 years ago
|
||
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.
Updated•9 years ago
|
Priority: -- → P5
Comment 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a909e2281ed1
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a909e2281ed1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 10•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox36:
--- → fixed
status-firefox37:
--- → fixed
Updated•9 years ago
|
Attachment #8543086 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•