1119208 - MSE: Implement source buffer data management as per spec.

Status: RESOLVED WORKSFORME

(Core :: Audio/Video: Playback, defect, P3)

Description

[Jean-Yves Avenard [:jya]]

This is a general task to change the current implementation of MediaSource buffers/data.

Right now, a TrackBuffer keep a list of SourceBufferDecoder; each keeping a reference to a demuxer (MP4 or WebM).

The SourceBufferResource, keeping the original raw data as originally fed to the SourceBuffer.

Each time a particular sample needs to be extracted, we need to demux the data, and with this all downsides (race conditions in demuxer, mp4's moof caching, having to deal with bytes length instead of composition timestamp etc...)

The aim of this ticket is to propose a new approach.
The data would be demuxed at the time of appendBuffer handling, and the extracted samples would be kept in a tree allowing for easy retrieval.

It would make implementation of Range Removal trivial (bug 1118126 and bug 1096089). Same for data eviction.
It would also greatly facilitate proper support of combined audio/video sourcebuffer (bug 1118533)

Also, it would totally remove any of the demuxer racing conditions and it associated crashes.

Most of the work around currently put in place (MoofParser, demuxer locking etc..) could be removed.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Add ContainerParser::MediaSegmentRange() method

Expand ContainerParser so it can report if the first media segment encountered is complete. Currently only MP4 is supported.

Comment 2

[Jean-Yves Avenard [:jya]]

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

Add -= and - to IntervalSet. Allows to remove a range from an interval set.

Comment 3

[Jean-Yves Avenard [:jya]]

This task will be become about rewriting MSE as per spec and using the algorithms described instead.

Comment 5

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

Comment on attachment 8612692 [details] [diff] [review]
Add ContainerParser::MediaSegmentRange() method

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

::: media/libstagefright/binding/MoofParser.cpp
@@ +68,2 @@
>        foundValidMoof = true;
> +    } else if (box.IsType("mdat") && !Moofs().IsEmpty()) {

Comment: We're baking in the assumption that the moof appears before the mdat. At some point this may come back and bite us. However it probably isn't the only implementation that makes this assumption.

@@ +69,5 @@
> +    } else if (box.IsType("mdat") && !Moofs().IsEmpty()) {
> +      // Check if we have all our data from last moof.
> +      Moof& moof = Moofs().LastElement();
> +      media::Interval<int64_t> datarange(moof.mMdatRange.mStart, moof.mMdatRange.mEnd, 0);
> +      media::Interval<int64_t> mdat(box.Range().mStart, box.Range().mEnd, 0);

Suggestion: you could add a method to MediaByteRange that creates an Interval rather than having to do this every time you need an Interval.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8612694 [details] [diff] [review]
Part2. Add -= and - operator to IntervalSet

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

::: dom/media/Intervals.h
@@ +394,5 @@
> +    if (aInterval.IsEmpty() || mIntervals.IsEmpty()) {
> +      return *this;
> +    }
> +    T firstEnd = std::max(mIntervals[0].mStart, aInterval.mStart);
> +    T secondStart = std::min(mIntervals.LastElement().mEnd, aInterval.mEnd);

Maybe add a comment about what this code is doing (inverting aInterval within the bounds of mIntervals and then doing the intersection).

Comment 7

[Jean-Yves Avenard [:jya]]

I'm making this a meta bug and will be moving things across bug 1171330 and others.

Comment 9

[Jean-Yves Avenard [:jya]]

all done now