816726 - DASH: Add support for seeking in DASH-WebM streams

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Steve Workman [:sworkman] (INACTIVE)]

Add support for seeking in the byte-range-downloaded DASH-WebM streams.

Should take previous stream switches into consideration.
Should read byte ranges/subsegments (WebM clusters) from the media cache when available instead of downloading.

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Return timestamp in nestegg_get_cue_point to support DASH-WebM Seeking

In order to support seeking for DASH-WebM, we need to know which cluster to seek to for a download. By adding tstamp as a return value in nestegg_get_cue_point, we can store the timestamp of each cluster in addition to the byte ranges. Thus, we can determine which cluster, i.e. byte range, should be downloaded for a time-based seek.

Comment 2

[Matthew Gregan [:kinetik]]

Comment on attachment 686807 [details] [diff] [review]
v1.0 Return timestamp in nestegg_get_cue_point to support DASH-WebM Seeking

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

::: media/libnestegg/src/nestegg.c
@@ +1606,5 @@
>  
>    /* Initialise return values */
>    *start_pos = -1;
>    *end_pos   = -1;
> +  *tstamp    = 0;

Spacing is off here.  I'd prefer just a single space on either side of the = for all of these, since that's the prevailing style in this code.

@@ +1634,5 @@
>              return -1;
>            if (cluster_count == cluster_num) {
>              *start_pos = ctx->segment_offset+seek_pos;
> +            if (ne_get_uint(cue_point->time, &time) == 0)
> +              *tstamp = time * tc_scale;

Return an error if ne_get_uint fails?  Or otherwise we need a value for tstamp to make it clear that a valid value wasn't provided.

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Add seeking capabilities to DASH-WebM code

The following changes are made in this patch to enable seeking. I've organized it roughly from the element first calling decoder::Seek(), then through to reader::Seek() called from the state machine. Let me know if you have any questions.


DASHDecoder::Seek
-- Called from media element.
-- Calls CancelByteRangeLoad for each DASHRepDecoder to stop current downloads and prepare for seeking.
-- Finally calls MediaDecoder::Seek.

ChannelMediaResource::CancelByteRangeOpen
-- Cancels byte range downloads. Called from DASHDecoder::Seek, before it passes the seek onto the state machine, and thus before the state machine tells the reader to seek. Calls MediaCacheStream::NotifyDownloadCancelled.

MediaCacheStream::NotifyDownloadCancelled and MediaCacheStream::mDownloadCancelled
-- If a Seek happens during a byte range download, and the media stream is waiting for more data from ChannelMediaResource, we need to tell it to stop waiting, and subsequently to stop reading. This means that readers will not block indefinitely, but instead, will stop reading and be available to seek.


DASHReader::Seek
-- Called from state machine
-- Resets the reader and all sub readers
-- Gets the correct subsegment/byte range to download based on the current reader - assumes that the subsegments are correctly aligned time-wise, which they should be according to spec.
-- Notifies the main decoder which subsegment is being seeked (so it can start a download), and then fires of audio|video reader seek.

Back to DASHDecoder for NotifySeekInAudio|VideoSubsegment
-- dispatches DASHRepDecoder::LoadNextByteRange

DASHRepDecoder for LoadNextByteRange
-- Will only download if IsSubsegmentCached() returns false. Otherwise, it notifies DASHDecoder that the byte range is downloaded, and let's it decide which rep decoder to choose next.
*** So, basically, we have inserted knowledge of the cache into the byte range load cycle ***



Other Changes:

DASHReader::RequestVideoReaderSwitch
-- Adjusts the load history stored in mSwitchToVideoSubsegmentIndexes so that it ends at the seek subsegment. So, we maintain load history before that point, but have a clean slate for future loads.

DASHRepDecoder::GetByteRangeForSeek
-- Adjusted the logic so that decoders have permission to download based on the current subsegment as well as their own identity.


Added TimestampedMediaByteRange
-- Adds starting timestamp to the structure, in addition to start and end byte offset. Used to determine which byte range is appropriate for a time-based seek (as opposed to offset-based seeking).
-- Timestamps obtained in WebMReader::ReadMetadata when cluster byte ranges are populated.

In PossiblySwitchDecoder, adjusted the switching/adaptation logic
-- In choosing the next stream for downloading, DASHDecoder will choose a higher-rate stream if it is already in the cache.

Removed an assertion for DASHRepDecoder::GetImageContainer, because it was wrong.

Added ReentrantMonitor protection for DASHRepDecoder::mByteRanges

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 Add seeking capabilities to DASH-WebM code

Removed a call to CloseChannel() from mozilla::ChannelMediaResource::CancelByteRangeOpen(). In this context, all that is needed is to notify to media cache that the byte range request was canceled. In turn, this will wake any readers waiting for data and cancel the reads. The byte range load will already be canceled in ::CacheClientSeek() if need be. 

FYI, this is in the context of a byte range being downloaded when a seek occurs. We need to cancel the current download, and then request the byte range for the seek point.

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: Changes from v1.0 to v1.1 Add seeking capabilities to DASH-WebM code

Added for information only.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.2 Add seeking capabilities to DASH-WebM code

Undid a change in DASHDecoder::PossiblySwitchDecoder - returned code to previous state where 'switch from' decoder is told to PrepareForSwitch, as opposed to the 'switch to' decoder. This results in a cache flush for the 'switch from' decoder's media resource, just after it has downloaded a subsegment/byte range, and just before the next byte range is requested via the 'switch to' decoder.

Note: I had changed this in an attempt to fix a seeking test, but it is not needed and is causing more problems than solving any.

Comment 7

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

Comment on attachment 691075 [details] [diff] [review]
v1.2 Add seeking capabilities to DASH-WebM code

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

::: content/media/MediaCache.h
@@ +281,5 @@
>    void NotifyDataEnded(nsresult aStatus);
> +  // Notifies the cache that the download was cancelled. Sets
> +  // |mDownloadCancelled| to false and notifies any thread waiting on the media
> +  // cache monitor. In this way, if |Read| is waiting when a download is
> +  // cancelled, it will be wakened and should stop trying to read.

Also say what "un-cancels" the download/stream.

::: content/media/MediaResource.cpp
@@ +571,5 @@
> +ChannelMediaResource::CancelByteRangeOpen()
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
> +
> +  // Byte range download will be canceled in |CacheClientSeek|. Here, we only

You spelt canceled with two 'l's in MediaCache.cpp (the British way), but with one 'l' (the American way) here. ;)

