Refactor MediaSource/decoder integration in preparation for multiple decoders

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

Trunk
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

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

Comment 2

6 years ago
Posted patch wip v0.1 (obsolete) — Splinter Review
Checkpoint: existing mochitest passes, leaks fixed, some cleanup still to do.
Attachment #790630 - Attachment is obsolete: true
(Assignee)

Comment 3

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

Comment 4

6 years ago
Posted patch v0.3 (obsolete) — Splinter Review
With crude multiple decoder support; sufficient to play mozstory_audio.mpd in http://bluishcoder.co.nz/mse/dash/.
Attachment #798351 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 889712
(Assignee)

Updated

6 years ago
Attachment #801378 - Attachment is obsolete: true
(Assignee)

Comment 6

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

Comment 7

6 years ago
Simple typo fixes.
Attachment #808394 - Flags: review?(chris.double)
(Assignee)

Comment 8

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

Comment 9

6 years ago
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)

Updated

6 years ago
Attachment #808394 - Flags: review?(chris.double) → review+

Updated

6 years ago
Attachment #808395 - Flags: review?(chris.double) → review+

Comment 10

6 years ago
Comment on attachment 808973 [details] [diff] [review]
bug905513_refactor_v0.patch

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+
(Assignee)

Comment 11

6 years ago
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)

Updated

6 years ago
Attachment #810909 - Flags: review?(chris.double) → review+
You need to log in before you can comment on or make changes to this bug.