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

RESOLVED FIXED in Firefox 39

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Martin Storsjö, Assigned: jya)

Tracking

Trunk
mozilla39
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8577780 [details]
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.
(Reporter)

Comment 1

3 years ago
Created attachment 8577781 [details] [diff] [review]
multiple-trun.patch

This patch fixes playback of such files.
(Reporter)

Updated

3 years ago
Component: Untriaged → Video/Audio
Product: Firefox → Core
(Assignee)

Updated

3 years ago
Assignee: nobody → jyavenard
(Assignee)

Comment 2

3 years ago
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-
(Reporter)

Comment 3

3 years ago
(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.
(Reporter)

Comment 4

3 years ago
Created attachment 8581493 [details] [diff] [review]
multiple-trun.patch

Updated patch according to Jean-Yves's comments.
Attachment #8577781 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
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-
(Reporter)

Comment 6

3 years ago
Created attachment 8581506 [details] [diff] [review]
multiple-trun.patch

Updated according to the review comments - thanks, it sure looks much nicer this way.
Attachment #8581493 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8581506 - Flags: review+
(Assignee)

Comment 7

3 years ago
remote:  https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7d0845db2a
(Assignee)

Comment 8

3 years ago
wrong commit, it is :

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/24f54332e22e
https://hg.mozilla.org/mozilla-central/rev/1b7d0845db2a
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39

Updated

3 years ago
Depends on: 1151360

Updated

3 years ago
No longer depends on: 1151360
You need to log in before you can comment on or make changes to this bug.