Fix flow control in media::DecoderBase

Fix flow control in media::DecoderBase

The flow control in media::DecoderBase was incorrect because it reads too
aggressively to the demuxer stream and failed some DCHECKs when asynchronous
decoding like OpenMAX is used.

An example of a failing case is:
Action    Pending Read    Pending Decode   Read Request
Read           1                0               1
Read           2                0               2
ReadComplete   1                1               2
ReadComplete   0                2               2
DecodeComplete 1                1               1
DecodeComplete 1                0               0

Because of the aggressive read in OnDecodeComplete in DecoderBase. Even
if all the read requests are fulfiled there is still on pending read issued
to the demuxer stream. This mismatch is fixed in this patch.

BUG=32947
TEST=media_unittests

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

[scherkus (not reviewing), 2010-02-26 19:39:39]

looking good!  few comments

http://codereview.chromium.org/660170/diff/1/3
File media/filters/decoder_base_unittest.cc (right):

http://codereview.chromium.org/660170/diff/1/3#newcode12
media/filters/decoder_base_unittest.cc:12: //using ::testing::_;
comment out?

http://codereview.chromium.org/660170/diff/1/3#newcode53
media/filters/decoder_base_unittest.cc:53: Task* done_cb));
alignment.. .maybe drop the void(..) bit to next line?

http://codereview.chromium.org/660170/diff/1/3#newcode64
media/filters/decoder_base_unittest.cc:64: class MockReadCallback {
this is somewhat overkill

you can rely on valgrind to catch OnCallbackDestroyed and you can create a
normal callback on a mock method to catch the expectations

refer to pipeline_impl_unittest.cc:

    EXPECT_CALL(callbacks_, OnStart());
    pipeline_->Start(mocks_, "",
                     NewCallback(reinterpret_cast<CallbackHelper*>(&callbacks_),
                                 &CallbackHelper::OnStart));

[Alpha Left Google, 2010-02-26 20:18:52]

http://codereview.chromium.org/660170/diff/1/3
File media/filters/decoder_base_unittest.cc (right):

http://codereview.chromium.org/660170/diff/1/3#newcode12
media/filters/decoder_base_unittest.cc:12: //using ::testing::_;
On 2010/02/26 19:39:39, scherkus wrote:
> comment out?

Done.

http://codereview.chromium.org/660170/diff/1/3#newcode53
media/filters/decoder_base_unittest.cc:53: Task* done_cb));
On 2010/02/26 19:39:39, scherkus wrote:
> alignment.. .maybe drop the void(..) bit to next line?

Done.

http://codereview.chromium.org/660170/diff/1/3#newcode64
media/filters/decoder_base_unittest.cc:64: class MockReadCallback {
On 2010/02/26 19:39:39, scherkus wrote:
> this is somewhat overkill
> 
> you can rely on valgrind to catch OnCallbackDestroyed and you can create a
> normal callback on a mock method to catch the expectations
> 
> refer to pipeline_impl_unittest.cc:
> 
>     EXPECT_CALL(callbacks_, OnStart());
>     pipeline_->Start(mocks_, "",
>                     
NewCallback(reinterpret_cast<CallbackHelper*>(&callbacks_),
>                                  &CallbackHelper::OnStart));
> 

This is not for destroyed callback though. It is for catching the callback with
parameter.

(I can see now why Task is better :))

[Alpha Left Google, 2010-02-26 21:42:11]

changed the callback code for review

[scherkus (not reviewing), 2010-02-26 22:03:17]

LGTM w/ warning :)

http://codereview.chromium.org/660170/diff/14/16
File media/filters/decoder_base_unittest.cc (right):

http://codereview.chromium.org/660170/diff/14/16#newcode110
media/filters/decoder_base_unittest.cc:110: &MockReadCallback::ReadCallback));
make sure this passes on try servesr.. I had to use some strange
reinterpret_cast syntax