1171330 - Add TrackBuffersManager object

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

This very fancy name is for rewriting the whole sourcebuffer / trackbuffer code as per spec.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: P1. Add ContainerParser::MediaSegmentRange() method

Moved from bug 1119208

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: P2. Add -= and - operator to IntervalSet

Carrying r+ changes, and move bug from 1119208

Comment 3

[Jean-Yves Avenard [:jya]]

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

Add += and -= operator, and ability to do a multiplication with an integer. This is useful when a TimeUnit is really a TimeDuration and to calculate gaps

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: P4. Make ResetParserState returns a media promise

Have ResetParserState returns a promise. The spec requires a synchronous part, however this makes no sense in that it can interrupt an asynchronous step (via abort()) so it was decided that we will diverge there from the spec (for the better I believe).

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: P5. Split AppendData task to be closer to spec

Split AppendData between a synchronous part (adding to the Input Buffer) and the Processing as per spec.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Buf : P6. Make RangeRemoval use promises

Range Removal algorithm is asynchronous. Make it so by having RangeRemoval returns a promise rather than messing with tasks scheduled on the main thread. That and I like playing with lambdas.

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: P7. Add ability to retrieve init range to ContainerParser

Add ContainerParser::InitSegmentRange()

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: P8. Check MoofParser index before demuxing

