Pipeline_Impl was modified to properly render a stream that has video but no ...

Pipeline_Impl was modified to properly render a stream that has video but no audio.  A unit test accompanies the change.

Note that one minor other change was snuck in with this change.  The data_source_impl.cc had a TODO to set a more specific error
when a read failed.  Because I was already updating the pipeline error enum, I added the error code, changed the call to
host_->Error(), and removed the TODO.

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

[ralphl, 2009-03-05 01:01:50]

Pipeline is now happy to render a video with no audio (it has worked with audio
but no video already).

[scherkus (not reviewing), 2009-03-05 08:16:21]

first pass - few comments

http://codereview.chromium.org/39170/diff/1/2
File media/base/pipeline.h (right):

http://codereview.chromium.org/39170/diff/1/2#newcode30
Line 30: PIPELINE_STOPPING
You mentioned OK and STOPPING were special value used internally, maybe add
comments here or by the Error() method?

http://codereview.chromium.org/39170/diff/1/4
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/39170/diff/1/4#newcode359
Line 359: if (PipelineOk()) {
what if there are no streams available (couldn't demux, unsupported, etc..)

http://codereview.chromium.org/39170/diff/1/4#newcode402
Line 402: const std::string major_mime_type = Decoder::major_mime_type();
I guess this would be kMajorMimeType if its a constant

http://codereview.chromium.org/39170/diff/1/4#newcode490
Line 490: host_initializing_ = new FilterHostImpl(this, filter.get());
I recently found out that C++ is smart and automatically converts a
scoped_refptr<T> into a T* for you, so you can get rid of the .get()s if you
find it makes it clearer (your call -- just a readability thing I suppose)

http://codereview.chromium.org/39170/diff/1/4#newcode517
Line 517: // if the fiter returned false from it's Initialize method), so make
sure
Initialize -> Initialize()

[Erik does not do reviews, 2009-03-05 17:27:35]

http://codereview.chromium.org/39170/diff/1/4
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/39170/diff/1/4#newcode110
Line 110: if (PIPELINE_OK == error_) {
This means you lose errors after the first.  It seems like you should at least
log these errors.  If it's illegal to set multiple errors, then perhaps DCHECK.

http://codereview.chromium.org/39170/diff/1/4#newcode521
Line 521: // While this looks strange, it is correct.  The FilterHost will hold
a
I disagree.  While it may work now, returning a bare ref-counted object is
asking for trouble later.  The correct pattern is to return scoped_refptr<T>
rather than T.  In cases where perf matters, then take a scoped_refptr<T>* as an
out ptr.

http://codereview.chromium.org/39170/diff/1/3
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/39170/diff/1/3#newcode237
Line 237: // Uses the FilterFactory to create a new filter of the NewFilter
class, and
comment out of date

http://codereview.chromium.org/39170/diff/1/5
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/39170/diff/1/5#newcode138
Line 138: h.TimedWait(base::TimeDelta::FromSeconds(5));
a hard-coded timeout seems pretty fragile and just slows down unit tests.  Is
there some reason it's 5 seconds?  Can you have a test helper that allows you to
wait on a condition instead?

http://codereview.chromium.org/39170/diff/1/5#newcode142
Line 142: EXPECT_TRUE(media::PIPELINE_OK == p.GetError());
shouldn't this use your new helper method?

[ralphl, 2009-03-06 00:20:46]

The number of files has grown quite a bit to address some of Erik's concerns
(which I agree with).  Most changes are quite minor, but it touched a bunch more
files.  Big changes are always using scoped_refptr's when returning Filters and
changed the test InitializationHelper to do more work, and always wait on
Start() to simplify tests and make wait times consistent across all tests.  We
now wait 3 seconds if we do not expect to hang in initialization (so the actual
wait time should be much less than 3 seconds) and only 1/3 second if we are
specifically testing the hang-the-pipeline-and-see-if-we-can-stop-it-anyway
condition.

http://codereview.chromium.org/39170/diff/1/2
File media/base/pipeline.h (right):

http://codereview.chromium.org/39170/diff/1/2#newcode30
Line 30: PIPELINE_STOPPING
On 2009/03/05 08:16:21, scherkus wrote:
> You mentioned OK and STOPPING were special value used internally, maybe add
> comments here or by the Error() method?

Done.

http://codereview.chromium.org/39170/diff/1/4
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/39170/diff/1/4#newcode110
Line 110: if (PIPELINE_OK == error_) {
On 2009/03/05 17:27:35, Erik Kay wrote:
> This means you lose errors after the first.  It seems like you should at least
> log these errors.  If it's illegal to set multiple errors, then perhaps
DCHECK.

It is sort of a feature of this method that it does not reset the error if one
is already set.  A specific error, like URL not found, would potentially
generate a more general error (like initialization of a filter failed).  In any
case, the fact that error_ != PIPELINE_OK will cause everything to shut down
immediately.

Is there some sort of debug log that is standard to write to for Chrome code? 
If so, I can log it there.  If not, I don't want to write code (that could
potentially have bugs) to remember a list of errors.

http://codereview.chromium.org/39170/diff/1/4#newcode359
Line 359: if (PipelineOk()) {
On 2009/03/05 08:16:21, scherkus wrote:
> what if there are no streams available (couldn't demux, unsupported, etc..)

I suppose if you render absolutely nothing that you have got a problem. I've
added an error code and now check that at least SOMETHING is rendered.

http://codereview.chromium.org/39170/diff/1/4#newcode402
Line 402: const std::string major_mime_type = Decoder::major_mime_type();
On 2009/03/05 08:16:21, scherkus wrote:
> I guess this would be kMajorMimeType if its a constant

This is a static function, which does return a constant, but   I prefer calling
a Class::Static_Fctn over Class::const.

http://codereview.chromium.org/39170/diff/1/4#newcode490
Line 490: host_initializing_ = new FilterHostImpl(this, filter.get());
On 2009/03/05 08:16:21, scherkus wrote:
> I recently found out that C++ is smart and automatically converts a
> scoped_refptr<T> into a T* for you, so you can get rid of the .get()s if you
> find it makes it clearer (your call -- just a readability thing I suppose)

Oddly, this is the only place that I have found that actually requires the
.get() to compile.  I believe that it is because the constructor for
FilterHostImpl is a template that has the Filter parameter as a template class
type.  Even if I try new FilterHostImpl<Filter>(this, filter) it won't work. 
The .get() has to stay here.  I've removed all other .get()'s

http://codereview.chromium.org/39170/diff/1/4#newcode517
Line 517: // if the fiter returned false from it's Initialize method), so make
sure
On 2009/03/05 08:16:21, scherkus wrote:
> Initialize -> Initialize()

I have fixed this POST patch 2, so it is in there.  No reason to re-send code
review servers just for this though.

http://codereview.chromium.org/39170/diff/1/4#newcode521
Line 521: // While this looks strange, it is correct.  The FilterHost will hold
a
On 2009/03/05 17:27:35, Erik Kay wrote:
> I disagree.  While it may work now, returning a bare ref-counted object is
> asking for trouble later.  The correct pattern is to return scoped_refptr<T>
> rather than T.  In cases where perf matters, then take a scoped_refptr<T>* as
an
> out ptr.

Done, but this also means that our FilterFactory implementation needs to support
the same pattern.  I'll do that in a seperate CL since it would impact too many
files.  Everything internally now adheres to the scoped_refptr rules.  Perf is
not much of an issue.

http://codereview.chromium.org/39170/diff/1/3
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/39170/diff/1/3#newcode237
Line 237: // Uses the FilterFactory to create a new filter of the NewFilter
class, and
On 2009/03/05 17:27:35, Erik Kay wrote:
> comment out of date

Done.

http://codereview.chromium.org/39170/diff/1/5
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/39170/diff/1/5#newcode138
Line 138: h.TimedWait(base::TimeDelta::FromSeconds(5));
On 2009/03/05 17:27:35, Erik Kay wrote:
> a hard-coded timeout seems pretty fragile and just slows down unit tests.  Is
> there some reason it's 5 seconds?  Can you have a test helper that allows you
to
> wait on a condition instead?

This wait is expected to return immediately and never time-out.  It will only
time-out when something in the test breaks.  I just don't want the test to hang
if it fails.  I suppose I could add something like h.WaitExpectSuccess() which
would wait some standard amount of time and we could adjust it for all tests.

Every test was using the helper class the same way, so I moved lots of the
EXPECT's and all of the Wait()'s into the helper.  It makes the tests shorter,
and the default time is specified in a single place

http://codereview.chromium.org/39170/diff/1/5#newcode142
Line 142: EXPECT_TRUE(media::PIPELINE_OK == p.GetError());
On 2009/03/05 17:27:35, Erik Kay wrote:
> shouldn't this use your new helper method?

The helper is private.  Pipeline.h specifies a virtual interface, and I don't
want to expose IsOk() as a method in that contract.

[Erik does not do reviews, 2009-03-06 00:44:42]

LGTM

http://codereview.chromium.org/39170/diff/1/4
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/39170/diff/1/4#newcode110
Line 110: if (PIPELINE_OK == error_) {
On 2009/03/06 00:20:47, ralphl wrote:
> On 2009/03/05 17:27:35, Erik Kay wrote:
> > This means you lose errors after the first.  It seems like you should at
least
> > log these errors.  If it's illegal to set multiple errors, then perhaps
> DCHECK.
> 
> It is sort of a feature of this method that it does not reset the error if one
> is already set.  A specific error, like URL not found, would potentially
> generate a more general error (like initialization of a filter failed).  In
any
> case, the fact that error_ != PIPELINE_OK will cause everything to shut down
> immediately.
> 
> Is there some sort of debug log that is standard to write to for Chrome code? 
> If so, I can log it there.  If not, I don't want to write code (that could
> potentially have bugs) to remember a list of errors.

Yes.  You can use LOG(WARNING) << msg; or LOG(ERROR) << msg;.  There are plenty
of examples of this in the code.

http://codereview.chromium.org/39170/diff/1006/17#newcode493
Line 493: } else {
For this you need to say NULL == *filter_out.  It won't implicitly dereference.

[ralphl, 2009-03-06 01:22:50]

Fixed a bug in unit test.  I was waiting for 3000 MICROSEDONDS which was making
tests flake out since we'd time-out before a test like opening a file completed.

Also fixed bug Erik pointed out and added a TODO in the pipeline unit test to
make sure we actually make a call to the audio renderer.  I'll modify the mocks
tomorrow and get that test completed.  The code I had written would never have
called SetVolume() on the renderer, even though all unit tests passed.

http://codereview.chromium.org/39170/diff/1006/17
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/39170/diff/1006/17#newcode493
Line 493: iter != filter_hosts_.end() && NULL == filter_out;
On 2009/03/06 00:44:43, Erik Kay wrote:
> For this you need to say NULL == *filter_out.  It won't implicitly
dereference.

Good catch.  I need a unit test for this.  It used to be a core function, but
now it's only used to find the audio filter to set the volume.

[scherkus (not reviewing), 2009-03-06 18:52:20]

LGTM with some typos

http://codereview.chromium.org/39170/diff/29/32
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/39170/diff/29/32#newcode302
Line 302: // the filter, |*filer_out| will be NULL.
typo on filter_out