MediaResource refactoring to support multiple streams

media/base/media_resource.h

Index: media/base/media_resource.h
diff --git a/media/base/demuxer_stream_provider.h b/media/base/media_resource.h
similarity index 57%
rename from media/base/demuxer_stream_provider.h
rename to media/base/media_resource.h
index 353455051e79e154aa615893d15c555878d189e6..798839ca7de0d0584bd9b04849c2bc09f2b78e14 100644
--- a/media/base/demuxer_stream_provider.h
+++ b/media/base/media_resource.h
@@ -2,8 +2,10 @@
 // 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_
+#ifndef MEDIA_BASE_MEDIA_RESOURCE_H_
+#define MEDIA_BASE_MEDIA_RESOURCE_H_
+
+#include <vector>
 
 #include "base/macros.h"
 #include "media/base/demuxer_stream.h"
@@ -13,42 +15,34 @@
 
 namespace media {
 
+using StreamStatusChangeCB =
+    base::Callback<void(DemuxerStream*, bool, base::TimeDelta)>;

[xhwang, 2017/02/01 18:26:03]

Add include for base::Callback and TimeDelta

Please add a comment about what the |bool| is.

nit: This probably should be a RepeatingCallback, but it's up to you.

[servolk, 2017/02/01 22:29:11]

Done.

+
 // Abstract class that defines how to retrieve "media sources" in DemuxerStream

[xhwang, 2017/02/01 18:26:03]

nit: Does it make sense to s/sources/resource?

[servolk, 2017/02/01 22:29:17]

Done.

 // 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 class would be a good candidate for renaming, if
-// ever Pipeline were to support this class directly, instead of the Demuxer
-// interface.
-// TODO(tguilbert): Rename this class. See crbug.com/658062.
-//
 // The derived classes must return a non-null value for the getter method
 // associated with their type, and return a null/empty value for other getters.
-class MEDIA_EXPORT DemuxerStreamProvider {
+class MEDIA_EXPORT MediaResource {
  public:
   enum Type {
     STREAM,  // Indicates GetStream() should be used

[xhwang, 2017/02/01 18:26:03]

GetStreams

[servolk, 2017/02/01 22:29:12]

Done (in the next CL)

     URL,     // Indicates GetUrl() should be used
   };
 
-  DemuxerStreamProvider();
-  virtual ~DemuxerStreamProvider();
+  MediaResource();
+  virtual ~MediaResource();
 
-  // For Type::STREAM:
-  //   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 collection of available DemuxerStream objects. Note that some of

[xhwang, 2017/02/01 18:26:03]

Do you want to keep the  

For Type::STREAM:

part given line 47?

[servolk, 2017/02/01 22:29:11]

Done (in the next CL)

+  // those streams might be in disabled state (check DemuxerStream::enabled() ).

[xhwang, 2017/02/01 18:26:03]

I thought we agreed that disabled streams should not be exposed?

[servolk, 2017/02/01 22:29:17]

Yes. Although I haven't done that yet in this CL, and your comment about whether
HasEncryptedStreams actually makes me wonder if we'll need to reconsider this.
Let's continue this discussion in a follow up CL that moves around the
SetStreamStatusChangeCB.

   //   NOTE: Once a DemuxerStream pointer is returned from GetStream it is
-  //   guaranteed to stay valid for as long as the Demuxer/DemuxerStreamProvider
+  //   guaranteed to stay valid for as long as the Demuxer/MediaResource
   //   is alive. But make no assumption that once GetStream returned a non-null
   //   pointer for some stream type then all subsequent calls will also return
   //   non-null pointer for the same stream type. In MSE Javascript code can
   //   remove SourceBuffer from a MediaSource at any point and this will make
   //   some previously existing streams inaccessible/unavailable.
-  // Other types:
-  //   Should not be called.
-  virtual DemuxerStream* GetStream(DemuxerStream::Type type) = 0;
+  virtual std::vector<DemuxerStream*> GetStreams() = 0;
 
   // For Type::URL:
   //   Returns the URL parameters of the media to play. Empty URLs are legal,
@@ -57,12 +51,17 @@ class MEDIA_EXPORT DemuxerStreamProvider {
   //   Should not be called.
   virtual MediaUrlParams GetMediaUrlParams() const;
 
-  virtual DemuxerStreamProvider::Type GetType() const;
+  virtual MediaResource::Type GetType() const;
+
+  // The StreamStatusChangeCB allows clients to receive notifications about one
+  // of the streams being disabled or enabled. The first parameter is the stream
+  // and the second parameter is the playback position where the change occured.

[xhwang, 2017/02/01 18:26:03]

Are these callback parameters? If so the comment should go with the callback
definition. Also, the callback takes 3 params, only two are documented here.

nit: Is it okay for a client to set this multiple times, or multiple clients set
this?

[servolk, 2017/02/01 22:29:17]

Moved the comment about params to the callback definition.

Re multiple clients - no, currently only one client is supported. But that's
probably ok, because the subscriber to those callbacks is the renderer, and IIUC
we have only one renderer at a time.

+  virtual void SetStreamStatusChangeCB(const StreamStatusChangeCB& cb) = 0;
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(DemuxerStreamProvider);
+  DISALLOW_COPY_AND_ASSIGN(MediaResource);
 };
 
 }  // namespace media
 
-#endif  // MEDIA_BASE_DEMUXER_STREAM_PROVIDER_H_
+#endif  // MEDIA_BASE_MEDIA_RESOURCE_H_