Move Pipeline permanent callbacks into Pipeline::Client interface.

media/base/pipeline.h

 // Copyright (c) 2012 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_PIPELINE_H_
 #define MEDIA_BASE_PIPELINE_H_
 
 #include <memory>
 
 #include "base/memory/ref_counted.h"
 #include "base/time/time.h"
 #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"
 #include "media/base/video_rotation.h"
 #include "ui/gfx/geometry/size.h"
 
 namespace media {
 
 class Demuxer;
 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;
+  class Client {
+   public:
+    // Executed whenever an error occurs but hasn't been reported already
+    // through another callback.

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

What do you mean by "reported already through another callback"?

[alokp, 2016/05/04 17:40:01]

clarified.

+    virtual void OnError(PipelineStatus status) = 0;
+
+    // Executed whenever the media reaches the end.
+    virtual void OnEnded() = 0;
+
+    // Executed when the content duration, container video size, start time,
+    // and whether the content has audio and/or video in supported formats are
+    // known.
+    virtual void OnMetadata(PipelineMetadata metadata) = 0;
+
+    // Executed whenever there are changes in the buffering state of the
+    // pipeline.
+    virtual void OnBufferingStateChange(BufferingState state) = 0;
+
+    // Executed whenever the presentation duration changes.
+    virtual void OnDurationChange() = 0;
+
+    // Executed whenever a text track is added.

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

When should |done_cb| be fired?

[alokp, 2016/05/04 17:40:01]

Added comment to the best of my knowledge. The old code did not have comment
about this.

+    virtual void OnAddTextTrack(const TextTrackConfig& config,
+                                const AddTextTrackDoneCB& done_cb) = 0;
+
+    // Executed whenever the key needed to decrypt the stream is not available.
+    virtual void OnWaitingForDecryptionKey() = 0;
+  };
+
+  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
+  //

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

These are calls on the Client interface, not really callbacks. Also, since this
is the list of all methods, it's probably clearer to just say "methods on Client
interface may be called up until Stop() has completed".

[alokp, 2016/05/04 17:40:01]

Done.

   // It is an error to call this method after the pipeline has already started.
+  // TODO(xhwang): Rename |seek_cb| to |start_cb|.

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

Since Start/Seek/Resume all uses seek_cb, after a second thought, I think
seek_cb still makes some sense.

Could you please drop this TODO for me? Thanks! Sorry for being back and forth.

[alokp, 2016/05/04 17:40:01]

Done.

   virtual void Start(Demuxer* demuxer,
                      std::unique_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;
+                     Client* client,
+                     const PipelineStatusCB& seek_cb) = 0;
 
-  // Asynchronously stops the pipeline, executing |stop_cb| when the pipeline
-  // teardown has completed.
+  // Stops the pipeline. This is a blocking function.
+  // The pipeline must be stopped before destroying it.

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

... "if the pipeline is started" according to the comment on IsRunning(). Either
way, we should be consistent.

[alokp, 2016/05/04 17:40:01]

Done.

+  // It it permissible to call Stop() at any point during the lifetime of the
+  // pipeline.
   //
-  // Stop() must complete before destroying the pipeline. It it permissible to
-  // call Stop() at any point during the lifetime of the pipeline.
-  //
-  // It is safe to delete the pipeline during the execution of |stop_cb|.
-  virtual void Stop(const base::Closure& stop_cb) = 0;
+  // Once Stop is called any outstanding completion callbacks
+  // for Start/Seek/Suspend/Resume or PipelineClient callbacks will *not*

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

s/PipelineClient/Client

Since Client methods are not callbacks, s/Client callbacks/Client methods/?

[alokp, 2016/05/04 17:40:01]

Done.

+  // be called.
+  virtual void Stop() = 0;
 
   // Attempt to seek to the position specified by time.  |seek_cb| will be
   // executed when the all filters in the pipeline have processed the seek.
   //
   // Clients are expected to call GetMediaTime() to check whether the seek
   // succeeded.
   //
   // It is an error to call this method if the pipeline has not started or
   // has been suspended.
   virtual void Seek(base::TimeDelta time, const PipelineStatusCB& seek_cb) = 0;
@@ skipping 61 matching lines @@
   // Gets the current pipeline statistics.
   virtual PipelineStatistics GetStatistics() const = 0;
 
   virtual void SetCdm(CdmContext* cdm_context,
                       const CdmAttachedCB& cdm_attached_cb) = 0;
 };
 
 }  // namespace media
 
 #endif  // MEDIA_BASE_PIPELINE_H_