Move FilterType into MediaFilterCollection

media/base/media_filter_collection.h

 // 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_MEDIA_FILTER_COLLECTION_H_
 #define MEDIA_BASE_MEDIA_FILTER_COLLECTION_H_
 
 #include <list>
 
 #include "base/ref_counted.h"
 #include "media/base/filters.h"
 
 namespace media {
 
 // This is a collection of MediaFilter objects used to form a media playback
 // pipeline. See src/media/base/pipeline.h for more information.
 class MediaFilterCollection {
  public:
   MediaFilterCollection();
 
   // Adds a filter to the collection.
-  void AddFilter(MediaFilter* filter);
+  void AddFilter(DataSource* filter);

[scherkus (not reviewing), 2010/11/11 01:47:12]

FYI this will increase the size of this change but style guide isn't too hot on
function overloading and usually recommends it for equivalent types (i.e, char*
and std::string)

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

I tend to agree and I'm considering we make it painfully obvious what's going on
and have AddDataSource, AddDemuxer, etc.. and same with SelectXXX()

I don't think we're relying on function overloading in pipeline_impl (i.e.,
there isn't any crazy template magic code) so this should be ok.

[acolwell GONE FROM CHROMIUM, 2010/11/11 19:24:07]

Done. I totally agree.

On 2010/11/11 01:47:12, scherkus wrote:

+  void AddFilter(Demuxer* filter);
+  void AddFilter(VideoDecoder* filter);
+  void AddFilter(AudioDecoder* filter);
+  void AddFilter(VideoRenderer* filter);
+  void AddFilter(AudioRenderer* filter);
 
   // Is the collection empty?
   bool IsEmpty() const;
 
   // Remove remaining filters.
   void Clear();
 
   // Selects a filter of the specified type from the collection.
   // If the required filter cannot be found, NULL is returned.
   // If a filter is returned it is removed from the collection.
-  template <class Filter>
-  void SelectFilter(scoped_refptr<Filter>* filter_out) {
-    scoped_refptr<MediaFilter> filter;
-    SelectFilter(Filter::static_filter_type(), &filter);
-    *filter_out = reinterpret_cast<Filter*>(filter.get());
-  }
+  void SelectFilter(scoped_refptr<DataSource>* filter_out);
+  void SelectFilter(scoped_refptr<Demuxer>* filter_out);
+  void SelectFilter(scoped_refptr<VideoDecoder>* filter_out);
+  void SelectFilter(scoped_refptr<AudioDecoder>* filter_out);
+  void SelectFilter(scoped_refptr<VideoRenderer>* filter_out);
+  void SelectFilter(scoped_refptr<AudioRenderer>* filter_out);
 
  private:
+  // Identifies the type of filter implementation. Each filter has to be one of
+  // the following types. This is used to mark, identify, and support
+  // downcasting of different filter types stored in the filters_ list.
+  enum FilterType {

[scherkus (not reviewing), 2010/11/11 01:47:12]

I've always hated the Filter prefix and given this is private I say we lop off
the Filter and FILTER_ prefixes

[acolwell GONE FROM CHROMIUM, 2010/11/11 19:24:07]

Done.

+    FILTER_DATA_SOURCE,
+    FILTER_DEMUXER,
+    FILTER_AUDIO_DECODER,
+    FILTER_VIDEO_DECODER,
+    FILTER_AUDIO_RENDERER,
+    FILTER_VIDEO_RENDERER
+  };
+

[scherkus (not reviewing), 2010/11/11 01:47:12]

nit: remove extra blank line

[acolwell GONE FROM CHROMIUM, 2010/11/11 19:24:07]

Done.

+
   // List of filters managed by this collection.
-  std::list<scoped_refptr<MediaFilter> > filters_;
+  typedef std::list<Tuple2<FilterType,

[scherkus (not reviewing), 2010/11/11 01:47:12]

instead of mixing std and google Tuple2 maybe use std::pair/std::make_pair?

pretty indifferent about that one :)

[acolwell GONE FROM CHROMIUM, 2010/11/11 19:24:07]

Done.

+                           scoped_refptr<MediaFilter> > > FilterList;
+  FilterList filters_;
+
+  // Helper funtion that adds a filter to the filter list.

[scherkus (not reviewing), 2010/11/11 01:47:12]

funtion -> function

[acolwell GONE FROM CHROMIUM, 2010/11/11 19:24:07]

Done.

+  void AddFilter(FilterType filter_type, MediaFilter* filter);
+
+  // Helper function for SelectFilter() that manages the
+  // downcasting and mapping between FilterType & Filter class.

[scherkus (not reviewing), 2010/11/11 01:47:12]

nit: & -> and

from scanning I thought you meant a ref type so I imagine others might do the
same (pedantic nit, I know!)

[acolwell GONE FROM CHROMIUM, 2010/11/11 19:24:07]

Done.

+  template<FilterType filter_type, class Filter>
+  void SelectFilter(scoped_refptr<Filter>* filter_out);
 
   // Helper function that searches the filters list for a specific
   // filter type.
   void SelectFilter(FilterType filter_type,
                     scoped_refptr<MediaFilter>* filter_out);
 
   DISALLOW_COPY_AND_ASSIGN(MediaFilterCollection);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_BASE_MEDIA_FILTER_COLLECTION_H_