Closed Bug 1171314 Opened 5 years ago Closed 5 years ago

Add ability for MediaFormatReader to detect change of stream content

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Currently, with the existing MSE architecture multiple MediaFormatReader will be created, one for each init segment received.

This allows to easily handle change of content as a MediaFormatReader handles a single content type.

Through the use of the SharedDecoderManager however, it is required that a MediaDataDecoder be able to handle change of stream transparently. This is currently done H264 by using the AnnexB format, and for decoders not supporting AnnexB by using the H264Wrapper that monitors the SPS content and destroy/create a new decoder on the fly.

With the new MediaSource architecture and the new MediaSourceDemuxer, we have a single instance of MediaFormatReader. It can pass samples from different stream
We need the MediaFormatReader to be able to easily detect if the sample if from a different stream than the preceding one and recreate a decoder on the fly.

By doing so, we remove the requirement on the MediaDataDecoder to have to handle content change, making the entire design much simpler.
Attached patch P1. Add SharedTrackInfo object (obsolete) — Splinter Review
Add SharedTrackInfo object. A ref-counted TrackInfo. This will be added to all MediaRawData. Remove the size of the extradata array from the MediaRawData size calculation. As it's shared, there's no point having it counted multiple times.
Attachment #8617815 - Flags: review?(cpearce)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Fill SharedTrackInfo data in samples returned by MSE.
Attachment #8617819 - Flags: review?(cpearce)
Add SharedTrackInfo object. A ref-counted TrackInfo. This will be added to all MediaRawData. Remove the size of the extradata array from the MediaRawData size calculation. As it's shared, there's no point having it counted multiple times.
Attachment #8617829 - Flags: review?(cpearce)
Attachment #8617815 - Attachment is obsolete: true
Attachment #8617815 - Flags: review?(cpearce)
Comment on attachment 8617829 [details] [diff] [review]
P1. Add SharedTrackInfo object

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

::: dom/media/MediaInfo.h
@@ +360,5 @@
>    EncryptionInfo mCrypto;
>  };
>  
> +class SharedTrackInfo {
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(SharedTrackInfo)

As written, it looks like the visibility of all the methods here will be private, except that NS_INLINE_DECL_THREADSAFE_REFCOUNTING contains a "public" visibility declaration so they're not.

Please explicitly define the visibility of members here, i.e. please insert "public:" here, so it's obvious that these are public.
Attachment #8617829 - Flags: review?(cpearce) → review+
Comment on attachment 8617829 [details] [diff] [review]
P1. Add SharedTrackInfo object

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

::: dom/media/MediaInfo.h
@@ +374,5 @@
> +  {
> +    return mStreamSourceID;
> +  }
> +
> +  const TrackInfo* operator*() const

It kind of defeats the semantics of having a UniquePtr and then passing the raw pointer value out of the UniquePtr's scope; it means the pointer is no longer a unique reference to the object. Maybe you can make this return a reference to the UniquePtr instead?
Attachment #8617819 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #5)
> Comment on attachment 8617829 [details] [diff] [review]
> P1. Add SharedTrackInfo object
> 
> Review of attachment 8617829 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaInfo.h
> @@ +374,5 @@
> > +  {
> > +    return mStreamSourceID;
> > +  }
> > +
> > +  const TrackInfo* operator*() const
> 
> It kind of defeats the semantics of having a UniquePtr and then passing the
> raw pointer value out of the UniquePtr's scope; it means the pointer is no
> longer a unique reference to the object. Maybe you can make this return a
> reference to the UniquePtr instead?

We return a const pointer. You can't store that pointer to a non-const object. Could make it return a const TrackInfo* const to make it even more explicit, that not only the pointer is const, but so is the data pointed to.

This was mostly designed so you could use SharedTrackInfo in place of TrackInfo directly.
e.g. nsRefPtr<SharedTrackInfo> p; mInfo.mVideo = *p;
like we currently have:
nsAutoPtr<TrackInfo> p; mInfo.mVideo = *p;
Blocks: 1171379
Have MediaFormatReader detects change of stream content, recycle the decoder and seek to the previous keyframes, then drop all frames until we reach the time we want.
Attachment #8622179 - Flags: review?(cpearce)
Attachment #8622179 - Flags: review?(cpearce) → review+
You need to log in before you can comment on or make changes to this bug.