792935 - DASH: Add DASH-WebM cases to mochitests

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Steve Workman [:sworkman] (INACTIVE)]

DASH-WebM should be added to mochitests for regular unit testing.

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

About to upload multiple patches:

I've split up the fixes into different patches to help with reviews here, but also to add more information to the revision history. There is one patch of misc. fixes, because they are less focused on a specific test.

Also, there are a couple of tests that are still not 100%:
-- tests involving GetDuration being compared to GetBuffered are failing: the duration returned by GetDuration is updated during playback to match the actual decode rate, which seems to slip slightly with the DASH/WebM files I added.
-- test_seek_out_of_range seems to be getting errors from another test to do with width and height. I need to determine what's not being cleaned up for this one.

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Add tests for canPlayType for DASH (Media Mochitests)

AddCanPlayTypeDASH
-- Adds files to test canPlayType("application/dash+xml").

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Add DASH to gSmallTests and gSeekTests to manifest.js (Media Mochitests)

AddDASHToManifestJS
-- Adds sample DASH/WebM files to gSmallTests and gSeekTests in manifest.js.
-- Note: The subsequent patches are needed to make these tests pass.
-- Also adjusts getPlayableVideo to allow "application/dash+xml" types, assuming they're enabled in the build/in prefs.

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Add DASH to various Media Mochitests

AddDASHToMiscTests
-- Some tests specifically filter on "video/x"; this patch adds "application/dash+xml" for video tests.

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Miscellaneous fixes for DASH Mochitests

MiscFixesForTests
-- A slew of small bug fixes to improve stability etc.
-- Includes:
    -- Moving check for mDecoderStateMachine inside the monitor for MediaDecoder::NotifyBytesConsumed.
    -- Addition of some LOGs.
    -- Fix seek-ability detection in ChannelMediaResource::OnStartRequest.
    -- Allow CacheClientSeek to restart a previously canceled byte range download (assuming the desired offset is within that range).
    -- Add a copy constructor for MediaChannelStatistics - previously, member vars of this type were changed from objects to pointers … this supports copying from one object to another in ChannelMediaResource::Clone.
    -- Some other minor fixes.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Add decoder cloning support to DASH for mozLoadFrom

FixCloning
-- Add basic support for cloning DASHDecoder, to support mozLoadFrom etc.
-- Note: This doesn't include cloning of the sub-decoders, but if I understand correctly, MediaCache can still share the actual cached data.

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Do not close MPD resource before shutdown

FixCloseMPDResource
-- Previously I was closing the MPD resource in DASHDecoder::OnReaderMPDBufferCompleted. This was causing problems as it is still expected to be open/not closed until Shutdown.

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Restore correct playState after state machine initialization

FixPlayState
-- mPlayState was being reset to default in DASHDecoder::InitializeStateMachine called from ::LoadRepresentations. This affected autoplay, for example. New code remembers the state while the state machine initializes.

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Forward suspend and resume to sub-decoders

FixSuspendAndResume
-- Forwards suspend and resume from DASHDecoder to the sub-decoders.

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Aggregate results from GetBuffered from sub-decoders

FixGetBuffered
-- Aggregates GetBuffered results from sub-decoders in DASHDecoder::GetBuffered.
-- Note: This affects tests of GetBuffered. The patch works in two steps: 1) Aggregate audio and video sub-decoders separately into two buffered ranges: audio ranges and video ranges. 2) Find the intersection of the audio and video ranges to produce a list of ranges which have both audio and video, not just one of them. The problem is that if the audio file is a little bit longer, then the overall presentation is longer, but the buffered ranges will end when the video file ends. This is another reason why test_get_buffered fails. (The other reason is to do with calculated duration based on decoding time being longer than the encoded time).

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Aggregate results from IsResourceLoaded from DASH sub-decoders

FixResourceIsLoaded
-- Aggregate responses from sub-decoders.
-- Do not download a new byte range if the last one was downloaded, in DASHDecoder::NotifyDownloadEnded.
-- Also forwarded Start|StopProgressUpdates.

