1235427 - MoofParser in libstagefright incorrectly reads base data offset from tfhd box

Status: RESOLVED FIXED

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

Description

[rudolfs.bundulis]

Attachment: frag_ff.mp4

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.106 Safari/537.36

Steps to reproduce:

Try playing a valid fragmented MP4 file via media source extensions (the file frag_ff.mp4 is attached). The file plays fine with VLC and ffplay.


Actual results:

The source buffer object raised an error event.


Expected results:

The file should have been played back.

I build the nightly Firefox from sources and tried to find the issue and it seems that the Tfhd::Tfhd() constructor in media/libstagefright/binding/MoofParser.cpp:682 has an error. It assumes base_data_offset will have a size of 4 bytes and will be written before the track_id field. However if we look at the ISO/IEC 14496-12:2005 specification it states:

aligned(8) class TrackFragmentHeaderBox
extends FullBox(‘tfhd’, 0, tf_flags){
unsigned int(32) track_ID;
// all the following are optional fields
unsigned int(64) base_data_offset;
unsigned int(32) sample_description_index;
unsigned int(32) default_sample_duration;
unsigned int(32) default_sample_size;
unsigned int(32) default_sample_flags
}

Thus the track_id comes before base_data_offset and base_data_offset is 8 bytes not 4 bytes long.

Comment 1

[rudolfs.bundulis]

Attachment: frag_ff.mp4

Updated version of the mp4 file - the previous did not contain a valid width height in the trak box.

Comment 2

[rudolfs.bundulis]

Attachment: A patch that solves the issue - this works with the given file but most likely should be rewritten to match the Firefox coding guidelines.

Comment 3

[rudolfs.bundulis]

So we came down to the conclusion that the provided path works with fragmented mp4 that use the base_data_offset, but fragmented mp4 files with moof relative addressing seem to work out of the box.

Comment 4

[rudolfs.bundulis]

Attachment: The same video but with moof relative addressing - works with mse on Firefox and Chrome.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8702544 [details] [diff] [review]
A patch that solves the issue - this works with the given file but most likely should be rewritten to match the Firefox coding guidelines.

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

thank you.

Your analysis was spot on.

Very lucky for us that this flag is rarely used!

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fdff97e8109

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fdff97e8109

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: [mp4] Properly parse tfhd box.

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8716830 [details] [diff] [review]
[mp4] Properly parse tfhd box.

Approval Request Comment
[Feature/regressing bug #]: 1235427
[User impact if declined]: Valid MP4 will cause decoding error.
[Describe test coverage new/current, TreeHerder]: In central, manual test, confirmed that provided sample now play well. Lots of MP4 regression tests.
[Risks and why]: Low, we are just parsing a MP4 as per spec. Most MP4 content do not use this particular feature causing the bug to be bypassed (and will also bypass the fix)
[String/UUID change made/needed]: None

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8716830 [details] [diff] [review]
[mp4] Properly parse tfhd box.

Improve mp4 support, taking it.
Should be in 45 beta 4.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c72e77c8e1c

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ab34a57af869