1143491 - Only the first 'trun' of MP4 track fragments is played back

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Martin Storsjö]

Attachment: Sample file with multiple trun boxes per traf

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/600.3.18 (KHTML, like Gecko) Version/7.1.3 Safari/537.85.12

Steps to reproduce:

Play back a fragmented MP4 file (with H264 and AAC, but that's unrelated) in a video tag. The fragmented MP4 has got more than one 'trun' box in each track fragment (traf).


Actual results:

Only the video frames from the first 'trun' from each fragment is played back, resulting in very jumpy playback. (The audio is played back smoothly though.)


Expected results:

All 'trun' boxes should be played back, not only the first one.

Comment 1

[Martin Storsjö]

Attachment: multiple-trun.patch

This patch fixes playback of such files.

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8577781 [details] [diff] [review]
multiple-trun.patch

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

We should handle this more nicely. It feels a tad too much like a workaround

::: media/libstagefright/binding/MoofParser.cpp
@@ +315,3 @@
>    for (Box box = aBox.FirstChild(); box.IsAvailable(); box = box.Next()) {
>      if (box.IsType("trun")) {
>        ParseTrun(box, tfhd, tfdt, aMdhd, aEdts);

if IsValid() can't be relied on after the first traf, then when marking the moof as valid needs to be fixed too.

An invalid trun should still be detected and aborted early.

@@ +418,5 @@
>      mIndex.AppendElement(sample);
>  
>      mMdatRange = mMdatRange.Extents(sample.mByteRange);
>    }
> +  aTfdt.mBaseMediaDecodeTime = decodeTime;

aTfdt shouldn't be modified from within ParseTrun

instead ParseTrun should take the last base decode time as argument (instead of aTfdt), starting from the value found in the tfdt.

ParseTrun could return the end time, or a negative number if error.

That value could then be used to decide if the moof is valid or not.

Comment 3

[Martin Storsjö]

(In reply to Jean-Yves Avenard [:jya] from comment #2)
> Comment on attachment 8577781 [details] [diff] [review]
> multiple-trun.patch
> 
> Review of attachment 8577781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We should handle this more nicely. It feels a tad too much like a workaround
> 
> ::: media/libstagefright/binding/MoofParser.cpp
> @@ +315,3 @@
> >    for (Box box = aBox.FirstChild(); box.IsAvailable(); box = box.Next()) {
> >      if (box.IsType("trun")) {
> >        ParseTrun(box, tfhd, tfdt, aMdhd, aEdts);
> 
> if IsValid() can't be relied on after the first traf, then when marking the
> moof as valid needs to be fixed too.

I changed it so that ParseTrun doesn't set mValid any longer, but that is done by this calling loop instead.

> An invalid trun should still be detected and aborted early.

Actually, previously it didn't abort early on invalid truns, it kept looping through until it found a valid one. I changed it to break on the first invalid one as you requested. Thus, valid combinations is 1->N valid truns, while no truns at all is invalid, and if any single one is invalid, the whole moof is considered invalid as well.

> @@ +418,5 @@
> >      mIndex.AppendElement(sample);
> >  
> >      mMdatRange = mMdatRange.Extents(sample.mByteRange);
> >    }
> > +  aTfdt.mBaseMediaDecodeTime = decodeTime;
> 
> aTfdt shouldn't be modified from within ParseTrun
> 
> instead ParseTrun should take the last base decode time as argument (instead
> of aTfdt), starting from the value found in the tfdt.
> 
> ParseTrun could return the end time, or a negative number if error.
> 
> That value could then be used to decide if the moof is valid or not.

Ok, I updated the patch accordingly. The decode time variables had to be changed to int64_t, since the return value from ParseTrun needs to be able to return both decode times and negative numbers to indicate errors.

Comment 4

[Martin Storsjö]

Attachment: multiple-trun.patch

Updated patch according to Jean-Yves's comments.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8581493 [details] [diff] [review]
multiple-trun.patch

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

Much better. 

See comment on MoofParser.h

My earlier suggestion was really not good in the end.

aDecodeTime being a uint64_t* so it can be set on return, would look much better.
And a line of comment in MoofParser.h explaining so (thought that would be breaking coding-style :) )

Thank you for this.

::: media/libstagefright/binding/MoofParser.cpp
@@ +342,5 @@
> +    LOG(Moof, "Invalid tfdt dependency");
> +    return;
> +  }
> +  // Now search for TRUN boxes.
> +  int64_t decodeTime = (int64_t) tfdt.mBaseMediaDecodeTime;

See header file.
keep decodeTime as uint64_t

@@ +386,1 @@
>        !aMdhd.IsValid() || !aEdts.IsValid()) {

re-format so it takes one line (unless > 80 columns)

::: media/libstagefright/binding/include/mp4_demuxer/MoofParser.h
@@ +181,5 @@
>    nsTArray<Saio> mSaios;
>  
>  private:
>    void ParseTraf(Box& aBox, Trex& aTrex, Mdhd& aMdhd, Edts& aEdts, Sinf& aSinf, bool aIsAudio);
> +  int64_t ParseTrun(Box& aBox, Tfhd& aTfhd, Mdhd& aMdhd, Edts& aEdts, bool aIsAudio, int64_t aDecodeTime);

what about:
bool ParseTrun(Box& aBox, Tfhd& aTfhd, Tfdt& aTfdt, Mdhd& aMdhd, Edts& aEdts, uint64_t* aDecodeTime, bool aIsAudio);

(leave bool aIsAudio last thanks)

So ParseTrun returns false if failure, true if success.

Comment 6

[Martin Storsjö]

Attachment: multiple-trun.patch

Updated according to the review comments - thanks, it sure looks much nicer this way.

Comment 7

[Jean-Yves Avenard [:jya]]

remote:  https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7d0845db2a

Comment 8

[Jean-Yves Avenard [:jya]]

wrong commit, it is :

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/24f54332e22e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b7d0845db2a