Move Pipeline permanent callbacks into Pipeline::Client interface.

media/base/pipeline.h

Index: media/base/pipeline.h
diff --git a/media/base/pipeline.h b/media/base/pipeline.h
index db326c2928353f2aa9d6080ad650e4ecd56f22fb..fcd9f89dc409335d16e2ce5ecbbb8b252e5259ba 100644
--- a/media/base/pipeline.h
+++ b/media/base/pipeline.h
@@ -10,6 +10,7 @@
 #include "media/base/buffering_state.h"
 #include "media/base/cdm_context.h"
 #include "media/base/media_export.h"
+#include "media/base/pipeline_metadata.h"
 #include "media/base/pipeline_status.h"
 #include "media/base/ranges.h"
 #include "media/base/text_track.h"
@@ -19,57 +20,31 @@
 namespace media {
 
 class Demuxer;
+class PipelineClient;
 class Renderer;
 class VideoFrame;
 
-// Metadata describing a pipeline once it has been initialized.
-struct PipelineMetadata {
-  PipelineMetadata()
-      : has_audio(false), has_video(false), video_rotation(VIDEO_ROTATION_0) {}
-
-  bool has_audio;
-  bool has_video;
-  gfx::Size natural_size;
-  VideoRotation video_rotation;
-  base::Time timeline_offset;
-};
-
-typedef base::Callback<void(PipelineMetadata)> PipelineMetadataCB;
-
 class MEDIA_EXPORT Pipeline {
  public:
-  // Used to paint VideoFrame.
-  typedef base::Callback<void(const scoped_refptr<VideoFrame>&)> PaintCB;
+  virtual ~Pipeline() {}
 
   // Build a pipeline to using the given |demuxer| and |renderer| to construct
   // a filter chain, executing |seek_cb| when the initial seek has completed.
   //
-  // The following permanent callbacks will be executed as follows up until
-  // Stop() has completed:
-  //   |ended_cb| will be executed whenever the media reaches the end.
-  //   |error_cb| will be executed whenever an error occurs but hasn't been
-  //              reported already through another callback.
-  //   |metadata_cb| will be executed when the content duration, container video
-  //                 size, start time, and whether the content has audio and/or
-  //                 video in supported formats are known.
-  //   |buffering_state_cb| will be executed whenever there are changes in the
-  //                        overall buffering state of the pipeline.
-  //   |duration_change_cb| optional callback that will be executed whenever the
-  //                        presentation duration changes.
-  //   |add_text_track_cb| will be executed whenever a text track is added.
-  //   |waiting_for_decryption_key_cb| will be executed whenever the key needed
-  //                                   to decrypt the stream is not available.
+  // The following permanent callbacks will be executed on PipelineClient up
+  // until Stop() has completed:
+  //   OnError
+  //   OnEnded
+  //   OnMetadata
+  //   OnBufferingStateChange
+  //   OnDurationChange
+  //   OnAddTextTrack
+  //   OnWaitingForDecryptionKey
+  //
   // It is an error to call this method after the pipeline has already started.
   virtual void Start(Demuxer* demuxer,
                      scoped_ptr<Renderer> renderer,
-                     const base::Closure& ended_cb,
-                     const PipelineStatusCB& error_cb,
-                     const PipelineStatusCB& seek_cb,
-                     const PipelineMetadataCB& metadata_cb,
-                     const BufferingStateCB& buffering_state_cb,
-                     const base::Closure& duration_change_cb,
-                     const AddTextTrackCB& add_text_track_cb,
-                     const base::Closure& waiting_for_decryption_key_cb) = 0;
+                     const PipelineStatusCB& seek_cb) = 0;

[sandersd (OOO until July 31), 2016/04/21 00:36:30]

Are we going to rename this init_cb while we are at it?

[xhwang, 2016/04/21 05:50:35]

Or |start_cb|?

[alokp, 2016/04/21 21:56:38]

This particular callback is shared between Start, Seek, and Resume. Renaming it
would make it "wrong" for Seek. Nothing wrong with renaming the Start argument
though, but internally we might still want to call it seek_cb?

[xhwang, 2016/04/21 22:26:04]

Right. We should separate the initial seek_cb (for Start) and the rest (for Seek
etc). Can we use a start_cb for the former, and have a OnSeeked() call on the
client for the latter?

[alokp, 2016/04/22 05:08:59]

I will need to look into this closely. I think this can be done in a separate
patch though. I would prefer to keep this focused on just moving the permanent
callbacks into Pipeline::Client class.

[xhwang, 2016/04/22 16:05:05]

sgtm. A TODO would be nice here.

 
   // Asynchronously stops the pipeline, executing |stop_cb| when the pipeline
   // teardown has completed.
@@ -155,6 +130,14 @@ class MEDIA_EXPORT Pipeline {
 
   virtual void SetCdm(CdmContext* cdm_context,
                       const CdmAttachedCB& cdm_attached_cb) = 0;
+
+ protected:
+  Pipeline(PipelineClient* client) : client_(client) {}

[xhwang, 2016/04/21 05:50:35]

What's the value of putting this in "protected"? Pipeline is pure virtual and
can't be instantiated anyways?

[alokp, 2016/04/21 21:56:38]

Done.

+
+  PipelineClient* client() const { return client_; }
+
+ private:
+  PipelineClient* client_;

[xhwang, 2016/04/21 05:50:35]

This would work. But I am not a huge fan of storing members on the pure
interface. Will it work if we pass the PipelineClient in Start() (part of the
interface) instead of in the constructor?

[alokp, 2016/04/21 21:56:38]

Yeah this will work since client is only being used during playback. Done.
Although I do not like the interface name - PipelineClient - anymore. Clients
are usually passed in the constructor or Initialize calls.

[xhwang, 2016/04/21 22:26:04]

The Start() is really a combination of both Initialize() and StartPlayback().
Maybe the following will look nicer to you?

virtual void Initialize(Demuxer* demuxer,
                        scoped_ptr<Renderer> renderer,
                        Client* client) = 0;

virtual void Start(const PipelineStatusCB& start_cb) = 0;

[xhwang, 2016/04/21 22:32:05]

To be clear. I am not proposing this change. Just trying to show that  maybe
passing the Client in Start() isn't as bad as you thought.

[alokp, 2016/04/22 05:08:59]

This is a minor thing. I have kept it in Start for now.

 };
 
 }  // namespace media