::: content/media/dash/DASHDecoder.cpp
@@ +999,5 @@
> +
> +  return MediaDecoder::Seek(aTime);
> +}
> +
> +

Extra line break here.

@@ +1002,5 @@
> +
> +
> +void
> +DASHDecoder::NotifySeekInVideoSubsegment(int64_t aRepDecoderIdx,
> +                                           int64_t aSubsegmentIdx)

Can you fix the indenting here? I accidentally left the indentation broken when I removed the ns prefix, no point propagating it further.

::: content/media/dash/DASHDecoder.h
@@ +63,5 @@
>  
> +  // Notification from |DASHReader| that a seek has occurred in
> +  // |aSubsegmentIdx|. Passes notification onto subdecoder which downloaded
> +  // the subsegment already, if download is in the past. Otherwise, it returns.
> +  void NotifySeekInVideoSubsegment(int64_t aRepDecoderIdx,

Your subsegment and decoder indexes are 32bit values elsewhere, any reason they're not 32bit as well here?

God help us if we have an array which requires more than 32bits to index into it; we're stuck with being a 32bit process on Windows for the forseeable future for example.

@@ +79,5 @@
>    // read. Declared here to allow overloading.
>    void OnReadMetadataCompleted() MOZ_OVERRIDE { }
>  
> +  // Seeks to aTime in seconds
> +  nsresult Seek(double aTime);

nsresult Seek(double aTime) MOZ_OVERRIDE;

@@ +111,5 @@
>    // for current active decoders. Could be called from any thread.
>    // Requires monitor because of write to |mAudioSubsegmentIdx| or
>    // |mVideoSubsegmentIdx|.
>    void SetSubsegmentIndex(DASHRepDecoder* aRepDecoder,
> +                          uint32_t aSubsegmentIdx);

This index unsigned but it's signed elsewhere?

@@ +157,5 @@
>        return 0;
>      }
>    }
>  
> +  int32_t GetSwitchCountAtVideoSubsegment(int64_t aSubsegmentIdx)

This index is 64bit but you're using 32bits elsewhere...

