734546 - Add DASH Support (WebM): Initial Code Drop

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Steve Workman [:sworkman] (INACTIVE)]

Add code for basic support of DASH (WebM).  Code should be able to parse an MPD file sourced from a <video> tag and start playback of video. This bug to track non-adaptive playback only.

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: Summary of getting from MPD URL to MPD Classes populated

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: nsHTMLMediaElement WIP v1.0

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: DASH Managers WIP v1.0

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: nsDASHMPDStream WIP v1.0

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: nsDASHMPDParser WIP v1.0

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: MPD Classes WIP v1.0

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: Very basic test MPD and HTML file

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: nsDASHMPDParser WIP v1.0

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

**First Work in Progress patches for feedback, not full review**
(Note: code is not clean yet, so some of it is messy and incomplete.)
These patches allow an MPD file to be read to a DOM, and then MPD classes populated.  MPD file is downloaded via the 'src' attribute of a '<video>' tag in HTML.

Summary of Patch Files:
bug702122-wip20110309-nsHTMLMediaElement.diff
-- Added hooks to nsDASHManager

bug702122-wip20110309-nsDASHManager.diff
-- Main DASH class.  Manages streams, DOM parsing and MPD parsing.

bug702122-wip20110309-nsDASHMPDStream.diff
bug702122-wip20110309-nsDASHMPDParser.diff
-- The stream class is based off of nsMediaChannelStream, which is currently used for non-adaptive media coming in over the network.  Current version of nsDASHMPDStream is for MPD files only (hence the name) and is quite messy.  It does enough to get the file from the network and read data from it.  Parser reads the data into a DOM.

bug702122-wip20110309-MPDClasses.diff
-- Classes matching the structure of the MPD.  Populated from the DOM structure.

bug702122-wip20110309-Makefiles.diff
bug702122-wip20110309-TestHTML&MPDFiles.diff

-- Logs are provided for debug output to show MPD being parsed to DOM, and DOM to MPD classes
-- stdlib/io types have been converted to mozilla types, which makes the code look very different
-- Since big changes were already being made, I also started changes to more closely match Mozilla coding guidelines (indentation, variable naming convention, namespaces etc.)

I also uploaded a summary of the flow.

Christian, Christopher, anywhere you see something like nsCOMPtr, nsAutoPtr, nsRefPtr or do_QueryInterface(), that is Mozilla code relating to reference counting and XPCOM IDLs. Also, we have several different types of string classes.  And I'm sure there are other nuances that I've forgotten about at the moment.  Please feel free to ask questions - I'd be happy to explain more if you want.  In the meantime, I suggest you skip over the details of these pointers, strings etc. and get a high level grasp of the changes.  If you need to understand more, see:

https://developer.mozilla.org/En/Mozilla_internal_string_guide
https://developer.mozilla.org/en/Using_nsCOMPtr

That should get you started. Again, I'll be happy to explain more if need be.


TODO Items:
-- Top-level: read in URLs from a single Representation/AdaptationSet and send data to WebMDecoder (non-adaptive)
    ** Need help from Media/Content folks here
-- Add Adaptation code
In the meantime ...
-- Clean up nsDASHMPDStream files - they are very dirty at present, with #ifdef DASH_IGNORE etc.  Just delete whatever code is not to be used at present; it can always be added from nsMediaChannelStream later
-- Document events and behavior per the code.  A list is needed so that we can compare what is written for DASH to compare it to what is there for non-adaptive media, to what is specified in the spec.  This record will be needed for review, and also to prioritize future development.
-- Convert libdash interface classes to XPCOM IDLs
-- Review object creation/deletion in the MPD classes - use more ref ptrs if possible and useful

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How are you going to get data from nsDASHMPDStream to the media decoders? Are you going to use nsMediaCache? Will nsDASHMPDStream subclass MediaResource? (which is what nsMediaStream got renamed to)

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> How are you going to get data from nsDASHMPDStream to the media decoders?
> Are you going to use nsMediaCache? Will nsDASHMPDStream subclass
> MediaResource? (which is what nsMediaStream got renamed to)

Using nsMediaCache and MediaResource is my intention, yes. There is a lot of work to be done there and I haven't figured out all the details yet. I hope to get started on it sometime in the next couple of weeks.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds good!

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.0 Complete patch - Demos non-adaptive, WebM On Demand Video

Patches for non-adaptive, DASH/WebM On Demand Demo:
-- Please patch and build the complete diff or run try builds (https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b02d11614fe1/) and point your browser to http://people.mozilla.com/~sworkman/Elephants_Dream.html
Notes: 
1)  I've tested this on Mac, Android and Windows - let me know if your system doesn't work.
2)  please add/set media.dash.enabled=true in your prefs.


What's working:
-- Current patches parse MPD according to WebM On Demand profile, pick a video and audio stream and mix them to a single decoder to play them non-adaptively
-- Seeking
-- Refreshing the page without issue

What's not working/not right:
-- no adaption/stream switching yet; initial work done on this, but not reflected in this code
-- Seeking far enough from the current offset adds a buffering icon on the video, which does not go away
-- some URLs are leaking in the MPD files
-- stats are not correct - some data only pertains to the video stream, not both - to be examined/fixed.

Patches:
1)  Decoder files 
*** Description of interaction in nsDASHDecoder.cpp at top of file. ***
-- Muxes multiple resources through multiple readers to a single decoder and state machine. Proxy classes distribute/aggregate calls as need be.
-- Note: I said in an email earlier on dev-media that I was going to stick with multiple decoders for now, but I quickly discovered that audio and video do not sync easily like this. Hence the change to a single decoder and multiple readers/resources :)
-- includes minor changes to existing files.
-- aiming for abstract - less work when adding DASH profiles/decoders later - to be improved.
-- used nsIByteBuffer in a hacky way to store the downloaded MPD data; to be cleaned up.
-- some #ifdef DASH_IGNORES are present. The ignored code is to be reviewed for inclusion.

2) WebMOnDemand, MPD files
-- files to parse MPD into classes, and report MPD contents to DASHMediaResource
-- based on WebM On Demand profile: http://wiki.webmproject.org/adaptive-streaming/webm-dash-specification

Other patches include changes to nsHTMLMediaElement and build files.

General Patch notes:
-- Makefile/config note - I've hardcoded MOZ_DASH something similar to MOZ_WEBM, but not to the extent of having a "--disable-dash" just yet.
-- The complete patch file includes some Adaptation files from libdash (ITEC) but these are not included in the build yet.

TODO:
-- Stream switching and Adaptation
-- rate limiting video
-- Other decoder/DASH profile types

Comment 14

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.0 Build files

Comment 15

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.0 Media Decoder and Resource Classes

Comment 16

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.0 MPD Classes

Comment 17

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.0 Minor changes to nsHTMLMediaElement

Comment 18

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.0 MPD Parser and Manager classes

Comment 19

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 624937 [details] [diff] [review]
WIP 2.0 Media Decoder and Resource Classes

Feedback request for Chris, Jason and Rob.
Reminder: There is a summary of the main classes at the top of nsDASHDecoder.cpp - I suggest you start there to see what the high level design is.

Jason,  the networking parts are mostly in ChannelMediaResource and DASHMediaResource, with a little bit in nsDASHDecoder to start the load for the MPD.

Chris, Rob, you can share this one or delegate to each other/someone else. Please do what works best for you. This patch contains the bulk of the changes and additions to decoders and readers.

Comment 20

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 624939 [details] [diff] [review]
WIP 2.0 MPD Classes

Christian, Christopher, please take a look at the MPD classes. The changes are mostly about using Mozilla code rather than STL, e.g. strings. Let me know if you see any issues.

Comment 21

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 624943 [details] [diff] [review]
WIP 2.0 MPD Parser and Manager classes

This one is not specifically networking nor media engine, but deals with the MPD parsing for WebM and management of it. Note that changes will go here to support adaptation in the future (as well as DASHMediaResource).
Christian, Christopher, these grew out of libdash's MPD Parser and manager classes. There are some API changes to suit Gecko, and the big changes is the use of our DOM parser. Please take a look.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You need to switch to the MPL2 header.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 624937 [details] [diff] [review]
WIP 2.0 Media Decoder and Resource Classes

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

DashMediaResource duplicates a bunch of code from ChannelMediaResource, is there any hope of avoiding that duplication?

::: content/media/dash/nsDASHWebMProxyDecoder.h
@@ +47,5 @@
> +
> +class nsDASHDecoder;
> +
> +class nsDASHWebMProxyDecoder : public nsWebMDecoder
> +{

You need a comment here explaining what this class is for.

In fact, we generally need some big comments explaining how this DASH setup works.

Comment 24

[Steve Workman [:sworkman] (INACTIVE)]

Thanks for the comments Rob:
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> You need to switch to the MPL2 header.

Ok, will do.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> DashMediaResource duplicates a bunch of code from ChannelMediaResource, is
> there any hope of avoiding that duplication?

I will go over that file again to remove what I can. Any function in particular or a common theme that you see?

> You need a comment here explaining what this class is for.
> 
> In fact, we generally need some big comments explaining how this DASH setup
> works.

Yes, comments need some work. How does the comment look in nsDASHDecoder.cpp? Does it explain enough from a high level? Are there elements that aren't clear to you immediately? What seems clear to me may not be so clear to a fresh pair of eyes.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Steve Workman [:sworkman] from comment #24)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> > DashMediaResource duplicates a bunch of code from ChannelMediaResource, is
> > there any hope of avoiding that duplication?
> 
> I will go over that file again to remove what I can. Any function in
> particular or a common theme that you see?

OpenChannel, SetupChannelHeaders, OnMPDStartRequest all looks very familiar :-). I see there's some commented out and #ifdefed out code; if you just remove that, it'll look better :-).

> > In fact, we generally need some big comments explaining how this DASH setup
> > works.
> 
> Yes, comments need some work. How does the comment look in
> nsDASHDecoder.cpp? Does it explain enough from a high level? Are there
> elements that aren't clear to you immediately? What seems clear to me may
> not be so clear to a fresh pair of eyes.

Ah, that big comment helps a ton. Clean that up and reference it from the other files and we'll be in good shape.

The basic setup looks reasonable.

BTW have you tried adding DASH resources to manifest.js and running mochitests?

Comment 26

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

Comment on attachment 624940 [details] [diff] [review]
WIP 2.0 Minor changes to nsHTMLMediaElement

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

Wow, lots of progress made here Steve. Keep up the good work.

::: content/media/MediaResource.cpp
@@ +1210,5 @@
>    }
> +#ifdef MOZ_DASH
> +  nsCAutoString mimeType;
> +  rv = aChannel->GetContentType(mimeType);
> +  if (mimeType.Equals("application/dash+xml"))

Seems like this would be better to call nsHTMLMediaElement::IsDASHMPDType() (maybe make it static?) to avoid duplication.

::: content/media/dash/nsDASHDecoder.cpp
@@ +110,5 @@
> + *     Copies data to the byte buffer
> + *   |OnMPDStopRequest|()
> + *     Dispatches an MPD Download complete event
> + *
> + *   |NotifyMPDDownloadComplete|()

It seems like all subresource/subreader/subdecoder management code should be pushed up into the nsDASHDecoder.

@@ +240,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +nsDASHDecoder::LoadResources(DASHMediaResource  *aResource,

This repeats a lot of code with nsBuiltinDecoder::Load, can you share implementation here somehow?

@@ +311,5 @@
> +  return mVideoProxyDecoder ? mVideoProxyDecoder : NULL;
> +}
> +
> +void
> +nsDASHDecoder::Shutdown()

The nsBuiltindDecoder shutdown dance is quite fragile, and you're repeating a lot of code here. I'd rather you shared as much implementation as possible, and added hooks into the relevant methods in nsBuiltinDecoder methods to call out to Dash code when you need to diverge from the existing behaviour. These could be virtual functions which are empty for the other decoders, or using some other mechanism if you like. I just want to reduce our maintenance burden where possible.

::: content/media/dash/nsDASHDecoder.h
@@ +80,5 @@
> +  nsRefPtr<nsDASHWebMProxyReader> mProxyReader;
> +
> +  // The current audio stream
> +  nsRefPtr<nsDASHWebMProxyDecoder> mAudioProxyDecoder;
> +  // Array of pointers for the different video streams on offer

s/video/audio/ right ;)

::: content/media/dash/nsDASHWebMProxyDecoder.h
@@ +46,5 @@
> +#include "nsBuiltinDecoder.h"
> +
> +class nsDASHDecoder;
> +
> +class nsDASHWebMProxyDecoder : public nsWebMDecoder

This class seems to just defer everything back to the main decoder. Presumably so in future you can cut the reference to the main decoder when you switch readers? I wonder if you can instead just add a SetDecoder() method to nsWebMReader instead, to reduce the number of classes involved?

