Move VideoRenderer out of Filter heirarchy.

Move VideoRenderer out of Filter heirarchy.

As a result the following classes no longer exist: Filter, FilterHost, CompositeFilter. I guess you could say we no longer have filters!

BUG=108342, 126069
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147834

[scherkus (not reviewing), 2012-07-21 00:59:15]

both of you PTAL

http://www.bradbranson.com/wp-content/uploads/2012/06/his_mind-blown.gif

[Ami GONE FROM CHROMIUM, 2012-07-21 05:02:35]

Yay!

LGTM % nits

http://codereview.chromium.org/10796074/diff/1/media/base/filter_collection_u...
File media/base/filter_collection_unittest.cc (right):

http://codereview.chromium.org/10796074/diff/1/media/base/filter_collection_u...
media/base/filter_collection_unittest.cc:12: class FilterCollectionTest : public
::testing::Test {
FilterCollection is a strange name for a collection of things that are no longer
Filters...  Another CL, another day.
(hopefully dropping FilterCollection entirely)

http://codereview.chromium.org/10796074/diff/1/media/base/filters.h
File media/base/filters.h (left):

http://codereview.chromium.org/10796074/diff/1/media/base/filters.h#oldcode1
media/base/filters.h:1: // Copyright (c) 2012 The Chromium Authors. All rights
reserved.
\o/!!!

http://codereview.chromium.org/10796074/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10796074/diff/1/media/base/pipeline.cc#newcode...
media/base/pipeline.cc:1149: filter_collection_->SelectVideoRenderer(
single line

http://codereview.chromium.org/10796074/diff/1/media/base/video_renderer.cc
File media/base/video_renderer.cc (right):

http://codereview.chromium.org/10796074/diff/1/media/base/video_renderer.cc#n...
media/base/video_renderer.cc:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
Wow; git copy detection is really working overtime...

http://codereview.chromium.org/10796074/diff/1/media/base/video_renderer.h
File media/base/video_renderer.h (right):

http://codereview.chromium.org/10796074/diff/1/media/base/video_renderer.h#ne...
media/base/video_renderer.h:32: // Used to query the current time and duration
of the media.
s/and/or/

http://codereview.chromium.org/10796074/diff/1/media/base/video_renderer.h#ne...
media/base/video_renderer.h:43: const PipelineStatusCB& error_cb,
This highlights that "status_cb" is a bad name (should include "initialize"
somewhere in there).

http://codereview.chromium.org/10796074/diff/1/media/filters/video_renderer_b...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/10796074/diff/1/media/filters/video_renderer_b...
media/filters/video_renderer_base_unittest.cc:59: // We expect these to be
called but we don't care how/when.
Not sure how to interpret this comment vis-a-vis the previous two EXPECT_CALL's.
 Why does it not apply to them?

[acolwell GONE FROM CHROMIUM, 2012-07-21 15:52:40]

LGTM \o/

[scherkus (not reviewing), 2012-07-23 03:21:12]

thanks for the review!

https://chromiumcodereview.appspot.com/10796074/diff/1/media/base/filter_coll...
File media/base/filter_collection_unittest.cc (right):

https://chromiumcodereview.appspot.com/10796074/diff/1/media/base/filter_coll...
media/base/filter_collection_unittest.cc:12: class FilterCollectionTest : public
::testing::Test {
On 2012/07/21 05:02:35, Ami Fischman wrote:
> FilterCollection is a strange name for a collection of things that are no
longer
> Filters...  Another CL, another day.
> (hopefully dropping FilterCollection entirely)

Agreed -- updated FilterCollection header to point at http://crbug.com/110800

https://chromiumcodereview.appspot.com/10796074/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/10796074/diff/1/media/base/pipeline.cc...
media/base/pipeline.cc:1149: filter_collection_->SelectVideoRenderer(
On 2012/07/21 05:02:35, Ami Fischman wrote:
> single line

Done.

https://chromiumcodereview.appspot.com/10796074/diff/1/media/base/video_rende...
File media/base/video_renderer.h (right):

https://chromiumcodereview.appspot.com/10796074/diff/1/media/base/video_rende...
media/base/video_renderer.h:32: // Used to query the current time and duration
of the media.
On 2012/07/21 05:02:35, Ami Fischman wrote:
> s/and/or/

Done.

https://chromiumcodereview.appspot.com/10796074/diff/1/media/base/video_rende...
media/base/video_renderer.h:43: const PipelineStatusCB& error_cb,
On 2012/07/21 05:02:35, Ami Fischman wrote:
> This highlights that "status_cb" is a bad name (should include "initialize"
> somewhere in there).

Agreed - changed status_cb to init_cb

https://chromiumcodereview.appspot.com/10796074/diff/1/media/filters/video_re...
File media/filters/video_renderer_base_unittest.cc (right):

https://chromiumcodereview.appspot.com/10796074/diff/1/media/filters/video_re...
media/filters/video_renderer_base_unittest.cc:59: // We expect these to be
called but we don't care how/when.
On 2012/07/21 05:02:35, Ami Fischman wrote:
> Not sure how to interpret this comment vis-a-vis the previous two
EXPECT_CALL's.
>  Why does it not apply to them?

I guess one is more about configuring the decoder and one is the renderer --
will move decoder statements down here

[commit-bot: I haz the power, 2012-07-23 03:21:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/10796074/1039

[commit-bot: I haz the power, 2012-07-23 03:42:57]

Try job failure for 10796074-1039 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[scherkus (not reviewing), 2012-07-23 06:25:56]

FYI I had to rename Pipeline::GetCurrentTime to GetMediaTime (lines up with
GetMediaDuration)

Apparently for the last 3+ years Pipeline::GetCurrentTime() was defined as
Pipeline::GetTickCount() on windows and we never noticed. winbase.h:55 has
"#define GetCurrentTime() GetTickCount()" which caused us to call
Pipeline::GetTickCount().

The only reason we caught it now is because I added
base::Bind(Pipeline::GetCurrentTime, ...) which doesn't have the trailing () at
the end so we get a windows compile failure that GetCurrentTime() isn't a member
of Pipeline.

I felt switching to GetMediaDuration() instead of #undef'ing GetCurrentTime()
was the lesser of the two evils.


#include <stdio.h>
#define GetCurrentTime() GetTickCount()  // winbase.h:55

class Foo {
 public:
  int GetCurrentTime() { return 5; }
};

int main() {
  Foo* foo = new Foo();

  // Will print "5"
  printf("%d\n", foo->GetCurrentTime());

  // Works fine.
  int (Foo::*b)() = &Foo::GetTickCount;

  // error: ‘GetCurrentTime’ is not a member of ‘Foo’
  int (Foo::*a)() = &Foo::GetCurrentTime;

  return 0;
}

[commit-bot: I haz the power, 2012-07-23 06:26:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/10796074/6002

[commit-bot: I haz the power, 2012-07-23 08:42:56]

Change committed as 147834