Closed Bug 1119208 Opened 5 years ago Closed 4 years ago

MSE: Implement source buffer data management as per spec.

Categories

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

defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: jya, Assigned: jya)

References

(Blocks 2 open bugs)

Details

(Keywords: meta)

Attachments

(2 obsolete files)

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.
Depends on: 1119456
Priority: -- → P2
Depends on: 1104475
Depends on: 1148102
Depends on: 1148103
Depends on: 1148292
Blocks: 1148234
Priority: P2 → P3
Blocks: 1151375
Depends on: 1165819
Expand ContainerParser so it can report if the first media segment encountered is complete. Currently only MP4 is supported.
Attachment #8612692 - Flags: review?(ajones)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Add -= and - to IntervalSet. Allows to remove a range from an interval set.
Attachment #8612694 - Flags: review?(matt.woodrow)
Blocks: 1169559
This task will be become about rewriting MSE as per spec and using the algorithms described instead.
Summary: MSE: Input data should be demuxed upon call to appendBuffer → MSE: Implement source buffer data management as per spec.
Duplicate of this bug: 1151375
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.
Attachment #8612692 - Flags: review?(ajones) → review+
Blocks: 1171311
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).
Attachment #8612694 - Flags: review?(matt.woodrow) → review+
Depends on: 1171330
No longer blocks: 1171311
Depends on: 1171311
I'm making this a meta bug and will be moving things across bug 1171330 and others.
Attachment #8612692 - Attachment is obsolete: true
Attachment #8612694 - Attachment is obsolete: true
Depends on: 1171379
Keywords: meta
Depends on: 1174577
Duplicate of this bug: 1116353
Component: Audio/Video → Audio/Video: Playback
all done now
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.