::: content/media/nsBuiltinDecoder.h
@@ +523,5 @@
>    }
>  
>    // Returns the monitor for other threads to synchronise access to
>    // state.
> +  virtual ReentrantMonitor& GetReentrantMonitor() { 

We use the decoder monitor *everywhere* throughout the nsBuiltinDecoderStateMachine, so this change makes me nervous... Though it looks like you're safely deferring this back to the "main" decoder.

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +859,4 @@
>    // be greater than ampleAudioThreshold, else we'd stop decoding!).
>    PRInt64 ampleAudioThreshold = AMPLE_AUDIO_USECS;
>  
> +  MediaQueue<VideoData>& videoQueue = mReader->VideoQueue();

So did you make nsBuiltinDecoderReader::mVideoQueue have a virtual accessor method because a reader subclass can change the queue during playback? If so we shouldn't be caching a reference to the video queue here else it'll become stale when the video switches streams.

::: content/media/nsMediaDecoder.h
@@ +91,5 @@
>    virtual bool Init(nsHTMLMediaElement* aElement);
>  
> +  nsHTMLMediaElement* GetElement()
> +  {
> +    return mElement;

Since we're going to the trouble of adding a reference to the media element, can we assert that we only access this on the main thread?

@@ +405,4 @@
>    virtual PRInt64 AudioQueueMemoryInUse() = 0;
>  
>    VideoFrameContainer* GetVideoFrameContainer() { return mVideoFrameContainer; }
> +  virtual ImageContainer* GetImageContainer()

This doesn't need to be virtual?

::: netwerk/dash/manager/DASHMediaResource.cpp
@@ +861,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +DASHMediaResource::CreateVideoSubResource(nsAString &aUrl)

Why are you passing around strings rather than nsIURIs?

::: netwerk/dash/manager/DASHMediaResource.h
@@ +33,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +class DASHMediaResource : public ChannelMediaResource

I don't understand why you need to subclass ChannelMediaResource. Why can't you just use ChannelMediaResource to download the MPD, and have nsDASHDecoder instantiate the MPD parser and create the subdecoders inside nsDASHDecoder? Then you don't have to duplicate all the channel management and principal checking code. You may need to proxy calls to nsHTMLMediaElement::DownloadSuspened() through nsMediaDecoder to make things behave sensibly. Rewriting ChannelMediaResource doesn't seem necessary.

::: netwerk/dash/mpd/WebMOnDemandManager.h
@@ +31,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +class WebMOnDemandManager : public IMPDManager

Same thing here as for the WebMOnDemandParser; the only codec specific thing I see here is the WebM mime-type checks, can we generalize this to make supporting other codecs in future easier?

::: netwerk/dash/mpd/WebMOnDemandParser.h
@@ +78,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +class WebMOnDemandParser : public IMPDParser

This is an MPD parser right? It seems like naming it WebMOnDemandParser implies that it's a WebM parser that parses on demand, rather than an MPD parser.

Other than the mime type checks, how much of this actually needs to be WebM specific? Can we generalize this class to make it easier to support H.264 if we wanted to in future?

Comment 27

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.1 Complete patch - Demos non-adaptive, WebM On Demand Video

Same as 2.1 except I removed some bitrot to apply to mozilla-central as of June 15, 2012.

Available for folks from Cisco ABR Workshop (June 14-15) to play with.

Comment 28

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.2 Complete patch - Demos non-adaptive, WebM On Demand Video

Complete patch. Updated based on feedback comments.

Comment 29

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.2 Build files

Comment 30

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.2 Media Decoder and Resource Classes

Comment 31

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.2 MPD Classes

Comment 32

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.2 MPD Parser and Manager classes

Comment 33

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.2 Minor changes to nsHTMLMediaElement

Comment 34

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> (In reply to Steve Workman [:sworkman] from comment #24)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> > > DashMediaResource duplicates a bunch of code from ChannelMediaResource, is
> > > there any hope of avoiding that duplication?

DASHMediaResource is now removed - went over the code again and decided I can re-use ChannelMediaResource. So, no duplication for this file.

> > > In fact, we generally need some big comments ...
> 
> Ah, that big comment (in nsDASHDecoder.cpp) helps a ton. Clean that up and reference it from the
> other files and we'll be in good shape.

I've added pointers to this comment from the files touching the media engine/decoders directly. I've also added some descriptions to the MPD files. I'll keep reviewing comments as I progress

> The basic setup looks reasonable.

Good to know :)

> BTW have you tried adding DASH resources to manifest.js and running
> mochitests?

No testing yet. It's on the todo list. And I may come looking for help/advice :)

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Basically you should be able to add DASH resources to manifest.js and then a lot of our tests will automatically start running against those resources. If those tests don't find bugs, you've probably done something wrong :-).

Comment 36

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Chris Pearce (:cpearce) from comment #26)
> Wow, lots of progress made here Steve. Keep up the good work.

Thanks!

> ::: content/media/MediaResource.cpp
> @@ +1210,5 @@
> >    }
> > +#ifdef MOZ_DASH
> > +  nsCAutoString mimeType;
> > +  rv = aChannel->GetContentType(mimeType);
> > +  if (mimeType.Equals("application/dash+xml"))
> 
> Seems like this would be better to call nsHTMLMediaElement::IsDASHMPDType()
> (maybe make it static?) to avoid duplication.

Removed DASHMediaResource, so don't need to worry about it in MediaResource::Create.
  
> It seems like all subresource/subreader/subdecoder management code should be
> pushed up into the nsDASHDecoder.

Yup - especially after removing DASHMediaResource and reusing ChannelMediaResource.
 
> > +nsresult
> > +nsDASHDecoder::LoadResources(DASHMediaResource  *aResource,
> 
> This repeats a lot of code with nsBuiltinDecoder::Load, can you share
> implementation here somehow?

I reviewed this and nsDASHDecoder::Load and nsBuiltinDecoder::Load. Seems like the latter two have more in common, so I was able to do some sharing there. LoadResources (now LoadMPDResources) stands on its own a little more. There may be some more sharing I can do there.
 
> The nsBuiltindDecoder shutdown dance is quite fragile, and you're repeating
> a lot of code here. I'd rather you shared as much implementation as
> possible, and added hooks into the relevant methods in nsBuiltinDecoder
> methods to call out to Dash code when you need to diverge from the existing
> behaviour. These could be virtual functions which are empty for the other
> decoders, or using some other mechanism if you like. I just want to reduce
> our maintenance burden where possible.

Agreed. I've shared some code here by adding a virtual function like you suggested.

> > +  // Array of pointers for the different video streams on offer
> 
> s/video/audio/ right ;)

Oops. Yup. Done.
 
> ::: content/media/dash/nsDASHWebMProxyDecoder.h
> @@ +46,5 @@
> > +#include "nsBuiltinDecoder.h"
> > +
> > +class nsDASHDecoder;
> > +
> > +class nsDASHWebMProxyDecoder : public nsWebMDecoder
> 
> This class seems to just defer everything back to the main decoder.
> Presumably so in future you can cut the reference to the main decoder when
> you switch readers? I wonder if you can instead just add a SetDecoder()
> method to nsWebMReader instead, to reduce the number of classes involved?

I agree that the class just does a lot of forwarding. However, it currently holds the reference to the correct ChannelMediaResource for its data. From what I can tell, reader and media resource are tightly coupled, so adding an nsWebMReader::SetResource for switching seems cumbersome and more complex than just having the proxy decoder manager the correct resource reference.

> We use the decoder monitor *everywhere* throughout the
> nsBuiltinDecoderStateMachine, so this change makes me nervous... Though it
> looks like you're safely deferring this back to the "main" decoder.

I took a look at this one again. There is a monitor created but not used in the proxy decoder class. But in order to keep things sync'd properly, I'd prefer to use the one in the main decoder, i.e. nsDASHDecoder. It is a little risky making this virtual, yes, but I think it's cleaner and less risky than internally wrapping each proxy decoder's functions with the main decoder's monitor - seems better to have it overidden in this one place rather than having to remember to add it to multiple places.

I can look at it again if it still makes you nervous.

> > +  MediaQueue<VideoData>& videoQueue = mReader->VideoQueue();
> 
> So did you make nsBuiltinDecoderReader::mVideoQueue have a virtual accessor
> method because a reader subclass can change the queue during playback? If so
> we shouldn't be caching a reference to the video queue here else it'll
> become stale when the video switches streams.

Right. I have this one added to the todo list, so it's not reflected in the current patches.

> > +  nsHTMLMediaElement* GetElement()
> > +  {
> > +    return mElement;
> 
> Since we're going to the trouble of adding a reference to the media element,
> can we assert that we only access this on the main thread?

Removed this one - it was an artifact from an earlier draft. I wasn't even using it in the code.
 
> > +  virtual ImageContainer* GetImageContainer()
> 
> This doesn't need to be virtual?

Looks like it does need to be to me. nsDASHProxyDecoder needs to forward to the main decoder when nsWebMReader calls it, right? I assume we only want to use one ImageContainer, i.e. the one from the main nsDASHDecoder, no?
 
> > +nsresult
> > +DASHMediaResource::CreateVideoSubResource(nsAString &aUrl)
> 
> Why are you passing around strings rather than nsIURIs?

So, DASHMediaResource is gone, but this function is now in nsDASHDecoder. I have changed it to pass nsIURIs where appropriate; in some places, like in the MPD classes, I still use strings because they're only parts of the URL, not the full thing.
 
> > +class DASHMediaResource : public ChannelMediaResource
> 
> I don't understand why you need to subclass ChannelMediaResource. Why can't
> you just use ChannelMediaResource to download the MPD, and have
> nsDASHDecoder instantiate the MPD parser and create the subdecoders inside
> nsDASHDecoder? Then you don't have to duplicate all the channel management
> and principal checking code. You may need to proxy calls to
> nsHTMLMediaElement::DownloadSuspened() through nsMediaDecoder to make things
> behave sensibly. Rewriting ChannelMediaResource doesn't seem necessary.

Indeed. *Poof!* - it's gone. Magic. I had in my head that nsMediaCache wasn't a suitable storage for the MPD data - seems like it's fine.

> > +class WebMOnDemandManager : public IMPDManager
> 
> Same thing here as for the WebMOnDemandParser; the only codec specific thing
> I see here is the WebM mime-type checks, can we generalize this to make
> supporting other codecs in future easier?

> Other than the mime type checks, how much of this actually needs to be WebM
> specific? Can we generalize this class to make it easier to support H.264 if
> we wanted to in future?

I took a look at this one and it's pretty specific to the MPD profile. I will keep generalization in mind for the future and revisit this.
 
> > +class WebMOnDemandParser : public IMPDParser
> 
> This is an MPD parser right? It seems like naming it WebMOnDemandParser
> implies that it's a WebM parser that parses on demand, rather than an MPD
> parser.

Renamed the prefix to nsDASHWebMOD~

Comment 37

[distler]

> [S]upport[ing] H.264 if we wanted to in future ...

presuably means supporting the MPD profiles for H.264-ecoded DASH content.

The DASH profile, being implemented here, uses non-muxed non-chunked webm-encoded streams.

The DASH profile, supported by VLC, uses muxed H.264-encoded streams, which are either chunked (.m4s) or non-chunked (.mp4).
 
Also likely to be popular is the

    "urn:mpeg:dash:profile:mp2t-main:2011"

profile, which is HLS-compatible: muxed (chunked or non-chunked) H.264-encoded streams in .ts containers.

Maybe you want a general-purpose MPD parser class, with different sub-classes for different profiles?

Comment 38

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 3.0 Complete patch - Non-adaptive DASH-WebM, byte-range downloads

Added support for download of DASH-WebM files in chunks.

Background: DASH-WebM files shoudl have their cues synchronised by time, so that players can switch streams at cluster offsets. This requires downloading the cues byte ranges before the clusters, in order to get the offsets. The DASH-WebM MPD manifest file should contain both initialization and index byte ranges - this new patch uses these to get EBML and cues bytes, before determining the byte ranges for clusters. Subsequent HTTP requests are on a per cluster basis.

See nsDASHDecoder.cpp for more details in the top-of-file comment.

Note: this required the addition of a function to nestegg to read the cues and populated the nestegg context variable.

Comment 39

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 3.0 Build files

Comment 40

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 3.0 Media Decoder and Resource Classes

Comment 41

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 3.0 MPD Classes

Comment 42

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 3.0 MPD Parser and Manager classes

Comment 43

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 3.0 Minor changes to nsHTMLMediaElement

Comment 44

[Steve Workman [:sworkman] (INACTIVE)]

Reminder: Only the complete diff needs to be patched - the others are just provided separately for ease of reading.

Comment 45

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 646408 [details] [diff] [review]
WIP 3.0 Media Decoder and Resource Classes

Rob, Chris, please take a look at these files, specifically to examine the changes needed for byte range downloads. The code path kicks off in nsDASHRepDecoder::OpenMetadata and continues from there. Thanks!

Note: I'm going on vacation for two weeks, so I won't be able to respond to any comments until after that.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 646408 [details] [diff] [review]
WIP 3.0 Media Decoder and Resource Classes

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

It would help to have documentation for how seeking works with DASH here. I don't really want to reverse-engineer it.

Can we split this patch up a bit? Seems like support for restricting a MediaResource load to a particular byte range should be its own patch. The nestegg changes should definitely be their own patch. Some of the refactorings you do could also be their own patches.

::: content/media/MediaResource.cpp
@@ +738,5 @@
> +        }
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        // If range is not cached, store it for seeking later
> +        if (!cached)
> +          mByteRange = byteRange;

{}

@@ +741,5 @@
> +        if (!cached)
> +          mByteRange = byteRange;
> +        // Else clear it so we don't use it later
> +        else
> +          mByteRange.Clear();

{}

::: content/media/MediaResource.h
@@ +285,5 @@
>     */
>    virtual nsresult Open(nsIStreamListener** aStreamListener) = 0;
>  
> +  virtual nsresult OpenByteRange(nsIStreamListener **aStreamListener,
> +                                 MediaByteRange& aByteRange)

Should this be const?

You should document the semantics of this method. For example how does Seek() behave when used with this method?

@@ +440,5 @@
>    // Opens the channel, using an HTTP byte range request to start at mOffset
>    // if possible. Main thread only.
>    nsresult OpenChannel(nsIStreamListener** aStreamListener);
> +
> +  nsresult ReopenByteRange();

What does this do?

::: content/media/nsMediaDecoder.h
@@ +74,5 @@
>    virtual nsresult Seek(double aTime) = 0;
>  
> +  virtual nsresult QueryByteRange(PRInt64 aOffset,
> +                                  MediaByteRange& aByteRange) {
> +    return NS_OK;

What does this do?

@@ +379,5 @@
>      return mVideoFrameContainer ? mVideoFrameContainer->GetImageContainer() : nsnull;
>    }
>  
> +  virtual void OnByteRangeComplete(ChannelMediaResource *aRes,
> +                                   nsresult aStatus) { }

When does this get called?

Comment 47

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #46)
> Comment on attachment 646408 [details] [diff] [review]
> WIP 3.0 Media Decoder and Resource Classes
> 
> Review of attachment 646408 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It would help to have documentation for how seeking works with DASH here. I
> don't really want to reverse-engineer it.

Ah, apologies about that. I was perhaps a little too eager to get the patch uploaded before vacation :)
 
> Can we split this patch up a bit? Seems like support for restricting a
> MediaResource load to a particular byte range should be its own patch. The
> nestegg changes should definitely be their own patch. Some of the
> refactorings you do could also be their own patches.

Makes a lot of sense. I've started working on two new patched for nestegg and byte-range enabling ChannelMediaResource, and I'll figure out how to split up the rest after that.

Thanks for the comments.

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You don't need to put separate patches in their own bugs, although it's OK if you do.

Comment 49

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #48)
> You don't need to put separate patches in their own bugs, although it's OK
> if you do.

Thanks Rob. I'll not go crazy with it. For nestegg at least, if it's going to be a separate patch, I'd prefer to have comments focused on that patch alone, with references back to this bug. But, don't worry, I don't feel the need to open a slew of new bugs.

Comment 50

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 3.5 Add byte range download support to ChannelMediaResource

** 1st patch in series **
(Note: Apply patch from 782457 first!)

Adds byte range downloads to ChannelMediaResource.

Call OpenByteRange instead of Open to open a stream using a specific byte range only. Behaves similarly to Open, and adds a byte range parameter, aByteRange.

Upon the internal call to OnStopRequest, ChannelMediaResource will call the decoder's NotifyByteRangeDownloaded function. The Media Cache will not be notified via NotifyDataEnded, since the data is not fully downloaded.

The idea would be that nsDASHDecoder would receive a call to NotifyByteRangeDownloaded after each chunk is downloaded, and then initiate a call to OpenByteRange using the next range in the sequence.

