Closed Bug 1064128 Opened 5 years ago Closed 5 years ago

Implement SourceBuffer.timeStampOffset

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: kinetik, Assigned: bholley)

References

Details

Attachments

(3 files)

Blocks: MSE
Blocks: 881514
No longer blocks: MSE
I've been looking at this. Looks like this is going to require some hacking on the demuxer.
Assignee: nobody → bobbyholley
Youtube's html5player.js uses this, so I think this is probably p1 - feel free to reset if I misunderstood something.
Priority: P2 → P1
Blocks: 1116643
Blocks: 1116644
Blocks: 1116649
Depends on: 1116883
This is probably the cause of AV sync issues in 1105066.
Blocks: 1105066
(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.
Blocks: 1118370
I'm still digging into a little bit if weirdness discovered by the tests, but
we can start getting this reviewed.
Attachment #8544918 - Flags: review?(karlt)
Attachment #8544918 - Flags: review?(ajones)
Attached patch Tests. v0.9Splinter Review
Done enough to be reviewed.
Attachment #8544921 - Flags: review?(karlt)
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
Attachment #8544918 - Flags: review?(ajones) → review+
Attachment #8544920 - Flags: review?(karlt)
Comment on attachment 8544921 [details] [diff] [review]
Tests. v0.9

These aren't passing yet.
Attachment #8544921 - Flags: review?(karlt)
Blocks: ytb37
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.
Attachment #8544918 - Flags: review?(karlt) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/20ab622b36c1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(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?
Flags: needinfo?(ajones)
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.
Attachment #8544918 - Flags: approval-mozilla-aurora?
(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.
Flags: needinfo?(ajones)
Attachment #8544918 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1120030
You need to log in before you can comment on or make changes to this bug.