1154164 - Add MediaDataDemuxer and MediaTrackDemuxer object

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Create MediaDataDemuxer and MediaTrackDemuxer (name to be confirmed) API.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Add MediaDataDemuxer and MediaTrackDemuxer classes

Definition of the (almost pure) MediaDataDemuxer and MediaTrackDemuxer classes.

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8602443 [details] [diff] [review]
Add MediaDataDemuxer and MediaTrackDemuxer classes

Will amend a new version shortly

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Add MediaDataDemuxer and MediaTrackDemuxer classes

Windows doesn't like the word ERROR used in a constant. Must be a #define somewhere. Added SkipToNextRandomAccessPoint API

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8603118 [details] [diff] [review]
Add MediaDataDemuxer and MediaTrackDemuxer classes

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

::: dom/media/MediaDataDemuxer.h
@@ +16,5 @@
> +#include "nsRefPtr.h"
> +#include "nsTArray.h"
> +
> +namespace mozilla {
> +

namespace media?  :)

@@ +37,5 @@
> +// will never be called from more than one thread at once.
> +class MediaDataDemuxer
> +{
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaTrackDemuxer)

NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaDataDemuxer)

i.e. you have MediaTrackDemuxer in there, but should have MediaDataDemuxer.

@@ +47,5 @@
> +  // Typically a demuxer will wait to parse the metadata before resolving the
> +  // promise. The promise will be rejected with WAITING_FOR_RESOURCES should
> +  // insufficient data be available at the time. Init() would have to be called
> +  // again to retry once more data has been received.
> +  virtual nsRefPtr<InitPromise> Init() = 0;

Your comment says "WAITING_FOR_RESOURCES", but you probably meant WAITING_FOR_DATA?

Do we need to fail with WAITING_FOR_DATA if we have insufficient data, or can we just wait until that data is appended?

Presumably we can also wait indefinitely until sufficient decoder resources becomes available on B2G?

@@ +134,5 @@
> +  // returned.
> +  // A aNumSamples value of -1 indicates to return all remaining samples.
> +  // A video sample is typically made of a single video frame while an audio
> +  // sample will contains multiple audio frames.
> +  virtual nsRefPtr<SamplesPromise> GetSamples(int32_t aNumSamples = 1) = 0;

Why do you want to request more than one sample at once?

It doesn't really make sense to have a -1 "return all samples" parameter, since at least in the case of MSE, you could end up returning the entire resource in a single array passed to you. For a decoder which has inherent latency in its pipeline I think a drain makes sense, but not for the demuxer right?

@@ +140,5 @@
> +  // Cancel all pending actions (Seek, GetSamples) and reset current state
> +  // All pending promises are to be rejected with CANCEL.
> +  // The next call to GetSamples would return the first sample available in the
> +  // track.
> +  virtual void Reset() = 0;

Are we likely to need to block waiting for somethings to stop here, or can we rely on canceling promises etc in order to prevent us fulfilling any promises or async operations after Reset() returns. Hopefully we can... There's lots of places where we call ResetDecode() and we expect it to be synchronous. We really want to be getting away from blocking threads, but it would be convenient if this was synchronous.

@@ +144,5 @@
> +  virtual void Reset() = 0;
> +
> +  // Returns timestamp of next random access point or an error if the demuxer
> +  // can't report this.
> +  virtual nsresult GetNextRandomAccessPoint(media::TimeUnit* aTime)

Skip-to-next-keyframe will rely on this right? So seems like making it optional might not be a good idea?

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #4)
> namespace media?  :)

I thought it would be pushing things a bit :)

If it was, I could actually name it whatever I want. MediaFormatDecoder is actually the only name I could think of that didn't compete with an existing name.


> 
> Do we need to fail with WAITING_FOR_DATA if we have insufficient data, or
> can we just wait until that data is appended?

With MSE, we need to be able to immediately tell if our init segment is complete or partial. Not resolving a promise until it is done make that impossible.
As for all we know, we could have incomplete init segment and we are waiting for more data to be appended.
As we only want to fire updateend if we have a complete init segment *and* it was valid, but also fire updateend with an incomplete init segment.

It is important to keep in mind that I've designed things that way with the requirements of MSE in mind and what I know is going to be needed.

> 
> Presumably we can also wait indefinitely until sufficient decoder resources
> becomes available on B2G?

This would be part of bug 1146086. It's the initialization of the MediaDataDecoder that could take a while with gonk, not the initialization of the demuxer.

