Implementation of Pipeline and FilterHost interfaces. This is a large change...

Implementation of Pipeline and FilterHost interfaces.  This is a large change, but all of the objects are interrelated.  

I am also checking in a basic unit test that creates pipeline,
and the data source hangs during initialization.  The test sleeps one second and then stops the pipeline.

Andrew has already done a first pass on this, and the code has come largely from our working experimental branch.

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

[ralphl, 2009-01-23 20:32:59]

Actual code behind the Pipeline and FilterHost interfaces

[brettw, 2009-01-24 19:55:36]

First pass. The code looks very nice overall.

http://codereview.chromium.org/18546/diff/229/55
File media/base/filter_host_impl.h (right):

http://codereview.chromium.org/18546/diff/229/55#newcode51
Line 51: bool GetFilter(Filter** filter_out) const {
Is there an advantage of the way you've done it over returning Filter* and NULL
on failure? I usually prefer the latter way, as it's easier to read.

http://codereview.chromium.org/18546/diff/229/55#newcode61
Line 61: // Call the filter if it has registered a time update callback.
Can you say "see X for more information" where X is the place I should look to
figure out what a time update callback is?

http://codereview.chromium.org/18546/diff/229/55#newcode74
Line 74: PipelineThread* const pipeline_thread_;
Can you write const PipelineThread* instead?

http://codereview.chromium.org/18546/diff/229/55#newcode77
Line 77: FilterType const filter_type_;
Can you write const FilterType instead?

http://codereview.chromium.org/18546/diff/229/63
File media/base/filters.h (right):

http://codereview.chromium.org/18546/diff/229/63#newcode97
Line 97: static inline const FilterType filter_type() {
We don't usually write inline for these getters (same below). The fact that you
implemented it in the class definition is "inline" enough.

http://codereview.chromium.org/18546/diff/229/59
File media/base/media_format.cc (right):

http://codereview.chromium.org/18546/diff/229/59#newcode55
Line 55: const char MediaFormat::kURI[]              = "Uri";
Just noticed this: Can you use URL instead of URI? We've standardized on URL to
avoid a mismash of different names. Unless this string must match something
external.

http://codereview.chromium.org/18546/diff/229/60
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/18546/diff/229/60#newcode107
Line 107: int64 time_;
Hvae you considered using base::TimeDelta for your times? It also has
microsecond resolution, and makes more clear the units, and has a variety of
convenience functions the frontend might want to use.

[scherkus (not reviewing), 2009-01-26 19:22:52]

Little nits.  Looking great, though!

http://codereview.chromium.org/18546/diff/229/55
File media/base/filter_host_impl.h (right):

http://codereview.chromium.org/18546/diff/229/55#newcode49
Line 49: // this method returns true, and *filter_out contains the Filter
interface.
Surround variables in comments with pipes: |filter_out|

http://codereview.chromium.org/18546/diff/229/55#newcode54
Line 54: *filter_out = reinterpret_cast<Filter *>(filter);
Pointer goes with the type name (remove space)

http://codereview.chromium.org/18546/diff/229/63
File media/base/filters.h (right):

http://codereview.chromium.org/18546/diff/229/63#newcode32
Line 32: 
Remove extra line here

http://codereview.chromium.org/18546/diff/229/63#newcode42
Line 42: class MediaFormat;
You can remove this forward declaration since you're including media_format.h

http://codereview.chromium.org/18546/diff/229/60
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/18546/diff/229/60#newcode208
Line 208: // or it could be a string in the case of a DataSource
Period at the end of comments.

http://codereview.chromium.org/18546/diff/229/60#newcode210
Line 210: bool CreateFilter(FilterFactory* filter_factory,
Its actually unclear what this method does exactly, and looking at the source
the comment doesn't seem to explain everything that actually happens.

http://codereview.chromium.org/18546/diff/229/60#newcode211
Line 211: Source source,
Should |source| be a pointer?  Could is be const?

http://codereview.chromium.org/18546/diff/229/60#newcode231
Line 231: // false, and *filter_out = NULL.
Surround variables like |*filter_out|

http://codereview.chromium.org/18546/diff/229/60#newcode260
Line 260: FilterHostImpl* host_initializing_;
I understand the first part of the comment, but I don't quite understand what
the 3 points refer to.  Also I'm not sure what "~~~" means :)

http://codereview.chromium.org/18546/diff/229/60#newcode262
Line 262: Lock initialization_lock_;
Your comments have been great so far -- go for the gold and add one to
|initialization_lock_|!!!

[cpu_(ooo_6.6-7.5), 2009-01-26 19:34:27]

http://codereview.chromium.org/18546/diff/229/55
File media/base/filter_host_impl.h (right):

http://codereview.chromium.org/18546/diff/229/55#newcode55
Line 55: return true;
space between Filter and  *

http://codereview.chromium.org/18546/diff/229/55#newcode78
Line 78: 
Maybe I missed something, but why have the type here if you can always call
Filter::filter_type() ?

http://codereview.chromium.org/18546/diff/229/58
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/18546/diff/229/58#newcode83
Line 83: }
Maybe I am crazy but it seems we have a double delete of the 
init_complete_callback or a really strange ownership model.

http://codereview.chromium.org/18546/diff/229/58#newcode108
Line 108: DCHECK(PIPELINE_OK == error_);
Consider moving the 3 tests into a function:
AllIsGood() {
  return (a && b && c)
}
This way you can do DCHECK(AllIsGood()) and
if (AllIsGood() && time >=0)

Can also simply further by not calling it twice and instead having an else
clause with a NOTREACHED()

http://codereview.chromium.org/18546/diff/229/58#newcode377
Line 377: 
nit: Less empty lines.

http://codereview.chromium.org/18546/diff/229/58#newcode401
Line 401: if (host) {
It would be easier to read:

if (!host) {
  ....
}

Other cases where if(!x) reduces nesting are welcomed.

http://codereview.chromium.org/18546/diff/229/60
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/18546/diff/229/60#newcode98
Line 98: 
Are this values modified on one thread and read on another or modifiable by two
threads?

http://codereview.chromium.org/18546/diff/229/60#newcode197
Line 197: bool HasVideo();
nit: maybe bool HasVideo() const ?

http://codereview.chromium.org/18546/diff/229/60#newcode249
Line 249: 
Also the thread_.thread_id() is non-zero. Thread running or not can be a simple
enhancement to our thread class.

[ralphl, 2009-01-27 00:11:30]

OK, I have fixed everything that anyone pointed out.  I added an "IsRunning()"
method to the Thread class that compares the thread ID to 0.  I also made sure
to zero the thread ID when the thread exits (looks like an existing minor bug).

http://codereview.chromium.org/18546/diff/229/55
File media/base/filter_host_impl.h (right):

http://codereview.chromium.org/18546/diff/229/55#newcode49
Line 49: // this method returns true, and *filter_out contains the Filter
interface.
On 2009/01/26 19:22:52, scherkus wrote:
> Surround variables in comments with pipes: |filter_out|

Changed function to return Filter* or NULL so comment does not have a variable
name anymore.

http://codereview.chromium.org/18546/diff/229/55#newcode51
Line 51: bool GetFilter(Filter** filter_out) const {
On 2009/01/24 19:55:37, brettw wrote:
> Is there an advantage of the way you've done it over returning Filter* and
NULL
> on failure? I usually prefer the latter way, as it's easier to read.

I changed it, but now it requires the caller to be explicit in invoking the
template.  The old code looked like this:

VideoDecoder* video_decoder;
if (GetFilter(&video_decoder)) ...

It now looks like this:
VideoDecoder* video_decoder = GetFilter<VideoDecoder>();
if (video_decoder) ....

Because the template argument can not be deduced by the complier.  It still is
type safe, etc, just slightly more verbose.

http://codereview.chromium.org/18546/diff/229/55#newcode54
Line 54: *filter_out = reinterpret_cast<Filter *>(filter);
On 2009/01/26 19:22:52, scherkus wrote:
> Pointer goes with the type name (remove space)

Done.

http://codereview.chromium.org/18546/diff/229/55#newcode55
Line 55: return true;
On 2009/01/26 19:34:27, cpu wrote:
> space between Filter and  *
> 
> 

Done.

http://codereview.chromium.org/18546/diff/229/55#newcode61
Line 61: // Call the filter if it has registered a time update callback.
On 2009/01/24 19:55:37, brettw wrote:
> Can you say "see X for more information" where X is the place I should look to
> figure out what a time update callback is?

Done.

http://codereview.chromium.org/18546/diff/229/55#newcode74
Line 74: PipelineThread* const pipeline_thread_;
On 2009/01/24 19:55:37, brettw wrote:
> Can you write const PipelineThread* instead?

Can't do that because we don't use the PipelineThread* in a const way.  The
const following a * tells the complier that this member variable can never be
changed, and can only be assigned in the class initialization list.  This means
that the POINTER is constant and never changes, but the underlying state of the
class can change.  This is useful since you guarantee that the pipeline_thread_
member will always remain the same, and could never, for example, be set to NULL
or otherwise changed.

http://codereview.chromium.org/18546/diff/229/55#newcode77
Line 77: FilterType const filter_type_;
On 2009/01/24 19:55:37, brettw wrote:
> Can you write const FilterType instead?

Same as above.  This syntax means that the member can never be changed
throughout the life of the object.  That's exactly the behavior we want.

http://codereview.chromium.org/18546/diff/229/55#newcode78
Line 78: 
On 2009/01/26 19:34:27, cpu wrote:
> Maybe I missed something, but why have the type here if you can always call
> Filter::filter_type() ?

The filter_type() method is static.  When the template constructor is called, is
stores the filter_type() by using the specified class in the template.  After
the invocation of the constructor, the FilterHostImpl holds a pointer to the
base class MediaFilter, NOT to the derived class.  This allows us to then have a
vector of generic FilterHostImpl objects.  If we tried to templatize the entire
class, not just the constructor, we would not be able to treat them as objects
we could iterate over.  

So, this member stores away the type of filter we actually hold, and the
GetFilter() method can safely re-cast the base pointer to the desired interface.

If you can think of a cooler/cleaner way to do this I'd be happy to change it. 
This is the best way I came up with to do it -- a generic container that still
allows for type-safety when accessing derived interfaces from a base class.

http://codereview.chromium.org/18546/diff/229/63
File media/base/filters.h (right):

http://codereview.chromium.org/18546/diff/229/63#newcode32
Line 32: 
On 2009/01/26 19:22:52, scherkus wrote:
> Remove extra line here

Done.

http://codereview.chromium.org/18546/diff/229/63#newcode42
Line 42: class MediaFormat;
On 2009/01/26 19:22:52, scherkus wrote:
> You can remove this forward declaration since you're including media_format.h

Done.

http://codereview.chromium.org/18546/diff/229/63#newcode97
Line 97: static inline const FilterType filter_type() {
On 2009/01/24 19:55:37, brettw wrote:
> We don't usually write inline for these getters (same below). The fact that
you
> implemented it in the class definition is "inline" enough.

Done.

http://codereview.chromium.org/18546/diff/229/59
File media/base/media_format.cc (right):

http://codereview.chromium.org/18546/diff/229/59#newcode55
Line 55: const char MediaFormat::kURI[]              = "Uri";
On 2009/01/24 19:55:37, brettw wrote:
> Just noticed this: Can you use URL instead of URI? We've standardized on URL
to
> avoid a mismash of different names. Unless this string must match something
> external.

Done.

http://codereview.chromium.org/18546/diff/229/58
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/18546/diff/229/58#newcode83
Line 83: }
On 2009/01/26 19:34:27, cpu wrote:
> Maybe I am crazy but it seems we have a double delete of the 
> init_complete_callback or a really strange ownership model.
> 

If everything works OK, that is the Start method is called correctly, then the
function exits without deleting the callback.
>

http://codereview.chromium.org/18546/diff/229/58#newcode108
Line 108: DCHECK(PIPELINE_OK == error_);
On 2009/01/26 19:34:27, cpu wrote:
> Consider moving the 3 tests into a function:
> AllIsGood() {
>   return (a && b && c)
> }
> This way you can do DCHECK(AllIsGood()) and
> if (AllIsGood() && time >=0)
> 
> Can also simply further by not calling it twice and instead having an else
> clause with a NOTREACHED()

Done.

http://codereview.chromium.org/18546/diff/229/58#newcode377
Line 377: 
On 2009/01/26 19:34:27, cpu wrote:
> nit: Less empty lines.

Done.

http://codereview.chromium.org/18546/diff/229/58#newcode401
Line 401: if (host) {
On 2009/01/26 19:34:27, cpu wrote:
> It would be easier to read:
> 
> if (!host) {
>   ....
> }
> 
> Other cases where if(!x) reduces nesting are welcomed.
> 
> 

I reworked it to look more like the StartTask logic.  See if you like it better.
 It seems much more clear to me.

http://codereview.chromium.org/18546/diff/229/60
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/18546/diff/229/60#newcode98
Line 98: 
On 2009/01/26 19:34:27, cpu wrote:
> Are this values modified on one thread and read on another or modifiable by
two
> threads?

All modifications take place on the PipelineThread.  They will be read from
multiple threads.  The only real race condition I can see is with the video
size.  I have added a Lock around access to video width and height to make sure
it is an atomic operation.

http://codereview.chromium.org/18546/diff/229/60#newcode107
Line 107: int64 time_;
On 2009/01/24 19:55:37, brettw wrote:
> Hvae you considered using base::TimeDelta for your times? It also has
> microsecond resolution, and makes more clear the units, and has a variety of
> convenience functions the frontend might want to use.

Done.

http://codereview.chromium.org/18546/diff/229/60#newcode197
Line 197: bool HasVideo();
On 2009/01/26 19:34:27, cpu wrote:
> nit: maybe bool HasVideo() const ?

Done.

http://codereview.chromium.org/18546/diff/229/60#newcode208
Line 208: // or it could be a string in the case of a DataSource
On 2009/01/26 19:22:52, scherkus wrote:
> Period at the end of comments.

Done.

http://codereview.chromium.org/18546/diff/229/60#newcode210
Line 210: bool CreateFilter(FilterFactory* filter_factory,
On 2009/01/26 19:22:52, scherkus wrote:
> Its actually unclear what this method does exactly, and looking at the source
> the comment doesn't seem to explain everything that actually happens.

Done.

http://codereview.chromium.org/18546/diff/229/60#newcode211
Line 211: Source source,
On 2009/01/26 19:22:52, scherkus wrote:
> Should |source| be a pointer?  Could is be const?

In one case source is a reference to a string, so it's not a pointer.  It can't
be declared to be const since the object will not be treated in a const method
-- the data source will be read from, for example.  I think the current
declaration is correct.

http://codereview.chromium.org/18546/diff/229/60#newcode231
Line 231: // false, and *filter_out = NULL.
On 2009/01/26 19:22:52, scherkus wrote:
> Surround variables like |*filter_out|

Changed this method, so issue no longer exists.

http://codereview.chromium.org/18546/diff/229/60#newcode249
Line 249: 
On 2009/01/26 19:34:27, cpu wrote:
> Also the thread_.thread_id() is non-zero. Thread running or not can be a
simple
> enhancement to our thread class.
> 
> 

I changed Thread.h and Thread.cc.  When the thread exited it did not set
thread_id_ back to 0 so I added that.  In addition, I added an inline method
IsRunning() to the Thread class which returns a bool if the thread has been
started, but not yet stopped.

http://codereview.chromium.org/18546/diff/229/60#newcode260
Line 260: FilterHostImpl* host_initializing_;
On 2009/01/26 19:22:52, scherkus wrote:
> I understand the first part of the comment, but I don't quite understand what
> the 3 points refer to.  Also I'm not sure what "~~~" means :)

Done.

http://codereview.chromium.org/18546/diff/229/60#newcode262
Line 262: Lock initialization_lock_;
On 2009/01/26 19:22:52, scherkus wrote:
> Your comments have been great so far -- go for the gold and add one to
> |initialization_lock_|!!!

You're right.  DONE!

[scherkus (not reviewing), 2009-01-27 20:28:47]

Mostly vertical line spacing nits.  In general, functions are separated by one
line of white spaces, and comments have one line of white space preceding them
if they're not the first comment in the same block of scope.

http://codereview.chromium.org/18546/diff/286/153
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/18546/diff/286/153#newcode279
Line 279: // Add ourselves as a destruction observer of the thread's message
loop so
vertical space here

http://codereview.chromium.org/18546/diff/286/153#newcode304
Line 304: // No matter what, we're done with the filter factory, and
vertical space here

http://codereview.chromium.org/18546/diff/286/153#newcode345
Line 345: 
get rid of extra vertical spaces here

http://codereview.chromium.org/18546/diff/286/153#newcode372
Line 372: 
get rid of extra vertical spaces here

http://codereview.chromium.org/18546/diff/286/153#newcode382
Line 382: 
get rid of extra vertical spaces here

http://codereview.chromium.org/18546/diff/286/153#newcode393
Line 393: 
get rid of extra vertical spaces here

http://codereview.chromium.org/18546/diff/286/153#newcode396
Line 396: bool PipelineThread::CreateFilter(FilterFactory * filter_factory,
* goes with the type

http://codereview.chromium.org/18546/diff/286/153#newcode414
Line 414: // The filter must return true from initialize and there must still
not
vertical space here

http://codereview.chromium.org/18546/diff/286/153#newcode420
Line 420: message_loop()->SetNestableTasksAllowed(true);
could we have a comment here describing what's happening?  It looks like some
voodoo magic happens with |host_initializing_| magically becoming NULL.

http://codereview.chromium.org/18546/diff/286/153#newcode424
Line 424: // If an error occurred while we were in the nested Run state, then
vertical space here

http://codereview.chromium.org/18546/diff/286/153#newcode429
Line 429: // This could still be set if we never ran the message loop (for
example,
vertical space here

http://codereview.chromium.org/18546/diff/286/153#newcode433
Line 433: // If this method fails, but no error set, then indicate a general
vertical space here

http://codereview.chromium.org/18546/diff/286/153#newcode440
Line 440: 
get rid of extra vertical spaces here

http://codereview.chromium.org/18546/diff/286/159
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/18546/diff/286/159#newcode14
Line 14: using media::PipelineImpl;
could you alphabetize the forward declarations?

[ralphl, 2009-01-27 21:06:40]

Fixed all of Andrew's comments

http://codereview.chromium.org/18546/diff/286/153
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/18546/diff/286/153#newcode279
Line 279: // Add ourselves as a destruction observer of the thread's message
loop so
On 2009/01/27 20:28:47, scherkus wrote:
> vertical space here

Done.

http://codereview.chromium.org/18546/diff/286/153#newcode304
Line 304: // No matter what, we're done with the filter factory, and
On 2009/01/27 20:28:47, scherkus wrote:
> vertical space here

Done.

http://codereview.chromium.org/18546/diff/286/153#newcode345
Line 345: 
On 2009/01/27 20:28:47, scherkus wrote:
> get rid of extra vertical spaces here

Done.

http://codereview.chromium.org/18546/diff/286/153#newcode372
Line 372: 
On 2009/01/27 20:28:47, scherkus wrote:
> get rid of extra vertical spaces here

Done.

http://codereview.chromium.org/18546/diff/286/153#newcode382
Line 382: 
On 2009/01/27 20:28:47, scherkus wrote:
> get rid of extra vertical spaces here

Done.

http://codereview.chromium.org/18546/diff/286/153#newcode393
Line 393: 
On 2009/01/27 20:28:47, scherkus wrote:
> get rid of extra vertical spaces here

Done.

http://codereview.chromium.org/18546/diff/286/153#newcode396
Line 396: bool PipelineThread::CreateFilter(FilterFactory * filter_factory,
On 2009/01/27 20:28:47, scherkus wrote:
> * goes with the type

Done.

http://codereview.chromium.org/18546/diff/286/153#newcode414
Line 414: // The filter must return true from initialize and there must still
not
On 2009/01/27 20:28:47, scherkus wrote:
> vertical space here

Done.

http://codereview.chromium.org/18546/diff/286/153#newcode420
Line 420: message_loop()->SetNestableTasksAllowed(true);
On 2009/01/27 20:28:47, scherkus wrote:
> could we have a comment here describing what's happening?  It looks like some
> voodoo magic happens with |host_initializing_| magically becoming NULL.

Done.

http://codereview.chromium.org/18546/diff/286/153#newcode424
Line 424: // If an error occurred while we were in the nested Run state, then
On 2009/01/27 20:28:47, scherkus wrote:
> vertical space here

Done.

http://codereview.chromium.org/18546/diff/286/153#newcode429
Line 429: // This could still be set if we never ran the message loop (for
example,
On 2009/01/27 20:28:47, scherkus wrote:
> vertical space here

Done.

http://codereview.chromium.org/18546/diff/286/153#newcode433
Line 433: // If this method fails, but no error set, then indicate a general
On 2009/01/27 20:28:47, scherkus wrote:
> vertical space here

Done.

http://codereview.chromium.org/18546/diff/286/153#newcode440
Line 440: 
On 2009/01/27 20:28:47, scherkus wrote:
> get rid of extra vertical spaces here

Done.

http://codereview.chromium.org/18546/diff/286/159
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/18546/diff/286/159#newcode14
Line 14: using media::PipelineImpl;
On 2009/01/27 20:28:47, scherkus wrote:
> could you alphabetize the forward declarations?

Done.

[scherkus (not reviewing), 2009-01-27 21:21:42]

Just one question, otherwise LGTM pending what brettw and cpu have to say.

http://codereview.chromium.org/18546/diff/172/178
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/18546/diff/172/178#newcode119
Line 119: if (pipeline_thread_ && initialized_ && PIPELINE_OK == error_ &&
Can you use OkToCallThread() here?

[brettw, 2009-01-27 23:36:32]

LGTM

http://codereview.chromium.org/18546/diff/172/185
File base/thread.cc (right):

http://codereview.chromium.org/18546/diff/172/185#newcode32
Line 32: explicit StartupData(const Options& opt) : options(opt),
When you have to wrap initializers, put the colon on the next line indented 4
spaces.

http://codereview.chromium.org/18546/diff/172/178
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/18546/diff/172/178#newcode128
Line 128: duration_         = base::TimeDelta::FromMicroseconds(0);
Feel free to just say base::TimeDelta() which is guaranteed to make a 0 delta.

[ralphl, 2009-01-28 00:27:47]

OK, I fixed the final issues, and we fixed a problem with mac builds.  I'm going
to check this in.

http://codereview.chromium.org/18546/diff/172/185
File base/thread.cc (right):

http://codereview.chromium.org/18546/diff/172/185#newcode32
Line 32: explicit StartupData(const Options& opt) : options(opt),
On 2009/01/27 23:36:32, brettw wrote:
> When you have to wrap initializers, put the colon on the next line indented 4
> spaces.

Done.

http://codereview.chromium.org/18546/diff/172/178
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/18546/diff/172/178#newcode119
Line 119: if (pipeline_thread_ && initialized_ && PIPELINE_OK == error_ &&
On 2009/01/27 21:21:42, scherkus wrote:
> Can you use OkToCallThread() here?

Done.

http://codereview.chromium.org/18546/diff/172/178#newcode128
Line 128: duration_         = base::TimeDelta::FromMicroseconds(0);
On 2009/01/27 23:36:32, brettw wrote:
> Feel free to just say base::TimeDelta() which is guaranteed to make a 0 delta.

Done.

http://codereview.chromium.org/18546/diff/172/178#newcode128
Line 128: duration_         = base::TimeDelta::FromMicroseconds(0);
On 2009/01/27 23:36:32, brettw wrote:
> Feel free to just say base::TimeDelta() which is guaranteed to make a 0 delta.

Done.