::: content/media/dash/DASHReader.h
@@ +28,5 @@
>  {
>  public:
>    DASHReader(AbstractMediaDecoder* aDecoder);
>    ~DASHReader();
> +  nsresult ResetDecode();

MOZ_OVERRIDE; you may as well add that to any other methods in this class that override a method in the parent class.

::: content/media/dash/DASHRepDecoder.h
@@ +84,5 @@
> +  // Cancels current byte range loads. Called on the main thread only.
> +  void CancelByteRangeLoad();
> +
> +  // Returns true if the subsegment is already in the media cache.
> +  bool IsSubsegmentCached(int64_t aSubsegmentIdx);

int32_t aSubsegmentIdx?

::: content/media/dash/DASHRepReader.h
@@ +40,5 @@
>  
>    // Sets range for index frame bytes; used by DASH.
>    virtual void SetIndexByteRange(MediaByteRange &aByteRange) = 0;
>  
> +  // Returns the index of the subsegment which contains the seek time.

State units, microseconds I would presume.

::: content/media/webm/WebMReader.cpp
@@ +956,5 @@
>    NS_ENSURE_TRUE(aByteRanges.IsEmpty(), NS_ERROR_ALREADY_INITIALIZED);
>    NS_ENSURE_FALSE(mClusterByteRanges.IsEmpty(), NS_ERROR_NOT_INITIALIZED);
>    NS_ENSURE_FALSE(mCuesByteRange.IsNull(), NS_ERROR_NOT_INITIALIZED);
>  
> +  for (uint32_t i = 0; i < mClusterByteRanges.Length(); i++) {

Why can't you still use the copy constructor here?

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Chris Pearce (:cpearce) from comment #7)
> Comment on attachment 691075 [details] [diff] [review]
> v1.2 Add seeking capabilities to DASH-WebM code
> 
> Review of attachment 691075 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaCache.h
> @@ +281,5 @@
> > +  // |mDownloadCancelled| to false and notifies any thread waiting on the media
> 
> Also say what "un-cancels" the download/stream.

Done.

> ::: content/media/MediaResource.cpp
> @@ +571,5 @@
> > +  // Byte range download will be canceled in |CacheClientSeek|. Here, we only
> 
> You spelt canceled with two 'l's in MediaCache.cpp (the British way), but
> with one 'l' (the American way) here. ;)

See, that's what happens when you grow up in N. Ireland and then move to the US - hybrid vocabulary and spelling. One assumes NZ follows the British mode of spelling? I added an extra 'l' :p

> ::: content/media/dash/DASHDecoder.cpp
> @@ +999,5 @@
> > +  return MediaDecoder::Seek(aTime);
> > +}
> > +
> > +
> 
> Extra line break here.

That's also the British way of doing things, don't you know ;) Lines removed.

> @@ +1002,5 @@
> > +DASHDecoder::NotifySeekInVideoSubsegment(int64_t aRepDecoderIdx,
> > +                                           int64_t aSubsegmentIdx)
> 
> Can you fix the indenting here? I accidentally left the indentation broken
> when I removed the ns prefix, no point propagating it further.

Done.

> ::: content/media/dash/DASHDecoder.h
> @@ +63,5 @@
> > +  void NotifySeekInVideoSubsegment(int64_t aRepDecoderIdx,
> 
> Your subsegment and decoder indexes are 32bit values elsewhere, any reason
> they're not 32bit as well here?

I just hadn't updated these patches after changing the previous one from 64 to 32.

> God help us if we have an array which requires more than 32bits to index
> into it; we're stuck with being a 32bit process on Windows for the
> foreseeable future for example.

Indeed.

> @@ +79,5 @@
> > +  nsresult Seek(double aTime);
> 
> nsresult Seek(double aTime) MOZ_OVERRIDE;

Done.

> @@ +111,5 @@
> >    void SetSubsegmentIndex(DASHRepDecoder* aRepDecoder,
> > +                          uint32_t aSubsegmentIdx);
> 
> This index unsigned but it's signed elsewhere?

Yup - fixed. All signed, and I added an NS_ASSERTION here for '>= 0'.

> @@ +157,5 @@
> > +  int32_t GetSwitchCountAtVideoSubsegment(int64_t aSubsegmentIdx)
> 
> This index is 64bit but you're using 32bits elsewhere...

Fixed.

> ::: content/media/dash/DASHReader.h
> @@ +28,5 @@
> > +  nsresult ResetDecode();
> 
> MOZ_OVERRIDE; you may as well add that to any other methods in this class
> that override a method in the parent class.

Done.

> ::: content/media/dash/DASHRepDecoder.h
> > +  bool IsSubsegmentCached(int64_t aSubsegmentIdx);
> 
> int32_t aSubsegmentIdx?

Yup.

> ::: content/media/dash/DASHRepReader.h
> @@ +40,5 @@
> > +  // Returns the index of the subsegment which contains the seek time.
> 
> State units, microseconds I would presume.

Done, usecs.

> ::: content/media/webm/WebMReader.cpp
> @@ +956,5 @@
> > +  for (uint32_t i = 0; i < mClusterByteRanges.Length(); i++) {
> 
> Why can't you still use the copy constructor here?

Because the copy would be from a nsTArray<TimestampedMediaByteRange> to a regular nsTArray<MediaByteRange>. I've changed the code a little though - I added a copy constructor for single objects and kept the for loop. So, it's element by element copy in the for loop; field by field is kept for the copy constructor.

Thanks for the review!

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

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

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 686807 [details] [diff] [review]
v1.0 Return timestamp in nestegg_get_cue_point to support DASH-WebM Seeking

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b09b636bbf

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 691075 [details] [diff] [review]
v1.2 Add seeking capabilities to DASH-WebM code

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b6583b20a80

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e7b09b636bbf
https://hg.mozilla.org/mozilla-central/rev/5b6583b20a80