1064128 - Implement SourceBuffer.timeStampOffset

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Matthew Gregan [:kinetik]]

See https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#widl-SourceBuffer-timestampOffset

Tests 18, 19, and 36 from bug 881514 currently fail due to incomplete support.

Comment 1

[Bobby Holley (:bholley)]

I've been looking at this. Looks like this is going to require some hacking on the demuxer.

Comment 2

[Bobby Holley (:bholley)]

Youtube's html5player.js uses this, so I think this is probably p1 - feel free to reset if I misunderstood something.

Comment 3

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

This is probably the cause of AV sync issues in 1105066.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #3)
> This is probably the cause of AV sync issues in 1105066.

Yes, that is my hypothesis as well. I have working patches, just dealing with test coverage issues.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Implement support for timestampOffset in segments mode. v1

I'm still digging into a little bit if weirdness discovered by the tests, but
we can start getting this reviewed.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Factor some machinery out of test_BufferingWait into mediasource.js and make it Promise-friendly. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Tests. v0.9

Done enough to be reviewed.

Comment 8

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e8d84fb99975

Comment 9

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

Comment on attachment 8544918 [details] [diff] [review]
Implement support for timestampOffset in segments mode. v1

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

::: media/libstagefright/binding/DecoderData.cpp
@@ +237,5 @@
>  {
>    sp<MetaData> m = mMediaBuffer->meta_data();
> +  // XXXbholley - Why don't we adjust decode_timestamp for aMediaTime?
> +  decode_timestamp = FindInt64(m, kKeyDecodingTime) + aTimestampOffset;
> +  composition_timestamp = FindInt64(m, kKeyTime) - aMediaTime + aTimestampOffset;

Note: This code path is no longer used.

::: media/libstagefright/binding/MoofParser.cpp
@@ +163,5 @@
>      }
>    }
>  }
>  
> +Moof::Moof(Box& aBox, Trex& aTrex, Mdhd& aMdhd, Edts& aEdts, int64_t aTimestampOffset) :

s/int64_t/Microseconds/g

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8544921 [details] [diff] [review]
Tests. v0.9

These aren't passing yet.

Comment 11

[cajbir (:cajbir)]

Comment on attachment 8544918 [details] [diff] [review]
Implement support for timestampOffset in segments mode. v1

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

I was asked to take this review from karl.

Everywhere a time offset is used in a header as part of the method definition, or field, add a comment explaining what the time unit is for that offset. Either that or use a type if there is one.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #8)
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e8d84fb99975

This try push was green except for the test added by this bug, which is orange for other reasons. We need to get this in pronto, so pushed to inbound. I'll followup with the test later.

https://hg.mozilla.org/integration/mozilla-inbound/rev/20ab622b36c1

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/20ab622b36c1

Comment 14

[Bobby Holley (:bholley)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #9)
> Comment on attachment 8544918 [details] [diff] [review]
> Implement support for timestampOffset in segments mode. v1
> 
> Review of attachment 8544918 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libstagefright/binding/DecoderData.cpp
> @@ +237,5 @@
> >  {
> >    sp<MetaData> m = mMediaBuffer->meta_data();
> > +  // XXXbholley - Why don't we adjust decode_timestamp for aMediaTime?
> > +  decode_timestamp = FindInt64(m, kKeyDecodingTime) + aTimestampOffset;
> > +  composition_timestamp = FindInt64(m, kKeyTime) - aMediaTime + aTimestampOffset;
> 
> Note: This code path is no longer used.

Actually, it totally appears to be used at the end MP4Demuxer::DemuxAudioSample. Can you elaborate what you meant?

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 8544918 [details] [diff] [review]
Implement support for timestampOffset in segments mode. v1

Landing on aurora with pre-approval. https://hg.mozilla.org/releases/mozilla-aurora/rev/6d028e37ba05

Risk is low. This does affect non-MSE code, but just threads a new argument through code which does nothing outside of the MSE case.

Comment 16

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

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #14)
> (In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #9)
> > Comment on attachment 8544918 [details] [diff] [review]
> > Implement support for timestampOffset in segments mode. v1
> > 
> > Review of attachment 8544918 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: media/libstagefright/binding/DecoderData.cpp
> > @@ +237,5 @@
> > >  {
> > >    sp<MetaData> m = mMediaBuffer->meta_data();
> > > +  // XXXbholley - Why don't we adjust decode_timestamp for aMediaTime?
> > > +  decode_timestamp = FindInt64(m, kKeyDecodingTime) + aTimestampOffset;
> > > +  composition_timestamp = FindInt64(m, kKeyTime) - aMediaTime + aTimestampOffset;
> > 
> > Note: This code path is no longer used.
> 
> Actually, it totally appears to be used at the end
> MP4Demuxer::DemuxAudioSample. Can you elaborate what you meant?

Sorry - it no longer gets used for fragmented MP4, i.e. when mPrivate->mAudioIterator != nullptr.