Add TrackBuffersManager object

RESOLVED FIXED in Firefox 41

Status

()

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

People

(Reporter: jya, Assigned: jya)

Tracking

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(19 attachments, 19 obsolete attachments)

12.50 KB, patch
Details | Diff | Splinter Review
12.50 KB, patch
Details | Diff | Splinter Review
14.38 KB, patch
Details | Diff | Splinter Review
1.24 KB, patch
Details | Diff | Splinter Review
15.18 KB, patch
Details | Diff | Splinter Review
9.46 KB, patch
Details | Diff | Splinter Review
4.15 KB, patch
Details | Diff | Splinter Review
1.60 KB, patch
Details | Diff | Splinter Review
2.46 KB, patch
Details | Diff | Splinter Review
71.33 KB, patch
Details | Diff | Splinter Review
13.42 KB, patch
Details | Diff | Splinter Review
4.03 KB, patch
Details | Diff | Splinter Review
6.03 KB, patch
Details | Diff | Splinter Review
4.62 KB, patch
Details | Diff | Splinter Review
21.09 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
15.92 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
16.98 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
1.40 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
4.99 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
This very fancy name is for rewriting the whole sourcebuffer / trackbuffer code as per spec.
(Assignee)

Comment 1

3 years ago
Created attachment 8615148 [details] [diff] [review]
P1. Add ContainerParser::MediaSegmentRange() method

Moved from bug 1119208
(Assignee)

Updated

3 years ago
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8615151 [details] [diff] [review]
P2. Add -= and - operator to IntervalSet

Carrying r+ changes, and move bug from 1119208
(Assignee)

Comment 3

3 years ago
Created attachment 8615156 [details] [diff] [review]
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
Attachment #8615156 - Flags: review?(matt.woodrow)
(Assignee)

Comment 4

3 years ago
Created attachment 8615160 [details] [diff] [review]
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).
Attachment #8615160 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 5

3 years ago
Created attachment 8615164 [details] [diff] [review]
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.
Attachment #8615164 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 6

3 years ago
Created attachment 8615165 [details] [diff] [review]
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.
Attachment #8615165 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 7

3 years ago
Created attachment 8615167 [details] [diff] [review]
P7. Add ability to retrieve init range to ContainerParser

Add ContainerParser::InitSegmentRange()
Attachment #8615167 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 8

3 years ago
Created attachment 8615168 [details] [diff] [review]
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.
Attachment #8615168 - Flags: review?(cpearce)
(Assignee)

Comment 9

3 years ago
Created attachment 8615240 [details] [diff] [review]
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.
Attachment #8615240 - Flags: review?(cpearce)
(Assignee)

Comment 10

3 years ago
Created attachment 8615247 [details] [diff] [review]
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
Attachment #8615247 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Attachment #8615240 - Attachment is obsolete: true
Attachment #8615240 - Flags: review?(cpearce)
Attachment #8615156 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 11

3 years ago
Created attachment 8615629 [details] [diff] [review]
P9. Add TrackBuffersManager object

The big one...
Attachment #8615629 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 12

3 years ago
Created attachment 8615642 [details] [diff] [review]
P11. Add eviction support

Add eviction support. We ensure we don't remove keyframes that would make current playback position undecodable.
Attachment #8615642 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 13

3 years ago
Created attachment 8615691 [details] [diff] [review]
P11. Add eviction support

rebasing
Attachment #8615691 - Flags: review?(cajbir.bugzilla)
(Assignee)

Updated

3 years ago
Attachment #8615642 - Attachment is obsolete: true
Attachment #8615642 - Flags: review?(cajbir.bugzilla)
Attachment #8615168 - Flags: review?(cpearce) → review+
(Assignee)

Updated

3 years ago
Blocks: 1171760

Updated

3 years ago
Attachment #8615160 - Flags: review?(cajbir.bugzilla) → review+

Updated

3 years ago
Attachment #8615164 - Flags: review?(cajbir.bugzilla) → review+

Updated

3 years ago
Attachment #8615165 - Flags: review?(cajbir.bugzilla) → review+

Updated

3 years ago
Attachment #8615167 - Flags: review?(cajbir.bugzilla) → review+
(Assignee)

Comment 14

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

Comment 15

3 years ago
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.
Depends on: 1168040
(Assignee)

Comment 16

3 years ago
Created attachment 8615850 [details] [diff] [review]
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.
Attachment #8615850 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 17

3 years ago
Created attachment 8615853 [details] [diff] [review]
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
Attachment #8615853 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 18

3 years ago
Created attachment 8616981 [details] [diff] [review]
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)
Attachment #8616981 - Flags: review?(ajones)
Attachment #8616981 - Flags: review?(ajones) → review+
(Assignee)

Comment 19

3 years ago
Created attachment 8617040 [details] [diff] [review]
P15. Better andle partial media segments

Better handle partial media segments. Tidy up documentation.
Attachment #8617040 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 20

3 years ago
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
Attachment #8617040 - Attachment is obsolete: true
Attachment #8617040 - Flags: review?(cajbir.bugzilla)
Attachment #8615247 - Flags: review?(cpearce) → review+

Comment 21

3 years ago
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.
Attachment #8615629 - Flags: review?(cajbir.bugzilla) → review+

Comment 22

3 years ago
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.
Attachment #8615691 - Flags: review?(cajbir.bugzilla) → review+

Updated

3 years ago
Attachment #8615850 - Flags: review?(cajbir.bugzilla) → review+

Comment 23

3 years ago
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.
Attachment #8615853 - Flags: review?(cajbir.bugzilla) → review+
(Assignee)

Comment 24

3 years ago
Created attachment 8617092 [details] [diff] [review]
P1. Add ContainerParser::MediaSegmentRange() method

