792404 - DASH: Add adaptation algorithm to DASH code

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Steve Workman [:sworkman] (INACTIVE)]

DASH in Firefox needs an adaptive stream switching algorithm. Base code (non-adaptive) for DASH-WebM is covered in bug 734546.

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP v1.0 Add nestegg_offset_seek

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP v1.0 Add decoder/download and reader/resource switching

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Chris, Matthew, if you get a chance, can you take a look at the patches I uploaded for feedback (comment 1 and comment 2).
Here's the gist:

-- Switching decoder/download source: Initiated in nsDASHDecoder::PossiblySwitchDecoder (hardcoded at present from video decoder #0 to #1, only deals with one switch - work ongoing).

-- Switching reader/reading source: pre-requested in nsDASHDecoder::PossiblySwitchDecoder, but final switch happens in nsDASHReader::PossiblySwitchVideoReaders.

These two cannot happen at the same time. Decoders must be switched first, in order to download the correct data subsegment. Then, when readers reach this offset, they must be switched.

To achieve this, I've added nestegg_offset_seek to get the next reader to seek to the right cluster, so it is ready to be switched to.

In addition, when a WebM data packet is obtained in NextPacket for DecodeVideoFrame, I add it to the VideoQueue of the main nsDASHReader, and not it's own queue in nsWebMReader. This is so that nsBuiltinDecoderStateMachine::AdvanceFrame just reads from the one queue that the sub-readers feed into. Also, in DecodeVideoFrame, the subsequent packet is obtained to get the end time of the current packet - at the switch access point, this data will not be available, since we didn't download it. So, I get nsWebMReader::NextPacket to get it from the next nsWebMReader, i.e. the one that will be switched TO.

So, I wanted you guys to take a look at that approach for the mechanism of the decoder and reader switching. Let me know if there's something I've missed or done horribly wrong.

FYI, managing multiple switches over the course of a presentation is what I'm working on in the meantime, before adding basic bandwidth estimation.

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 672128 [details] [diff] [review]
WIP v1.0 Add nestegg_offset_seek

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

Looks fine, but offset_seek's impl is virtually identical to the last half of track_seek's, so please split it out into a helper so they can share the code.

::: layout/media/symbols.def.in
@@ +19,5 @@
>  nestegg_track_codec_data_count
>  nestegg_track_codec_id
>  nestegg_track_count
>  nestegg_get_cue_point
> +nestegg_offset_seek

I think this list should be sorted.

::: media/libnestegg/include/nestegg.h
@@ +193,5 @@
>  int nestegg_get_cue_point(nestegg * context, unsigned int cluster_num,
>                            int64_t max_offset, int64_t * start_pos,
>                            int64_t * end_pos);
>  
> +/** Seek @a track to @a offset.  Stream will seek directly to offset.

Make it clear that offset is expected to point to a Cluster.

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1 Enables DASH build

Enables DASH code to build by default.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP v2.0 Add nestegg_offset_seek

Changed this so it doesn't call ne_parse. This avoids webm_read being called and the media cache blocking on a DASH subsegment seek. The goal is to get nsWebMReader ready to read in DecodeVideoFrame at the offset specified.

(I can do what's needed on github after I get your feedback here).

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP v1.0 Add decoder/download and reader/resource switching

Same concept as last patch, but this one deals with multiple switches.

I introduced an array in nsDASHDecoder to store the index of the sub decoder used to load a byte range. This is then used by nsDASHReader in determining which sub reader to use when it's ready to read.

Caveats:
-- Seeking is broken

-- I'm forcing the media cache to read from/seek to offsets that correspond to DASH byte ranges, and not just blocks it decides to read from/seek to.

-- Also, I force it to allocate the partial block for a stream when a switch away from that stream happens. If I don't do this, the partial block is overwritten if/when DASH decides to switch back to that stream. This means that a full block is written, with the remaining data being invalid. In turn, this requires nsDASHReader to only read from ranges it knows are valid, i.e. downloaded. But I added seeks and switches at the right points in the code, so nsDASHReader should already be doing this.

-- Adaptation algorithm is not included in this patch. I'm going to experiment with the download rate already stored in decoder statistics. But that is to come...

Comment 8

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

Comment on attachment 675878 [details] [diff] [review]
v1 Enables DASH build

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

Do you have unit tests for our DASH support somewhere? We should really have tests before we enable DASH support.

Comment 9

[Matthew Gregan [:kinetik]]

(In reply to Steve Workman [:sworkman] from comment #6)
> Created attachment 675879 [details] [diff] [review]
> WIP v2.0 Add nestegg_offset_seek

Wrong patch?

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP v2.0 Add nestegg_offset_seek

(In reply to Matthew Gregan [:kinetik] from comment #9)
> Wrong patch?

Yup, sorry - corrected that now. Notes from comment 6 still apply. I *think* I'm doing the right thing with the context var, but I don't understand it completely. So, correct me if I'm going to mess things up here.

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP v2.0 Add decoder/download and reader/resource switching

Some minor changes in prep for the adaptation algorithm (subsequent patch). Notes in comment 7 still apply.

Comment 12

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP v2.0 Add basic adaptation algorithm

Adds a very basic bandwidth estimation and stream adaptation.

-- This initial patch just uses the ChannelMediaResource's stats to estimate bandwidth. It needs to be fixed to consider all the resources from all the streams, and to do some sort of running average. (Note: bandwidth estimation is something that no doubt will have many iterations in the future).

-- Responding to the changes in bandwidth is in two parts:
1) Getting the most suitable Representation from the MPD in response to the estimated bandwidth (this is relatively simple code).
2) Upgrading and downgrading the streams: Upgrading is better (for the UX and pos the network) if done gracefully (according to some what I've read/learned), hence only upgrading 1 stream at a time. Downgrading immediately seems like a good approach. So, we have a vague notion of responding slowly to improvements and very quickly to degradation. (Like b/w estimation, this part will no doubt have many iterations).

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Chris Pearce (:cpearce) from comment #8)
> Comment on attachment 675878 [details] [diff] [review]
> v1 Enables DASH build
> 
> Review of attachment 675878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you have unit tests for our DASH support somewhere? We should really have
> tests before we enable DASH support.

Unit tests are being tracked in bug 792935, although none currently exist :)

I should have explained more about turning DASH on by default. The goal is to have it on in Aurora for a while for demo puproses, letting interested folks get a chance to play with it and provide feedback. It would be great it we could keep it on, but like you say, we need automated testing to be on and those tests to be passing. If that doesn't happen before the subsequent uplift from Aurora to Beta, then off goes the pref. I only want this to be switched on when it's ready (and not breaking other things).

Let me know what you think about that.

Comment 14

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

If you only want DASH on for demos, then I think we should build it by default, but have it disabled by default via an about:config pref. People who want to demo/test it can pref it on in about:config.

Comment 15

[Steve Workman [:sworkman] (INACTIVE)]

That's true, although it would be nice to have it on by default so folks don't have to do anything :) In any case, there are variations of what we can do based on the risk level. I can pref it off now in these patches, and then pref it on later, once tests are done. And, since I was able to get a basic adaptation algorithm done (needs finessing, of course) I'm able to divert my attention to the unit tests.

It seems like this approach (pref off now; pref on when tests are ready/passing) is aligned with what you're thinking, correct?

Comment 16

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

(In reply to Steve Workman [:sworkman] from comment #15)
> It seems like this approach (pref off now; pref on when tests are
> ready/passing) is aligned with what you're thinking, correct?

Yup. :)

We shouldn't be preffing it on until we're sure it's not going to blow up in our faces. ;)

Comment 17

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

Comment on attachment 676434 [details] [diff] [review]
WIP v2.0 Add decoder/download and reader/resource switching

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

Most of my comments are related to reducing the perceived impact of DASH on other media code, i.e. keeping the dash code's modular and contained. This will make it easier to maintain and will hopefully make it easier to add H.264 support for DASH in future.

::: content/media/MediaResource.cpp
@@ +1017,5 @@
>        ReentrantMonitorAutoEnter mon(mSeekOffsetMonitor);
> +      // Only continue with seek request if a prior call to |Seek| was made.
> +      // If |Seek| was not called previously, it means the media cache is
> +      // seeking on its own.
> +      // E.g. When WebM parses cue position bytes at the end of the file, the

Cues are actually specified to be at the start of the file, not the end, it's just that certain tools are sloppy about enforcing this. :(

@@ +1030,5 @@
> +        // seek if it's after the range containing the offset from |Seek|.
> +        // Note: This may happen if a byte range is downloaded BEFORE the
> +        // reader has been switched and |Seek|s to the correct offset.
> +        if (NS_SUCCEEDED(rv) && !mByteRange.IsNull()
> +            && aOffset > mByteRange.mEnd) {

We normally have && riding high in multiline if statements, e.g.:

  if (NS_SUCCEEDED(rv) && !mByteRange.IsNull() &&
      aOffset > mByteRange.mEnd) {
/*.... */

::: content/media/MediaResource.h
@@ +384,5 @@
>    // Resume the current load since data is wanted again
>    nsresult CacheClientResume();
>  
> +  // Notify that a switch is about to happen. Called on the main thread.
> +  void PrepareForSwitch();

How about you call this Flush() or FlushPartialBlock() or something like that? That way it's not DASH specific.

::: content/media/dash/nsDASHDecoder.cpp
@@ +651,5 @@
>    }
>  
>    if (NS_SUCCEEDED(aStatus)) {
>      // Return error if |aRepDecoder| does not match current audio/video decoder.
> +    if (!AllowDecoderToDownloadData(aRepDecoder)) {

The name of AllowDecoderToDownloadData() makes it sound like it grants the ability to a decoder to download data, rather than checks to see if a download has been approved. Maybe rename it to IsDecoderAllowedToDownloadData() ?

@@ +669,3 @@
>  
> +      // Increment to the subsegment index.
> +      IncrSubsegmentIndex(aRepDecoder);

Code is typically read more times that it is written, so optimize code for readability. So I think it'd be better to rename IncrSubsegmentIndex() to IncrementSubsegmentIndex(). Then you might not feel so inclined to add a gratuitous comment here. ;)

@@ +845,5 @@
> +  uint32_t toDecoderIdx = (mVideoRepDecoderIdx+1)%mVideoRepDecoders.Length();
> +  NS_ENSURE_TRUE(toDecoderIdx < mVideoRepDecoders.Length(),
> +                 NS_ERROR_ILLEGAL_VALUE);
> +
> +  // Store index of decoder used to download this index.

You mean "store index of decoder used to download this byte range"?

::: content/media/dash/nsDASHDecoder.h
@@ +266,3 @@
>  
> +  // Current index of subsegments downloaded for audio/video decoder.
> +  int64_t        mAudioSubsegmentIdx;

Style nit: here you have multi-space whitespace between variable type and name, but above you don't.

::: content/media/dash/nsDASHReader.cpp
@@ +163,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    // Use metadata from current video sub reader to populate aInfo.
> +    if (mVideoReaders[i] == mVideoReader) {
> +      mInfo.mHasVideo      = videoInfo.mHasVideo;
> +      mInfo.mDisplay       = videoInfo.mDisplay;

Hmmm, not really sure what we should be doing with regards to metadata/aTags here.

aTags is supposed acquire the metadata (artist, song name, copyright, etc) from the resource, and it gets send to the HTML media element. Last time I checked we didn't implement it for WebM, but we will someday hopefully.

For Ogg chains (bug 455165) we're going to dispatch a loadedmetadata every time we encounter a new sub-resource in the stream, as the metadata genuinely does change (since we're (probably) playing a different song). But with DASH we should be playing the same logical resource when we switch DASH decoders, so the metadata should be the same right?

So assuming all sub-resources in a DASH stream have the same metadata, I guess you should only pass aTags to one sub reader (mVideoReader maybe?) and pass nullptr to others.

Also, once nsWebMReader::ReadMetadata() reads the metadata correctly, it should allocate a new nsHTMLMediaElement::MetadataTags, storing it in *aTags. So if you call ReadMetadata() in a loop passing in the same aTags here you'll leak all instances except the one from the last iteration (which is free'd elsewhere).

@@ +360,5 @@
>              (mAudioReader && !mAudioReader->IsSeekableInBufferedRanges()));
>  }
> +
> +void
> +nsDASHReader::RequestVideoReaderSwitch(uint32_t aFromReaderIdx,

That master reader doesn't store the index of the reader it's playing?

@@ +406,5 @@
> +
> +  // Only switch if we reached a switch access point.
> +  NS_ENSURE_TRUE(mSwitchCount < mSwitchToVideoSubsegmentIndexes.Length(), );
> +  uint32_t switchIdx = mSwitchToVideoSubsegmentIndexes[mSwitchCount];
> +  if (mVideoReader->HasReachedDASHSwitchAccessPoint(switchIdx)) {

Rather than adding indentation, return early if this is not true, i.e.:

  if (!mVideoReader->HasReachedDASHSwitchAccessPoint(switchIdx)) {
    return;
  }

::: content/media/dash/nsDASHReader.h
@@ +25,5 @@
> +// monitor in its constructor, but only if the conditional value |aEnter| is
> +// true. Used here to allow read access on the sub-readers' owning thread,
> +// i.e. the decode thread, while locking write accesses from all threads,
> +// and read accesses from non-decode threads.
> +class ReentrantMonitorConditionallyEnter

How about putting this in nsVideoUtils.h?

::: content/media/nsBuiltinDecoderReader.h
@@ +533,5 @@
>    virtual nsresult GetIndexByteRanges(nsTArray<MediaByteRange>& aByteRanges) {
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
> +  // Switches subreaders if a DASH stream-switch event flag has been set.

Again, I'd like to keep backend specific code in the specific readers where possible. Granted it's not always practical, but it would be good if as many of these DASH specific methods as possible were pushed down into the sub readers, or at least renamed to make them more general. For example renaming PossiblySwitchVideoReaders() to AfterDecode() or PostDecodeStep().

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +820,5 @@
> +    //     may be in shutdown state. So, need to initiate any queued up seeks
> +    //     here, as soon as the thread starts.
> +    //     Note: ChannelMediaResource can seek at switch time if there is no
> +    //     decode thread. The seek here ensures the reader's offset is in sync.
> +    mReader->SeekToDASHSubsegmentIfRequested();

I'd prefer to keep backend-specific code in the backends, that is, the readers.

So can you instead check inside nsDASHReader::Decode*Data() whether you need to seek to DASH subsegment?

@@ +906,5 @@
>  
> +    // Switch sub-readers if requested and we have reached the right offset.
> +    // Do it here so that video decoding uses the same reader for one iteration
> +    // of the decode loop.
> +    mReader->PossiblySwitchVideoReaders();

Again, I'd prefer to keep backend-specific code in readers. Could you rename PossiblySwitchVideoReaders() to OnDataDecoded() or AfterDecode() or PostDecodeStep() or something else appropriate?

::: content/media/nsMediaCache.cpp
@@ +1766,5 @@
> +  NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
> +
> +  ReentrantMonitorAutoEnter mon(gMediaCache->GetReentrantMonitor());
> +
> +  // Write the current partical block to memory so that the decoder can access

s/partical/partial/

::: content/media/nsMediaCache.h
@@ +282,5 @@
> +  // Notifies the cache that the current bytes should be written to disk, as a
> +  // download stream switch is about to happen. We don't want to lose the bytes
> +  // in the case where the decoder switches back to this stream.
> +  // Called on the main thread.
> +  void NotifyDecoderSwitch();

How about you call this Flush() or something like that? That way it's not DASH specific.

::: content/media/webm/nsWebMReader.cpp
@@ +937,5 @@
> +
> +  // XXX Hack to get the resource to seek to the correct offset if the decode
> +  // thread is in shutdown, e.g. if the video is not autoplay.
> +  if (mDecoder->GetDecodeState()
> +      == nsDecoderStateMachine::DECODER_STATE_SHUTDOWN) {

Operators riding high in multi-line if statements please.

Wait, why do you try to seek of the decoder is shutdown? Normally we just fail and bail in when the decoder is shutdown.

::: content/media/webm/nsWebMReader.h
@@ +241,5 @@
>  };
>  
> +// |nsDASHWebMReader| extends nsWebMReader to provide seeking and switching
> +// capabilities, primarily by overriding |NextPacket| and |PushVideoPacket|.
> +class nsDASHWebMReader : public nsWebMReader

So the only reason you need to extend nsWebMReader is so that you can get the start time of next reader's first frame when switching, in order to get the current reader's last frame's end time? It'd be nice if we could avoid creating yet another reader subclass somehow... Is there a way we can avoid creating nsDASHWebMReader?

Comment 18

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

Comment on attachment 676435 [details] [diff] [review]
WIP v2.0 Add basic adaptation algorithm

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

In general the approach seems fine.

::: content/media/dash/nsDASHDecoder.cpp
@@ +854,5 @@
> +  NS_ASSERTION(VideoRepDecoder(), "Video decoder should not be null.");
> +  NS_ASSERTION(VideoRepDecoder()->GetResource(),
> +               "Video resource should not be null");
> +  bool reliable = false;
> +  double downloadRate = VideoRepDecoder()->GetResource()->GetDownloadRate(&reliable);

|reliable| is false for the first 3 seconds of the download, and you should probably only rely on the return value here when reliable is true. I'm not a network guy, so you might have a better idea when we should consider the channel stat's download rate as reliable.

Comment 19

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP v3.0 Add adaptive decoder/download and reader/resource switching

(In reply to Chris Pearce (:cpearce) from comment #17)
> Comment on attachment 676434 [details] [diff] [review]
> WIP v2.0 Add decoder/download and reader/resource switching
> 
> Review of attachment 676434 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Most of my comments are related to reducing the perceived impact of DASH on
> other media code, i.e. keeping the dash code's modular and contained. This
> will make it easier to maintain and will hopefully make it easier to add
> H.264 support for DASH in future.

Makes perfect sense :) Thanks for taking a look from that perspective.

> ::: content/media/MediaResource.cpp
> @@ +1017,5 @@
> >        ReentrantMonitorAutoEnter mon(mSeekOffsetMonitor);
> > ...
> > +      // E.g. When WebM parses cue position bytes at the end of the file, the
> 
> Cues are actually specified to be at the start of the file, not the end,
> it's just that certain tools are sloppy about enforcing this. :(

I adjusted the comment to start "For those WebM files which are encoded with cues at the end…".

> @@ +1030,5 @@
> > ...
> > +        if (NS_SUCCEEDED(rv) && !mByteRange.IsNull()
> > +            && aOffset > mByteRange.mEnd) {
> 
> We normally have && riding high in multiline if statements, […]

OK. I checked through the patches for &&, || and == at the start of lines and adjusted them accordingly.
 
> ::: content/media/MediaResource.h
> @@ +384,5 @@
> >    // Resume the current load since data is wanted again
> >    nsresult CacheClientResume();
> >  
> > +  // Notify that a switch is about to happen. Called on the main thread.
> > +  void PrepareForSwitch();
> 
> How about you call this Flush() or FlushPartialBlock() or something like
> that? That way it's not DASH specific.

…and…

> ::: content/media/nsMediaCache.h
> @@ +282,5 @@
> > ...
> > +  void NotifyDecoderSwitch();
> 
> How about you call this Flush() or something like that? That way it's not
> DASH specific.

:) Yup - wasn't sure entirely what to call these functions, and this sounds appropriate. So, I have nsDASHRepDecoder::PrepareForSwitch calling ChannelMediaResource::FlushCache, subsequently calling nsMediaCacheStream::FlushPartialBlock.

> ::: content/media/dash/nsDASHDecoder.cpp
> @@ +651,5 @@
> > ...
> > +    if (!AllowDecoderToDownloadData(aRepDecoder)) {
> 
> The name of AllowDecoderToDownloadData() makes it sound like it grants the
> ability to a decoder to download data, rather than checks to see if a
> download has been approved. Maybe rename it to
> IsDecoderAllowedToDownloadData() ?

Done.

> @@ +669,3 @@
> > +      // Increment to the subsegment index.
> > +      IncrSubsegmentIndex(aRepDecoder);
> 
> Code is typically read more times that it is written, so optimize code for
> readability. So I think it'd be better to rename IncrSubsegmentIndex() to
> IncrementSubsegmentIndex(). Then you might not feel so inclined to add a
> gratuitous comment here. ;)

Done.

> @@ +845,5 @@
> > +  uint32_t toDecoderIdx = (mVideoRepDecoderIdx+1)%mVideoRepDecoders.Length();
> > +  NS_ENSURE_TRUE(toDecoderIdx < mVideoRepDecoders.Length(),
> > +                 NS_ERROR_ILLEGAL_VALUE);
> > +
> > +  // Store index of decoder used to download this index.
> 
> You mean "store index of decoder used to download this byte range"?

Yup - I think I had "subsegment index" in there at one stage. This has been shifted around a bit with the adaptation code, so it says "subsegment index" again. But I get your point.

> ::: content/media/dash/nsDASHDecoder.h
> @@ +266,3 @@
> >  
> > +  // Current index of subsegments downloaded for audio/video decoder.
> > +  int64_t        mAudioSubsegmentIdx;
> 
> Style nit: here you have multi-space whitespace between variable type and
> name, but above you don't.

Fixed.

> ::: content/media/dash/nsDASHReader.cpp
> @@ +163,5 @@
> >      NS_ENSURE_SUCCESS(rv, rv);
> > +    // Use metadata from current video sub reader to populate aInfo.
> > +    if (mVideoReaders[i] == mVideoReader) {
> > +      mInfo.mHasVideo      = videoInfo.mHasVideo;
> > +      mInfo.mDisplay       = videoInfo.mDisplay;
> 
> Hmmm, not really sure what we should be doing with regards to metadata/aTags
> here.
> 
> aTags is supposed acquire the metadata (artist, song name, copyright, etc)
> from the resource, and it gets send to the HTML media element. Last time I
> checked we didn't implement it for WebM, but we will someday hopefully.
> 
> For Ogg chains (bug 455165) we're going to dispatch a loadedmetadata every
> time we encounter a new sub-resource in the stream, as the metadata
> genuinely does change (since we're (probably) playing a different song). But
> with DASH we should be playing the same logical resource when we switch DASH
> decoders, so the metadata should be the same right?

For DASH-WebM, yes. There is only supposed to be one Period per media presentation. This might need to be revisited when/if you implement tags for WebM - it may be that it's read from the manifest instead of the WebM file. But still, only one per presentation currently.

> So assuming all sub-resources in a DASH stream have the same metadata, I
> guess you should only pass aTags to one sub reader (mVideoReader maybe?) and
> pass nullptr to others.

Ok. Passing to audio reader only for now, since there is just one of those used.

> Also, once nsWebMReader::ReadMetadata() reads the metadata correctly, it
> should allocate a new nsHTMLMediaElement::MetadataTags, storing it in
> *aTags. So if you call ReadMetadata() in a loop passing in the same aTags
> here you'll leak all instances except the one from the last iteration (which
> is free'd elsewhere).

For now, I have a local var passed into nsWebMReader, with a comment saying that reconciling multiple tags and dealing with memory management needs to be considered if tags are to be used. Since WebM doesn't assign anything, I've left the memory cleanup out for now - the comment should be enough.

> @@ +360,5 @@
> >              (mAudioReader && !mAudioReader->IsSeekableInBufferedRanges()));
> >  }
> > +
> > +void
> > +nsDASHReader::RequestVideoReaderSwitch(uint32_t aFromReaderIdx,
> 
> That master reader doesn't store the index of the reader it's playing?

Download and reading are not sync'd so precisely, so we can't say for sure which offset or subsegment is being read in this context. All we know is a particular subsegment has been downloaded. Hence, the decoder sends a request to the reader to switch at the start of the specified subsegment.
Makes sense?

> @@ +406,5 @@
> > ...
> > +  if (mVideoReader->HasReachedDASHSwitchAccessPoint(switchIdx)) {
> 
> Rather than adding indentation, return early if this is not true, i.e.:
> 
>   if (!mVideoReader->HasReachedDASHSwitchAccessPoint(switchIdx)) {
>     return;
>   }

Done.

> ::: content/media/dash/nsDASHReader.h
> @@ +25,5 @@
> > +// monitor in its constructor, but only if the conditional value |aEnter| is
> > +// true. Used here to allow read access on the sub-readers' owning thread,
> > +// i.e. the decode thread, while locking write accesses from all threads,
> > +// and read accesses from non-decode threads.
> > +class ReentrantMonitorConditionallyEnter
> 
> How about putting this in nsVideoUtils.h?

OK, I've put it in beside ReentrantMonitorAutoExit.

> ::: content/media/nsBuiltinDecoderStateMachine.cpp
> > …
> > +    mReader->SeekToDASHSubsegmentIfRequested();
> 
> I'd prefer to keep backend-specific code in the backends, that is, the
> readers.
> 
> So can you instead check inside nsDASHReader::Decode*Data() whether you need
> to seek to DASH subsegment?
>
> ::: content/media/nsBuiltinDecoderReader.h
> …
> > +  // Switches subreaders if a DASH stream-switch event flag has been set.
> 
> Again, I'd like to keep backend specific code in the specific readers where
> possible. Granted it's not always practical, but it would be good if as many
> of these DASH specific methods as possible were pushed down into the sub
> readers, or at least renamed to make them more general. For example renaming
> PossiblySwitchVideoReaders() to AfterDecode() or PostDecodeStep().

> @@ +906,5 @@
> > +    mReader->PossiblySwitchVideoReaders();
> 
> Again, I'd prefer to keep backend-specific code in readers. Could you rename
> PossiblySwitchVideoReaders() to OnDataDecoded() or AfterDecode() or
> PostDecodeStep() or something else appropriate?

> ::: content/media/webm/nsWebMReader.h
> > +class nsDASHWebMReader : public nsWebMReader
> 
> So the only reason you need to extend nsWebMReader is so that you can get
> the start time of next reader's first frame when switching, in order to get
> the current reader's last frame's end time? It'd be nice if we could avoid
> creating yet another reader subclass somehow... Is there a way we can avoid
> creating nsDASHWebMReader?

I considered all of these comments together, and did two things:

1: Make DecodeLoop more generic: I removed PossiblySwitchVideoReaders from after DecodeVideoFrame, and put it inside PrepareToDecode, which is a replacement for Seek..IfRequested. In summary, the change to the DecodeLoop is just the addition of a call to PrepareToDecode, which sounds nicely generic and non-DASH-specific.

2: I added a new abstract class nsDASHRepReader. It's a collection of the prototypes used for DASH specific functionality:
-- Those prototypes were pulled from nsBuiltinDecoderReader, so it is now clean(er).
-- nsWebMReader now inherits from nsDASHRepReader, which itself inherits from nsBuiltinDecoderReader.
-- In addition, I removed the definition for nsDASHWebMReader and just packed the extra functions and member variables into nsWebMReader.

This does add a new class definition, which I know you wanted to avoid, but it keeps DASH away from nsBuiltinDecoderReader, yet still giving us a general class type to use within DASH code for future code reuse between WebM and H264 etc. decoder readers. And we get to avoid static_cast a but more.

I also remember that I had previously written and removed such a class, but it seems like it's needed. If you have another solution, let me know.

> ::: content/media/nsMediaCache.cpp
> @@ +1766,5 @@
> > +  NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
> > +
> > +  ReentrantMonitorAutoEnter mon(gMediaCache->GetReentrantMonitor());
> > +
> > +  // Write the current partical block to memory so that the decoder can access
> 
> s/partical/partial/

Done.

> ::: content/media/webm/nsWebMReader.cpp
> @@ +937,5 @@
> > +
> > +  // XXX Hack to get the resource to seek to the correct offset if the decode
> > +  // thread is in shutdown, e.g. if the video is not autoplay.
> > +  if (mDecoder->GetDecodeState()
> > +      == nsDecoderStateMachine::DECODER_STATE_SHUTDOWN) {
> 
> Operators riding high in multi-line if statements please.
> 
> Wait, why do you try to seek of the decoder is shutdown? Normally we just
> fail and bail in when the decoder is shutdown.

Right - it's not pleasant. Basically, unless I trigger a resource->Seek(), the media cache will work off of mStreamOffset and keep on trying to CacheClientSeek to the first Cluster, trying to download that byte range. I only want it to do that for the first decoder; all of the rest should be seeking to the cluster I want them to download first. So, if the decoder is in shutdown state, I can't do a reader->Seek(), because it's supposed to work on the decode thread which doesn't exist. I shouldn't be telling resource to Seek either, because that's supposed to be off the main thread. 
In summary, I am open to suggestions for this one :)

TODO:
-- Fix Seeking
-- Cleanup the bandwidth estimation.

Comment 20

[Matthew Gregan [:kinetik]]

Comment on attachment 676433 [details] [diff] [review]
WIP v2.0 Add nestegg_offset_seek

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

Sorry about the delay reviewing this.

::: media/libnestegg/include/nestegg.h
@@ +195,5 @@
>                            int64_t * end_pos);
>  
> +/** Seek @a track to @a offset.  Stream will seek directly to offset.
> +    Could be used to seek to any offset, but added to seek to the start of a
> +    cluster.

I don't think it can be used to seek anywhere without confusing the parser (or additional changes), it must seek to a resync point (i.e. Cluster).

::: media/libnestegg/src/nestegg.c
@@ +1650,5 @@
>  int
> +nestegg_offset_seek(nestegg * ctx, uint64_t offset)
> +{
> +  int r;
> +  struct ebml_list_node * node = ctx->segment.cues.cue_point.head;

node is unused.

Comment 21

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

Comment on attachment 679026 [details] [diff] [review]
WIP v3.0 Add adaptive decoder/download and reader/resource switching

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

Sorry for slow review! r=cpearce with my comments below addressed.

Could you please annotate methods which override a parent class methods with MOZ_OVERRIDE? That should make things clearer, thanks.

I'm not sure I mentioned this before, but I wrote a webserver which allows you to specify the download rate with a query parameter. This is useful for testing behaviour while downloading under sub-optimal network conditions, as I found existing webservers were very "bursty" with their rate limiting.

https://github.com/cpearce/HttpMediaServer

It may be of use to you.

::: content/media/MediaResource.cpp
@@ +1021,5 @@
> +        // Note: This may happen if a byte range is downloaded BEFORE the
> +        // reader has been switched and |Seek|s to the correct offset.
> +        if (NS_SUCCEEDED(rv) && !mByteRange.IsNull() &&
> +            aOffset > mByteRange.mEnd) {
> +          rv = NS_ERROR_NOT_AVAILABLE;

I'm still not clear why you do this. Is the main reason you do this so that inactive rep decoders can suspend their download? I think the comments would be better if they went more into the reasons _why_ we do this here.

::: content/media/VideoUtils.h
@@ +72,5 @@
>  
> +/**
> + * ReentrantMonitorConditionallyEnter
> + *
> + * Enters the supplied only if the conditional value |aEnter| is true.

s/Enters the supplied only if/Enters the supplied monitor only if/

::: content/media/dash/nsDASHDecoder.cpp
@@ +855,5 @@
> +  NS_ASSERTION(VideoRepDecoder(), "Video decoder should not be null.");
> +  NS_ASSERTION(VideoRepDecoder()->GetResource(),
> +               "Video resource should not be null");
> +  bool reliable = false;
> +  double downloadRate = VideoRepDecoder()->GetResource()->GetDownloadRate(&reliable);

I'm a bit worried about trusting the download rate while it's not "reliable", which means during the first few seconds of the download. What do you think? Should we refuse to switch decoders until the download is considered reliable?

::: content/media/dash/nsDASHDecoder.h
@@ +37,5 @@
>  public:
>    typedef class mozilla::net::IMPDManager IMPDManager;
>    typedef class mozilla::net::nsDASHMPDParser nsDASHMPDParser;
>    typedef class mozilla::net::Representation Representation;
> +  typedef mozilla::ReentrantMonitorAutoEnter ReentrantMonitorAutoEnter;

You won't need these "typedef mozilla::T T"s after you rebase this patch, as DASHDecoder is now in mozilla namespace. :)

@@ +83,5 @@
> +
> +  // Refers to downloading data bytes, i.e. non metadata.
> +  // Returns true if |aRepDecoder| is an active audio or video sub decoder AND
> +  // if metadata for all audio or video decoders has been read.
> +  // Could be called from any thread; enter monitor for rep/sub decoders.

It's not clear what you mean by "enter monitor for rep/sub decoders". Can you clarify that comment?

@@ +141,5 @@
> +  {
> +    ReentrantMonitorConditionallyEnter mon(!OnDecodeThread(),
> +                                           GetReentrantMonitor());
> +    if (aSubsegmentIdx < mVideoSubsegmentLoads.Length()) {
> +      return mVideoSubsegmentLoads[aSubsegmentIdx];

Do you need a 64bit value to store all indexes for the array? If so the array would be huge, and you'd be better of with a different data structure.

Why don't you just use 32bit values for now?

::: content/media/dash/nsDASHReader.cpp
@@ +178,5 @@
> +  for (uint i = 0; i < mVideoReaders.Length(); i++) {
> +    // XXX Need to consider reconciling multiple tags from readers and
> +    // memory management for the new objects assigned on the heap.
> +    nsHTMLMediaElement::MetadataTags* tags;
> +    rv = mVideoReaders[i]->ReadMetadata(&videoInfo, &tags);

This would start leaking if we implemented metadata in webm. Just pass nullptr for now.

@@ +193,2 @@
>    if (mAudioReader) {
>      rv = mAudioReader->ReadMetadata(&audioInfo, aTags);

Long term, you might want some sort of merging of the tags in sub streams; it's possible that some encoders would only put some of the metadata in some streams.

Better yet, you could bring this up with the guys editing the DASH spec, and get it standardized in which segment the metadata resides.

@@ +483,5 @@
> +  }
> +
> +  PossiblySwitchVideoReaders();
> +
> +  // For each sub reader, update the seek point if a switch was requested.

Can this loop be in PossiblySwitchVideoReaders(), so that we don't do it if we don't need to?

Or might we need to do this loop even if we bail out of PossiblySwitchVideoReaders() early?

::: content/media/dash/nsDASHRepDecoder.h
@@ +20,5 @@
>  #include "nsDASHDecoder.h"
>  #include "nsWebMDecoder.h"
>  #include "nsWebMReader.h"
>  #include "nsBuiltinDecoder.h"
> +#include "nsDASHRepReader.h"

Can you just predeclare class nsDASHRepReader instead of including this?

@@ +195,5 @@
>  
>    // The current byte range being requested.
>    MediaByteRange  mCurrentByteRange;
> +  // Index of the current byte range. Initialized to -1.
> +  int64_t         mSubsegmentIdx;

int32_t ?

::: content/media/dash/nsDASHRepReader.h
@@ +45,5 @@
> +  // Returns list of ranges for index frame start/end offsets. Used by DASH.
> +  virtual nsresult GetSubsegmentByteRanges(nsTArray<MediaByteRange>& aByteRanges) = 0;
> +
> +  // Returns true if the reader has reached a DASH switch access point.
> +  virtual bool HasReachedSubsegment(uint64_t aSubsegmentIndex) = 0;

Should you use 32 bit values for the indexes of these two functions?

::: netwerk/dash/mpd/nsDASHWebMODManager.cpp
@@ +214,5 @@
> +  // Iterate until the current |Representation|'s bitrate is higher than the
> +  // estimated available bandwidth.
> +  for (uint32_t i = 1; i < GetNumRepresentations(aAdaptSetIdx); i++) {
> +    NS_ENSURE_TRUE(GetRepresentation(aAdaptSetIdx, i), false);
> +    if (aBandwidth < GetRepresentation(aAdaptSetIdx, i)->GetBitrate()) {

Maybe you should leave a bit of wiggle room here, i.e.:

if (aBandwidth*1.05 < GetRep()->GetBitrate()) ...

That way if the available bandwidth is close to the rep's bandwith, a small variability in available bandwidth won't cause playback to stutter. What do you think?

Comment 22

[Steve Workman [:sworkman] (INACTIVE)]

Chris, thanks for the r+. FYI: Seeking has proven to be awkward (ugh, threads) and has forced some changes to the patched you just r+'d. Although it forms a good base patch, I'm not going to land anything until seeking is done and is passing most, if not all, of the seeking tests. I'll also include as many of your comments as are applicable before uploading another patch.

Matthew, I'll get on those comments for nestegg next week.

Comment 23

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

Comment on attachment 675878 [details] [diff] [review]
v1 Enables DASH build

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

(In reply to Chris Pearce (:cpearce) from comment #14)
> If you only want DASH on for demos, then I think we should build it by
> default, but have it disabled by default via an about:config pref. People
> who want to demo/test it can pref it on in about:config.

Just getting this old request out of my review queue... I stand by the above comment; I'm happy to build DASH by default, provided it's prefed off by default, until it's stabilized and has (green!) tests.

Comment 24

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP v3.1 Add adaptive decoder/download and reader/resource switching

(In reply to Chris Pearce (:cpearce) from comment #21)
> Comment on attachment 679026 [details] [diff] [review]
> WIP v3.0 Add adaptive decoder/download and reader/resource switching
> 
> Review of attachment 679026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for slow review! r=cpearce with my comments below addressed.
> 
> Could you please annotate methods which override a parent class methods with
> MOZ_OVERRIDE? That should make things clearer, thanks.

DONE for the functions added in this patch. I can open another bug to do the other classes added for DASH.

> I'm not sure I mentioned this before, but I wrote a webserver which allows
> you to specify the download rate with a query parameter. This is useful for
> testing behaviour while downloading under sub-optimal network conditions, as
> I found existing webservers were very "bursty" with their rate limiting.
> 
> https://github.com/cpearce/HttpMediaServer
> 
> It may be of use to you.

Thanks!

> ::: content/media/MediaResource.cpp
> @@ +1021,5 @@
> > +        // Note: This may happen if a byte range is downloaded BEFORE the
> > +        // reader has been switched and |Seek|s to the correct offset.
> > +        if (NS_SUCCEEDED(rv) && !mByteRange.IsNull() &&
> > +            aOffset > mByteRange.mEnd) {
> > +          rv = NS_ERROR_NOT_AVAILABLE;
> 
> I'm still not clear why you do this. Is the main reason you do this so that
> inactive rep decoders can suspend their download? I think the comments would
> be better if they went more into the reasons _why_ we do this here.

I replaced it with the following comment (after I spent some time scratching my head wondering why I did it that way…):

"Cache may try to seek from the next uncached byte: this offset may be after the byte range being seeked, i.e. the range containing |mSeekOffset|, which is the offset actually requested by the reader. This case means that the seeked range is already cached. For byte range downloads, we do not permit the cache to request bytes outside the seeked range. Instead, the decoder is responsible for controlling the sequence of byte range downloads. As such, return silently, and do NOT request a new download."

Let me know if this doesn't explain it any better.

> ::: content/media/VideoUtils.h
> @@ +72,5 @@
> >  
> > +/**
> > + * ReentrantMonitorConditionallyEnter
> > + *
> > + * Enters the supplied only if the conditional value |aEnter| is true.
> 
> s/Enters the supplied only if/Enters the supplied monitor only if/

Done.

> ::: content/media/dash/nsDASHDecoder.cpp
> @@ +855,5 @@
> > +  NS_ASSERTION(VideoRepDecoder(), "Video decoder should not be null.");
> > +  NS_ASSERTION(VideoRepDecoder()->GetResource(),
> > +               "Video resource should not be null");
> > +  bool reliable = false;
> > +  double downloadRate = VideoRepDecoder()->GetResource()->GetDownloadRate(&reliable);
> 
> I'm a bit worried about trusting the download rate while it's not
> "reliable", which means during the first few seconds of the download. What
> do you think? Should we refuse to switch decoders until the download is
> considered reliable?

So, I want to work up a better adaptation algorithm anyway - not the end goal of algorithms, just something that takes the download rate from all the streams into consideration - I'm going to leave it as is for this patch, and consider the reliability angle for the next iteration.

> ::: content/media/dash/nsDASHDecoder.h
> @@ +37,5 @@
> >  public:
> >    typedef class mozilla::net::IMPDManager IMPDManager;
> >    typedef class mozilla::net::nsDASHMPDParser nsDASHMPDParser;
> >    typedef class mozilla::net::Representation Representation;
> > +  typedef mozilla::ReentrantMonitorAutoEnter ReentrantMonitorAutoEnter;
> 
> You won't need these "typedef mozilla::T T"s after you rebase this patch, as
> DASHDecoder is now in mozilla namespace. :)

Yay - cleaner code.

> @@ +83,5 @@
> > +
> > +  // Refers to downloading data bytes, i.e. non metadata.
> > +  // Returns true if |aRepDecoder| is an active audio or video sub decoder AND
> > +  // if metadata for all audio or video decoders has been read.
> > +  // Could be called from any thread; enter monitor for rep/sub decoders.
> 
> It's not clear what you mean by "enter monitor for rep/sub decoders". Can
> you clarify that comment?

I think that was a copy and paste error. The function just enters the decoder's monitor. I've fixed the comment, and a similar one for a nearby prototype.

> @@ +141,5 @@
> > +  {
> > +    ReentrantMonitorConditionallyEnter mon(!OnDecodeThread(),
> > +                                           GetReentrantMonitor());
> > +    if (aSubsegmentIdx < mVideoSubsegmentLoads.Length()) {
> > +      return mVideoSubsegmentLoads[aSubsegmentIdx];
> 
> Do you need a 64bit value to store all indexes for the array? If so the
> array would be huge, and you'd be better of with a different data structure.
> 
> Why don't you just use 32bit values for now?

Yeah - I think 32 bits is fine. 64 is good for the offset, but the number of subsegments is more of a time-based interval that byte-based. So, 32 should work.

> ::: content/media/dash/nsDASHReader.cpp
> @@ +178,5 @@
> > +  for (uint i = 0; i < mVideoReaders.Length(); i++) {
> > +    // XXX Need to consider reconciling multiple tags from readers and
> > +    // memory management for the new objects assigned on the heap.
> > +    nsHTMLMediaElement::MetadataTags* tags;
> > +    rv = mVideoReaders[i]->ReadMetadata(&videoInfo, &tags);
> 
> This would start leaking if we implemented metadata in webm. Just pass
> nullptr for now.

No can do. WebMReader::ReadMetadata sets tags to nullptr, so &tags can't be nullptr too. But I get what you're saying, so I added an nsAutoPtr to take control of memory once the ptr is passed back, with it's scope restricted to a single iteration of the for loop.

> @@ +193,2 @@
> >    if (mAudioReader) {
> >      rv = mAudioReader->ReadMetadata(&audioInfo, aTags);
> 
> Long term, you might want some sort of merging of the tags in sub streams;
> it's possible that some encoders would only put some of the metadata in some
> streams.
> 
> Better yet, you could bring this up with the guys editing the DASH spec, and
> get it standardized in which segment the metadata resides.

Sounds like a possibility.

> @@ +483,5 @@
> > +  }
> > +
> > +  PossiblySwitchVideoReaders();
> > +
> > +  // For each sub reader, update the seek point if a switch was requested.
> 
> Can this loop be in PossiblySwitchVideoReaders(), so that we don't do it if
> we don't need to?
> 
> Or might we need to do this loop even if we bail out of
> PossiblySwitchVideoReaders() early?

I'd rather leave it so that each sub reader is responsible for deciding if there's any prep to do or not. In the case of the first subsegment loaded from the first reader, there is no switch involved, but there is a Seek to the start of the subsegment. (This seek is requested in DASHRepDecoder::LoadNextByteRange() but needs to be issued here on the decode thread). So, I don't want PossibleSwitchVideoReaders to make the decision to call it or not. It also leaves the function as a nice placeholder, already being called, for future decode preparations (possibly required for dash/h265?), but the first reason is the main one; making sure seeking happens. However, the function name and the comment are mismatched in terms of specificity - I've adjusted that.

> ::: content/media/dash/nsDASHRepDecoder.h
> @@ +20,5 @@
> >  #include "nsDASHDecoder.h"
> >  #include "nsWebMDecoder.h"
> >  #include "nsWebMReader.h"
> >  #include "nsBuiltinDecoder.h"
> > +#include "nsDASHRepReader.h"
> 
> Can you just predeclare class nsDASHRepReader instead of including this?

Yes I can :) Done.

> @@ +195,5 @@
> >  
> >    // The current byte range being requested.
> >    MediaByteRange  mCurrentByteRange;
> > +  // Index of the current byte range. Initialized to -1.
> > +  int64_t         mSubsegmentIdx;
> 
> int32_t ?

Done.

> ::: content/media/dash/nsDASHRepReader.h
> @@ +45,5 @@
> > +  // Returns list of ranges for index frame start/end offsets. Used by DASH.
> > +  virtual nsresult GetSubsegmentByteRanges(nsTArray<MediaByteRange>& aByteRanges) = 0;
> > +
> > +  // Returns true if the reader has reached a DASH switch access point.
> > +  virtual bool HasReachedSubsegment(uint64_t aSubsegmentIndex) = 0;
> 
> Should you use 32 bit values for the indexes of these two functions?

Done done.

> ::: netwerk/dash/mpd/nsDASHWebMODManager.cpp
> @@ +214,5 @@
> > +  // Iterate until the current |Representation|'s bitrate is higher than the
> > +  // estimated available bandwidth.
> > +  for (uint32_t i = 1; i < GetNumRepresentations(aAdaptSetIdx); i++) {
> > +    NS_ENSURE_TRUE(GetRepresentation(aAdaptSetIdx, i), false);
> > +    if (aBandwidth < GetRepresentation(aAdaptSetIdx, i)->GetBitrate()) {
> 
> Maybe you should leave a bit of wiggle room here, i.e.:
> 
> if (aBandwidth*1.05 < GetRep()->GetBitrate()) ...
> 
> That way if the available bandwidth is close to the rep's bandwith, a small
> variability in available bandwidth won't cause playback to stutter. What do
> you think?

Yeah, this change is very simple. Like I said earlier, though, there's a next iteration of the adaptation algorithm on the way :) (Also, if (aBandwidth*0.95 < GetRep()->GetBitrate()) ... right? We want to consider the available bandwidth resource as being 5% less than that estimated. So, aBandwidth will have to be (1/0.95)*bitrate) or more in order for the stream to be considered downloadable. Right?)



In addition to these changes based on your comments, I adjusted this patch based on the subsequent Seeking patch. There were some changes in the seeking patch that undid or rearranged base code in this patch. I wanted to keep the Adaptation patch as more of a baseline. So, I'm uploading a diff of diffs next to help you see the changes. Comments with the upload.

Comment 25

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: Diff from v3.0 to v3.1 Add adaptive decoder/download and reader/resource switching

Most changes are removing bit rot or attending to your comments; other things:
-- Added ChannelMediaResource::NotifyLastByteRange to call mCacheStream.NotifyDataEnded at the end of the downloads (This is in prep for unit tests).
-- Adjusted call to IncrementSubsegmentIndex() in DASHDecoder::NotifyDownloadEnded so that it is only called for correct sub decoders and correct subsegments, thus enforcing the sequence of subsegment downloads (This is in prep for Seeking. If we changed position in the load sequence due to a seek, we must ensure that previous downloads completing be prohibited from changing the current position in the download sequence).
-- Adjusted appending elements to/changing elements in mVideoSubsegmentLoads so that the load sequence is accurate. Also moved append/change to NotifyDownloadEnded, just before call to LoadNextByteRange, rather than in PossiblySwitchDecoder - seems more appropriate.
-- Added IsDecoderAllowedToDownloadSubsegment, also used to enforce download sequence.
-- Added NS_INLINE_DECL_THREADSAFE_REFCOUNTING to DASHRepReader. Assignment at init is on the main thread; re-assignment at reader switching time is on the decode thread. Thus thread safe refcounting is needed. MediaDecoderReader does not have this.
-- In MediaDecoderReader I had to change mAudio|VideoQueue to Audio|VideoQueue() to fix some unit test bugs.

Let me know if these don't make sense, and sorry for re-arranging things and asking for a re-review.

Comment 26

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2 Enables DASH build; but DASH pref'd off

DASH will be enabled to build by default, but will be functionally hidden behind the pref "media.dash.enabled" (set to false). Motivation is to get code into mozilla-central and work on unit tests separately.

Comment 27

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

Comment on attachment 686429 [details] [diff] [review]
WIP v3.1 Add adaptive decoder/download and reader/resource switching

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

r=cpearce with the comments below addressed, but I also want to hear about what you're expecting when you're calling mDecoder->IsShutdown() in WebMReader::RequestSeekToSubsegment(), it doesn't look like your using it right.

::: content/media/MediaDecoder.h
@@ +560,5 @@
>    bool OnDecodeThread() const MOZ_OVERRIDE;
>  
>    // Returns the monitor for other threads to synchronise access to
>    // state.
> +  virtual ReentrantMonitor& GetReentrantMonitor() MOZ_OVERRIDE;

This is virtual in the parent class, so you shouldn't need to add "virtual" here?

Ditto for OnreadMetadataCompleted().

::: content/media/dash/DASHDecoder.cpp
@@ +676,5 @@
> +    nsRefPtr<DASHRepDecoder> decoder = aRepDecoder;
> +    {
> +      ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> +
> +      if (!IsDecoderAllowedToDownloadSubsegment(aRepDecoder,

If this occurs only when there's been a logic error, maybe you should add an NS_WARNING() here (or NS_ASSERTION) as well.

::: content/media/dash/DASHDecoder.h
@@ +98,5 @@
> +  nsresult PossiblySwitchDecoder(DASHRepDecoder* aRepDecoder);
> +
> +  // Sets the byte range index for audio|video downloads. Will only increment
> +  // for current active decoders. Could be called from any thread. Requires
> +  // monitor since it's write access.

" Requires monitor since it's write access." could be worded better. ;)

Ditto for the comment on IncrementSubsegmentIndex.

::: content/media/dash/DASHRepDecoder.cpp
@@ +337,5 @@
> +
> +  // Ensure that the media cache writes any data held in its partial block.
> +  mResource->FlushCache();
> +}
> +

Extra line break here.

::: content/media/webm/WebMReader.cpp
@@ +414,5 @@
>    *aInfo = mInfo;
>  
>    *aTags = nullptr;
>  
> +#ifdef MOZ_DASH

MediaDecoderStateMachine should be the one calling OnReadMetadataCompleted()(say in MediaDecoderStateMachine::DecodeMetadata()), otherwise the other decoders won't be called back here, and the interface/contract of OnReadMetadataCompleted() is broken?

@@ +972,5 @@
> +  mSeekToCluster = aIdx;
> +
> +  // XXX Hack to get the resource to seek to the correct offset if the decode
> +  // thread is in shutdown, e.g. if the video is not autoplay.
> +  if (mDecoder->IsShutdown()) {

This doesn't seem right; mDecoder->IsShutdown() returns true when the decoder is shutdown, not when the decode thread is shutdown. If mDecoder::IsShutdown() returns true, you should be shutting down your readers.

You should probably just bail out here if mDecoder->IsShutdown() returns true?

::: content/media/webm/WebMReader.h
@@ +341,5 @@
> +
> +  // Indicates if the reader has reached a switch access point.  Set in
> +  // |NextPacket| and read in |HasReachedSubsegment|. Accessed on
> +  // decode thread only.
> +  bool mReachedSAP;

mReachedStreamAccessPoint

Optimize code for readability, not writability, so don't abbreviate things, within reason of course.

Comment 28

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Chris Pearce :cpearce (mostly offline 4-8 December NZST) from comment #27)
> Comment on attachment 686429 [details] [diff] [review]
> WIP v3.1 Add adaptive decoder/download and reader/resource switching
> 
> Review of attachment 686429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=cpearce with the comments below addressed, but I also want to hear about
> what you're expecting when you're calling mDecoder->IsShutdown() in
> WebMReader::RequestSeekToSubsegment(), it doesn't look like your using it
> right.

Thanks for the review - yeah, I read your comment on IsShutdown and never mind me not understanding it correctly, I don't think it's needed. It was a hack anyway, so I'm removing that code. See later for more info.

> ::: content/media/MediaDecoder.h
> @@ +560,5 @@
> > +  virtual ReentrantMonitor& GetReentrantMonitor() MOZ_OVERRIDE;
> 
> This is virtual in the parent class, so you shouldn't need to add "virtual"
> here?

Ah, I learned something new about 'virtual'. So, if it's virtual in the most base class, it's virtual everywhere. Good to know.
Change removed.

> Ditto for OnreadMetadataCompleted().

OK, change removed.

> ::: content/media/dash/DASHDecoder.cpp
> @@ +676,5 @@
> > +    nsRefPtr<DASHRepDecoder> decoder = aRepDecoder;
> > +    {
> > +      ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> > +
> > +      if (!IsDecoderAllowedToDownloadSubsegment(aRepDecoder,
> 
> If this occurs only when there's been a logic error, maybe you should add an
> NS_WARNING() here (or NS_ASSERTION) as well.

Good addition. Done.

> ::: content/media/dash/DASHDecoder.h
> @@ +98,5 @@
> > +  // for current active decoders. Could be called from any thread. Requires
> > +  // monitor since it's write access.
> 
> " Requires monitor since it's write access." could be worded better. ;)
> 
> Ditto for the comment on IncrementSubsegmentIndex.

Yeah - I think that sounded in my head when I wrote it. Not so much now.
 
> ::: content/media/dash/DASHRepDecoder.cpp
> @@ +337,5 @@
> > +
> > +  // Ensure that the media cache writes any data held in its partial block.
> > +  mResource->FlushCache();
> > +}
> > +
> 
> Extra line break here.

Removed.

> ::: content/media/webm/WebMReader.cpp
> @@ +414,5 @@
> >    *aInfo = mInfo;
> >  
> >    *aTags = nullptr;
> >  
> > +#ifdef MOZ_DASH
> 
> MediaDecoderStateMachine should be the one calling
> OnReadMetadataCompleted()(say in
> MediaDecoderStateMachine::DecodeMetadata()), otherwise the other decoders
> won't be called back here, and the interface/contract of
> OnReadMetadataCompleted() is broken?

So, the comment in AbstracMediaDecoder.h is potentially misleading here. The point is for WebMReader, which is a sub-reader, to call the sub-decoder to tell it that cues data has been read and it's ok to populate the sub-decoder's own array of byte range data. Then it calls the main decoder with a pointer to it's self in a overloaded version of OnReadMetadataCompleted.

But, based on the comment in AbstractMediaDecoder.h, it would suggest that every reader will be calling it's associated decoder, DASH or not. Which is not correct. I've adjusted the comment, and can follow up with a better clarification if you need.  Here's the new comment, also in MediaDecoder.h:

  // May be called by the reader to notify this decoder that the metadata from
  // the media file has been read. Call on the decode thread only.

> @@ +972,5 @@
> > +  mSeekToCluster = aIdx;
> > +
> > +  // XXX Hack to get the resource to seek to the correct offset if the decode
> > +  // thread is in shutdown, e.g. if the video is not autoplay.
> > +  if (mDecoder->IsShutdown()) {
> 
> This doesn't seem right; mDecoder->IsShutdown() returns true when the
> decoder is shutdown, not when the decode thread is shutdown. If
> mDecoder::IsShutdown() returns true, you should be shutting down your
> readers.
> 
> You should probably just bail out here if mDecoder->IsShutdown() returns
> true?

So, what I was trying to do, without doubt in a hacky way, was to force an immediate resource->Seek if the decoder thread did not exist. The motive was to get the media cache specifically to seek to the desired offset and stop trying to request CacheClientSeek for the previous offset. For now, it's fine to just remove this - the resource->Seek() happens at the start of the DecodeLoop, and I have undesired CacheClientSeeks blocked in that function anyway. Possibly something to be revisited, but it's not causing a problem and not breaking functionality.

> ::: content/media/webm/WebMReader.h
> @@ +341,5 @@
> > +
> > +  // Indicates if the reader has reached a switch access point.  Set in
> > +  // |NextPacket| and read in |HasReachedSubsegment|. Accessed on
> > +  // decode thread only.
> > +  bool mReachedSAP;
> 
> mReachedStreamAccessPoint
> 
> Optimize code for readability, not writability, so don't abbreviate things,
> within reason of course.

:) Done.

FYI: Mozilla-inbound is currently closed, but I'll try pushing the change sets later.

Comment 29

[Steve Workman [:sworkman] (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=03036e724c43

Comment 30

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Matthew Gregan [:kinetik] from comment #20)
> Comment on attachment 676433 [details] [diff] [review]
> WIP v2.0 Add nestegg_offset_seek
> 
> Review of attachment 676433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry about the delay reviewing this.

No worries. Apologies for the late review comments. And thanks for accepting the merge request on github.
 
> ::: media/libnestegg/include/nestegg.h
> @@ +195,5 @@
> >                            int64_t * end_pos);
> >  
> > +/** Seek @a track to @a offset.  Stream will seek directly to offset.
> > +    Could be used to seek to any offset, but added to seek to the start of a
> > +    cluster.
> 
> I don't think it can be used to seek anywhere without confusing the parser
> (or additional changes), it must seek to a resync point (i.e. Cluster).

Comment updated.
 
> ::: media/libnestegg/src/nestegg.c
> @@ +1650,5 @@
> >  int
> > +nestegg_offset_seek(nestegg * ctx, uint64_t offset)
> > +{
> > +  int r;
> > +  struct ebml_list_node * node = ctx->segment.cues.cue_point.head;
> 
> node is unused.

Removed.

Comment 31

[Steve Workman [:sworkman] (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a192e4bab599
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cba543bf482
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e5618ca8bb

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a192e4bab599
https://hg.mozilla.org/mozilla-central/rev/8cba543bf482
https://hg.mozilla.org/mozilla-central/rev/a4e5618ca8bb