Closed Bug 1171330 Opened 4 years ago Closed 4 years ago

Add TrackBuffersManager object

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(19 files, 19 obsolete files)

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
bholley
: review+
Details | Diff | Splinter Review
This very fancy name is for rewriting the whole sourcebuffer / trackbuffer code as per spec.
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Carrying r+ changes, and move bug from 1119208
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)
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)
Split AppendData between a synchronous part (adding to the Input Buffer) and the Processing as per spec.
Attachment #8615164 - Flags: review?(cajbir.bugzilla)
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)
Add ContainerParser::InitSegmentRange()
Attachment #8615167 - Flags: review?(cajbir.bugzilla)
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)
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)
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)
Attachment #8615240 - Attachment is obsolete: true
Attachment #8615240 - Flags: review?(cpearce)
Attachment #8615156 - Flags: review?(matt.woodrow) → review+
The big one...
Attachment #8615629 - Flags: review?(cajbir.bugzilla)
Attached patch P11. Add eviction support (obsolete) — Splinter Review
Add eviction support. We ensure we don't remove keyframes that would make current playback position undecodable.
Attachment #8615642 - Flags: review?(cajbir.bugzilla)
Attached patch P11. Add eviction support (obsolete) — Splinter Review
rebasing
Attachment #8615691 - Flags: review?(cajbir.bugzilla)
Attachment #8615642 - Attachment is obsolete: true
Attachment #8615642 - Flags: review?(cajbir.bugzilla)
Attachment #8615168 - Flags: review?(cpearce) → review+
Blocks: 1171760
Attachment #8615160 - Flags: review?(cajbir.bugzilla) → review+
Attachment #8615164 - Flags: review?(cajbir.bugzilla) → review+
Attachment #8615165 - Flags: review?(cajbir.bugzilla) → review+
Attachment #8615167 - Flags: review?(cajbir.bugzilla) → review+
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
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
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)
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)
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+
Better handle partial media segments. Tidy up documentation.
Attachment #8617040 - Flags: review?(cajbir.bugzilla)
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 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 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+
Attachment #8615850 - Flags: review?(cajbir.bugzilla) → review+
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+
Attachment #8615151 - Attachment is obsolete: true
Attachment #8615156 - Attachment is obsolete: true
Attachment #8615160 - Attachment is obsolete: true
Attachment #8615164 - Attachment is obsolete: true
Attachment #8615165 - Attachment is obsolete: true
Attachment #8615167 - Attachment is obsolete: true
Attachment #8615168 - Attachment is obsolete: true
Attachment #8615247 - Attachment is obsolete: true
Rebase and carrying r+
Attachment #8615629 - Attachment is obsolete: true
rebase, carrying r+
Attachment #8615691 - Attachment is obsolete: true
Attachment #8615850 - Attachment is obsolete: true
rebase and carrying r+. Actually rewrote it to be more explicit with the use of a mLongestFrameDuration
Attachment #8615853 - Attachment is obsolete: true
Attachment #8616981 - Attachment is obsolete: true
Better handling of partial media segments. This handle the case where only the media segment header is appended.
Attachment #8617109 - Flags: review?(cajbir.bugzilla)
Use ProxyMediaCall and get rid of the use of the monitor unless absolutely necessary.
Attachment #8617117 - Flags: review?(cajbir.bugzilla)
Add extra diagnostic to ensure we have no pending append going.
Attachment #8617121 - Flags: review?(cajbir.bugzilla)
Blocks: 1171311
Attachment #8617109 - Flags: review?(cajbir.bugzilla) → review+
Attachment #8617117 - Flags: review?(cajbir.bugzilla) → review+
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+
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)
Attachment #8617170 - Flags: review?(cajbir.bugzilla) → review+
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)
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.
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)
Attachment #8617721 - Attachment is obsolete: true
Attachment #8617721 - Flags: review?(bobbyholley)
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
Rebasing, removing dependency on patch from bug 1171311
Attachment #8617785 - Flags: review?(bobbyholley)
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+
You need to log in before you can comment on or make changes to this bug.