Rebase
(Assignee)

Comment 25

3 years ago
Created attachment 8617093 [details] [diff] [review]
P2. Add -= and - operator to IntervalSet

Rebase
(Assignee)

Updated

3 years ago
Attachment #8615151 - Attachment is obsolete: true
(Assignee)

Comment 26

3 years ago
Created attachment 8617094 [details] [diff] [review]
P3. Add -=, - and * (with integer) operators to TimeUnit

Rebase
(Assignee)

Updated

3 years ago
Attachment #8615156 - Attachment is obsolete: true
(Assignee)

Comment 27

3 years ago
Created attachment 8617095 [details] [diff] [review]
P4. Make ResetParserState returns a media promise

rebase
(Assignee)

Updated

3 years ago
Attachment #8615160 - Attachment is obsolete: true
(Assignee)

Comment 28

3 years ago
Created attachment 8617096 [details] [diff] [review]
P5. Split AppendData task to be closer to spec

rebase
(Assignee)

Updated

3 years ago
Attachment #8615164 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
Created attachment 8617097 [details] [diff] [review]
P6. Make RangeRemoval use promises

rebase
(Assignee)

Updated

3 years ago
Attachment #8615165 - Attachment is obsolete: true
(Assignee)

Comment 30

3 years ago
Created attachment 8617098 [details] [diff] [review]
P7. Add ability to retrieve init range to ContainerParser

rebase
(Assignee)

Updated

3 years ago
Attachment #8615167 - Attachment is obsolete: true
(Assignee)

Comment 31

3 years ago
Created attachment 8617099 [details] [diff] [review]
P8. Check MoofParser index before demuxing

rebase
(Assignee)

Updated

3 years ago
Attachment #8615168 - Attachment is obsolete: true
(Assignee)

Comment 32

3 years ago
Created attachment 8617100 [details] [diff] [review]
P9. Remove "Diamond Problem" with MediaDecoder inheritance

rebase
(Assignee)

Updated

3 years ago
Attachment #8615247 - Attachment is obsolete: true
(Assignee)

Comment 33

3 years ago
Created attachment 8617101 [details] [diff] [review]
P10. Add TrackBuffersManager object

Rebase and carrying r+
(Assignee)

Updated

3 years ago
Attachment #8615629 - Attachment is obsolete: true
(Assignee)

Comment 34

3 years ago
Created attachment 8617102 [details] [diff] [review]
P11. Add eviction support

rebase, carrying r+
(Assignee)

Updated

3 years ago
Attachment #8615691 - Attachment is obsolete: true
(Assignee)

Comment 35

3 years ago
Created attachment 8617103 [details] [diff] [review]
P12. Properly insert frames in DTS order

rebase
(Assignee)

Updated

3 years ago
Attachment #8615850 - Attachment is obsolete: true
(Assignee)

Comment 36

3 years ago
Created attachment 8617107 [details] [diff] [review]
P13. Relax frame discontinuity detection

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

Updated

3 years ago
Attachment #8615853 - Attachment is obsolete: true
(Assignee)

Comment 37

3 years ago
Created attachment 8617108 [details] [diff] [review]
P14. Add ContainerParser::FirstCompleteMediaHeader() method

rebase
(Assignee)

Updated

3 years ago
Attachment #8616981 - Attachment is obsolete: true
(Assignee)

Comment 38

3 years ago
Created attachment 8617109 [details] [diff] [review]
P15. Better andle partial media segments

Better handling of partial media segments. This handle the case where only the media segment header is appended.
Attachment #8617109 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 39

3 years ago
Created attachment 8617117 [details] [diff] [review]
P16. Use ProxyMediaCall and remove need for monitor

Use ProxyMediaCall and get rid of the use of the monitor unless absolutely necessary.
Attachment #8617117 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 40

3 years ago
Created attachment 8617121 [details] [diff] [review]
P17. Add diagnostic to ensure no pending append is going

Add extra diagnostic to ensure we have no pending append going.
Attachment #8617121 - Flags: review?(cajbir.bugzilla)
(Assignee)

Updated

3 years ago
Blocks: 1171311

Updated

3 years ago
Attachment #8617109 - Flags: review?(cajbir.bugzilla) → review+

Updated

3 years ago
Attachment #8617117 - Flags: review?(cajbir.bugzilla) → review+

Comment 41

3 years ago
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'
Attachment #8617121 - Flags: review?(cajbir.bugzilla) → review+
(Assignee)

Comment 42

3 years ago
Created attachment 8617170 [details] [diff] [review]
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.
Attachment #8617170 - Flags: review?(cajbir.bugzilla)

Updated

3 years ago
Attachment #8617170 - Flags: review?(cajbir.bugzilla) → review+
(Assignee)

Comment 43

3 years ago
Created attachment 8617721 [details] [diff] [review]
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.
Attachment #8617721 - Flags: review?(bobbyholley)
(Assignee)

Comment 44

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

Comment 45

3 years ago
Created attachment 8617722 [details] [diff] [review]
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.
Attachment #8617722 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Attachment #8617721 - Attachment is obsolete: true
Attachment #8617721 - Flags: review?(bobbyholley)
(Assignee)

Comment 46

3 years ago
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...
Attachment #8617095 - Attachment is obsolete: true
(Assignee)

Comment 47

3 years ago
Created attachment 8617785 [details] [diff] [review]
P19. Use state mirroring for reading mediasource duration

Rebasing, removing dependency on patch from bug 1171311
Attachment #8617785 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Attachment #8617722 - Attachment is obsolete: true
Attachment #8617722 - Flags: review?(bobbyholley)
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."
Attachment #8617785 - Flags: review?(bobbyholley) → review+

Comment 49

3 years ago
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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.