Comment 12

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Remove code to cancel DASH subsegment downloads during a seek

RemoveCancelByteRangeOpen
-- Previously I had added code to notify the MediaCache when a seek was about to happen. This was to waken any readers who were waiting on new data at the previous offset, and tell them to return, rather than blocking indefinitely. However, after implementing DASHDecoder::Suspend/Resume, I see that this is no longer needed, and adds redundant complexity.

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 series file for hg queue

The order of applying the patches shouldn't matter too much, but here is the series file I've been using anyway.

Comment 14

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 Add DASH to various Media Mochitests

Updated to fix test_seek_out_of_range, and correct element type for test_bug465498 and test_timeupdate_small_files.

In test_seek_out_of_range, checkMetadata was requesting videoWidth|Height and comparing them to the values in manifest.js. However, it had created an audio element due to incorrect mimetype detection, which meant that videoWidth|Height reported 'undefined'. Tested locally, passes.

Comment 15

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 Aggregate results from GetBuffered from sub-decoders

Changed patch so that GetBuffered returns the union of buffered ranges from sub-decoders, not the intersection.

This is in response to one of the problems identified in comment 10: "... if the audio file is a little bit longer, then the overall presentation is longer, but the buffered ranges will end when the video file ends. ... test_get_buffered fails."

In an email from Chris: "You can assume that the last video frame will always be displayed while the audio samples after the end of the video track are being played, and this effectively extends the length of the last frame of the video track. So you can report the union of video and audio ranges. We do this when playing Ogg files for example, as having one stream longer than the other(s) is quite common in Ogg files."

Hence, union of ranges rather than intersection.

Comment 16

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

Comment on attachment 701252 [details] [diff] [review]
v1.0 Add tests for canPlayType for DASH (Media Mochitests)

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

::: content/media/test/can_play_type_dash.js
@@ +16,5 @@
> +  check("application/dash+xml; codecs=\"vp8, vorbis\"", "probably");
> +  check("application/dash+xml; codecs=\"vp8.0, vorbis\"", "probably");
> +  check("application/dash+xml; codecs=vp8", "probably");
> +  check("application/dash+xml; codecs=vp8.0", "probably");
> +

How about checking:
codecs="webm"
codecs="dash"
codecs="someBogusString,anotherBogusString"

presumably these should all return "".

::: content/media/test/test_can_play_type_no_dash.html
@@ +4,5 @@
> +https://bugzilla.mozilla.org/show_bug.cgi?id=734546
> +-->
> +<head>
> +  <title>Test for Bug 734546: Add DASH Support (WebM): Initial Code Drop</title>
> +  <script type="application/javascript" src="/MochiKit/MochiKit.js"></script>

You include MochiKit.js here but not in test_can_play_type_dash, do you need it in both?

Comment 17

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

Do you have a test that somehow ensures that stream switching occurs? We'll need that.

Comment 18

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

Comment on attachment 701261 [details] [diff] [review]
v1.0 Miscellaneous fixes for DASH Mochitests

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

::: content/media/MediaResource.h
@@ +51,5 @@
> +  {
> +    MOZ_ASSERT(aCopyFrom);
> +    *this = *aCopyFrom;
> +  }
> +  

Nit: trailing whitespace.

Comment 19

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

Comment on attachment 701267 [details] [diff] [review]
v1.0 Forward suspend and resume to sub-decoders

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

