Closed Bug 1180101 Opened 7 years ago Closed 6 years ago

Ignore extra 0s in mp4 files in MoofParser in plain MP4

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox42 --- fixed

People

(Reporter: ajones, Assigned: ajones)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Some MP4 videos have 0 length atoms floating around at the end of the moov box. We need to skip over them.
Assignee: nobody → ajones
Status: NEW → ASSIGNED
Comment on attachment 8629210 [details] [diff] [review]
Skip four bytes when we hit a zero length box

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

I see no references in the spec anywhere that a size of 4 is valid.
An atom header is made of a minimum of 8 bytes: ISOBMFF chapter 4.2 Object Structure.
"Boxes start with a header which gives both size and type."

FFmpeg simply appears to stop parsing if it finds a size that is less than 8 bytes rather than error.
In this file, the data box is found right after , so stopping parsing seems fine.

Will also need to handle the same case in MoofParser/Box.cpp to be consistent.

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +746,5 @@
>      uint32_t hdr[2];
> +    ssize_t nbytes;
> +    if ((nbytes = mDataSource->readAt(*offset, hdr, 8)) < 8) {
> +        if (nbytes == 4) {
> +          if (!hdr[0]) {

4 spaces indent to keep style.

We need to narrow the work around as here you would also skip an atom with a 0 < size < 8, and you end up in unknown territory in regards to what is going to be parsed next.

Either test that hdr[0] == BigEndian(4), or split the read in two readAt calls, one for chunk_size and if chunk_size == 4 -> *offset += chunk_size
and only then read chunk_type.
Attachment #8629210 - Flags: review?(jyavenard) → review-
Comment on attachment 8629210 [details] [diff] [review]
Skip four bytes when we hit a zero length box

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

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +746,5 @@
>      uint32_t hdr[2];
> +    ssize_t nbytes;
> +    if ((nbytes = mDataSource->readAt(*offset, hdr, 8)) < 8) {
> +        if (nbytes == 4) {
> +          if (!hdr[0]) {

actually, my bad... this is correct as you only consider length of 0.

off to bed..

if (nbytes == 4 && !hdr[0]) {
 ....
}
Comment on attachment 8651636 [details] [diff] [review]
Bug 1180101 - Test 0 length atom inside moov

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

Seems you forgot to add the MP4 file to the commit
Attachment #8651636 - Flags: review?(jyavenard) → review+
It's in the patch. Not sure why it doesn't show up in the review.

https://bug1180101.bmoattachments.org/attachment.cgi?id=8651636
Needs fix on MoofParser too.
Component: Audio/Video → Audio/Video: Playback
Blocks: 1212628
Duplicate of this bug: 1212628
Anthony, any plans to uplift that in 42 (or not)?
Flags: needinfo?(ajones)
Comment on attachment 8629210 [details] [diff] [review]
Skip four bytes when we hit a zero length box

Approval Request Comment
[Feature/regressing bug #]: Switching to stagefright demuxer in 33/34
[User impact if declined]: Some videos won't play
[Describe test coverage new/current, TreeHerder]: Regression test in bug
[Risks and why]: Low risk. Been in aurora/nightly for a while.
[String/UUID change made/needed]: none.
Flags: needinfo?(ajones)
Attachment #8629210 - Flags: approval-mozilla-beta?
Comment on attachment 8629210 [details] [diff] [review]
Skip four bytes when we hit a zero length box

Thanks, taking it. Should be in 42 beta 7.
Attachment #8629210 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Summary: Ignore extra 0s in mp4 files → Ignore extra 0s in mp4 files in MoofParser
Anthony says he still needs to write a unit test.
Priority: -- → P2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: leave-open
Priority: P2 → --
Resolution: --- → FIXED
Summary: Ignore extra 0s in mp4 files in MoofParser → Ignore extra 0s in mp4 files in MoofParser in plain MP4
See Also: → 1244523
You need to log in before you can comment on or make changes to this bug.