Turned out that the MoofParser doesn't properly demux if the index isn't refreshed before retrieving a frame (it doesn't reset mOffset.

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Remove "Diamond Problem" with MediaDecoder inheritance

MediaDecoder inherited from nsIObserver which inherits from nsISupport while also inheriting from AbstractMediaDecoder which itself inherit from nsISupport. This causes a diamond problem, and prevent using some utility classes like NS_ProxyRelease or nsMainThreadPtrHandle. MediaDecoder will assert if destructed on anything but the main thread.

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Remove "Diamond Problem" with MediaDecoder inheritance

MediaDecoder inherited from nsIObserver which inherits from nsISupport while also inheriting from AbstractMediaDecoder which itself inherit from nsISupport. This causes a diamond problem, and prevent using some utility classes like NS_ProxyRelease or nsMainThreadPtrHandle. MediaDecoder will assert if destructed on anything but the main thread. v2

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: P9. Add TrackBuffersManager object

The big one...

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: P11. Add eviction support

Add eviction support. We ensure we don't remove keyframes that would make current playback position undecodable.

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: P11. Add eviction support

rebasing

Comment 14

[Jean-Yves Avenard [:jya]]

There is an issue in the current handling of frames in the trackbuffer.

Frames are only inserted within other frames if earlier frame were removed. Or added at the end.

This won't work with out of order append. And with out of order append, the proper DTS order needs to be respected ; which makes it non trivial when dealing with previous streams with a different dts order reference base.

I will take this in a follow up bug/patch

Comment 15

[Jean-Yves Avenard [:jya]]

Will need fixes (part2 and part3) for playback to works with YouTube and ability to sort frame in DTS order. Without it, we can't compare DTS from different streams as they are all in a different time base.

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: P12. Properly insert frames in DTS order

Insert frame in DTS order and not at the end of the segment. This is not optimised on purpose for clarity, iit's a terrible way to search for the new frame position, especially if we can assume all frames in a media segment are in the proper decode order.

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: P13. Relax frame discontinuity detection

Relax discontinuity detection, this could be falsely trigger when a frame is of exceptionally low duration (like as seen in the bipbop video) http://people.mozilla.org/~jyavenard/tests/mse_mp4/bipbop.html it plays now perfectly fine, including proper A/V sync (which was never the case), segments are added in 2, 1 3, 5, 6, 4, 7

Comment 18

[Jean-Yves Avenard [:jya]]

Attachment: P14. Add ContainerParser::FirstCompleteMediaHeader() method

Add ContainerParser::FirstCompleteMediaHeader() method. Turned out, ContainerParser doesn't return true if it has a media segment, but true if at least one sample is present with a new moof. We need to be able to detect the actual media segment header (moof)

Comment 19

[Jean-Yves Avenard [:jya]]

Attachment: P15. Better andle partial media segments

Better handle partial media segments. Tidy up documentation.

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8617040 [details] [diff] [review]
P15. Better andle partial media segments

this relies on patches from bug 1171311... going to split things to make it easier to review

Comment 21

[cajbir (:cajbir)]

Comment on attachment 8615629 [details] [diff] [review]
P9. Add TrackBuffersManager object

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

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +546,5 @@
> +  mCurrentInputBuffer = new SourceBufferResource(mType);
> +  mCurrentInputBuffer->AppendData(mParser->InitData());
> +  CreateDemuxerforMIMEType();
> +  if (!mInputDemuxer) {
> +    MOZ_ASSERT(false, "Todo type not supported");

Capital TODO.

Comment 22

[cajbir (:cajbir)]

Comment on attachment 8615691 [details] [diff] [review]
P11. Add eviction support

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

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +139,5 @@
> +  }
> +  MSE_DEBUG("Reaching our size limit, schedule eviction of %lld bytes", toEvict);
> +
> +  nsCOMPtr<nsIRunnable> task =
> +  NS_NewRunnableMethodWithArgs<TimeUnit, uint32_t>(

Indent.

Comment 23

[cajbir (:cajbir)]

Comment on attachment 8615853 [details] [diff] [review]
P13. Relax frame discontinuity detection

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

::: dom/media/mediasource/TrackBuffersManager.h
@@ +172,5 @@
>      uint32_t mNumTracks;
>      Maybe<TimeUnit> mLastDecodeTimestamp;
>      Maybe<TimeUnit> mLastFrameDuration;
>      Maybe<TimeUnit> mHighestEndTimestamp;
> +    bool mNeedDurationReset;

It would be useful to have a comment here explaining what "Need Duration Reset" means.

Comment 24

[Jean-Yves Avenard [:jya]]

Attachment: P1. Add ContainerParser::MediaSegmentRange() method

Rebase

Comment 25

[Jean-Yves Avenard [:jya]]

Attachment: P2. Add -= and - operator to IntervalSet

Rebase

Comment 26

[Jean-Yves Avenard [:jya]]

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

Rebase

Comment 27

[Jean-Yves Avenard [:jya]]

Attachment: P4. Make ResetParserState returns a media promise

rebase

Comment 28

[Jean-Yves Avenard [:jya]]

Attachment: P5. Split AppendData task to be closer to spec

rebase

Comment 29

[Jean-Yves Avenard [:jya]]

Attachment: P6. Make RangeRemoval use promises

rebase

Comment 30

[Jean-Yves Avenard [:jya]]

Attachment: P7. Add ability to retrieve init range to ContainerParser

rebase

Comment 31

[Jean-Yves Avenard [:jya]]

Attachment: P8. Check MoofParser index before demuxing

rebase

Comment 32

[Jean-Yves Avenard [:jya]]

Attachment: P9. Remove "Diamond Problem" with MediaDecoder inheritance

rebase

Comment 33

[Jean-Yves Avenard [:jya]]

Attachment: P10. Add TrackBuffersManager object

Rebase and carrying r+

Comment 34

[Jean-Yves Avenard [:jya]]

Attachment: P11. Add eviction support

rebase, carrying r+

Comment 35

[Jean-Yves Avenard [:jya]]

Attachment: P12. Properly insert frames in DTS order

rebase

Comment 36

[Jean-Yves Avenard [:jya]]

Attachment: P13. Relax frame discontinuity detection

rebase and carrying r+. Actually rewrote it to be more explicit with the use of a mLongestFrameDuration

Comment 37

[Jean-Yves Avenard [:jya]]

Attachment: P14. Add ContainerParser::FirstCompleteMediaHeader() method

rebase

Comment 38

[Jean-Yves Avenard [:jya]]

Attachment: P15. Better andle partial media segments

Better handling of partial media segments. This handle the case where only the media segment header is appended.

Comment 39

[Jean-Yves Avenard [:jya]]

Attachment: P16. Use ProxyMediaCall and remove need for monitor

Use ProxyMediaCall and get rid of the use of the monitor unless absolutely necessary.

Comment 40

[Jean-Yves Avenard [:jya]]

Attachment: P17. Add diagnostic to ensure no pending append is going

Add extra diagnostic to ensure we have no pending append going.

Comment 41

[cajbir (:cajbir)]

Comment on attachment 8617121 [details] [diff] [review]
P17. Add diagnostic to ensure no pending append is going

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

::: dom/media/mediasource/TrackBuffersManager.h
@@ +224,5 @@
>    MediaPromiseRequestHolder<CodedFrameProcessingPromise> mProcessingRequest;
>    MediaPromiseHolder<CodedFrameProcessingPromise> mProcessingPromise;
>  
>    MediaPromiseHolder<AppendPromise> mAppendPromise;
> +  // Set to true while SegmentParserLoop is running. This is use for

s/use/used/

@@ +225,5 @@
>    MediaPromiseHolder<CodedFrameProcessingPromise> mProcessingPromise;
>  
>    MediaPromiseHolder<AppendPromise> mAppendPromise;
> +  // Set to true while SegmentParserLoop is running. This is use for
> +  // diagnostic only. We can't rely on mAppendPromise to be empty as this

'diagnostic purposes only'

Comment 42

[Jean-Yves Avenard [:jya]]

Attachment: P18. Error when finding invalid data

Error when finding invalid data (e.g. not a media segment nor an init segment). As we only check the first bytes to see if we have either of those, we can't let just happening data past the invalid one we will never come out of that state otherwise.

Comment 43

[Jean-Yves Avenard [:jya]]

Attachment: P19. Use state mirroring for reading mediasource duration

mirror the mediasource duration as we can't read it directly anymore since bug 1160695. Had to reshuffle initialization order a bit.

Comment 44

[Jean-Yves Avenard [:jya]]

Comment on attachment 8617721 [details] [diff] [review]
P19. Use state mirroring for reading mediasource duration

actually that crashes upon destruction, as disconnect must be done on the owner's thread.

Comment 45

[Jean-Yves Avenard [:jya]]

Attachment: P19. Use state mirroring for reading mediasource duration

mirror the mediasource duration as we can't read it directly anymore since bug 1160695. Had to reshuffle initialization order a bit.

Comment 46

[Jean-Yves Avenard [:jya]]

Comment on attachment 8617095 [details] [diff] [review]
P4. Make ResetParserState returns a media promise

I'm going to revert that change. as it causes failure of test, that does require resetparserstate to be synchronous.

The way TrackBuffersManager::ResetParserState() is now handled it is safe to simply queue the task and let it be...

Comment 47

[Jean-Yves Avenard [:jya]]

Attachment: P19. Use state mirroring for reading mediasource duration

Rebasing, removing dependency on patch from bug 1171311

Comment 48

[Bobby Holley (:bholley)]

Comment on attachment 8617785 [details] [diff] [review]
P19. Use state mirroring for reading mediasource duration

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

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +59,5 @@
> +  nsCOMPtr<nsIRunnable> task =
> +    NS_NewRunnableFunction([self] () {
> +      self->mMediaSourceDuration.Connect(self->mParentDecoder->CanonicalExplicitDuration());
> +    });
> +  GetTaskQueue()->Dispatch(task.forget());

Unrelated to this patch, but please rename this to TaskQueue() at some point, so that it matches the naming on MDSM and MDR (as of bug 1173001).

@@ +246,5 @@
> +  task =
> +    NS_NewRunnableFunction([self] () {
> +      self->mMediaSourceDuration.DisconnectIfConnected();
> +    });
> +  GetTaskQueue()->Dispatch(task.forget());

Depending on how much functionality you think TracBuffersManager is going to grow, it might be worth making explicit init/teardown routines for TrackBuffersManager that run on the task queue, rather than doing it inline. In general, I think we should try to do as much of the class' work as possible on its task queue.

Up to you though.

::: dom/media/mediasource/TrackBuffersManager.h
@@ +263,5 @@
>    // Strong references to external objects.
>    nsMainThreadPtrHandle<dom::SourceBuffer> mParent;
>    nsMainThreadPtrHandle<MediaSourceDecoder> mParentDecoder;
>  
> +  // MediaSource duration mirrorred on SourceBufferDecoder::ExplicitDuration.

s/mirrorred/mirrored/. Also, what does SourceBufferDecoder have to do with it?

It seems like this should say "MediaSource duration, mirrored from MediaDecoder on the main thread."

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a6c90231154
https://hg.mozilla.org/integration/mozilla-inbound/rev/7afec49604d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d48cf35d58b
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d30edb9048a
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd08ea9d9193
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aef130d13cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/90da82b5997e
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b9bd01428c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/34ec7493164c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4d64177e02f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d62627bbe3b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f99ebe7c88
https://hg.mozilla.org/integration/mozilla-inbound/rev/06f657ea4160
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb28ba1d607b
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b2f7c764372
https://hg.mozilla.org/integration/mozilla-inbound/rev/299c650797f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/86014e576ae3
https://hg.mozilla.org/integration/mozilla-inbound/rev/47411d97c705

Comment 50

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

https://hg.mozilla.org/mozilla-central/rev/1a6c90231154
https://hg.mozilla.org/mozilla-central/rev/7afec49604d3
https://hg.mozilla.org/mozilla-central/rev/5d48cf35d58b
https://hg.mozilla.org/mozilla-central/rev/5d30edb9048a
https://hg.mozilla.org/mozilla-central/rev/cd08ea9d9193
https://hg.mozilla.org/mozilla-central/rev/2aef130d13cd
https://hg.mozilla.org/mozilla-central/rev/90da82b5997e
https://hg.mozilla.org/mozilla-central/rev/3b9bd01428c3
https://hg.mozilla.org/mozilla-central/rev/34ec7493164c
https://hg.mozilla.org/mozilla-central/rev/b4d64177e02f
https://hg.mozilla.org/mozilla-central/rev/d62627bbe3b2
https://hg.mozilla.org/mozilla-central/rev/d3f99ebe7c88
https://hg.mozilla.org/mozilla-central/rev/06f657ea4160
https://hg.mozilla.org/mozilla-central/rev/cb28ba1d607b
https://hg.mozilla.org/mozilla-central/rev/7b2f7c764372
https://hg.mozilla.org/mozilla-central/rev/299c650797f2
https://hg.mozilla.org/mozilla-central/rev/86014e576ae3
https://hg.mozilla.org/mozilla-central/rev/47411d97c705