Add GetUrl to DemuxerStreamProvider interface

media/base/demuxer_stream_provider.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef MEDIA_BASE_DEMUXER_STREAM_PROVIDER_H_
 #define MEDIA_BASE_DEMUXER_STREAM_PROVIDER_H_
 
 #include "base/macros.h"
 #include "media/base/demuxer_stream.h"
 #include "media/base/media_export.h"
+#include "url/gurl.h"
 
 namespace media {
 
+// Interface that defines how to retrieve "media sources" in DemuxerStream form
+// (for most cases) or URL form (for the MediaPlayerRenderer case).
+//
+// The sub-classes do not stricly provide demuxer streams, but because all
+// sub-classes are for the moment Demuxers, this class has not been renamed to
+// "MediaProvider". This interface would be a good candidate for renaming, if
+// ever Pipeline were to use this interface directly, instead of the Demuxer
+// interface.
+//
+// The derived classes must return a non-null value for either of the getter
+// methods, and return a null value for the other.
+// Ex: A Demuxer should return the DemuxerStreams it supports, but return a
+// empty URL.
 class MEDIA_EXPORT DemuxerStreamProvider {
  public:
   DemuxerStreamProvider();
   virtual ~DemuxerStreamProvider();
 

[DaleCurtis, 2016/06/24 00:42:30]

Should we have a type() or something that says which method should be used?

[tguilbert, 2016/06/24 03:13:20]

This sounds good to me.

However, I have been flip-flopping between making this an abstract method or
not. While I prefer forcing subclasses to implement their own type(), since
GetUrl() is abstract, it's simpler and less clutter to make this abstract. WDYT?
I am open to either solutions.

   // Returns the first stream of the given stream type (which is not allowed
   // to be DemuxerStream::TEXT), or NULL if that type of stream is not present.
+  // Returns a null value if we are using a URL instead.
   virtual DemuxerStream* GetStream(DemuxerStream::Type type) = 0;

[DaleCurtis, 2016/06/24 00:42:30]

Did you consider implementing DemuxerStream instead? I.e. having a
DemuxerStream::Type of URL?

[tguilbert, 2016/06/24 03:13:20]

I did, and that is how I originally implemented it in my MediaPlayerRenderer
prototype CL.

I had the DemuxerStream::Type::URL CL ready for review, and then, after thinking
about xhwang@'s comment on my prototype and talking to him offline, I decided
this way was probably cleaner.
Because the CL was ready to go however, I uploaded it earlier today, closed it
and added it to the bug attached to CL.

Here is the link again for reference:
https://codereview.chromium.org/2099453002


The current CL also addresses the question I had trouble phrasing when we went
over my prototype CL in person ("Is the presence of a DemuxerStream of type URL 
a sufficiently clear indicator to let WMPI/readers know that we are operating in
a 'special case', or should a more explicit flag be set?"). Using GetUrl() and
the DemuxerStreamProvider::Type::URL is clearer than the DemuxerStream of type
URL. In one case, different code paths are taken based off the *nature* of the
Demuxer; in the other, different paths are taken based of the presence/absence
of certain stream types, which tells us implicitly tell us something about the
nature of the Demuxer (but is not obvious for a first time reader).

The above paragraph might have been a bit dense... I will gladly discuss this
offline if need be!

 
+  // Returns the URL of the media to play. To be used when DemuxerStreams are
+  // not relevant (e.g. in the MediaPlayerRender case).
+  // Returns nullptr if we are using DemuxerStreams instead.
+  virtual GURL* GetUrl() = 0;

[DaleCurtis, 2016/06/24 00:42:30]

I think this is gnarly enough that we should make this an abstract method to
avoid cluttering other implementors. 

[tguilbert, 2016/06/24 03:13:20]

I am fine with this if you are.

+
  private:
   DISALLOW_COPY_AND_ASSIGN(DemuxerStreamProvider);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_BASE_DEMUXER_STREAM_PROVIDER_H_