Refactor PipelineImpl to use CompositeFilter to manage Filter state transitions.

media/base/composite_filter.h

Index: media/base/composite_filter.h
diff --git a/media/base/composite_filter.h b/media/base/composite_filter.h
new file mode 100644
index 0000000000000000000000000000000000000000..e32a42f1c7ce84979319b49628d3e09487abbd0e
--- /dev/null
+++ b/media/base/composite_filter.h
@@ -0,0 +1,180 @@
+// Copyright (c) 2010 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_COMPOSITE_FILTER_H_
+#define MEDIA_BASE_COMPOSITE_FILTER_H_
+
+#include "base/thread.h"
+#include "media/base/filter_host.h"
+#include "media/base/filters.h"
+
+namespace media {
+
+class CompositeFilter : public Filter,

[scherkus (not reviewing), 2010/12/15 16:44:02]

nit: all of this + { can fit on one line

[acolwell GONE FROM CHROMIUM, 2010/12/15 18:20:11]

Done.

+                        private FilterHost

[scherkus (not reviewing), 2010/12/15 16:44:02]

woah private inheritance!

style recommends public

in fact I've never used anything other than public so I can't really offer any
concrete reasons why other than style guide (but feel free to explain!)

[acolwell GONE FROM CHROMIUM, 2010/12/15 18:20:11]

FilterHost is private because I don't want other code to know that
CompositeFilter "is a" FilterHost. The only reason it implements this interface
is so that it can intercept calls from the Filters in the composite. 

I'll change this to an inner class so it matches the style guide. It just
requires a little more interaction between the CompositeFilter and the inner
class.

On 2010/12/15 16:44:02, scherkus wrote:

+{
+ public:
+  typedef base::Thread* (*ThreadFactoryFunction)(const char* thread_name);
+
+  CompositeFilter(MessageLoop* message_loop);
+
+  // Constructor that allows the default thread creation strategy to be
+  // overridden.
+  CompositeFilter(MessageLoop* message_loop,
+                  ThreadFactoryFunction thread_factory);
+
+  // Adds a filter to the composite. This is only allowed after set_host()
+  // is called and before the first state changing operation such as Play(),
+  // Flush(), Stop(), or Seek(). True is returned if the filter was successfully
+  // added to the composite. False is returned if the filter couldn't be added
+  // because the composite is in the wrong state or the filter needed a thread
+  // and the composite was unable to create one.
+  bool AddFilter(scoped_refptr<Filter> filter);
+
+  // media::Filter methods.
+  virtual const char* major_mime_type() const;
+  virtual void set_host(FilterHost* host);
+  virtual FilterHost* host();
+  virtual bool requires_message_loop() const;
+  virtual const char* message_loop_name() const;
+  virtual void set_message_loop(MessageLoop* message_loop);

[scherkus (not reviewing), 2010/12/15 16:44:02]

does CompositeFilter need to implement Filter, or just a subset of those
methods?

we might be able to get away with simply duplicating the signatures so we aren't
forced to implement methods that PipelineImpl will never use

would also avoid making CompositeFilter ref-counted

[acolwell GONE FROM CHROMIUM, 2010/12/15 18:20:11]

In my opinion it should implement Filter. The idea is that you can replace any
use of a Filter with one of these. This allows you to group Filters in a variety
of ways without other pieces of code having to know it is happening.

I think eventually the host & message_loop methods should be removed from this
interface and turned into constructor parameters.

On 2010/12/15 16:44:02, scherkus wrote:

[scherkus (not reviewing), 2010/12/15 18:57:44]

Rietveld probably isn't the best for discussing, but I have a motivation to get
rid of the notion of generic filters one day because the abstraction hasn't
proved useful.

We more or less have the following structure duplicated everywhere:
Filter < VideoDecoder < VideoDecoderImpl < FFmpegVideoDecodeEngine

...and I'm tempted to see whether we can squash a few abstraction layers and
simply have stuff like a VideoDecoder instead of dealing with a Filter that is
secretly a VideoDecoder, which is secretly always a VideoDecoderImpl, etc...

We can leave it implementing Filter for now and then modify the Filter interface
to move some stuff into the ctor in follow up CL (see below), but let's try to
sync up on the overall plan for refactoring
Agreed -- we can tackle that in separate CL!

+  virtual MessageLoop* message_loop();
+  virtual void Play(FilterCallback* play_callback);
+  virtual void Pause(FilterCallback* pause_callback);
+  virtual void Flush(FilterCallback* flush_callback);
+  virtual void Stop(FilterCallback* stop_callback);
+  virtual void SetPlaybackRate(float playback_rate);
+  virtual void Seek(base::TimeDelta time, FilterCallback* seek_callback);
+  virtual void OnAudioRendererDisabled();
+
+ protected:
+  virtual ~CompositeFilter();

[scherkus (not reviewing), 2010/12/15 16:44:02]

we could also make CompositeFilter non-ref-counted and make this public

[acolwell GONE FROM CHROMIUM, 2010/12/15 18:20:11]

Why? This is ref-counted like other filters.

[scherkus (not reviewing), 2010/12/15 18:57:44]

We've been trying to move away from ref-counting since the lifetime of our
filters are known.  Not that it's a performance thing, but more for having clear
ownership + deterministic shutdown.

+
+  /// Default thread factory strategy.
+  static base::Thread* DefaultThreadFactory(const char* thread_name);
+
+  // media::FilterHost methods.
+  virtual void SetError(PipelineError error);
+  virtual base::TimeDelta GetTime() const;
+  virtual base::TimeDelta GetDuration() const;
+  virtual void SetTime(base::TimeDelta time);
+  virtual void SetDuration(base::TimeDelta duration);
+  virtual void SetBufferedTime(base::TimeDelta buffered_time);
+  virtual void SetTotalBytes(int64 total_bytes);
+  virtual void SetBufferedBytes(int64 buffered_bytes);
+  virtual void SetVideoSize(size_t width, size_t height);
+  virtual void SetStreaming(bool streaming);
+  virtual void NotifyEnded();
+  virtual void SetLoaded(bool loaded);
+  virtual void SetNetworkActivity(bool network_activity);
+  virtual void DisableAudioRenderer();
+  virtual void SetCurrentReadPosition(int64 offset);
+  virtual int64 GetCurrentReadPosition();
+
+ private:
+  enum State {
+    kInvalid,
+    kCreated,
+    kPaused,
+    kPlayPending,
+    kStopWhilePlayPending,
+    kPlaying,
+    kPausePending,
+    kStopWhilePausePending,
+    kFlushPending,
+    kStopWhileFlushPending,
+    kSeekPending,
+    kStopWhileSeekPending,
+    kStopPending,
+    kStopped,
+    kError
+  };
+
+  // Initialization method called by constructors.
+  void Init(MessageLoop* message_loop, ThreadFactoryFunction thread_factory);
+
+  // Transition to a new state.
+  void ChangeState(State new_state);
+
+  // Start calling filters in a sequence.
+  void StartSerialCallSequence();
+
+  // Call filters in parallel.
+  void StartParallelCallSequence();
+
+  // Call the filter based on the current value of state_.
+  void CallFilter(scoped_refptr<Filter>& filter, FilterCallback* callback);
+
+  // Calls |callback_| and then clears the reference.
+  void DispatchPendingCallback();
+
+  // Gets the state to transition to given |state|.
+  State GetNextState(State state) const;
+
+  // Filter callback for a serial sequence.
+  void SerialCallback();
+
+  // Filter callback for a parallel sequence.
+  void ParallelCallback();
+
+  // Called when a parallel or serial call sequence completes.
+  void OnCallSequenceDone();
+
+  // Helper function for sending an error to the FilterHost.
+  void SendErrorToHost(PipelineError error);
+
+  // Helper function for handling errors during call sequences.
+  void HandleError(PipelineError error);
+
+  // Creates a callback that can be called from any thread, but is guaranteed
+  // to call the specified method on the thread associated with this filter.
+  FilterCallback* NewThreadSafeCallback(void (CompositeFilter::*method)());
+
+  // Helper function used by NewThreadSafeCallback() to make sure the
+  // method gets called on the right thread.
+  void OnCallback(MessageLoop* message_loop,
+                  void (CompositeFilter::*method)());
+
+  // Helper function that indicates whether SetError() calls can be forwarded
+  // to the host of this filter.
+  bool CanForwardError();
+
+  // Vector of threads owned by the composite and used by filters in |filters_|.
+  typedef std::vector<base::Thread*> FilterThreadVector;
+  FilterThreadVector filter_threads_;
+
+  // Vector of the filters added to the composite.
+  typedef std::vector<scoped_refptr<Filter> > FilterVector;
+  FilterVector filters_;
+
+  // Factory function used to create filter threads.
+  ThreadFactoryFunction thread_factory_;
+
+  // Callback for the pending request.
+  scoped_ptr<FilterCallback> callback_;
+
+  // Time parameter for the pending Seek() request.
+  base::TimeDelta pending_seek_time_;
+
+  // Current state of this filter.
+  State state_;
+
+  // The index of the filter currently processing a request.
+  unsigned int sequence_index_;

[scherkus (not reviewing), 2010/12/15 16:44:02]

bit confused.. if this is an unsigned int I notice you're setting it to -1  as
well?

[acolwell GONE FROM CHROMIUM, 2010/12/15 18:20:11]

I'll fix this. The -1 was a hack to get around having to special case the empty
composite case.

+
+  // Message loop passed into the constructor.
+  MessageLoop* message_loop_;
+
+  // FilterHost passed in via set_host().
+  FilterHost* host_;
+
+  // Error passed in the last SetError() call.
+  PipelineError error_;
+
+  DISALLOW_COPY_AND_ASSIGN(CompositeFilter);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_COMPOSITE_FILTER_H_