::: content/media/dash/DASHDecoder.cpp
@@ +783,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  // Suspend MPD download if not yet complete. 
> +  if (!mMPDManager && mResource) {
> +    LOG1("Suspending MPD download.");  

Nit: trailing whitespace in these two new functions.

Comment 20

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

Comment on attachment 701269 [details] [diff] [review]
v1.0 Aggregate results from IsResourceLoaded from DASH sub-decoders

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

::: content/media/MediaDecoder.cpp
@@ +658,5 @@
> +{
> +  NS_ASSERTION(!mShuttingDown, "Don't call during shutdown!");
> +  GetReentrantMonitor().AssertCurrentThreadIn();
> +
> +  return (!mResourceLoaded && mResource &&

It doesn't make sense that a function named IsResourceLoaded() would return false if the field mResourceLoaded is true...

Something needs to be renamed to make the distinction between these to bits of state clear.

Comment 21

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

Comment on attachment 701271 [details] [diff] [review]
v1.0 Remove code to cancel DASH subsegment downloads during a seek

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

::: content/media/dash/DASHDecoder.cpp
@@ +1040,5 @@
>    LOG("Seeking to [%.2fs]", aTime);
>  
>    {
>      ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> +    // Set the seeking flag, so that when current subsegments download (if

Should this read: "subsegment downloads"?

Comment 22

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

Comment on attachment 701362 [details] [diff] [review]
v1.1 Add DASH to various Media Mochitests

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

::: content/media/test/test_bug465498.html
@@ +42,5 @@
>    manager.finished(v.token);
>  }
>  
>  function initTest(test, token) {
> +  var type = (/^video/.test(test.type) ||

Can you wrap this test up into a new function GetMajorMimeType() and put that in manifest.js, and use that instead everywhere we do this test? Thanks.

Comment 23

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

Comment on attachment 701899 [details] [diff] [review]
v1.1 Aggregate results from GetBuffered from sub-decoders

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

::: content/media/dash/DASHReader.cpp
@@ +320,3 @@
>    ReentrantMonitorConditionallyEnter mon(!mDecoder->OnDecodeThread(),
>                                           mDecoder->GetReentrantMonitor());
> +  for (uint32_t i = 0; i < mAudioReaders.Length(); i++) {

Why do you include the non-active streams in the buffered calculation? It seems to misrepresent the what ranges are buffered to me. If you happen to have loaded the entire audio stream (which isn't unreasonable I think), but you've only loaded say 10% of the video stream, with this approach you'll report the entire media as buffered, which is not true because if you seek to some position after 10% the video won't be buffered and there'll be a stall while the video is fetched.

I don't think I was clear enough with what I said the other day. You should only extend the end time of the *last* range in active stream A to the end time of the other active stream B if stream A is cached up to the end of its resource (i.e. the data for all remaining samples for stream A have been downloaded).

i.e. you should return the intersection of the buffered ranges only when neither of the active streams are cached up until the end of stream. If either of the active readers are cached up until the end of stream, then you return the union of the two readers' buffered ranges.

This is because if there's more audio than video we'll show the last video frame until the extra audio finishes playing, or if there's more video than audio we'll play silence and show the remaining video frames using the system clock to enforce the frame rate. i.e. we'll keep playing and the media's current time will keep advancing until all streams samples have finished playing.

Hopefully that rationale makes sense.

Comment 24

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.2 Aggregate results from GetBuffered from sub-decoders

-- Reverted to finding the intersection of audio and video ranges.
-- If both have downloaded the last subsegment, then the shorter of the two is extended, so that their end times are the same.
-- Note: The code includes the buffered ranges of all decoders, active and inactive, because subsegments could be played back from any decoder. For example, think of a seek back; subsegments have already been downloaded, but not just for the active decoder. The subsegment load history is stored so that as we play forward, we can playback cached data from any decoder. So, we need to consider playable buffered ranges from all decoders.

Comment 25

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 Aggregate results from IsResourceLoaded from DASH sub-decoders

Renamed bool mResourceLoaded to mCalledResourceLoaded, and local bool resourceIsLoaded to notifyResourceIsLoaded.

Comment 26

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

Comment on attachment 703692 [details] [diff] [review]
v1.2 Aggregate results from GetBuffered from sub-decoders

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

::: content/media/dash/DASHReader.cpp
@@ +356,5 @@
> +  audioBuffered.Normalize();
> +  videoBuffered.Normalize();
> +
> +#ifdef PR_LOGGING
> +  rv = audioBuffered.GetLength(&audioRangeCount);

You need to init audioRangeCount and videoRangeCount outside of the PR_LOGGING block, otherwise in release or non-logging builds you'll be using them uninitialized in the "Calculate intersecting ranges for video and audio" block below.

@@ +386,5 @@
> +  if (audioCachedToEnd && videoCachedToEnd) {
> +    double audioEndTime = 0, videoEndTime = 0;
> +    uint32_t audioBufferedLength = 0, videoBufferedLength = 0;
> +    // Get end time of the last range of buffered audio.
> +    rv = audioBuffered.GetLength(&audioBufferedLength);

You don't need to have audioRangeCount and audioBufferedLength, they're the same value right? Ditto for videoRangeCount/BufferedLength.

@@ +392,5 @@
> +    NS_ENSURE_TRUE(audioBufferedLength, NS_ERROR_FAILURE);
> +    rv = audioBuffered.End(audioBufferedLength-1, &audioEndTime);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    // Get end time of the last range of buffered video.
> +    rv = videoBuffered.GetLength(&videoBufferedLength);

Factoring out the logic to get the end time of the last range in an nsTimeRanges to its own function would improve the readibility here.

::: content/media/webm/WebMReader.cpp
@@ +929,5 @@
> +                                         nsTArray<MediaByteRange>& aRanges)
> +{
> +  NS_ENSURE_TRUE(mContext, NS_ERROR_NOT_INITIALIZED);
> +  NS_ENSURE_FALSE(aRanges.IsEmpty(), NS_ERROR_ILLEGAL_VALUE);
> +  

Nit: Trailing whitespace.

@@ +997,5 @@
> +  uint64_t finalSubStart = mClusterByteRanges[finalSubsegmentIndex].mStart;
> +  uint64_t finalSubEnd = mClusterByteRanges[finalSubsegmentIndex].mEnd;
> +  uint32_t finalRangeIndex = ranges.Length()-1;
> +  uint64_t finalRangeStart = ranges[finalRangeIndex].mStart;
> +  uint64_t finalRangeEnd = ranges[finalRangeIndex].mEnd;

Can't you check whether (resource->Length() == ranges[finalRangeIndex].mEnd)? (or maybe that should be mEnd+1 ?) And if that's the case, then we don't need to use mClusterByteRanges, and you don't need to add another virtual function to MediaDecoderReader.

I'd much rather implement this using just MediaResource than adding another virtual function to MediaDecoderReader which most of the other readers won't implement.

Comment 27

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

Comment on attachment 704130 [details] [diff] [review]
v1.1 Aggregate results from IsResourceLoaded from DASH sub-decoders

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

Is this the right patch, the string mCalledResourceLoaded doesn't appear in this patch.

Comment 28

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.3 Aggregate results from GetBuffered from sub-decoders

(In reply to Chris Pearce (:cpearce) from comment #26)
> Comment on attachment 703692 [details] [diff] [review]
> v1.2 Aggregate results from GetBuffered from sub-decoders
> 
> Review of attachment 703692 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/dash/DASHReader.cpp
> @@ +356,5 @@
> > +  rv = audioBuffered.GetLength(&audioRangeCount);
> 
> You need to init audioRangeCount and videoRangeCount outside of the
> PR_LOGGING block, otherwise in release or non-logging builds you'll be using
> them uninitialized in the "Calculate intersecting ranges for video and
> audio" block below.

Done. I also moved the GetLength call to outside the PR_LOGGING block. I kept the init to 0 as a guard against weird happenings in GetLength.

> @@ +386,5 @@
> > +    uint32_t audioBufferedLength = 0, videoBufferedLength = 0;
> > +    // Get end time of the last range of buffered audio.
> > +    rv = audioBuffered.GetLength(&audioBufferedLength);
> 
> You don't need to have audioRangeCount and audioBufferedLength, they're the
> same value right? Ditto for videoRangeCount/BufferedLength.

Yup - thanks for catching that one.

> @@ +392,5 @@
> > +    NS_ENSURE_TRUE(audioBufferedLength, NS_ERROR_FAILURE);
> > +    rv = audioBuffered.End(audioBufferedLength-1, &audioEndTime);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    // Get end time of the last range of buffered video.
> > +    rv = videoBuffered.GetLength(&videoBufferedLength);
> 
> Factoring out the logic to get the end time of the last range in an
> nsTimeRanges to its own function would improve the readibility here.

Done.

> ::: content/media/webm/WebMReader.cpp
> @@ +929,5 @@
> > +                                         nsTArray<MediaByteRange>& aRanges)
> > +{
> > +  NS_ENSURE_TRUE(mContext, NS_ERROR_NOT_INITIALIZED);
> > +  NS_ENSURE_FALSE(aRanges.IsEmpty(), NS_ERROR_ILLEGAL_VALUE);
> > +  
> 
> Nit: Trailing whitespace.

Removed.

> @@ +997,5 @@
> > +  uint64_t finalSubStart = mClusterByteRanges[finalSubsegmentIndex].mStart;
> > +  uint64_t finalSubEnd = mClusterByteRanges[finalSubsegmentIndex].mEnd;
> > +  uint32_t finalRangeIndex = ranges.Length()-1;
> > +  uint64_t finalRangeStart = ranges[finalRangeIndex].mStart;
> > +  uint64_t finalRangeEnd = ranges[finalRangeIndex].mEnd;
> 
> Can't you check whether (resource->Length() ==
> ranges[finalRangeIndex].mEnd)? (or maybe that should be mEnd+1 ?) And if
> that's the case, then we don't need to use mClusterByteRanges, and you don't
> need to add another virtual function to MediaDecoderReader.
> 
> I'd much rather implement this using just MediaResource than adding another
> virtual function to MediaDecoderReader which most of the other readers won't
> implement.

I changed these functions a bit after spending time thinking about your comments. First, IsFinalSubsegmentCached was declared in DASHRepReader, so it would only exist in WebMReader, not the other readers. Note: that function is renamed now to IsDataCachedAtEndOfSubsegments.

Second, I changed the concept here from getting the ranges of only whole subsegments that are cached, back to how it's currently handled - whatever bytes are cached converted to time ranges. The reason I did it the previous way was to work around WebM timestamps - only the starting timestamp is encoded with a frame. Currently, WebMReader::NextPacket gets the end timestamp by getting the start timestamp of the next frame. But this is not possible in DASH for a frame at the end of a cluster which is the last one before a stream switch - in NextPacket I got around that by querying the next WebMReader, i.e. the "switch to" reader. That's too complex for GetBuffered, so my first solution, in the previous version of this patch, was only to consider whole subsegments and use the timestamps stored in mClusterByteRanges. This worked fine, but there's obviously a loss in granularity.

Your comments inspired me to review this and I've come up with something better, a little more elegant code wise. GetBufferedClusters is removed, and in it's place there is a MOZ_DASH code-block in GetBuffered which checks if a range is at the end of a cluster. If it is, then the end time of that range may be updated to reflect the cluster's end time. Cluster timestamps are stored in mClusterByteRanges during ReadMetadata.

You also asked if I couldn't just use resource->Length(). The problem with that is if the cues are stored at the end of the file, then resource->Length() will not reflect the end of the clusters. I know that cues at the end of a file is a tolerated thing, not desirable, but we should also handle such cases here.

Let me know if this isn't clear.

Comment 29

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 Aggregate results from IsResourceLoaded from DASH sub-decoders

Sorry about that.

Comment 30

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 Add tests for canPlayType for DASH (Media Mochitests)

(In reply to Chris Pearce (:cpearce) from comment #16)
> Comment on attachment 701252 [details] [diff] [review]
> v1.0 Add tests for canPlayType for DASH (Media Mochitests)
> 
> Review of attachment 701252 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/test/can_play_type_dash.js
> @@ +16,5 @@
> > +  check("application/dash+xml; codecs=\"vp8, vorbis\"", "probably");
> 
> How about checking:
> codecs="webm"
> codecs="dash"
> codecs="someBogusString,anotherBogusString"
> 
> presumably these should all return "".

There are already checks for "xyz" etc. in can_play_type_dash.js, so I think this is covered.

> ::: content/media/test/test_can_play_type_no_dash.html
> @@ +4,5 @@
> > +  <script type="application/javascript" src="/MochiKit/MochiKit.js"></script>
> 
> You include MochiKit.js here but not in test_can_play_type_dash, do you need
> it in both?

Actually, it's not needed in either file. I was copying from test_can_play_dash|no_webm.html, and I checked _ogg and _wav as well - they all have it, but I don't think it's needed. I removed it for DASH.

Comment 31

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.2 Add tests for canPlayType for DASH (Media Mochitests)

Changing the DASH audio file to one which seems to be encoded better. 
(Refers to conversations with Chris about discrepancy between .buffered and .getDuration - previous file did not have duration encoded accurately; new one works with test_get_buffered.html).
Please read comment 30 for other minor changes since review.

Comment 32

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Add DASH to test_info_leak, test_progress and test_standalone

Add DASH to test_info_leak, test_progress and test_standalone, along with a minor fix to get DASH into nsContentDLF and nsContentUtils.

Comment 33

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 705185 [details] [diff] [review]
v1.2 Add tests for canPlayType for DASH (Media Mochitests)

https://hg.mozilla.org/integration/mozilla-inbound/rev/64fe42ecc281

Comment 34

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 701254 [details] [diff] [review]
v1.0 Add DASH to gSmallTests and gSeekTests to manifest.js (Media Mochitests)

https://hg.mozilla.org/integration/mozilla-inbound/rev/766ad7085964

Comment 35

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 701362 [details] [diff] [review]
v1.1 Add DASH to various Media Mochitests

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a1e65795577

Comment 36

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 701261 [details] [diff] [review]
v1.0 Miscellaneous fixes for DASH Mochitests

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

Comment 37

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 701262 [details] [diff] [review]
v1.0 Add decoder cloning support to DASH for mozLoadFrom

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d0906299c15

Comment 38

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 701264 [details] [diff] [review]
v1.0 Do not close MPD resource before shutdown

https://hg.mozilla.org/integration/mozilla-inbound/rev/19a7d34aa7cd

Comment 39

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 701265 [details] [diff] [review]
v1.0 Restore correct playState after state machine initialization

https://hg.mozilla.org/integration/mozilla-inbound/rev/994d70120d98

Comment 40

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 701267 [details] [diff] [review]
v1.0 Forward suspend and resume to sub-decoders

https://hg.mozilla.org/integration/mozilla-inbound/rev/97d3de3539ca

Comment 41

[Steve Workman [:sworkman] (INACTIVE)]

First 8 patches landed on inbound. Note that DASH is still pref'd off in these patches - we can't turn it on until all the patches land and the tests show green.
Try job (https://tbpl.mozilla.org/?tree=Try&rev=95dbf7bd3498) for mochitest-1 passed fine.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64fe42ecc281
https://hg.mozilla.org/mozilla-central/rev/766ad7085964
https://hg.mozilla.org/mozilla-central/rev/3a1e65795577
https://hg.mozilla.org/mozilla-central/rev/b4c77bcee9d7
https://hg.mozilla.org/mozilla-central/rev/1d0906299c15
https://hg.mozilla.org/mozilla-central/rev/19a7d34aa7cd
https://hg.mozilla.org/mozilla-central/rev/994d70120d98
https://hg.mozilla.org/mozilla-central/rev/97d3de3539ca

Comment 43

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

Comment on attachment 705135 [details] [diff] [review]
v1.3 Aggregate results from GetBuffered from sub-decoders

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

::: content/media/dash/DASHReader.cpp
@@ +322,4 @@
>  
>    nsresult rv = NS_OK;
>  
> +  // Get all audio and video buffered ranges. Include inactive streams, since

I think intersecting the union of audio ranges with the union on the video like you're doing here is the correct approach.

@@ +388,5 @@
> +
> +  // If both audio and video are cached to the end of their subsegments, extend
> +  // the end time of the lesser of the two. Presentation of the shorter stream
> +  // will stop while the other continues until it finishes.
> +  if (audioCachedAtEnd && videoCachedAtEnd) {

I think that should be (hasAudio && hasVideo && (audioCachedAtEnd || videoCachedAtEnd)). As written now if the audio finishes at 20s and the video extends to 40s and audio is fully cached but video is cached to 30s, you'll only report that you're buffered to 20s. Right?

::: content/media/webm/WebMReader.cpp
@@ +993,5 @@
>          double startTime = start * timecodeScale / NS_PER_S - aStartTime;
>          double endTime = end * timecodeScale / NS_PER_S - aStartTime;
> +#ifdef MOZ_DASH
> +        // If this range extends to the end of a cluster, the true end time is
> +        // the cluster's end timestamp.

Please explain in this comment why that is the case.

Comment 44

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

Comment on attachment 705139 [details] [diff] [review]
v1.1 Aggregate results from IsResourceLoaded from DASH sub-decoders

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

::: content/media/MediaDecoder.cpp
@@ +654,5 @@
>    mDecoderStateMachine->QueueMetadata(aPublishTime, aChannels, aRate, aHasAudio, aTags);
>  }
>  
> +bool
> +MediaDecoder::IsResourceLoaded()

Call this IsDataCachedToEndOfResource()

Comment 45

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Add DASH to gPlayedTests and gPlayTests

Add DASH to gPlayedTests and gPlayTests:
Affects test_load_source.html, test_playback.html, test_playback_rate.html, test_played.html.
Note: test_played.html is currently filtered out of the testsuite, but I was able to get 10 consecutive runs passing locally. This included DASH in gPlayedTests.

Comment 46

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.4 Aggregate results from GetBuffered from sub-decoders

Thanks for the r+ Chris - just want to confirm the logic for audio|videoCachedToEnd etc. before I land the patch.

(In reply to Chris Pearce (:cpearce) from comment #43)
> Comment on attachment 705135 [details] [diff] [review]
> v1.3 Aggregate results from GetBuffered from sub-decoders
> 
> Review of attachment 705135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/dash/DASHReader.cpp
> @@ +322,4 @@
d
> > +  // Get all audio and video buffered ranges. Include inactive streams, since
> 
> I think intersecting the union of audio ranges with the union on the video
> like you're doing here is the correct approach.

Good to have consensus - thanks.

> @@ +388,5 @@
> > +
> > +  // If both audio and video are cached to the end of their subsegments, extend
> > +  // the end time of the lesser of the two. Presentation of the shorter stream
> > +  // will stop while the other continues until it finishes.
> > +  if (audioCachedAtEnd && videoCachedAtEnd) {
> 
> I think that should be (hasAudio && hasVideo && (audioCachedAtEnd ||
> videoCachedAtEnd)). As written now if the audio finishes at 20s and the
> video extends to 40s and audio is fully cached but video is cached to 30s,
> you'll only report that you're buffered to 20s. Right?

Right - it would report that until the video data was fully cached, and then it would extend the audio.

Using your suggestion, however, what happens if audio is fully cached to 40s, but video is partially cached at 20s, but has 30s total. In this case, video would be extended to 40s, since audio is fully cached. But 10s of video data is not cached.

So, I've coded it like this:
if (audioCachedToEnd || videoCachedToEnd) {
  ...
  if (audioCachedToEnd && videoEndTime > audioEndTime) {
    // extend audio end time, so intersecting time will reflect video
  else if (videoCachedToEnd && audioEndTime > videoEndTime) {
    // extend video end time ...
  }
}

There's a long comment and some ascii art to try to explain this in the patch. Let me know if I'm way off here.

> ::: content/media/webm/WebMReader.cpp
> @@ +993,5 @@
> >          double startTime = start * timecodeScale / NS_PER_S - aStartTime;
> >          double endTime = end * timecodeScale / NS_PER_S - aStartTime;
> > +#ifdef MOZ_DASH
> > +        // If this range extends to the end of a cluster, the true end time is
> > +        // the cluster's end timestamp.
> 
> Please explain in this comment why that is the case.

Done.

Comment 47

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

Comment on attachment 706181 [details] [diff] [review]
v1.4 Aggregate results from GetBuffered from sub-decoders

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

OK, I think we're good.

Comment 48

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 706181 [details] [diff] [review]
v1.4 Aggregate results from GetBuffered from sub-decoders

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

Comment 49

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 705139 [details] [diff] [review]
v1.1 Aggregate results from IsResourceLoaded from DASH sub-decoders

https://hg.mozilla.org/integration/mozilla-inbound/rev/20f89267f8c2

Comment 50

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 701271 [details] [diff] [review]
v1.0 Remove code to cancel DASH subsegment downloads during a seek

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

Comment 51

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 705189 [details] [diff] [review]
v1.0 Add DASH to test_info_leak, test_progress and test_standalone

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

Comment 52

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 706139 [details] [diff] [review]
v1.0 Add DASH to gPlayedTests and gPlayTests

https://hg.mozilla.org/integration/mozilla-inbound/rev/33ad7882738a

Comment 53

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Chris Pearce (:cpearce) from comment #47)
> Comment on attachment 706181 [details] [diff] [review]
> v1.4 Aggregate results from GetBuffered from sub-decoders
> 
> Review of attachment 706181 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, I think we're good.

Thanks, Chris! Stream switching test almost done, and it should be the last one before pref'ing on, barring any other bugs that pop up.

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a7ff4a52a009
https://hg.mozilla.org/mozilla-central/rev/20f89267f8c2
https://hg.mozilla.org/mozilla-central/rev/c3e9f94548f2
https://hg.mozilla.org/mozilla-central/rev/c9341e21fb33
https://hg.mozilla.org/mozilla-central/rev/33ad7882738a

Comment 55

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Test stream switching by verifying byte range requests

Adds three new files:
-- dash_detect_stream_switch.sjs
Server-side script to verify subsegment requests. Returns 404 if unexpected byte ranges are requested, i.e. if a wrong (media file, subsegment) pair is requested for download. In this way we can detect if stream switching occurred.

-- test_dash_detect_stream_switch.html
Simple front end to download and play DASH media. Fails on error events, which when combined with the .sjs file, suggests stream switching is broken.

-- dash-manifest-sjs.mpd
Has BaseURL elements which use the .sjs file.

Comment 56

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

Comment on attachment 707836 [details] [diff] [review]
v1.0 Test stream switching by verifying byte range requests

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

Looks good. Thanks!

Comment 57

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 707836 [details] [diff] [review]
v1.0 Test stream switching by verifying byte range requests

Thanks for the r+ Chris!

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

Comment 58

[Steve Workman [:sworkman] (INACTIVE)]

Now that the core tests are complete and landed on m-i, it's time to prepare for setting 'media.dash.enabled' to true by default. So, I'm removing [leave open] from this one, and it can be closed when the patches are merged to m-c. Once that happens and I get multiple green try runs (fingers crossed), I'll open a new bug to flip the switch.

Comment 59

[Steve Workman [:sworkman] (INACTIVE)]

Oops, not quite:

Backed out due to orange on mochitest-1.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ce8f535ff87

I know what it is: the test fails because DASH isn't pref'd on and no media is playable :) I'll fix it and upload a new patch here to check it. And I'll verify it on try too, pref'd on and off.

Comment 60

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 Test stream switching by verifying byte range requests

Patch update to fix orange on mochitest-1 when DASH pref'd off.

Test now reports a todo if DASH is disabled and the test array is empty. However, if DASH is ENABLED and the array is still empty, a failure is reported - I want to ensure that the test array has items in it if DASH is pref'd on.

Passed locally.

Pushed to try with DASH pref'd off:
https://tbpl.mozilla.org/?tree=Try&rev=d915eeeeb713

... and pref'd on:
https://tbpl.mozilla.org/?tree=Try&rev=939037744e3d

Comment 61

[Steve Workman [:sworkman] (INACTIVE)]

Thanks, Chris.

Try runs from comment 60 all green.

Landed on m-i again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0b435ca93f9

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c0b435ca93f9