For seeking, the current process is that the reader calls the resource with a specific offset. The resource then forwards that call to the media cache, which determines the offset of the first data block which contains the reader's requested offset. The media cache then returns this call to the resource and tells it to download from that offset, which is probably earlier in the byte stream than the one requested by reader.

For chunked downloads, e.g. DASH, we need to determine which chunk contains the requested offset, |mOffset|. This is either previously requested in |Seek| or updated to the most recent bytes downloaded. So the process below is:
1 - Query decoder for chunk containing desired offset, |mOffset|.
    Return silently if the offset is out-of-range; suggests end of data.

2 - Adjust |mByteRange|.mStart to |aOffset|, requested by media cache.
    This is the start of the media cache block.
    XXX For "DASH-WebM On Demand" this means the first chunk after
    seeking may be larger/smaller than the encoded subsegment (cluster).
3 - Call |OpenByteRange| requesting |mByteRange| bytes.

Comment 51

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 3.5 MPD Classes

** 2nd in patch series **

These classes manage the data described in the MPD XML manifest, e.g. the MPD class represents the MPD tag, which represents global data pertaining to the whole media presentation. For more documentation, please see MPD.h, which contains a brief description an ASCII art of the hierarchy of the primary objects.

Comment 52

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 3.5 MPD Parser and Manager

** 3rd in patch series **

The MPD Parser and Manager use the MPD classes to manage information about the media presentation.  MPD Parser takes the DOM structure, created by parsing the XML, and populates the data into the MPD classes. MPD Manager uses the data in the classes to provide information to the decoders about the media presentation. See nsDASHMPDParser.h and IMPDManager.h for more documentation.

Comment 53

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 3.5 DASH Decoder, Reader etc.

** 4th in patch series **

This is where all the previous patches come together to form the DASH codebase.  nsDASHDecoder.cpp contains a explanation of the object relationship as well as the code flow.

Comment 54

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

Comment on attachment 653616 [details] [diff] [review]
WIP 3.5 DASH Decoder, Reader etc.

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

Phew, lots of work in this one, lots of progress.

In general, it looks pretty good.

I think a lot of this could be broken up into non-dash dependent patches and landed now, separately, so that you don't have to keep rebasing and unbitrotting. For example your changes to virtualize the methods in nsBuiltinDecoder* could land now.

You should add "showfunc = 1" and "unified = 8" to the [diff] section of your ~/.hgrc so that there's more context in your patches and so that the function name shows up to make your patches easier to review.

Nit: Can you put full-stops at the end of all your comments please? Thanks.

There's some whitespace at the end of some lines, can you remove them?

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +2191,5 @@
> +const char nsHTMLMediaElement::gDASHMPDTypes[1][21] = {
> +  "application/dash+xml"
> +};
> +
> +bool

Nit: Put a "/* static */" comment before static methods please.

::: content/media/dash/Makefile.in
@@ +47,5 @@
> +
> +
> +EXPORTS		+= \
> +		nsDASHDecoder.h \
> +    nsDASHRepDecoder.h \

Nit: Looks like your using an inconsistent mix of tabs and spaces to indent EXPORTS, CPPSRCS and INCLUDES.

::: content/media/dash/nsDASHDecoder.cpp
@@ +140,5 @@
> +#else
> +#define LOG(type, msg)
> +#define SEEK_LOG(type, msg)
> +#endif
> +

Assert what thread every method runs on please, and ensure the comments in the matching .h file include which method runs on what thread.

@@ +182,5 @@
> +  NS_ENSURE_TRUE(aRepDecoder, );
> +
> +  LOG(PR_LOG_DEBUG, ("nsDASHDecoder: MetadataDownloadEnded for decoder [%p]",
> +                     aRepDecoder));
> +  // XXX may need this for something else later

I'd prefer to add this when we need it (if possible), not before...

But maybe this should be where you decide whether to unblock the reader when it's waiting in ReadMetadata.

@@ +252,5 @@
> +    return;
> +
> +  PRInt64 length = mResource->GetLength();
> +  NS_ENSURE_TRUE(length > 0, );
> +  NS_ENSURE_TRUE(length < PR_UINT32_MAX, );

Maybe you should make that length < 50MB, or some other large enough value that's not likely to cause "new" to fail... Just in case...

@@ +292,5 @@
> +  NS_ENSURE_TRUE(mResource, );
> +  rv = mResource->Close();
> +  NS_ENSURE_SUCCESS(rv, );
> +
> +  rv = ParseMPDBuffer();

Do you really want to be parsing on the main thread? How slow is parsing? Why don't you do this on the mMDPReaderThread?

@@ +366,5 @@
> +}
> +
> +nsresult
> +nsDASHDecoder::CreateAudioRepDecoder(nsIURI *aUrl,
> +                                       mozilla::net::Representation *aRep)

Indentation

@@ +399,5 @@
> +}
> +
> +nsresult
> +nsDASHDecoder::CreateVideoRepDecoder(nsIURI *aUrl,
> +                                       mozilla::net::Representation *aRep)

Indentation.

@@ +487,5 @@
> +nsDASHDecoder::CreateSubChannel(nsIURI *aUrl, nsIChannel **aChannel)
> +{
> +  NS_ENSURE_ARG(aUrl);
> +
> +  //LOG(("SetupSubChannel for URL=\"%s\"", NS_ConvertUTF16toUTF8(aUrl).get()));

Don't add commented out code please. Put it behind an ifdef if you don't want it to run by default.

@@ +535,5 @@
> +    if (mAudioRepDecoder) {
> +      rv = mAudioRepDecoder->Load();
> +    }
> +    if (mVideoRepDecoder) {
> +      rv = mVideoRepDecoder->Load();

This could overwrite a failure value stored in rv by mAudioRepDecoder->Load().

@@ +572,5 @@
> +  }
> +
> +  ChangeState(PLAY_STATE_LOADING);
> +
> +  // Set volume based on element

Why does the decoder need to know about the volume? This should be handled by the nsBuiltinDecoderStateMachine as it's playing, not by the decoders?

::: content/media/dash/nsDASHReader.cpp
@@ +16,5 @@
> +#include "VideoFrameContainer.h"
> +#include "nsBuiltinDecoder.h"
> +#include "nsDASHReader.h"
> +
> +nsresult

You should assert in every method what thread you're on. Some methods may be called on multiple threads.

Some methods of this class are called from the main thread (like nsDASHDecoder::Create*RepDecoder()), some are called from the decode thread (DecodeVideoData, DecodeAudioData), and you're not synchronizing data written on one thread with reads on other threads. You may need to introduce a monitor here to protect access to shared data.

@@ +25,5 @@
> +    rv = mAudioReader->Init(nullptr);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +  if (mVideoReader) {
> +    mVideoReader->Init(nullptr);

rv = mVideoReader->Init(nullptr);

Right? ;)

@@ +65,5 @@
> +}
> +
> +bool
> +nsDASHReader::DecodeVideoFrame(bool &aKeyframeSkip,
> +                                        PRInt64 aTimeThreshold)

Indentation seems off here.

