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

Status: RESOLVED FIXED

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

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Some MP4 videos have 0 length atoms floating around at the end of the moov box. We need to skip over them.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Skip four bytes when we hit a zero length box

Comment 2

[Jean-Yves Avenard [:jya]]

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.

Comment 3

[Jean-Yves Avenard [:jya]]

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 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Bug 1180101 - Test 0 length atom inside moov

Comment 5

[Jean-Yves Avenard [:jya]]

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

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

It's in the patch. Not sure why it doesn't show up in the review.

https://bug1180101.bmoattachments.org/attachment.cgi?id=8651636

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27c19242c960
https://hg.mozilla.org/integration/mozilla-inbound/rev/3db89a9ca18a

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Needs fix on MoofParser too.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/27c19242c960
https://hg.mozilla.org/mozilla-central/rev/3db89a9ca18a

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Anthony, any plans to uplift that in 42 (or not)?

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8b303a74c515
https://hg.mozilla.org/releases/mozilla-beta/rev/3011eba21201

Comment 15

[Chris Peterson [:cpeterson]]

Anthony says he still needs to write a unit test.