Closed Bug 1143491 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: martin, Assigned: jya)

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch multiple-trun.patch (obsolete) — Splinter Review
This patch fixes playback of such files.
Component: Untriaged → Video/Audio
Product: Firefox → Core
Assignee: nobody → jyavenard
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.
Attachment #8577781 - Flags: review-
(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.
Attached patch multiple-trun.patch (obsolete) — Splinter Review
Updated patch according to Jean-Yves's comments.
Attachment #8577781 - Attachment is obsolete: true
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.
Attachment #8581493 - Flags: review-
Updated according to the review comments - thanks, it sure looks much nicer this way.
Attachment #8581493 - Attachment is obsolete: true
Attachment #8581506 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1b7d0845db2a
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1151360
No longer depends on: 1151360
You need to log in before you can comment on or make changes to this bug.