@@ +81,5 @@
> +}
> +
> +void
> +nsDASHReader::NotifyDataArrived(const char* aBuffer,
> +                                         PRUint32 aLength,

Indentation.

@@ +84,5 @@
> +nsDASHReader::NotifyDataArrived(const char* aBuffer,
> +                                         PRUint32 aLength,
> +                                         PRInt64 aOffset)
> +{
> +  // XXX This is incorrect; audio and video notifications will be forwarded to

This is for the MPD download right? The sub-resources receive data arrived notifications to their sub-nsBuiltinDecoders directly don't they? Doesn't that mean you don't need to forward this at all?

@@ +94,5 @@
> +nsresult
> +nsDASHReader::ReadMetadata(nsVideoInfo* aInfo,
> +                           nsHTMLMediaElement::MetadataTags** aTags)
> +{
> +  // Get metadata from child readers

This is called as soon as the decode thread starts... So what stops this running before mAudioReader and mVideoReader are setup? It seems to me like this has little hope of actually getting metadata... Maybe this function should wait on a signal from the MPD parser once the first readers have been created and set.

@@ +137,5 @@
> +}
> +
> +nsresult
> +nsDASHReader::GetBuffered(nsTimeRanges* aBuffered,
> +                                   PRInt64 aStartTime)

Indentation is off here.

@@ +191,5 @@
> +        NS_ENSURE_SUCCESS(res, res);
> +        res = videoBuffered.End(i, &endV);
> +        NS_ENSURE_SUCCESS(res, res);
> +
> +        if ((startA > endV) || (endA < startV))

You can expect that nsTimeRanges are in a sorted order, so you should be able to break if (endA < startV).

::: content/media/dash/nsDASHRepDecoder.cpp
@@ +22,5 @@
> +#ifdef PR_LOGGING
> +extern PRLogModuleInfo* gBuiltinDecoderLog;
> +#define LOG(msg) PR_LOG(gBuiltinDecoderLog, PR_LOG_DEBUG, msg)
> +#endif
> +

Please assert in every method what thread you're on.

@@ +65,5 @@
> +  return mResource->OpenByteRange(nullptr, mInitByteRange);
> +}
> +
> +void
> +nsDASHRepDecoder::NotifyByteRangeDownloaded(ChannelMediaResource * const aRes,

Did you mean "const ChannelMediaResource * aRes"? You've qualified the pointer as constant, not the object being pointed to.

@@ +166,5 @@
> +  }
> +  // Check metadata ranges; init range
> +  if (mInitByteRange.mStart <= aOffset && aOffset <= mInitByteRange.mEnd) {
> +    aByteRange = mInitByteRange;
> +    //mLoadingInitBytes = true;

Remove dead code.

@@ +173,5 @@
> +  }
> +  // ... index range
> +  if (mIndexByteRange.mStart <= aOffset && aOffset <= mIndexByteRange.mEnd) {
> +    aByteRange = mIndexByteRange;
> +    //mLoadingIndexBytes = true;

Ditto.

@@ +242,5 @@
> +
> +  if (aStatus == NS_BINDING_ABORTED) {
> +    // Download has been cancelled by user.
> +    if (mElement)
> +      mElement->LoadAborted();

If the download is canceled by the user by pressing ESC, won't this mean that we'll call mElement->LoadAborted() for every active sub-resource's channel, and we'll end up dispatching multiple "error" events to the HTML element?

::: content/media/dash/nsDASHRepDecoder.h
@@ +23,5 @@
> +#include "nsBuiltinDecoder.h"
> +
> +class nsDASHDecoder;
> +
> +class nsDASHRepDecoder : public nsWebMDecoder

Do you actually need to inherit from WebMDecoder? What functionality of nsWebMDecoder do you need here?

Can you make this inherit from nsBuiltinDecoder, then when we move to support H.264 in DASH in future we basically can get it for free.

@@ +31,5 @@
> +  typedef mozilla::net::SegmentBase SegmentBase;
> +  typedef mozilla::layers::ImageContainer ImageContainer;
> +
> +  // Constructor takes a ptr to the main decoder
> +  nsDASHRepDecoder(nsDASHDecoder  *aMainDecoder) :

s/(nsDASHDecoder  *aMainDecoder)/(nsDASHDecoder *aMainDecoder)/

@@ +141,5 @@
> +  // Index of the current byte range
> +  PRUint64        mSubsegmentIdx;
> +
> +  // Ptr to the reader object for this |Representation|
> +  nsWebMReader   *mReader;

nsBuiltinReader*, to make it more portable.

Also, Type* pointer, not Type *pointer.

::: content/media/nsBuiltinDecoder.h
@@ +361,1 @@
>    

Please add "Called on the main thread only" in the comment here, and for Shutdown() as well.

@@ +365,5 @@
>                          nsIStreamListener** aListener,
>                          nsMediaDecoder* aCloneDonor);
>  
> +  // Called in Load to open the media resource
> +  nsresult OpenResource(MediaResource* aResource,

This should really be protected, not public. Oh wait, everything's public in nsBuiltinDecoder. *sigh*... We'll let this slide for now...

@@ +587,5 @@
>    void ChangeState(PlayState aState);
>  
> +  // Called when the metadata from the media file has been read by the reader.
> +  // Call on the decode thread only.
> +  virtual void OnReadMetadataComplete() { }

OnReadMetadataCompleted (to match the "*ed" convention we already have).

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +827,4 @@
>    // be greater than ampleAudioThreshold, else we'd stop decoding!).
>    PRInt64 ampleAudioThreshold = AMPLE_AUDIO_USECS;
>  
> +  MediaQueue<VideoData>& videoQueue = mReader->VideoQueue();

If the reader's "active" video queue can change during the loop, then we shouldn't be storing a reference to it here, we should call mReader->VideoQueue() inside the decode loop; just like we do for mReader->AudioQueue().

Caching a reference to the video queue was only a convenience to save keystrokes, but now I think we should remove it.

Comment 55

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v 4.0 Add byte range download support to ChannelMediaResource

See comment 50.

Comment 56

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v4.0 MPD Classes

See comment 51.

Comment 57

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v4.0 MPD Parser and Manager

See comment 52.

Comment 58

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v4.0 Preparative changes to base media decoder files

As per Chris's suggestion in comment 54 (thanks for the comments, btw!) ...

> I think a lot of this could be broken up into non-dash dependent patches and landed now, separately, so that you don't have to keep rebasing and unbitrotting. For example your changes to virtualize the methods in nsBuiltinDecoder* could land now.

I have uploaded base/foundational patches for DASH. Specifically, I pulled out the new nsDASHDecoder, nsDASHReader etc. files and have uploaded only the changes to nsBuiltinDecoder etc.

I also went through Chris's list of feedback comments in comment 54. I'm most of the way through them, with the remaining few items applying to nsDASHDecoder etc. only. So, while I work on completing them, it seems useful to upload these base patches now.

I put Chris's name for the r? on all four patches - please share it out if need be.

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think Chris P can handle the reviews here; I'm happy to look at anything specific Chris P wants me to look at.

Comment 60

[Steve Workman [:sworkman] (INACTIVE)]

Sounds good to me - whatever works :)

Comment 61

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

Comment on attachment 658317 [details] [diff] [review]
v 4.0 Add byte range download support to ChannelMediaResource

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

Looks good. Only minor changes needed.

::: content/media/MediaResource.cpp
@@ +36,5 @@
> +// Debug logging macro with object pointer and class name
> +#define CMLOG(msg, ...) \
> +        LOG("%p [ChannelMediaResource]: " msg, this, ##__VA_ARGS__)
> +#else
> +#define LOG(msg)

You need to define CMLOG here, otherwise the build will fail when PR_LOGGING isn't defined.

@@ +228,5 @@
> +
> +      // Parse the range header: e.g. Content-Range: bytes 7000-7999/8000
> +      int32_t spacePos = byteRangeText.Find(NS_LITERAL_CSTRING(" "));
> +      int32_t dashPos = byteRangeText.Find(NS_LITERAL_CSTRING("-"), true, spacePos);
> +      int32_t slashPos = byteRangeText.Find(NS_LITERAL_CSTRING("/"), true, dashPos);

What if these finds fail? It returns -1. I guess we'll bail out at line 237 then. I think we should take the else "If we get an OK response but we were seeking" path below in that case, that would leave is in more consistent state...

Perhaps you could achieve this by putting the Content-Range parsing in a function which can return true/NS_OK upon success, and check its return value in the "if" statement on line 222, and fall into the else branch upon failure?

@@ +233,5 @@
> +
> +      nsAutoCString byteRangeStartText;
> +      byteRangeText.Mid(byteRangeStartText, spacePos+1, dashPos-(spacePos+1));
> +      int32_t byteRangeStart = byteRangeStartText.ToInteger(&rv);
> +      NS_ENSURE_SUCCESS(rv, rv);

Should we also fail if byteRangeStart < 0?

@@ +245,5 @@
> +#endif
> +
> +      nsAutoCString byteRangeTotalText;
> +      byteRangeText.Right(byteRangeTotalText, byteRangeText.Length()-(slashPos+1));
> +      int32_t byteRangeTotal = byteRangeTotalText.ToInteger(&rv);

Should we fail if byteRangeTotal is < 0?

@@ +918,5 @@
> +    // XXX Add GetByteRangeForSeek to nsDASHDecoder in subseqeuent patch.
> +    nsresult rv = mDecoder->GetByteRangeForSeek(mOffset, mByteRange);
> +
> +    if (NS_SUCCEEDED(rv) && (!mByteRange.IsNull())) {
> +      // Media cache decreases offset to start of cache data block.

Your comments here are implying GetByteRangeForSeek is related to lining blocks up with media cache blocks, but I think you're actually using it to ensure that you start the download at the start of the WebM cluster? Can you make the comment clearer?

@@ +942,5 @@
>      return NS_OK;
>    }
>  
> +  if (mByteRangeDownloads) {
> +    // Byte range is either cached or offset is not available; return silently.

What should happen if we try to read an offset that is not available? What does happen? Can it happen?

Comment 62

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

Comment on attachment 658323 [details] [diff] [review]
v4.0 Preparative changes to base media decoder files

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

r=cpearce with minor changes.

::: content/media/nsBuiltinDecoder.h
@@ +782,5 @@
>    // ReentrantMonitor for detecting when the video play state changes. A call
>    // to Wait on this monitor will block the thread until the next
>    // state change.
> +  // Using a wrapping class to impede direct access to the |ReentrantMonitor|
> +  // object and encourage use of the |nsBuiltinDecoder|::|GetReentrantMonitor|

Say *why* we want to encourage not accessing the monitor directly.

I think we should call this something else, like PrivateMonitor, HiddenMonitor, ProtectedMonitor, MonitorWrapper, IndirectMonitor, NotToBeUsedDirectlyMonitor or something else, to reduce the likelihood of name collisions, and to make its purpose clearer.

Comment 63

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

Comment on attachment 658318 [details] [diff] [review]
v4.0 MPD Classes

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

Please use MOZ_COUNT_CTOR/DTOR in all these classes constructors and destructors to ensure we don't and won't leak.

You should use nsAutoPtr<T> an nsTArray<nsAutoPtr<T> > for owning pointers to simplify and robustify memory management.

r=cpearce with those changes, I don't need to see the patch again, I trust you to make them.

::: netwerk/dash/Makefile.in
@@ +1,2 @@
> +#
> +# ***** BEGIN LICENSE BLOCK *****

Use the new MPL shorter boilerplate:
http://www.mozilla.org/MPL/headers/

I think we need a build person to look at Makefile changes, even though they look pretty simpe.

::: netwerk/dash/mpd/AdaptationSet.h
@@ +84,5 @@
> +  // Returns the number of media |Representations|.
> +  uint16_t GetNumRepresentations() const;
> +
> +  // En/Dis-ables switching between media |Representation|s.
> +  void SetBitstreamSwitching(bool const value);

aValue :(

I'd call this SetBitstreamSwitchingAllowed (or Enabled), as the current name implies that it's called when switching starts/ends.

@@ +97,5 @@
> +  int32_t                    mHeight;
> +  nsString                   mMIMEType;
> +
> +  // If true, switching between media |Representation|s is allowed.
> +  bool                       mIsBitstreamSwitching;

I'd call this mIsBitstreamSwitchingAllowed, as the current name implies it is true while a switch is in progress.

::: netwerk/dash/mpd/BaseUrl.h
@@ +23,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +class BaseUrl

It would be good to add a comment that documents why you have this class when nsIURI already exists.

::: netwerk/dash/mpd/MPD.h
@@ +65,5 @@
> +  }
> +
> +  // At least one media content |Period|s per Media Presentation. The |MPD|
> +  // contains 1+ available pieces of media, available during that time period
> +  // e.g. audio in various languages, a video component.

You might want to comment here that the MPD takes ownership of the Period henceforth, and will delete it.

@@ +80,5 @@
> +  // List of media content |Period|s in this media presentation.
> +  nsTArray<Period*>   mPeriods;
> +
> +  // List of |BaseURL|s which can be used to access the media.
> +  nsTArray<BaseUrl*>  mBaseUrls;

You could use nsTArray< nsAutoPtr<T> > instead of nsTArray<T>, and then you'd not need to manually free the resources in ~MPD(). If you do this you need to define copy constructors or all T though, since nsTArray memmoves when it resizes.

::: netwerk/dash/mpd/Period.cpp
@@ +55,5 @@
> +void
> +Period::AddAdaptationSet(AdaptationSet* aAdaptationSet)
> +{
> +  NS_ENSURE_TRUE(aAdaptationSet,);
> +  mAdaptationSets.AppendElement(aAdaptationSet);

Perhaps you should assert that aAdaptationSet isn't already in the array. Same with your other Add* functions.

::: netwerk/dash/mpd/Period.h
@@ +77,5 @@
> +  void SetDuration(double const aDuration);
> +
> +private:
> +  // List of |AdaptationSet|s of media in this |Period|.
> +  nsTArray<AdaptationSet*> mAdaptationSets;

Again, you have the option of using nsTArray< nsAutoPtr<AdaptationSet> > here.

::: netwerk/dash/mpd/Representation.cpp
@@ +104,5 @@
> +
> +void
> +Representation::SetSegmentBase(SegmentBase* aBase)
> +{
> +  NS_ENSURE_TRUE(aBase,);

If mSegmentBase is non-null should it be deleted?

::: netwerk/dash/mpd/Representation.h
@@ +61,5 @@
> +  { }
> +  virtual ~Representation() {
> +    if (mSegmentBase) {
> +      delete mSegmentBase;
> +    }

Do you need to clear mBaseUrls? If you'd used nsAutoPtr this wouldn't be an issue... ;)

@@ +77,5 @@
> +
> +  // Gets/Adds a |BaseURL| for the media files.
> +  void                       AddBaseUrl(BaseUrl* aUrl);
> +  nsTArray<BaseUrl*> const& GetBaseUrls() const;
> +  

Trailing space.

@@ +94,5 @@
> +  // List of absolute/relative |BaseURL|s which may be used to access the media.
> +  nsTArray<BaseUrl*>  mBaseUrls;
> +
> +  // The base |Segment| for the |Representation|.
> +  SegmentBase*        mSegmentBase;

Use nsAutoPtr<SegmentBase> so you don't have to manage memory.

::: netwerk/dash/mpd/SegmentBase.cpp
@@ +94,5 @@
> +    nsAutoString temp(Substring(start, dashStart));
> +    nsresult rv;
> +    aStart = temp.ToInteger(&rv);
> +    NS_ENSURE_SUCCESS(rv, );
> +

Should you check if start > end, or if either are negative?

::: netwerk/dash/mpd/SegmentBase.h
@@ +57,5 @@
> +  { }
> +  virtual ~SegmentBase() { }
> +
> +  // Get/Set the byte range for the initialization bytes.
> +  void GetInitRange(int64_t *aStartBytes, int64_t* aEndBytes) const;

Inconsistent placement of * here. This is new code, and it's your code, so you can chose how you do it. If you chose to be inconsistent, that's your choice. ;)

@@ +69,5 @@
> +  // Parses the string to get a start and end value.
> +  void SetRange(nsAString const &aRangeStr, int64_t &aStart, int64_t &aEnd);
> +
> +  // Start and end values for the init byte range.
> +  int64_t mInitRangeStart;

I almost feel that you should have some kind of "range" class for these.

Comment 64

[Steve Workman [:sworkman] (INACTIVE)]

Thanks Chris! Nice to get some r+s for the weekend!
FYI, I'm going to work on cleaning up and landing these first ones before uploading a new version of the DASH decoder one. Seems smart to include any necessary changes before you take another look at it.

Comment 65

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v5.0 Add byte range download support to ChannelMediaResource

(In reply to Chris Pearce (:cpearce) from comment #61)
> Comment on attachment 658317 [details] [diff] [review]
> v 4.0 Add byte range download support to ChannelMediaResource
> […]
> ::: content/media/MediaResource.cpp
> @@ +36,5 @@
> […]
> > +#define LOG(msg)
> 
> You need to define CMLOG here, otherwise the build will fail when PR_LOGGING
> isn't defined.

Yup - discovered I had made this mistake via a try build. Fixed.

> @@ +228,5 @@
> […]
> > +      int32_t slashPos = byteRangeText.Find(NS_LITERAL_CSTRING("/"), true, dashPos);
> 
> What if these finds fail? It returns -1. I guess we'll bail out at line 237
> then. I think we should take the else "If we get an OK response but we were
> seeking" path below in that case, that would leave is in more consistent
> state...
> 
> Perhaps you could achieve this by putting the Content-Range parsing in a
> function which can return true/NS_OK upon success, and check its return
> value in the "if" statement on line 222, and fall into the else branch upon
> failure?

Good points.  I thought about this and think that it's more than just an "I can't seek" recoverable condition. If "Content-Range" is not present for a partial response, or if it's not parse-able, then something is badly wrong with the header the server sent us. As such, I've changed the patch to throw a NetworkError to the decoder and just return, similar to other such error handling in OnStartRequest.

> @@ +233,5 @@
> […]
> 
> Should we also fail if byteRangeStart < 0?

Yes. Done.

> @@ +245,5 @@
> […]
> 
> Should we fail if byteRangeTotal is < 0?

Yes. However, I've also added the possibility for if to be '*', meaning the server doesn't know the length yet. In that case, the new ParseContentRangeHeader function sets the total to be -1, which is interpreted by the caller to mean infinite.

> @@ +918,5 @@
> […]
> 
> Your comments here are implying GetByteRangeForSeek is related to lining
> blocks up with media cache blocks, but I think you're actually using it to
> ensure that you start the download at the start of the WebM cluster? Can you
> make the comment clearer?

I made the comment clearer, but lining up with the media cache blocks is exactly what I'm doing. This will only matter on seeks, in which cases the media cache already adjusts the offset that the reader requested to line up with the media cache block. I just transferred that paradigm to DASH chunks. Note that it only matters for the first chunk to be sought. All subsequent chunks will be downloaded as normal.

It's definitely playing with the concept of DASH chunks, but I think it's fine for a first implementation. We can come back to adjusting the Media Cache later (something that looks like a not-simple-at-all job).

> @@ +942,5 @@
> […]
> 
> What should happen if we try to read an offset that is not available? What
> does happen? Can it happen?

I adjusted this too. I've changed the GetByteRangeForOffset function in nsDASHRepDecoder (in another patch) so that it returns UNAVAILABLE if the cluster offsets haven't been downloaded yet, or ILLEGAL_VALUE if the offset is not within any known range. This restricts DASH to static/on demand profiles in which there is a fixed range of offsets. Future DASH profiles which we include which may have update-able MPD manifests will require that we revisit this. But they're going to require other revisiting too.

Comment 66

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v5.0 MPD Classes

Note: I carried over the r+, but I want to land some of these at the same time. I'm thinking everything but the DASHDecoder patch. So, if you could take a look at the MPD Manager one as well, that would be great.

(In reply to Chris Pearce (:cpearce) from comment #63)
> Comment on attachment 658318 [details] [diff] [review]
> v4.0 MPD Classes
[…]
> Please use MOZ_COUNT_CTOR/DTOR in all these classes constructors and
> destructors to ensure we don't and won't leak.

Done.

> You should use nsAutoPtr<T> an nsTArray<nsAutoPtr<T> > for owning pointers
> to simplify and robustify memory management.

Done.

> r=cpearce with those changes, I don't need to see the patch again, I trust
> you to make them.

Awesome - thanks!

> ::: netwerk/dash/Makefile.in
> […]
> Use the new MPL shorter boilerplate:
> http://www.mozilla.org/MPL/headers/

Done.

> I think we need a build person to look at Makefile changes, even though they
> look pretty simpe.

OK - can you recommend someone?

> ::: netwerk/dash/mpd/AdaptationSet.h
> @@ +84,5 @@
> […]
> aValue :(

Oops. Fixed.

> @@ +97,5 @@
> […]
> I'd call this mIsBitstreamSwitchingAllowed, as the current name implies it
> is true while a switch is in progress.

Done. ("--Enabled")

> ::: netwerk/dash/mpd/BaseUrl.h
> @@ +23,5 @@
> […]
> > +class BaseUrl
> 
> It would be good to add a comment that documents why you have this class
> when nsIURI already exists.

Actually, I reviewed it again and just removed the class, and replaced it with a string, which will serve the spec's purpose very well.

> ::: netwerk/dash/mpd/MPD.h
> @@ +65,5 @@
> […]
> You might want to comment here that the MPD takes ownership of the Period
> henceforth, and will delete it.

Done.

> @@ +80,5 @@
> […]
> You could use nsTArray< nsAutoPtr<T> > instead of nsTArray<T>, and then
> you'd not need to manually free the resources in ~MPD(). If you do this you
> need to define copy constructors or all T though, since nsTArray memmoves
> when it resizes.

Done. But I don't think I need the copy constructors, since nsTArray is dealing with nsAutoPtrs and not any of the MPD classes? Right?

> ::: netwerk/dash/mpd/Period.cpp
> @@ +55,5 @@
> […]
> Perhaps you should assert that aAdaptationSet isn't already in the array.
> Same with your other Add* functions.

Done. The code only checks if the pointers are not the same, though. nsAutoPtr overrides "==" to check the pointers only and not the contents. I'd have to implement my own per-item content check. However, since BaseUrl's are now just stored in arrays of strings, their contents should be being checked. So, we should end up with an array of unique base URLs.

> ::: netwerk/dash/mpd/Period.h
> @@ +77,5 @@
> […]
> Again, you have the option of using nsTArray< nsAutoPtr<AdaptationSet> >
> here.

Yup.

> ::: netwerk/dash/mpd/Representation.cpp
> @@ +104,5 @@
> […]
> If mSegmentBase is non-null should it be deleted?

I'm not sure I got this comment, but mSegmentBase is an nsAutoPtr now anyway.

> ::: netwerk/dash/mpd/Representation.h
> @@ +61,5 @@
> […]
> Do you need to clear mBaseUrls? If you'd used nsAutoPtr this wouldn't be an
> issue... ;)

OK, OK - nsAutoPtrs are amazingly wonderful! I get it, I get it! ;)

> @@ +77,5 @@
> […]
> Trailing space.

An opportunity to play with Vim's replace using regex. Always fun.

> @@ +94,5 @@
> […]
> Use nsAutoPtr<SegmentBase> so you don't have to manage memory.

:)

> ::: netwerk/dash/mpd/SegmentBase.cpp
> @@ +94,5 @@
> […]
> Should you check if start > end, or if either are negative?

I added some checks for that here.

> ::: netwerk/dash/mpd/SegmentBase.h
> @@ +57,5 @@
> […]
> Inconsistent placement of * here. This is new code, and it's your code, so
> you can chose how you do it. If you chose to be inconsistent, that's your
> choice. ;)

Indeed. Thanks for catching that. Fixed.

> @@ +69,5 @@
> […]
> I almost feel that you should have some kind of "range" class for these.

I see what you're saying. I could use MediaByteRange from MediaResource.h, but I'd prefer not to include the whole header just for that class. Maybe it's something to add later.

Thanks again!

Comment 67

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v5.0 MPD Parser and Manager

Comment 68

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v5.0 Preparative changes to base media decoder files

(In reply to Chris Pearce (:cpearce) from comment #62)
> Comment on attachment 658323 [details] [diff] [review]
> v4.0 Preparative changes to base media decoder files
[…]
> r=cpearce with minor changes.

Awesome! I carried the r+ forward.

> ::: content/media/nsBuiltinDecoder.h
> @@ +782,5 @@
> […]
> > +  // object and encourage use of the |nsBuiltinDecoder|::|GetReentrantMonitor|
> 
> Say *why* we want to encourage not accessing the monitor directly.

Done.

> I think we should call this something else, like PrivateMonitor,
> HiddenMonitor, ProtectedMonitor, MonitorWrapper, IndirectMonitor,
> NotToBeUsedDirectlyMonitor or something else, to reduce the likelihood of
> name collisions, and to make its purpose clearer.

Done.

Thanks again!

Comment 69

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v5.0 DASH Decoder

Adds nsDASHDecoder, nsDASHReader and nsDASHRepDecoder to enable parsing of an MPD manifest file and streaming DASH media (non-adaptively as yet).

Description still in nsDASHDecoder.cpp.

Comment 70

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

Comment on attachment 659934 [details] [diff] [review]
v5.0 Add byte range download support to ChannelMediaResource

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

r=cpearce on the proviso that you add a 64 bit nsAString::ToInteger and alter your patch to use that as per below, plus the other nits here fixed too of course. ;)

::: content/media/MediaResource.cpp
@@ +232,5 @@
> +        return NS_OK;
> +      }
> +
> +      // Get Content-Range header.
> +      /*nsAutoCString rangeStr;

Remove commented out code.

@@ +250,5 @@
> +      // Parse Content-Range header.
> +      int32_t rangeStart = 0;
> +      int32_t rangeEnd = 0;
> +      int32_t rangeTotal = 0;
> +      rv = ParseContentRangeHeader(hc/*rangeStr*/, rangeStart, rangeEnd, rangeTotal);

Remove commented out code.

@@ +255,5 @@
> +      if (NS_FAILED(rv)) {
> +        // Content-Range header text should be parse-able.
> +        mDecoder->NetworkError();
> +        CloseChannel();
> +        return NS_OK;

Remove trailing white space.

@@ +364,5 @@
>  }
>  
>  nsresult
> +ChannelMediaResource::ParseContentRangeHeader(nsIHttpChannel * aHttpChan,
> +                                              int32_t& aRangeStart,

These int32_t's need to be int64_t's, otherwise you can't handle ranges after 2GB. Sorry, I should have picked this up last time.

I think you should add a 64bit version of ToInteger() to nsAString (I think the implementation would belong in nsStringAPI.cpp?) and use that here. You should do that work in a separate bug; file a bug in the xpcom component, and bsmedberg is probably the person to review that patch.

Thankfully AppendInt() already has a 64bit overload.

@@ +372,5 @@
> +  NS_ENSURE_ARG(aHttpChan);
> +
> +  nsAutoCString rangeStr;
> +  nsresult rv = aHttpChan->GetResponseHeader(NS_LITERAL_CSTRING("Content-Range"), rangeStr);
> +  if (rv == NS_ERROR_NOT_AVAILABLE

We'd usually put the "||" at the end of the end rather than at the start of the following line.

@@ +377,5 @@
> +      || (NS_SUCCEEDED(rv) && rangeStr.IsEmpty())) {
> +    // Content-Range should be present for an HTTP_PARTIAL_RESPONSE_CODE.
> +    CMLOG("Error! HTTP_PARTIAL_RESPONSE_CODE received but server did not "
> +          "specify a content range! Channel[%p]", aHttpChan);
> +    mDecoder->NetworkError();

You don't do this cleanup on the other failure paths here, but you are doing this cleanup in the caller when ParseContentRangeHeader fails. I think you should do this cleanup only in the caller; then you don't need to repeat the cleanup code here, or in the other failure paths here either.

@@ +390,5 @@
> +  int32_t dashPos = rangeStr.Find(NS_LITERAL_CSTRING("-"), true, spacePos);
> +  int32_t slashPos = rangeStr.Find(NS_LITERAL_CSTRING("/"), true, dashPos);
> +
> +  nsAutoCString aRangeStartText;
> +  //nsresult rv;

Remove commented code.

::: content/media/nsMediaDecoder.h
@@ +75,5 @@
>    // Seek to the time position in (seconds) from the start of the video.
>    virtual nsresult Seek(double aTime) = 0;
>  
> +  // Enables decoders to supply an enclosing byte range for a seek offset.
> +  // E.g. used by ChannelMediaResource to download whole cluster for DASH-WebM.

s/download whole/download a whole/

Comment 71

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

Comment on attachment 659940 [details] [diff] [review]
v5.0 DASH Decoder

Requesting additional review from Ted for the changes to configure.in and a few Makefiles.

Comment 72

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

Comment on attachment 659940 [details] [diff] [review]
v5.0 DASH Decoder

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

The base structure is fine, most of my comments are threading related. Unfortunately our threading model is not very pure WRT which objects access stuff on which threads, and you need to be very careful about what thread reads and writes what data, and ensure that it's synchronized between threads.

::: content/media/dash/nsDASHDecoder.cpp
@@ +145,5 @@
> +  mPrincipal(nullptr),
> +  mDASHReader(nullptr),
> +  mAudioRepDecoder(nullptr),
> +  mVideoRepDecoder(nullptr)
> +{}

MOZ_COUNT_{C,D}TOR, in all these classes.

@@ +160,5 @@
> +
> +void
> +nsDASHDecoder::ReleaseStateMachine()
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Must be on main thread."); 

There's trailing space here, and later on in a few places in this file.

@@ +240,5 @@
> +
> +void
> +nsDASHDecoder::ReadMPDBuffer()
> +{
> +  NS_ASSERTION(!NS_IsMainThread(), "Should not be on main thread.");

In fact you could assert that you're on the reader thread, but if you did that you'd need to synchronize mMPDReaderThread using a monitor.

@@ +245,5 @@
> +
> +  LOG("Started reading from the MPD buffer.");
> + 
> +  // Don't |ReadMPDBuffer| if we are shutting down.
> +  if (mShuttingDown) {

nsMediaDecoder::mShuttingDown is read/write from the main thread only, you need to find a thread-safe way to communicate to your reader thread that we're shutting down. Maybe another bool synchronized by the decoder monitor.

@@ +256,5 @@
> +    DecodeError();
> +    return;
> +  }
> +
> +  char* buffer = new char[length];

nsAutoArrayPtr<char> buffer ...

Otherwise, you'll leak buffer when you exit on the error paths.

@@ +267,5 @@
> +    DecodeError();
> +    return;
> +  }
> +  // Store buffer and length for processing on main thread.
> +  mBuffer = buffer;

Once you make buffer an autoptr, you need to call forget() to ensure its memory isn't freed when it goes out of scope, i.e.:

mBuffer = buffer.forget();

@@ +293,5 @@
> +  if (!mMPDReaderThread) {
> +    LOG("Error: MPD reader thread does not exist!");
> +    DecodeError();
> +    return;
> +  }

You should bail out here if mIsShuttingDown; the media element can call Shutdown() at any time (based on what the user/script does), and if it calls Shutdown we don't want to be continuing the load.

@@ +294,5 @@
> +    LOG("Error: MPD reader thread does not exist!");
> +    DecodeError();
> +    return;
> +  }
> +  nsresult rv = mMPDReaderThread ? mMPDReaderThread->Shutdown()

If you get here, mMPDReaderThread is non-null because of the preceding if statement. Right?

@@ +626,5 @@
> +  }
> +
> +  if (NS_SUCCEEDED(aStatus)) {
> +    // Hold monitor until stream is switched.
> +    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());

Um... Is m{Audio,Video}Decoder synchronized by the decoder monitor? You're not protecting it elsewhere with the decoder monitor.

@@ +657,5 @@
> +}
> +
> +void
> +nsDASHDecoder::LoadAborted()
> +{

What thread does this run on? The main thread?

::: content/media/dash/nsDASHDecoder.h
@@ +116,5 @@
> +  // True when media element has already been notified of an aborted load.
> +  bool mNotifiedLoadAborted;
> + 
> +  // Ptr for the MPD data.
> +  nsAutoPtr<char>              mBuffer;

nsAutoArrayPtr<char>

::: content/media/dash/nsDASHReader.cpp
@@ +99,5 @@
> +
> +  // Wait for MPD to be parsed and child readers created.
> +  {
> +    ReentrantMonitorAutoEnter mon(mMetadataReadyMonitor);
> +    if (!mReadyToReadMetadata) {

Probably that should be a "while" statement, not an "if" statement. And what happens if the decoder is shut down before this wait finishes?

@@ +154,5 @@
> +                          int64_t aStartTime)
> +{
> +  NS_ENSURE_ARG(aBuffered);
> +
> +  MediaResource* resource = nullptr;

This can be called on the main, decode, and state machine threads.

m{Audio,Video}Reader is set on the main thread (I think?), but read here on these other threads, so you need to synchronize access to it. Probably the by the decoder monitor. If the idea is to swap readers on the fly then I assume you'll need to synchronize access to m*Reader anyway.

For methods that require synchronization, you may either take the monitor directly in the function, or take the monitor in the caller. If you're taking the monitor in the caller, you should assert that the function holds the monitor and document in its header file that it must hold the monitor.

@@ +225,5 @@
> +  } else if (mAudioReader) {
> +    *aBuffered = audioBuffered;
> +  } else if (mVideoReader) {
> +    *aBuffered = videoBuffered;
> +  } else

We always put braces around the bodies of if statements in media code, even if they're single line, except for trivial error returns.

::: content/media/dash/nsDASHReader.h
@@ +91,5 @@
> +  VideoData* FindStartTime(int64_t& aOutStartTime);
> +
> +  // Call by state machine on multiple threads.
> +  bool IsSeekableInBufferedRanges() {
> +    return (mVideoReader ? mVideoReader->IsSeekableInBufferedRanges() : false)

With the current logic if you don't have a video reader but you do have an audio reader whose IsSeekableInBufferedRanges() returns true, this function will always return false.

This wouldn't matter for WebM (as nsWebMReader always returns false in IsSeekableInBufferedRanges()) and I assume a future H.264 implementation would do the same (since I assume seeking in an H.264 file would only work if the file contained an index) but someday it may matter, and we should leave the decision up to the sub reader anyway.

Or perhaps that should be:
  return !((mVideoReader && !mVideoReader->IsSeekableInBufferedRanges()) ||
           (mAudioReader && !mAudioReader->IsSeekableInBufferedRanges())

This prevents that problem, and won't return true if !mVideoReader && !mAudioReader.

::: content/media/dash/nsDASHRepDecoder.cpp
@@ +161,5 @@
> +    NS_NewRunnableMethod(this, &nsDASHRepDecoder::LoadNextByteRange);
> +  nsresult rv = NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
> +  if (NS_FAILED(rv)) {
> +    LOG("Error dispatching parse event to main thread: rv[%x]");
> +    DecodeError();

DecodeError() should be called only on the main thread. Dispatch a runnable to call it.

::: content/media/dash/nsDASHRepDecoder.h
@@ +81,5 @@
> +  // Called from the main thread to set the duration of the media resource
> +  // if it is able to be obtained via HTTP headers. Called from the
> +  // state machine thread to set the duration if it is obtained from the
> +  // media metadata. The decoder monitor must be obtained before calling this.
> +  // aDuration is in microseconds.

aDuration is in seconds I think?

Durations should only be passed around as microseconds as int64_t. A duration as a double is usually a value in seconds.

One of these days I'm going to add a "typedef usecs_t int64_t;" somewhere to enforce this...

@@ +171,5 @@
> +  MediaByteRange  mCurrentByteRange;
> +  // Index of the current byte range.
> +  uint64_t        mSubsegmentIdx;
> +
> +  // Ptr to the reader object for this |Representation|.

Is this an owning pointer? Document here who owns and is responsible for deleting the reader.

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +478,5 @@
>    { AUDIO_OGG, "oga", "Ogg Audio" },
>    { AUDIO_OGG, "opus", "Opus Audio" },
>    { VIDEO_WEBM, "webm", "Web Media Video" },
>    { AUDIO_WEBM, "webm", "Web Media Audio" },
> +#ifdef MOZ_DASH

Doesn't look like you need to ifdef this out here; we're not doing so when other media types are disabled.

Comment 73

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

Comment on attachment 659940 [details] [diff] [review]
v5.0 DASH Decoder

Oops! I'm not sure how I added the extra review for Kyle here; I'd already asked Ted to look at the build system changes. Of course you're welcome to look at them too Kyle!

Comment 74

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

Comment on attachment 659937 [details] [diff] [review]
v5.0 MPD Parser and Manager

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

MOZ_COUNT_{C,D}TOR please.

r=cpearce with minor changes.

Is there an upstream repo from maintained by the guys at Klagenfurt, or are we effectively the upstream repo? I'm just wondering if I'm effectively asking you to diverge from upstream.

::: netwerk/dash/mpd/IMPDManager.h
@@ +77,5 @@
> +  // XXX Future versions may have multiple tracks for audio.
> +  virtual uint32_t const GetNumAdaptationSets() const = 0;
> +
> +  // Returns the media type for the given |AdaptationSet|, audio/video.
> +  virtual AdaptationSetType const

Again doesn't make much sense to return a const value type, since you're returning a copy. The only advantage I can see it that the compiler won't let you (I think) do GetAdaptaionSetType(n) = 4, but you can do (x=GetAdaptationSetType(n))=4.

Up to you if you want to do this.

@@ +97,5 @@
> +  virtual nsresult GetFirstSegmentUrl(uint32_t const aAdaptSetIdx,
> +                                      uint32_t const aRepIdx,
> +                                      nsAString &aUrl) const = 0;
> +
> +  // Returns the start time of the presentation.

Document time units, and in GetDuration() below too.

::: netwerk/dash/mpd/nsDASHMPDParser.h
@@ +38,5 @@
> +class nsDASHMPDParser
> +{
> +public:
> +  // Constructor takes pointer to MPD byte stream and length, as well as the
> +  // current principal and URI from which the MPD download took place.

Document assumption regarding lifecycle of aMPDData; i.e. you assume it will be valid for the lifetime of this object?

@@ +59,5 @@
> +  // Debug: Prints a single DOM element.
> +  void      PrintDOMElement(nsIDOMElement* aElem, int32_t aOffset);
> +
> +  // Pointer to the MPD byte stream.
> +  char*                   mData;

Maybe this should be make this "const char* const mData;", since presumably mData isn't changed by the parser?

::: netwerk/dash/mpd/nsDASHWebMODManager.cpp
@@ +73,5 @@
> +                                        uint32_t const aRepIdx,
> +                                        nsAString &aUrl) const
> +{
> +  // Append base URL for MPD.
> +  if (mMpd->HasBaseUrls()) { // && mMpd->GetBaseUrl(0)) {

Remove commented out code please.

@@ +87,5 @@
> +                 NS_ERROR_ILLEGAL_VALUE);
> +  Representation const * rep = adaptSet->GetRepresentation(aRepIdx);
> +  NS_ENSURE_TRUE(rep, NS_ERROR_NULL_POINTER);
> +
> +  if (rep->HasBaseUrls()) { // && rep->GetBaseUrl(0)) {

Remove commented out code please.

@@ +121,5 @@
> +nsDASHWebMODManager::GetAdaptationSetType(nsAString const & aMimeType) const
> +{
> +  NS_ENSURE_TRUE(!aMimeType.IsEmpty(), DASH_ASTYPE_INVALID);
> +
> +  if (aMimeType == NS_LITERAL_STRING("video/webm")) {

Use {AUDIO,VIDEO}_WEBM from nsMimeTypes.h.

::: netwerk/dash/mpd/nsDASHWebMODParser.cpp
@@ +80,5 @@
> +  rv = SetPeriods();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // release root node.
> +  mRoot = nullptr;

Given that mRoot is an nsCOMPtr, do you need to release it here? It will be released when this object is destroyed. However if for some reason you do need to release it, surely you should also release it on the failure paths? ;)

@@ +125,5 @@
> +      nsAutoString baseUrlStr;
> +      rv = child->GetTextContent(baseUrlStr);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      //BaseUrl* url = new BaseUrl(baseUrlStr);

Remove commented out code please.

@@ +163,5 @@
> +      rv = GetAttribute(child, NS_LITERAL_STRING("start"), value);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      if (!value.IsEmpty()) {
> +        nsAutoString start;
> +        // Trim "PT" from start and "S" from end

I think you should search for PT and S and remove them if they exist, rather than assuming they're present. Then we'd gracefully handle the case where PT and S are missing; even though I assume that would be an invalid period attribute value, it would be better to not truncate the value in the case where PT/S isn't present.

Alternatively you could explicitly abort parsing if the search for PT or S fails to find them.

Ditto for duration below.

@@ +179,5 @@
> +        rv = value.Mid(duration, 2, (value.Length()-3));
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        period->SetDuration(PR_strtod(NS_ConvertUTF16toUTF8(duration).get(),
> +                                      nullptr));
> +        NS_ENSURE_SUCCESS(rv, rv);

You're checking rv here, but it's not changed since the last time it's been checked.

It would be nice if you could not set the duration if PR_strtod failed. Maybe you could use nsAString::ToDouble() instead?

@@ +222,5 @@
> +    if (!bAttributesValid)
> +      LOG("mimeType not present!");
> +  }
> +  // Validate attributes for video.
> +  if (bAttributesValid && mimeType.EqualsLiteral("video/webm")) {

VIDEO_WEBM

@@ +247,5 @@
> +      bAttributesValid = (!value.IsEmpty() && value.EqualsLiteral("true"));
> +      if (!bAttributesValid)
> +        LOG("bitstreamSwitching not present or invalid!");
> +    }
> +  } else if (bAttributesValid && mimeType.EqualsLiteral("audio/webm")) {

AUDIO_WEBM

@@ +278,5 @@
> +#endif
> +  while (child) {
> +    nsAutoString tagName;
> +    rv = child->GetTagName(tagName);
> +    NS_ENSURE_SUCCESS(rv, rv);

I assume you don't need to set bIgnoreThisPeriod=true on the error return paths here because the caller will abort?

@@ +440,5 @@
> +      nsAutoString baseUrlStr;
> +      rv = child->GetTextContent(baseUrlStr);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      //BaseUrl* url = new BaseUrl(baseUrlStr);

Remove commented out code please.

::: netwerk/dash/mpd/nsDASHWebMODParser.h
@@ +149,5 @@
> +  // The root DOM element of the MPD; populated after reading the XML file; used
> +  // to populate the MPD class structure.
> +  nsCOMPtr<nsIDOMElement>  mRoot;
> +
> +  // The root class of the MPD class structure; populated after parsing the DOM.

Comment who owns and is responsible for deleting *mMPD.

Comment 75

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 659936 [details] [diff] [review]
v5.0 MPD Classes

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

Comment 76

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 659937 [details] [diff] [review]
v5.0 MPD Parser and Manager

https://hg.mozilla.org/integration/mozilla-inbound/rev/75fbed6e0eeb

Comment 77

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 659938 [details] [diff] [review]
v5.0 Preparative changes to base media decoder files

https://hg.mozilla.org/integration/mozilla-inbound/rev/89085b91ea2c

Comment 78

[Steve Workman [:sworkman] (INACTIVE)]

Try job for patches landed as per comment 75, comment 76 and comment 77.
https://tbpl.mozilla.org/?tree=Try&rev=89ac34c1d14b

Comment 79

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/cb424c9fb4a6
https://hg.mozilla.org/mozilla-central/rev/75fbed6e0eeb
https://hg.mozilla.org/mozilla-central/rev/89085b91ea2c

Comment 80

[(not currently active) Ted Mielczarek]

Comment on attachment 659940 [details] [diff] [review]
v5.0 DASH Decoder

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

::: configure.in
@@ +5247,5 @@
> +[  --enable-dash          Enable support for DASH-WebM],
> +    MOZ_DASH=1,
> +    MOZ_DASH=)
> +
> +if test -n "$MOZ_DASH" -a -n "$MOZ_WEBM"; then

Maybe you want to explicitly error here if someone did --disable-webm --enable-dash?

Also, is this going to become default-on at some point? I hate accruing new configure options.

::: content/media/dash/Makefile.in
@@ +7,5 @@
> +#
> +# Contributor(s):
> +#     Steve Workman <[e-mail]
> +
> +DEPTH      = ../../..

DEPTH = @DEPTH@

Also, please don't use any tabs in Makefiles except where explicitly required.

@@ +16,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MODULE         = content
> +LIBRARY_NAME   = gkcondash_s
> +LIBXUL_LIBRARY = 1

nit: for these and other assignments where you are doing immediate assignment, use :=

@@ +23,5 @@
> +EXPORTS += \
> +   nsDASHDecoder.h \
> +   nsDASHRepDecoder.h \
> +   nsDASHReader.h \
> +   $(NULL)

nit: two-space indentation on continuations.

@@ +43,5 @@
> +INCLUDES += \
> +   -I$(srcdir)/../webm \
> +   -I$(srcdir)/../../base/src \
> +   -I$(srcdir)/../../html/content/src \
> +   $(MOZ_LIBVPX_INCLUDES) \

Is there a reason these are in INCLUDES and not LOCAL_INCLUDES?

::: content/media/webm/Makefile.in
@@ +35,5 @@
> +
> +ifdef MOZ_DASH
> +INCLUDES += \
> +   -I$(srcdir)/../dash \
> +   $(NULL)

LOCAL_INCLUDES, two-space indent.

::: layout/build/Makefile.in
@@ +177,5 @@
>  
> +ifdef MOZ_DASH
> +SHARED_LIBRARY_LIBS += \
> +   $(DEPTH)/content/media/dash/$(LIB_PREFIX)gkcondash_s.$(LIB_SUFFIX) \
> +   $(NULL)

two-space indent

Comment 81

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Chris Pearce (:cpearce) from comment #70)
> Comment on attachment 659934 [details] [diff] [review]
> v5.0 Add byte range download support to ChannelMediaResource
> 
> Review of attachment 659934 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=cpearce on the proviso that you add a 64 bit nsAString::ToInteger and
> alter your patch to use that as per below, plus the other nits here fixed
> too of course. ;)

Thanks for the r+! ToInteger64 already landed and used in new patch.
 
> ::: content/media/MediaResource.cpp
> @@ +232,5 @@
> > +        return NS_OK;
> > +      }
> > +
> > +      // Get Content-Range header.
> > +      /*nsAutoCString rangeStr;
> 
> Remove commented out code.

Done.

> @@ +250,5 @@
> > +      // Parse Content-Range header.
> > +      int32_t rangeStart = 0;
> > +      int32_t rangeEnd = 0;
> > +      int32_t rangeTotal = 0;
> > +      rv = ParseContentRangeHeader(hc/*rangeStr*/, rangeStart, rangeEnd, rangeTotal);
> 
> Remove commented out code.

Done.

> @@ +255,5 @@
> > +      if (NS_FAILED(rv)) {
> > +        // Content-Range header text should be parse-able.
> > +        mDecoder->NetworkError();
> > +        CloseChannel();
> > +        return NS_OK;
> 
> Remove trailing white space.

Done.

> @@ +364,5 @@
> >  }
> >  
> >  nsresult
> > +ChannelMediaResource::ParseContentRangeHeader(nsIHttpChannel * aHttpChan,
> > +                                              int32_t& aRangeStart,
> 
> These int32_t's need to be int64_t's, otherwise you can't handle ranges
> after 2GB. Sorry, I should have picked this up last time.
> 
> I think you should add a 64bit version of ToInteger() to nsAString (I think
> the implementation would belong in nsStringAPI.cpp?) and use that here. You
> should do that work in a separate bug; file a bug in the xpcom component,
> and bsmedberg is probably the person to review that patch.
> 
> Thankfully AppendInt() already has a 64bit overload.

Done, landed.

> @@ +372,5 @@
> > +  NS_ENSURE_ARG(aHttpChan);
> > +
> > +  nsAutoCString rangeStr;
> > +  nsresult rv = aHttpChan->GetResponseHeader(NS_LITERAL_CSTRING("Content-Range"), rangeStr);
> > +  if (rv == NS_ERROR_NOT_AVAILABLE
> 
> We'd usually put the "||" at the end of the end rather than at the start of
> the following line.

OK.

> @@ +377,5 @@
> > +      || (NS_SUCCEEDED(rv) && rangeStr.IsEmpty())) {
> > +    // Content-Range should be present for an HTTP_PARTIAL_RESPONSE_CODE.
> > +    CMLOG("Error! HTTP_PARTIAL_RESPONSE_CODE received but server did not "
> > +          "specify a content range! Channel[%p]", aHttpChan);
> > +    mDecoder->NetworkError();
> 
> You don't do this cleanup on the other failure paths here, but you are doing
> this cleanup in the caller when ParseContentRangeHeader fails. I think you
> should do this cleanup only in the caller; then you don't need to repeat the
> cleanup code here, or in the other failure paths here either.

Done.

> @@ +390,5 @@
> > +  int32_t dashPos = rangeStr.Find(NS_LITERAL_CSTRING("-"), true, spacePos);
> > +  int32_t slashPos = rangeStr.Find(NS_LITERAL_CSTRING("/"), true, dashPos);
> > +
> > +  nsAutoCString aRangeStartText;
> > +  //nsresult rv;
> 
> Remove commented code.

Done.

> ::: content/media/nsMediaDecoder.h
> @@ +75,5 @@
> >    // Seek to the time position in (seconds) from the start of the video.
> >    virtual nsresult Seek(double aTime) = 0;
> >  
> > +  // Enables decoders to supply an enclosing byte range for a seek offset.
> > +  // E.g. used by ChannelMediaResource to download whole cluster for DASH-WebM.
> 
> s/download whole/download a whole/

Done.

Comment 82

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Chris Pearce (:cpearce) from comment #74)
> Comment on attachment 659937 [details] [diff] [review]
> v5.0 MPD Parser and Manager
> 
> Review of attachment 659937 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review - I meant tot upload the new patch earlier, sorry. It's now been landed, but with your changes.

> MOZ_COUNT_{C,D}TOR please.
> 
> r=cpearce with minor changes.
> 
> Is there an upstream repo from maintained by the guys at Klagenfurt, or are
> we effectively the upstream repo? I'm just wondering if I'm effectively
> asking you to diverge from upstream.

Moz code is effectively a fork from their code, so this should be fine.

> ::: netwerk/dash/mpd/IMPDManager.h
> @@ +77,5 @@
> > +  // XXX Future versions may have multiple tracks for audio.
> > +  virtual uint32_t const GetNumAdaptationSets() const = 0;
> > +
> > +  // Returns the media type for the given |AdaptationSet|, audio/video.
> > +  virtual AdaptationSetType const
> 
> Again doesn't make much sense to return a const value type, since you're
> returning a copy. The only advantage I can see it that the compiler won't
> let you (I think) do GetAdaptaionSetType(n) = 4, but you can do
> (x=GetAdaptationSetType(n))=4.
> 
> Up to you if you want to do this.

I was const happy. Removed.

> @@ +97,5 @@
> > +  virtual nsresult GetFirstSegmentUrl(uint32_t const aAdaptSetIdx,
> > +                                      uint32_t const aRepIdx,
> > +                                      nsAString &aUrl) const = 0;
> > +
> > +  // Returns the start time of the presentation.
> 
> Document time units, and in GetDuration() below too.

Done.

> ::: netwerk/dash/mpd/nsDASHMPDParser.h
> @@ +38,5 @@
> > +class nsDASHMPDParser
> > +{
> > +public:
> > +  // Constructor takes pointer to MPD byte stream and length, as well as the
> > +  // current principal and URI from which the MPD download took place.
> 
> Document assumption regarding lifecycle of aMPDData; i.e. you assume it will
> be valid for the lifetime of this object?

Done.

> @@ +59,5 @@
> > +  // Debug: Prints a single DOM element.
> > +  void      PrintDOMElement(nsIDOMElement* aElem, int32_t aOffset);
> > +
> > +  // Pointer to the MPD byte stream.
> > +  char*                   mData;
> 
> Maybe this should be make this "const char* const mData;", since presumably
> mData isn't changed by the parser?

Done, but actually changed to "nsAutoPtr<char const>  mData"

> ::: netwerk/dash/mpd/nsDASHWebMODManager.cpp
> @@ +73,5 @@
> > +                                        uint32_t const aRepIdx,
> > +                                        nsAString &aUrl) const
> > +{
> > +  // Append base URL for MPD.
> > +  if (mMpd->HasBaseUrls()) { // && mMpd->GetBaseUrl(0)) {
> 
> Remove commented out code please.

Oops. Done.

> @@ +87,5 @@
> > +                 NS_ERROR_ILLEGAL_VALUE);
> > +  Representation const * rep = adaptSet->GetRepresentation(aRepIdx);
> > +  NS_ENSURE_TRUE(rep, NS_ERROR_NULL_POINTER);
> > +
> > +  if (rep->HasBaseUrls()) { // && rep->GetBaseUrl(0)) {
> 
> Remove commented out code please.

Double oops. Done.

> @@ +121,5 @@
> > +nsDASHWebMODManager::GetAdaptationSetType(nsAString const & aMimeType) const
> > +{
> > +  NS_ENSURE_TRUE(!aMimeType.IsEmpty(), DASH_ASTYPE_INVALID);
> > +
> > +  if (aMimeType == NS_LITERAL_STRING("video/webm")) {
> 
> Use {AUDIO,VIDEO}_WEBM from nsMimeTypes.h.

Done.

> ::: netwerk/dash/mpd/nsDASHWebMODParser.cpp
> @@ +80,5 @@
> > +  rv = SetPeriods();
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  // release root node.
> > +  mRoot = nullptr;
> 
> Given that mRoot is an nsCOMPtr, do you need to release it here? It will be
> released when this object is destroyed. However if for some reason you do
> need to release it, surely you should also release it on the failure paths?
> ;)

Removed mRoot - I didn't need to store it for the lifetime of the object. It just needs parsed and then it's no longer used.

> @@ +125,5 @@
> > +      nsAutoString baseUrlStr;
> > +      rv = child->GetTextContent(baseUrlStr);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +      //BaseUrl* url = new BaseUrl(baseUrlStr);
> 
> Remove commented out code please.

Oh boy. Done.

> @@ +163,5 @@
> > +      rv = GetAttribute(child, NS_LITERAL_STRING("start"), value);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +      if (!value.IsEmpty()) {
> > +        nsAutoString start;
> > +        // Trim "PT" from start and "S" from end
> 
> I think you should search for PT and S and remove them if they exist, rather
> than assuming they're present. Then we'd gracefully handle the case where PT
> and S are missing; even though I assume that would be an invalid period
> attribute value, it would be better to not truncate the value in the case
> where PT/S isn't present.
> 
> Alternatively you could explicitly abort parsing if the search for PT or S
> fails to find them.
> 
> Ditto for duration below.

Done.

> @@ +179,5 @@
> > +        rv = value.Mid(duration, 2, (value.Length()-3));
> > +        NS_ENSURE_SUCCESS(rv, rv);
> > +        period->SetDuration(PR_strtod(NS_ConvertUTF16toUTF8(duration).get(),
> > +                                      nullptr));
> > +        NS_ENSURE_SUCCESS(rv, rv);
> 
> You're checking rv here, but it's not changed since the last time it's been
> checked.
> 
> It would be nice if you could not set the duration if PR_strtod failed.
> Maybe you could use nsAString::ToDouble() instead?

Done.

> @@ +222,5 @@
> > +    if (!bAttributesValid)
> > +      LOG("mimeType not present!");
> > +  }
> > +  // Validate attributes for video.
> > +  if (bAttributesValid && mimeType.EqualsLiteral("video/webm")) {
> 
> VIDEO_WEBM

Done.

> @@ +247,5 @@
> > +      bAttributesValid = (!value.IsEmpty() && value.EqualsLiteral("true"));
> > +      if (!bAttributesValid)
> > +        LOG("bitstreamSwitching not present or invalid!");
> > +    }
> > +  } else if (bAttributesValid && mimeType.EqualsLiteral("audio/webm")) {
> 
> AUDIO_WEBM

Done.
 
> @@ +278,5 @@
> > +#endif
> > +  while (child) {
> > +    nsAutoString tagName;
> > +    rv = child->GetTagName(tagName);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> 
> I assume you don't need to set bIgnoreThisPeriod=true on the error return
> paths here because the caller will abort?

Right.

> @@ +440,5 @@
> > +      nsAutoString baseUrlStr;
> > +      rv = child->GetTextContent(baseUrlStr);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +      //BaseUrl* url = new BaseUrl(baseUrlStr);
> 
> Remove commented out code please.

Gone.

> ::: netwerk/dash/mpd/nsDASHWebMODParser.h
> @@ +149,5 @@
> > +  // The root DOM element of the MPD; populated after reading the XML file; used
> > +  // to populate the MPD class structure.
> > +  nsCOMPtr<nsIDOMElement>  mRoot;
> > +
> > +  // The root class of the MPD class structure; populated after parsing the DOM.
> 
> Comment who owns and is responsible for deleting *mMPD.

Done by way of making this an nsAutoPtr.

Comment 83

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Chris Pearce (:cpearce) from comment #72)
> Comment on attachment 659940 [details] [diff] [review]
> v5.0 DASH Decoder
> 
> Review of attachment 659940 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review! New patch uploaded!


> ::: content/media/dash/nsDASHDecoder.cpp
> @@ +145,5 @@
> MOZ_COUNT_{C,D}TOR, in all these classes.

Done - thought I had done all of these when I did the other patches.

> @@ +160,5 @@
> There's trailing space here, and later on in a few places in this file.

Done - same as last comment. I must have added some in the recent changes.

> @@ +240,5 @@
> > +nsDASHDecoder::ReadMPDBuffer()
> > +{
> > +  NS_ASSERTION(!NS_IsMainThread(), "Should not be on main thread.");
> 
> In fact you could assert that you're on the reader thread, but if you did
> that you'd need to synchronize mMPDReaderThread using a monitor.

Since this is a private function, I think it's ok to leave it as is. The most important thing is that it's some IO that's definitely off the main thread anyway. I can change it if you really think it's necessary.

> @@ +245,5 @@
> > +  // Don't |ReadMPDBuffer| if we are shutting down.
> > +  if (mShuttingDown) {
> 
> nsMediaDecoder::mShuttingDown is read/write from the main thread only, you
> need to find a thread-safe way to communicate to your reader thread that
> we're shutting down. Maybe another bool synchronized by the decoder monitor.

Good catch. I added nsDASHReader::NotifyDecoderShuttingDown with a monitor around a private bool. I also did a NotifyAll on nsDASHReader's ReadMetadata monitor, so that if it's waiting when a Shutdown happens, the function will just return. AND, I added a dereference for nsDASHReader in nsBuiltinDecoder::ReleaseStateMachine, since the state machine owns the reader, and the reference in nsDASHDecoder could be stale. So, you might want to check out this new code because of the new complexity.

> @@ +256,5 @@
> > +  char* buffer = new char[length];
> 
> nsAutoArrayPtr<char> buffer ...
> 
> Otherwise, you'll leak buffer when you exit on the error paths.

Done.

> @@ +267,5 @@
> > +  mBuffer = buffer;
> 
> Once you make buffer an autoptr, you need to call forget() to ensure its
> memory isn't freed when it goes out of scope, i.e.:
> 
> mBuffer = buffer.forget();

Yup, done.

> @@ +293,5 @@
> > +  if (!mMPDReaderThread) {
> > +    LOG("Error: MPD reader thread does not exist!");
> > +    DecodeError();
> > +    return;
> > +  }
> 
> You should bail out here if mIsShuttingDown; the media element can call
> Shutdown() at any time (based on what the user/script does), and if it calls
> Shutdown we don't want to be continuing the load.

Done.

> @@ +294,5 @@
> > +  nsresult rv = mMPDReaderThread ? mMPDReaderThread->Shutdown()
> 
> If you get here, mMPDReaderThread is non-null because of the preceding if
> statement. Right?

D'oh. Yup. Fixed.

> @@ +626,5 @@
> > +  }
> > +
> > +  if (NS_SUCCEEDED(aStatus)) {
> > +    // Hold monitor until stream is switched.
> > +    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> 
> Um... Is m{Audio,Video}Decoder synchronized by the decoder monitor? You're
> not protecting it elsewhere with the decoder monitor.

Ah, this was a vestige of earlier playing around with monitors in prep for stream switching. I had decided not to use monitors until stream switching was added, but since these comments I've changed my mind on that. Currently, it really shouldn't matter - sub readers are set during init and never changed. With stream switching, of course, it does matter.

So, I propose that whatever stream switching function is created, adds a switch request to the state machine. Then at an appropriate time in the decode loop, the switch can happen. In this way, it's all nicely serialized in the decode thread, and the decode monitor need only be used when the actual switch happens and in any functions which access the readers outside the decode thread. I did it this way because I was having deadlock issues just locking every access to the sub reader pointers.

Note: there is no switching function yet, so this is all preparation. Worst case, it can be rewritten when those patches are ready.

> @@ +657,5 @@
> > +nsDASHDecoder::LoadAborted()
> 
> What thread does this run on? The main thread?

Comment and thread assertion added.

> ::: content/media/dash/nsDASHDecoder.h
> > +  // Ptr for the MPD data.
> > +  nsAutoPtr<char>              mBuffer;
> 
> nsAutoArrayPtr<char>

Done.

> ::: content/media/dash/nsDASHReader.cpp
> @@ +99,5 @@
> > +    ReentrantMonitorAutoEnter mon(mMetadataReadyMonitor);
> > +    if (!mReadyToReadMetadata) {
> 
> Probably that should be a "while" statement, not an "if" statement. And what
> happens if the decoder is shut down before this wait finishes?

Done, see earlier for shutdown notification.

> @@ +154,5 @@
> > +                          int64_t aStartTime)
> > +{
> > +  NS_ENSURE_ARG(aBuffered);
> > +
> > +  MediaResource* resource = nullptr;
> 
> This can be called on the main, decode, and state machine threads.
> 
> m{Audio,Video}Reader is set on the main thread (I think?), but read here on
> these other threads, so you need to synchronize access to it. Probably the
> by the decoder monitor. If the idea is to swap readers on the fly then I
> assume you'll need to synchronize access to m*Reader anyway.
> 
> For methods that require synchronization, you may either take the monitor
> directly in the function, or take the monitor in the caller. If you're
> taking the monitor in the caller, you should assert that the function holds
> the monitor and document in its header file that it must hold the monitor.

See earlier for sub reader protection.

> @@ +225,5 @@
> > +  } else if (mAudioReader) {
> > +    *aBuffered = audioBuffered;
> > +  } else if (mVideoReader) {
> > +    *aBuffered = videoBuffered;
> > +  } else
> 
> We always put braces around the bodies of if statements in media code, even
> if they're single line, except for trivial error returns.

Yup - I missed that one while fixing all the if statements.

> ::: content/media/dash/nsDASHReader.h
> @@ +91,5 @@
> > +  VideoData* FindStartTime(int64_t& aOutStartTime);
> > +
> > +  // Call by state machine on multiple threads.
> > +  bool IsSeekableInBufferedRanges() {
> > +    return (mVideoReader ? mVideoReader->IsSeekableInBufferedRanges() : false)
> 
> With the current logic if you don't have a video reader but you do have an
> audio reader whose IsSeekableInBufferedRanges() returns true, this function
> will always return false.
> 
> This wouldn't matter for WebM (as nsWebMReader always returns false in
> IsSeekableInBufferedRanges()) and I assume a future H.264 implementation
> would do the same (since I assume seeking in an H.264 file would only work
> if the file contained an index) but someday it may matter, and we should
> leave the decision up to the sub reader anyway.
> 
> Or perhaps that should be:
>   return !((mVideoReader && !mVideoReader->IsSeekableInBufferedRanges()) ||
>            (mAudioReader && !mAudioReader->IsSeekableInBufferedRanges())
> 
> This prevents that problem, and won't return true if !mVideoReader &&
> !mAudioReader.

Good one. I added (mVideoReader || mAudioReader) & … because it looks to me like if both readers are nullptr, then it would return true.

> ::: content/media/dash/nsDASHRepDecoder.cpp
> > +    DecodeError();
> 
> DecodeError() should be called only on the main thread. Dispatch a runnable
> to call it.

DecodeError is now overridden in nsDASH{Rep}Decoder to dispatch a runnable if it's off the main thread.

> ::: content/media/dash/nsDASHRepDecoder.h
> @@ +81,5 @@
> > +  // Called from the main thread to set the duration of the media resource
> > +  // if it is able to be obtained via HTTP headers. Called from the
> > +  // state machine thread to set the duration if it is obtained from the
> > +  // media metadata. The decoder monitor must be obtained before calling this.
> > +  // aDuration is in microseconds.
> 
> aDuration is in seconds I think?
> 
> Durations should only be passed around as microseconds as int64_t. A
> duration as a double is usually a value in seconds.
> 
> One of these days I'm going to add a "typedef usecs_t int64_t;" somewhere to
> enforce this...

Ah, there was a change when streamer code was added. It seems to want int64_t, but previously it was in doubles. Fixed.

> @@ +171,5 @@
> > +  MediaByteRange  mCurrentByteRange;
> > +  // Index of the current byte range.
> > +  uint64_t        mSubsegmentIdx;
> > +
> > +  // Ptr to the reader object for this |Representation|.
> 
> Is this an owning pointer? Document here who owns and is responsible for
> deleting the reader.

Documented.

> ::: uriloader/exthandler/nsExternalHelperAppService.cpp
> @@ +478,5 @@
> >    { AUDIO_OGG, "oga", "Ogg Audio" },
> >    { AUDIO_OGG, "opus", "Opus Audio" },
> >    { VIDEO_WEBM, "webm", "Web Media Video" },
> >    { AUDIO_WEBM, "webm", "Web Media Audio" },
> > +#ifdef MOZ_DASH
> 
> Doesn't look like you need to ifdef this out here; we're not doing so when
> other media types are disabled.

Removed, although MOZ_MEDIA_PLUGINS has since added similar code.

Comment 84

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Ted Mielczarek [:ted] from comment #80)
> Comment on attachment 659940 [details] [diff] [review]
> v5.0 DASH Decoder
> 
> Review of attachment 659940 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the r+ Ted!
 
> ::: configure.in
> @@ +5247,5 @@
> > +[  --enable-dash          Enable support for DASH-WebM],
> > +    MOZ_DASH=1,
> > +    MOZ_DASH=)
> > +
> > +if test -n "$MOZ_DASH" -a -n "$MOZ_WEBM"; then
> 
> Maybe you want to explicitly error here if someone did --disable-webm
> --enable-dash?

Done.

> Also, is this going to become default-on at some point? I hate accruing new
> configure options.

Yes, the plan is to make it default. When I remove that though, I might add a conditional to undefined MOZ_DASH if MOZ_WEBM is also undefined.

> ::: content/media/dash/Makefile.in
> @@ +7,5 @@
> > +#
> > +# Contributor(s):
> > +#     Steve Workman <[e-mail]
> > +
> > +DEPTH      = ../../..
> 
> DEPTH = @DEPTH@

Done.

> Also, please don't use any tabs in Makefiles except where explicitly
> required.

Didn't find any? There were some already in the file though.

> @@ +16,5 @@
> > +include $(DEPTH)/config/autoconf.mk
> > +
> > +MODULE         = content
> > +LIBRARY_NAME   = gkcondash_s
> > +LIBXUL_LIBRARY = 1
> 
> nit: for these and other assignments where you are doing immediate
> assignment, use :=

Done.

> @@ +23,5 @@
> > +EXPORTS += \
> > +   nsDASHDecoder.h \
> > +   nsDASHRepDecoder.h \
> > +   nsDASHReader.h \
> > +   $(NULL)
> 
> nit: two-space indentation on continuations.

Done.

> @@ +43,5 @@
> > +INCLUDES += \
> > +   -I$(srcdir)/../webm \
> > +   -I$(srcdir)/../../base/src \
> > +   -I$(srcdir)/../../html/content/src \
> > +   $(MOZ_LIBVPX_INCLUDES) \
> 
> Is there a reason these are in INCLUDES and not LOCAL_INCLUDES?

No - fixed that. Thanks.

> ::: content/media/webm/Makefile.in
> @@ +35,5 @@
> > +
> > +ifdef MOZ_DASH
> > +INCLUDES += \
> > +   -I$(srcdir)/../dash \
> > +   $(NULL)
> 
> LOCAL_INCLUDES, two-space indent.

Done.

> ::: layout/build/Makefile.in
> @@ +177,5 @@
> >  
> > +ifdef MOZ_DASH
> > +SHARED_LIBRARY_LIBS += \
> > +   $(DEPTH)/content/media/dash/$(LIB_PREFIX)gkcondash_s.$(LIB_SUFFIX) \
> > +   $(NULL)
> 
> two-space indent

Done.

Comment 85

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v5.1 Add byte range download support to ChannelMediaResource

Carried r+ forward from cpearce.

Comment 86

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v5.1 DASH Decoder

r=ted for build files.

Comment 87

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

Comment on attachment 663069 [details] [diff] [review]
v5.1 DASH Decoder

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

Almost. Threading is painful. :(

::: content/media/dash/nsDASHDecoder.h
@@ +148,5 @@
> +  nsTArray<nsRefPtr<nsDASHRepDecoder> > mVideoRepDecoders;
> +
> +  // Thread safe flag to say we're shutting down. Protected by decoder monitor.
> +  // Set in |Shutdown|. Read on the MPD Reader thread.
> +  bool mThreadSafeShuttingDown;

This isn't written, read or even initialized anywhere.

I guess you don't need this as you don't do anything that can fail catastrophically in nsDASHDecoder::ReadMPDBuffer() if Shutdown() is called, and you check mShuttingDown in OnReadMPDBufferCompleted().

::: content/media/dash/nsDASHReader.cpp
@@ +27,5 @@
> +
> +#define CONDITIONALLY_ENTER_MONITOR() \
> +  if (!mDecoder->OnDecodeThread()) { \
> +    LOG("Waiting on decoder lock in %s", __FUNCTION__); \
> +    ReentrantMonitorAutoEnter decoderMon(mDecoder->GetReentrantMonitor()); \

This will exit the monitor when you leave this "if" block... This will serve as a memory barrier, but it won't ensure that the protected variables aren't changed while you're using them.

You could create an object that takes the lock *manually* in its constructor if we're running on the decode thread, and releases it in its dtor.

We use the this pattern of "owning thread can read freely, but must hold lock to write, other threads must hold lock to read" elsewhere. You need to document it explicitly.

@@ +38,5 @@
> +  NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
> +  NS_ASSERTION(mAudioReaders.Length() != 0 && mVideoReaders.Length() != 0,
> +               "Audio and video readers should exist already.");
> +
> +  CONDITIONALLY_ENTER_MONITOR();

Given that you know you're on the main thread, you can unconditionally enter the monitor, right? Ditto in a few places below.

@@ +123,5 @@
> +  // Wait for MPD to be parsed and child readers created.
> +  {
> +    ReentrantMonitorAutoEnter mon(mMetadataReadyMonitor);
> +    while (!mReadyToReadMetadata) {
> +      mon.Wait();

I'm a little concerned about mixing the two monitors here; you're holding mMetadataReadyMonitor and mDecoderShutdownMonitor while reading mDecoderIsShutdown in IsDecoderShuttingDown(), but you're not while accessing mDecoderIsShutdown elsewhere.

AFAICT the current code shouldn't deadlock, but if you had only one monitor to protect both mReadyToReadMonitor and mDecoderIsShuttingDown then the chance of future code changes adding a deadlock is reduced.

So how about having only 1 monitor here to protect both mReadyToReadMonitor and mDecoderIsShuttingDown, and make that:

{
  ReentrantMonitorAutoEnter mon(mDashMonitor);
  while (true) {
    if (mIsDecoderShuttingDown) {
      return NS_OK;
    }
    if (mReadyToReadMetadata) {
      break;
    }
    mon.Wait(); 
  }
}

This also means we won't reach code to read metadata later in the function if when we enter this function mReadyToReadMetadata and mDecoderIsShuttingDown are both true.

A name better than nsDashMonitor would be good. ;)

::: content/media/dash/nsDASHReader.h
@@ +99,5 @@
> +
> +  // Call by state machine on multiple threads.
> +  bool IsSeekableInBufferedRanges();
> +
> +  // Called by |nsDASHRepDecoder| when it starts Shutdown.

This is called by nsDASHDecoder isn't it?

Same for the comment for IsDecoderShuttingDown().

@@ +125,5 @@
> +  }
> +
> +  // Monitor and boolean used to wait for metadata bytes to be downloaded.
> +  ReentrantMonitor mMetadataReadyMonitor;
> +  bool mReadyToReadMetadata;

Better to have only one monitor to manage both of these to reduce chance of future code changes causing deadlocks.

Comment 88

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

Can you also remove nsBuiltinDecoderReader::DecodeToFirstData() which you commented out in nsBuiltinDecoderReader.cpp? Or remove its definition from the header file and move restore it to the cpp file. Thanks.

Comment 89

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Chris Pearce (:cpearce) from comment #87)
> Comment on attachment 663069 [details] [diff] [review]
> v5.1 DASH Decoder
> 
> Review of attachment 663069 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost. Threading is painful. :(

Indeed, but good to get it right and hopefully avoid bugs later.

> ::: content/media/dash/nsDASHDecoder.h
> @@ +148,5 @@
> > +  nsTArray<nsRefPtr<nsDASHRepDecoder> > mVideoRepDecoders;
> > +
> > +  // Thread safe flag to say we're shutting down. Protected by decoder monitor.
> > +  // Set in |Shutdown|. Read on the MPD Reader thread.
> > +  bool mThreadSafeShuttingDown;
> 
> This isn't written, read or even initialized anywhere.
> 
> I guess you don't need this as you don't do anything that can fail
> catastrophically in nsDASHDecoder::ReadMPDBuffer() if Shutdown() is called,
> and you check mShuttingDown in OnReadMPDBufferCompleted().

Removed. Leftover code.

> ::: content/media/dash/nsDASHReader.cpp
> @@ +27,5 @@
> > +
> > +#define CONDITIONALLY_ENTER_MONITOR() \
> > +  if (!mDecoder->OnDecodeThread()) { \
> > +    LOG("Waiting on decoder lock in %s", __FUNCTION__); \
> > +    ReentrantMonitorAutoEnter decoderMon(mDecoder->GetReentrantMonitor()); \
> 
> This will exit the monitor when you leave this "if" block... This will serve
> as a memory barrier, but it won't ensure that the protected variables aren't
> changed while you're using them.

Yup - stupid mistake.

> You could create an object that takes the lock *manually* in its constructor
> if we're running on the decode thread, and releases it in its dtor.

Done. I've added ReentrantMonitorConditionallyEnter as a private class to nsDASHReader. It's modeled after ReentrantMonitorAutoEnter, with an additional bool parameter in the constructor. I didn't include Wait or Notify as they're not used in nsDASHReader - those can be added later if needed.

In addition, I added MonitoredSubReader and MonitoredSubReaderList private classes to protect access to the sub-reader and array of sub-readers. Several operators and functions are overridden to include monitor assertions: some of these assertions are unconditional, e.g. write access through '=', and some conditional, e.g. read access through '*'. The checks are not all encompassing though: subreaders returned from operator[] on the list class are regular pointers; the assertion check only happens when the ptr is returned, not in subsequent uses of the pointer. Still, these two wrapper classes should provide more protection from harmful accesses when I start messing with the pointers in stream switching. On the other hand, you may think wrapping the pointers in this way is overkill - let me know what you think.

> We use the this pattern of "owning thread can read freely, but must hold
> lock to write, other threads must hold lock to read" elsewhere. You need to
> document it explicitly.

I've added lots of comments as well :) And hopefully the wrapper classes will enforce using the monitors correctly.

> @@ +38,5 @@
> > +  NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
> > +  NS_ASSERTION(mAudioReaders.Length() != 0 && mVideoReaders.Length() != 0,
> > +               "Audio and video readers should exist already.");
> > +
> > +  CONDITIONALLY_ENTER_MONITOR();
> 
> Given that you know you're on the main thread, you can unconditionally enter
> the monitor, right? Ditto in a few places below.

Yup, removed those checks.

> @@ +123,5 @@
> > +  // Wait for MPD to be parsed and child readers created.
> > +  {
> > +    ReentrantMonitorAutoEnter mon(mMetadataReadyMonitor);
> > +    while (!mReadyToReadMetadata) {
> > +      mon.Wait();
> 
> I'm a little concerned about mixing the two monitors here; you're holding
> mMetadataReadyMonitor and mDecoderShutdownMonitor while reading
> mDecoderIsShutdown in IsDecoderShuttingDown(), but you're not while
> accessing mDecoderIsShutdown elsewhere.
> 
> AFAICT the current code shouldn't deadlock, but if you had only one monitor
> to protect both mReadyToReadMonitor and mDecoderIsShuttingDown then the
> chance of future code changes adding a deadlock is reduced.
> 
> So how about having only 1 monitor here to protect both mReadyToReadMonitor
> and mDecoderIsShuttingDown ...
> 
> This also means we won't reach code to read metadata later in the function
> if when we enter this function mReadyToReadMetadata and
> mDecoderIsShuttingDown are both true.
> 
> A name better than nsDashMonitor would be good. ;)

mReadMetadataMonitor, since it is only for ReadMetadata anyway. Three functions now: ReadyToReadMetadata and NotifyDecoderShuttingDown for the notifications from nsDASHDecoder, and WaitForMetadata to carry out the shutdown check and wait for metadata being ready.

> ::: content/media/dash/nsDASHReader.h
> @@ +99,5 @@
> > +
> > +  // Call by state machine on multiple threads.
> > +  bool IsSeekableInBufferedRanges();
> > +
> > +  // Called by |nsDASHRepDecoder| when it starts Shutdown.
> 
> This is called by nsDASHDecoder isn't it?

Yup, my bad.

> Same for the comment for IsDecoderShuttingDown().

That function was removed.

> @@ +125,5 @@
> > +  }
> > +
> > +  // Monitor and boolean used to wait for metadata bytes to be downloaded.
> > +  ReentrantMonitor mMetadataReadyMonitor;
> > +  bool mReadyToReadMetadata;
> 
> Better to have only one monitor to manage both of these to reduce chance of
> future code changes causing deadlocks.

As above.

Comment 90

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v5.2 DASH Decoder

Comment 91

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Chris Pearce (:cpearce) from comment #88)
> Can you also remove nsBuiltinDecoderReader::DecodeToFirstData() which you
> commented out in nsBuiltinDecoderReader.cpp? Or remove its definition from
> the header file and move restore it to the cpp file. Thanks.

Sorry about that. That's now removed.

Comment 92

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

Comment on attachment 664336 [details] [diff] [review]
v5.2 DASH Decoder

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

Looks good.

::: content/media/dash/nsDASHReader.h
@@ +56,5 @@
> +  nsresult WaitForMetadata() {
> +    NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
> +    ReentrantMonitorAutoEnter mon(mReadMetadataMonitor);
> +    while (true) {
> +      // Return silently if the decoder has started shutting down.

It's the caller that checks for this code and returns silently isn't it? ;)

@@ +276,5 @@
> +
> +    // Override '[]' to assert threads other than the decode thread are "in
> +    // monitor" for accessing individual elems. Note: elems returned do not
> +    // have monitor assertions builtin like |MonitoredSubReader| objects.
> +    nsRefPtr<nsBuiltinDecoderReader> operator[](uint32_t i)

You're returning a copy of the nsRefPtr here. How about just returning nsRefPtr<nsBuiltinDecoderReader>& ? Then you can do monitoredReaderSubList[4] = someReader; and it should alter the list's element, not the copy of the list's element returned by the operator?

@@ +287,5 @@
> +    }
> +
> +    // Appends a reader to the end of |mSubReaderList|. Will always assert that
> +    // the thread is "in monitor".
> +    nsRefPtr<nsBuiltinDecoderReader> *

Similar to the previous comment, it seems you should be returning nsRefPtr<nsBuiltinDecoderReader>& here. Except you don't actually use the return value of this function, so perhaps you should just return void?

Comment 93

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 663068 [details] [diff] [review]
v5.1 Add byte range download support to ChannelMediaResource

https://hg.mozilla.org/integration/mozilla-inbound/rev/6df63191884b

Comment 94

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 664336 [details] [diff] [review]
v5.2 DASH Decoder

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

Comment 95

[Steve Workman [:sworkman] (INACTIVE)]

Some try pushes:

All builds for mochitest-1:
https://tbpl.mozilla.org/?tree=Try&rev=3868a54e404e

All opt builds for unit tests:
https://tbpl.mozilla.org/?tree=Try&rev=bd405321dfc2

Linux64 opt had infra error on the last one; this set is Linux 64 debug and opt builds:
https://tbpl.mozilla.org/?tree=Try&rev=9c908fb75de2

Comment 96

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Chris Pearce (:cpearce) from comment #92)
> Comment on attachment 664336 [details] [diff] [review]
> v5.2 DASH Decoder
> 
> Review of attachment 664336 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 

Thanks Chris!

Comment 97

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/6df63191884b
https://hg.mozilla.org/mozilla-central/rev/d882662a0bb6