1168040 - Stagefright/SampleIterator/MoofParser not properly handling edts' empty edit

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

In bug 1168004, a new file was added where video starts at 1s and audio at around 2s (actually 1.951s).

This is done via the use of an edit list (EDTS box).

From ISO 14496-12:

Starting offsets for tracks (streams) are represented by an initial empty edit. For example, to play a track from its start for 30 seconds, but at 10 seconds into the presentation, we have the following edit list:
  Entry-count = 2

  Segment-duration = 10 seconds Media-Time = -1
  Media-Rate = 1

  Segment-duration = 30 seconds (could be the length of the whole track)
  Media-Time = 0 seconds
  Media-Rate = 1

Actual:
When demuxing such file, the first frame will have a time of 0.

Expected:
When demuxing such file, the first frame will have a time of 10.

(note that if the file is fragmented, the MoofParser properly handle those)

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Part1. Properly handle MP4 starting offset

Have libstagefright properly reads edts with an empty offset

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Properly handle MP4 time offset in MoofParser

Properly handle fragmented MP4 and plain MP4 with edts box.

Comment 3

[Jean-Yves Avenard [:jya]]

actually, moofparser didn't handle those at all. It reads all edts regardless of the track ID and as such, the last edts found was the one used.

The mp4 added in bug 1168004 now plays properly, and so does http://people.mozilla.org/~jyavenard/mediatest/fragmented/bipbop-fragmented.mp4 which is one that always had A/V off by 1s (bug 1138786).

Comment 4

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> The mp4 added in bug 1168004 now plays properly, and so does
> http://people.mozilla.org/~jyavenard/mediatest/fragmented/bipbop-fragmented.
> mp4 which is one that always had A/V off by 1s (bug 1138786).

Awesome - thanks jya!

Comment 5

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

Comment on attachment 8610126 [details] [diff] [review]
Part2. Properly handle MP4 time offset in MoofParser

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

::: media/libstagefright/binding/MoofParser.cpp
@@ +415,5 @@
>  {
> +  if (!aTfhd.IsValid() || !aMvhd.IsValid(), !aMdhd.IsValid() ||
> +      !aEdts.IsValid()) {
> +    LOG(Moof, "Invalid dependencies: aTfhd(%d) aMvhd(%d) aMdhd(%d) aEdts(%d)",
> +        aTfhd.IsValid(), aMvhd.IsValid(), aMdhd.IsValid(), !aEdts.IsValid());

I'm guessing there should be no ! before aEdts.IsValid()

Comment 6

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

Comment on attachment 8610123 [details] [diff] [review]
Part1. Properly handle MP4 starting offset

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

Changes to MPEG4Extractor need to land with test cases.

Comment 7

[Jean-Yves Avenard [:jya]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #6)
> 
> Changes to MPEG4Extractor need to land with test cases.

Seeing I'm the one who fixed all the security issues in stagefright, I thought that rule could be relaxed a tad, the code is actually safer than it was as it won't attempt to perform a read if there's no data to read.

Plus, testing what? security? that it does what its supposed to do?

Comment 8

[Jean-Yves Avenard [:jya]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #5)
> Comment on attachment 8610126 [details] [diff] [review]
> Part2. Properly handle MP4 time offset in MoofParser
> 
> Review of attachment 8610126 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libstagefright/binding/MoofParser.cpp
> @@ +415,5 @@
> >  {
> > +  if (!aTfhd.IsValid() || !aMvhd.IsValid(), !aMdhd.IsValid() ||
> > +      !aEdts.IsValid()) {
> > +    LOG(Moof, "Invalid dependencies: aTfhd(%d) aMvhd(%d) aMdhd(%d) aEdts(%d)",
> > +        aTfhd.IsValid(), aMvhd.IsValid(), aMdhd.IsValid(), !aEdts.IsValid());
> 
> I'm guessing there should be no ! before aEdts.IsValid()

hmmm no. what's missing is || :)

Comment 9

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

(In reply to Jean-Yves Avenard [:jya] from comment #8)
> hmmm no. what's missing is || :)

That too. But this but looks wrong:

    aTfhd.IsValid(), aMvhd.IsValid(), aMdhd.IsValid(), !aEdts.IsValid());

Notice the !

Comment 10

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

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Plus, testing what? security? that it does what its supposed to do?

The test needs to validate the change that you've made. Would it be better to write a test case now, or debug it again later with the new demuxer?

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Add -=, - and * (with integer) operators to TimeUnit

Add +=, -= and * operators to TimeUnit. Started to think that TimeStamp and TimeDuration would have made a better choice for our usage.

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: P3. MP4 dts must use the same timeline as pts

dts must use the same timeline as pts. Makes it impossible otherwise to mix samples from different streams. as required by bug 1171330

Comment 13

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

Comment on attachment 8610123 [details] [diff] [review]
Part1. Properly handle MP4 starting offset

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

Add a test in a follow up bug instead.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afa316f85f4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/320a5acecc4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/888979f3f342

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/afa316f85f4c
https://hg.mozilla.org/mozilla-central/rev/320a5acecc4e
https://hg.mozilla.org/mozilla-central/rev/888979f3f342