Closed Bug 905513 Opened 7 years ago Closed 7 years ago

Refactor MediaSource/decoder integration in preparation for multiple decoders


(Core :: Audio/Video, defect)

Not set





(Reporter: kinetik, Assigned: kinetik)




(5 files, 4 obsolete files)

The initial implementation was limited to a single decoder supplied data from a single SourceBuffer.  Along with the changes in bug 896888, remove the SourceBuffer limitation and rework the decoder integration to provide a way for multiple live decoders each supplied by a different SourceBuffer.
Attached patch wip v0 (obsolete) — Splinter Review
Work in progress:

- Provide a MediaSourceDecoder/Reader inspired by the aggregating DASH versions.
  (currently uses data only from the first SourceBuffer created)
- Rework MediaSourceInputAdapter as a MediaResource supplied by a SourceBuffer.
- Provide a bare bones Decoder/Reader that uses the SourceBuffer MediaResource to decode data and shuttle it back to the MediaSourceDecoder machinery, inspired by WebAudio's BufferDecoder.
Attached patch wip v0.1 (obsolete) — Splinter Review
Checkpoint: existing mochitest passes, leaks fixed, some cleanup still to do.
Attachment #790630 - Attachment is obsolete: true
Attached patch v0.2 (obsolete) — Splinter Review
Almost ready for review.  Also implements SourceBuffer::buffered by special request.

Also now includes bug 896888, because there's no useful division between the two bugs.
Attachment #795256 - Attachment is obsolete: true
Attached patch v0.3 (obsolete) — Splinter Review
With crude multiple decoder support; sufficient to play mozstory_audio.mpd in
Attachment #798351 - Attachment is obsolete: true
Blocks: 889712
Attachment #801378 - Attachment is obsolete: true
Partially reverts the changes made in bug 855130.  With the changes for this bug MediaSource objects are attached to MediaElements with a new decoder, rather than passing a MediaSource channel to the existing decoder infrastructure.
Attachment #808393 - Flags: review?(khuey)
Simple typo fixes.
Attachment #808394 - Flags: review?(chris.double)
Change test to use pushPrefEnv instead of setBoolPref.  Re-enable on Android now that the test passes (with all patches in this bug landed).
Attachment #808395 - Flags: review?(chris.double)
Rework MediaSource/MediaElement integration so that rather than using an the existing decoder infrastructure with an nsIChannel mapped to the MediaSource (actually to the first created SourceBuffer) which limited us to a single decoder, we use a new MediaSourceDecoder which manages a set of child decoders and readers mapped to multiple SourceBuffers.  Much of the new decoder/reader code is stubbed out for now.
Attachment #808973 - Flags: review?(chris.double)
Attachment #808394 - Flags: review?(chris.double) → review+
Attachment #808395 - Flags: review?(chris.double) → review+
Comment on attachment 808973 [details] [diff] [review]

Review of attachment 808973 [details] [diff] [review]:

Just some nits. Some of the classes should have 'virtual' and MOZ_OVERRIDE used on the methods that are overridden from base classes. Can we have some comments on the classes briefly explaining what they are for, or a general overview of how they all fit together somewhere. It's ok if the overview is planned for a final patch.

::: content/media/mediasource/SourceBufferResource.h
@@ +132,5 @@
> +  nsCOMPtr<nsIPrincipal> mPrincipal;
> +  const nsAutoCString mType;
> +
> +  // Provides synchronization between SourceBuffers and InputAdapters.
> +  ReentrantMonitor mMonitor;

Explain what members need the monitor held before accessing. Is it all those below?
Attachment #808973 - Flags: review?(chris.double) → review+
A couple of small fixes/cleanups:
- Use video-specific fields from VideoInfo to initialize parent VideoInfo,
  to avoid clobbering audio configuration if it has been configured before video.
- Override BufferDecoder::GetResource with a covariant version to avoid a bunch of ugly static_casts.
- Restrict type accepted by MediaSourceReader's constructor since mDecoder is static_cast to MediaSourceDecoder.
Attachment #810909 - Flags: review?(chris.double)
Attachment #810909 - Flags: review?(chris.double) → review+
You need to log in before you can comment on or make changes to this bug.