> 
> @@ +134,5 @@
> > +  // returned.
> > +  // A aNumSamples value of -1 indicates to return all remaining samples.
> > +  // A video sample is typically made of a single video frame while an audio
> > +  // sample will contains multiple audio frames.
> > +  virtual nsRefPtr<SamplesPromise> GetSamples(int32_t aNumSamples = 1) = 0;
> 
> Why do you want to request more than one sample at once?

> 
> It doesn't really make sense to have a -1 "return all samples" parameter,
> since at least in the case of MSE, you could end up returning the entire
> resource in a single array passed to you. For a decoder which has inherent
> latency in its pipeline I think a drain makes sense, but not for the demuxer
> right?
> 

This is exactly what I want to be happening to complete bug 1119208.
Keeping the media raw content vs demuxed data, memory usage-wise is in effect fairly small that only add 30 bytes per demuxed samples, while giving us all the flexibility of evicting data and splitting audio and video (which we can't do at present)


> @@ +140,5 @@
> > +  // Cancel all pending actions (Seek, GetSamples) and reset current state
> > +  // All pending promises are to be rejected with CANCEL.
> > +  // The next call to GetSamples would return the first sample available in the
> > +  // track.
> > +  virtual void Reset() = 0;
> 
> Are we likely to need to block waiting for somethings to stop here, or can
> we rely on canceling promises etc in order to prevent us fulfilling any
> promises or async operations after Reset() returns. Hopefully we can...

We really only need to cancel the existing promise. How each demuxer will implement Reset() is obviously up to the demuxer, but in practice, for the two I'm currently working on (the MP4 now completed) and the WebM one: it' sjust be rejecting the current promise.

> There's lots of places where we call ResetDecode() and we expect it to be
> synchronous. We really want to be getting away from blocking threads, but it
> would be convenient if this was synchronous.

The only thing the needs blocking now is SharedDecoderManager::SetIdle() which once this API is in place and the MSE reader is adapted to it won't be required at all..

> > +  // Returns timestamp of next random access point or an error if the demuxer
> > +  // can't report this.
> > +  virtual nsresult GetNextRandomAccessPoint(media::TimeUnit* aTime)
> 
> Skip-to-next-keyframe will rely on this right? So seems like making it
> optional might not be a good idea?

This only need to be defined if GetNextRandomAccessPoint() is implemented, which by default isn't.
I can amend the comment.
If GetNextRandomAccessPoint() isn't supported, then we fall back to the old method of demuxing one at a time.

I can make it pure virtual, seeing that we will expect all demuxers to implement the new skip to next frame logic (which is to only skip frames we will never have a chance to play)

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8603118 [details] [diff] [review]
Add MediaDataDemuxer and MediaTrackDemuxer classes

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

OK. We need to be careful to not request all samples from the demuxer in the non-MSE case.

I'm still not very convinced of the usefulness of requesting any number of samples other than 1 or all of them, but we can try the design and see what happens.

Comment 7

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #6)

> OK. We need to be careful to not request all samples from the demuxer in the
> non-MSE case.
I can't imagine the demuxer being used outside the MediaFormatDecoder and the MSE::SourceBuffer.

MediaFormatDecoder only demux one at a time. Though I was thinking we should be demuxing up to the value set in the decoder_ahead pref (which is 2 by default)

> 
> I'm still not very convinced of the usefulness of requesting any number of
> samples other than 1 or all of them, but we can try the design and see what
> happens.

Because otherwise, demuxing N frames in a sourcebuffer would require 2*N task dispatches ! Now while I don't mind doing all those promises everywhere, when it comes to demux all frames to complete bug 1119208.
 It starts to become a bit silly I think.

Comment 8

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #4)
> Comment on attachment 8603118 [details] [diff] [review]
> Add MediaDataDemuxer and MediaTrackDemuxer classes
> 
> Review of attachment 8603118 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDataDemuxer.h
> > +
> > +  // Returns timestamp of next random access point or an error if the demuxer
> > +  // can't report this.
> > +  virtual nsresult GetNextRandomAccessPoint(media::TimeUnit* aTime)
> 
> Skip-to-next-keyframe will rely on this right? So seems like making it
> optional might not be a good idea?

I incorrectly answered that part.

If GetNextRandomAccessPoint isn't supported, then we fall back to the old method of skip-to-next-frame as implemented in all the other MediaDecoderReader (except MP4Reader): we demux all frames until we find the next keyframe after aTime.

While if implemented, we only skip frames if the next keyframe is found to be *before* our current playback time (and as such, could never have been decoded anyway).

For Live Streaming for example, GetNextRandomAccessPoint() can't be known, but we can still skip to the next keyframe using the old method.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/785c11ff4393

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/785c11ff4393