refactoring decoder interface

refactoring decoder interface

1. install permanent buffer exchange  callback.
2. render provide buffer in read=>fillthisbuffer.
3. for ffmpeg path, the provided buffer is just dummy. it had no relation to decoded buffer. so as to keep the code almost same.

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

[jiesun, 2010-05-25 20:56:20]

please review

[Alpha Left Google, 2010-05-25 22:30:21]

Looks mostly correct, need to add some DCHECKs back to DecoderBase.

http://codereview.chromium.org/2101022/diff/5001/6001
File media/base/filters.h (right):

http://codereview.chromium.org/2101022/diff/5001/6001#newcode269
media/base/filters.h:269: virtual void SetFillBufferCallback(FillBufferCallback*
callback) {
Explains what is a FillBufferCallback. I think FillBufferDoneCallback is a
better name though..
Also mention this callback is called with the filled buffer as parameter in
response to a FillThisBuffer() call.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode272
media/base/filters.h:272: 
Add a protected getter for fill_buffer_callback_.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode274
media/base/filters.h:274: virtual void FillThisBuffer(scoped_refptr<VideoFrame>
frame) = 0;
Also mention in the comment the frame can be null in which case the decoder will
allocate the memory.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode276
media/base/filters.h:276: scoped_ptr<FillBufferCallback> fill_buffer_callback_;
fill_buffer_callback_ needs to be private.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode298
media/base/filters.h:298: virtual void SetFillBufferCallback(FillBufferCallback*
callback) {
Explains what is a FillBufferCallback. I think FillBufferDoneCallback is a
better name though..
Also mention this callback is called with the filled buffer as parameter in
response to a FillThisBuffer() call.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode301
media/base/filters.h:301: 
Add a protected getter for fill_buffer_callback_.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode304
media/base/filters.h:304: virtual void FillThisBuffer(scoped_refptr<Buffer>
buffer) = 0;
The comment says not implemented, I believe what's not implemented is that
renderer provides the buffer. I would rather say that renderer can provide a
NULL buffer and Decoder will be responsible for allocating the output buffer and
returning it to the renderer.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode306
media/base/filters.h:306: scoped_ptr<FillBufferCallback> fill_buffer_callback_;
fill_buffer_callback_ needs to be private.

http://codereview.chromium.org/2101022/diff/5001/6003
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/2101022/diff/5001/6003#newcode234
media/filters/audio_renderer_base.cc:234: scoped_refptr<Buffer> buffer;
Add a TODO here saying that we can provide a buffer pool to reduce the amount of
buffer allocation calls.

http://codereview.chromium.org/2101022/diff/5001/6004
File media/filters/audio_renderer_base.h (right):

http://codereview.chromium.org/2101022/diff/5001/6004#newcode60
media/filters/audio_renderer_base.h:60: virtual void
OnReadComplete(scoped_refptr<Buffer> buffer_in);
Since you are renaming Read() to FillThisBuffer() this should be changed
accordingly.

http://codereview.chromium.org/2101022/diff/5001/6005
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/2101022/diff/5001/6005#newcode91
media/filters/audio_renderer_base_unittest.cc:91: void
EnqueueCallback(scoped_refptr<Buffer> buffer) {
This is not enqueuing anymore so rename this method. Also we don't need the
read_queue_ since the audio renderer doesn't provide the buffer pool, we can
simply increment the count here.

http://codereview.chromium.org/2101022/diff/5001/6006
File media/filters/decoder_base.h (left):

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode127
media/filters/decoder_base.h:127: DCHECK_LE(pending_reads_, read_queue_.size());
We still want to keep this DCHECK, this is a safety check for FFmpeg, this can
be done in a form of count instead of read_queue_.size().

OpenMAX won't be using DecoderBase so keeping this DCHECK doesn't affect
OpenMAX.

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode129
media/filters/decoder_base.h:129: DCHECK_LT(pending_reads_, read_queue_.size());
Keep this DCHECK, just do it in another form.

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode164
media/filters/decoder_base.h:164: DCHECK(read_queue_.empty()) << "Read requests
should be empty";
Do a DCHECK here to make sure something like |fill_buffer_count_| == 0;

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode228
media/filters/decoder_base.h:228: read_queue_.push_back(read_callback);
increment |fill_buffer_count_| here.

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode263
media/filters/decoder_base.h:263: if (read_queue_.empty() ||
result_queue_.empty()) {
if (!fill_buffer_count_ || ...)

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode271
media/filters/decoder_base.h:271: read_queue_.pop_front();
decrement |fill_buffer_count_|.

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode301
media/filters/decoder_base.h:301: ReadQueue read_queue_;
We don't need a readqueue now, but we still need a count of how many
FillThisBuffer() call received.

http://codereview.chromium.org/2101022/diff/5001/6006
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/2101022/diff/5001/6006#newcode260
media/filters/decoder_base.h:260: Decoder::fill_buffer_callback_->Run(output);
Use a getter instead of access Decoder's member directly.

http://codereview.chromium.org/2101022/diff/5001/6007
File media/filters/decoder_base_unittest.cc (right):

http://codereview.chromium.org/2101022/diff/5001/6007#newcode55
media/filters/decoder_base_unittest.cc:55: public:
Remove these lines, looks like they are not doing anything.

http://codereview.chromium.org/2101022/diff/5001/6007#newcode151
media/filters/decoder_base_unittest.cc:151: scoped_refptr<MockDecoderOutput>
output;
I don't think this is the right way of testing this.

I would expect you do
MockDecoderOutputConsumer consumer;
decoder->SetFillBufferCallback(NewCallback(&consumer,
MockDecoderOutputConsumer::FillBufferCallback));

and then you do decoder->FillThisBuffer(output); and expects
MockDecoderOutputConsumer be called.

http://codereview.chromium.org/2101022/diff/5001/6008
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/2101022/diff/5001/6008#newcode372
media/filters/video_renderer_base.cc:372: scoped_refptr<VideoFrame> video_frame;
// dummy frame;
Add some comment to explain why.

http://codereview.chromium.org/2101022/diff/5001/6009
File media/filters/video_renderer_base.h (right):

http://codereview.chromium.org/2101022/diff/5001/6009#newcode86
media/filters/video_renderer_base.h:86: void
OnReadComplete(scoped_refptr<VideoFrame> frame);
Again this should match FillThisBuffer().

http://codereview.chromium.org/2101022/diff/5001/6010
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/2101022/diff/5001/6010#newcode82
media/filters/video_renderer_base_unittest.cc:82: // Receives asynchronous read
requests sent to |decoder_|.
This comment neds to be updated.

[jiesun, 2010-05-26 01:03:42]

done.

http://codereview.chromium.org/2101022/diff/5001/6001
File media/base/filters.h (right):

http://codereview.chromium.org/2101022/diff/5001/6001#newcode269
media/base/filters.h:269: virtual void SetFillBufferCallback(FillBufferCallback*
callback) {
On 2010/05/25 22:30:21, Alpha wrote:
> Explains what is a FillBufferCallback. I think FillBufferDoneCallback is a
> better name though..
> Also mention this callback is called with the filled buffer as parameter in
> response to a FillThisBuffer() call.

Done.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode272
media/base/filters.h:272: 
On 2010/05/25 22:30:21, Alpha wrote:
> Add a protected getter for fill_buffer_callback_.

Done.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode274
media/base/filters.h:274: virtual void FillThisBuffer(scoped_refptr<VideoFrame>
frame) = 0;
On 2010/05/25 22:30:21, Alpha wrote:
> Also mention in the comment the frame can be null in which case the decoder
will
> allocate the memory.

Done.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode276
media/base/filters.h:276: scoped_ptr<FillBufferCallback> fill_buffer_callback_;
On 2010/05/25 22:30:21, Alpha wrote:
> fill_buffer_callback_ needs to be private.

Done.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode298
media/base/filters.h:298: virtual void SetFillBufferCallback(FillBufferCallback*
callback) {
On 2010/05/25 22:30:21, Alpha wrote:
> Explains what is a FillBufferCallback. I think FillBufferDoneCallback is a
> better name though..
> Also mention this callback is called with the filled buffer as parameter in
> response to a FillThisBuffer() call.

Done.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode301
media/base/filters.h:301: 
On 2010/05/25 22:30:21, Alpha wrote:
> Add a protected getter for fill_buffer_callback_.

Done.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode304
media/base/filters.h:304: virtual void FillThisBuffer(scoped_refptr<Buffer>
buffer) = 0;
On 2010/05/25 22:30:21, Alpha wrote:
> The comment says not implemented, I believe what's not implemented is that
> renderer provides the buffer. I would rather say that renderer can provide a
> NULL buffer and Decoder will be responsible for allocating the output buffer
and
> returning it to the renderer.

Done.

http://codereview.chromium.org/2101022/diff/5001/6001#newcode306
media/base/filters.h:306: scoped_ptr<FillBufferCallback> fill_buffer_callback_;
On 2010/05/25 22:30:21, Alpha wrote:
> fill_buffer_callback_ needs to be private.

Done.

http://codereview.chromium.org/2101022/diff/5001/6003
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/2101022/diff/5001/6003#newcode234
media/filters/audio_renderer_base.cc:234: scoped_refptr<Buffer> buffer;
On 2010/05/25 22:30:21, Alpha wrote:
> Add a TODO here saying that we can provide a buffer pool to reduce the amount
of
> buffer allocation calls.

Done.

http://codereview.chromium.org/2101022/diff/5001/6004
File media/filters/audio_renderer_base.h (right):

http://codereview.chromium.org/2101022/diff/5001/6004#newcode60
media/filters/audio_renderer_base.h:60: virtual void
OnReadComplete(scoped_refptr<Buffer> buffer_in);
On 2010/05/25 22:30:21, Alpha wrote:
> Since you are renaming Read() to FillThisBuffer() this should be changed
> accordingly.

Done.

http://codereview.chromium.org/2101022/diff/5001/6005
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/2101022/diff/5001/6005#newcode91
media/filters/audio_renderer_base_unittest.cc:91: void
EnqueueCallback(scoped_refptr<Buffer> buffer) {
On 2010/05/25 22:30:21, Alpha wrote:
> This is not enqueuing anymore so rename this method. Also we don't need the
> read_queue_ since the audio renderer doesn't provide the buffer pool, we can
> simply increment the count here.

Done.

http://codereview.chromium.org/2101022/diff/5001/6006
File media/filters/decoder_base.h (left):

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode127
media/filters/decoder_base.h:127: DCHECK_LE(pending_reads_, read_queue_.size());
On 2010/05/25 22:30:21, Alpha wrote:
> We still want to keep this DCHECK, this is a safety check for FFmpeg, this can
> be done in a form of count instead of read_queue_.size().
> 
> OpenMAX won't be using DecoderBase so keeping this DCHECK doesn't affect
> OpenMAX.

Done.

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode129
media/filters/decoder_base.h:129: DCHECK_LT(pending_reads_, read_queue_.size());
On 2010/05/25 22:30:21, Alpha wrote:
> Keep this DCHECK, just do it in another form.

Done.

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode164
media/filters/decoder_base.h:164: DCHECK(read_queue_.empty()) << "Read requests
should be empty";
On 2010/05/25 22:30:21, Alpha wrote:
> Do a DCHECK here to make sure something like |fill_buffer_count_| == 0;
> 

Done.

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode228
media/filters/decoder_base.h:228: read_queue_.push_back(read_callback);
On 2010/05/25 22:30:21, Alpha wrote:
> increment |fill_buffer_count_| here.

Done.

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode263
media/filters/decoder_base.h:263: if (read_queue_.empty() ||
result_queue_.empty()) {
On 2010/05/25 22:30:21, Alpha wrote:
> if (!fill_buffer_count_ || ...)
> 

Done.

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode271
media/filters/decoder_base.h:271: read_queue_.pop_front();
On 2010/05/25 22:30:21, Alpha wrote:
> decrement |fill_buffer_count_|.

Done.

http://codereview.chromium.org/2101022/diff/5001/6006#oldcode301
media/filters/decoder_base.h:301: ReadQueue read_queue_;
On 2010/05/25 22:30:21, Alpha wrote:
> We don't need a readqueue now, but we still need a count of how many
> FillThisBuffer() call received.

Done.

http://codereview.chromium.org/2101022/diff/5001/6006
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/2101022/diff/5001/6006#newcode260
media/filters/decoder_base.h:260: Decoder::fill_buffer_callback_->Run(output);
On 2010/05/25 22:30:21, Alpha wrote:
> Use a getter instead of access Decoder's member directly.

Done.

http://codereview.chromium.org/2101022/diff/5001/6007
File media/filters/decoder_base_unittest.cc (right):

http://codereview.chromium.org/2101022/diff/5001/6007#newcode55
media/filters/decoder_base_unittest.cc:55: public:
On 2010/05/25 22:30:21, Alpha wrote:
> Remove these lines, looks like they are not doing anything.

Done.

http://codereview.chromium.org/2101022/diff/5001/6007#newcode151
media/filters/decoder_base_unittest.cc:151: scoped_refptr<MockDecoderOutput>
output;
On 2010/05/25 22:30:21, Alpha wrote:
> I don't think this is the right way of testing this.
> 
> I would expect you do
> MockDecoderOutputConsumer consumer;
> decoder->SetFillBufferCallback(NewCallback(&consumer,
> MockDecoderOutputConsumer::FillBufferCallback));
> 
> and then you do decoder->FillThisBuffer(output); and expects
> MockDecoderOutputConsumer be called.

Done.

http://codereview.chromium.org/2101022/diff/5001/6008
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/2101022/diff/5001/6008#newcode372
media/filters/video_renderer_base.cc:372: scoped_refptr<VideoFrame> video_frame;
// dummy frame;
On 2010/05/25 22:30:21, Alpha wrote:
> Add some comment to explain why.

Done.

http://codereview.chromium.org/2101022/diff/5001/6009
File media/filters/video_renderer_base.h (right):

http://codereview.chromium.org/2101022/diff/5001/6009#newcode86
media/filters/video_renderer_base.h:86: void
OnReadComplete(scoped_refptr<VideoFrame> frame);
On 2010/05/25 22:30:21, Alpha wrote:
> Again this should match FillThisBuffer().

Done.

http://codereview.chromium.org/2101022/diff/5001/6010
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/2101022/diff/5001/6010#newcode82
media/filters/video_renderer_base_unittest.cc:82: // Receives asynchronous read
requests sent to |decoder_|.
On 2010/05/25 22:30:21, Alpha wrote:
> This comment neds to be updated.

Done.

[Alpha Left Google, 2010-05-26 01:09:50]

Looking pretty good now. Please also add scherkus@ as reviewer to have another
look.

http://codereview.chromium.org/2101022/diff/22001/23001
File media/base/filters.h (right):

http://codereview.chromium.org/2101022/diff/22001/23001#newcode270
media/base/filters.h:270: // runtime to downstream filter
nit: period at the end of line.

http://codereview.chromium.org/2101022/diff/22001/23005
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/2101022/diff/22001/23005#newcode90
media/filters/audio_renderer_base_unittest.cc:90: void
EnqueueCallback(scoped_refptr<Buffer> buffer) {
The name of this method needs to be updated.

[jiesun, 2010-05-26 01:13:13]

add scherkus

[scherkus (not reviewing), 2010-05-26 07:41:15]

awesome!!  so glad to see those queues go away

http://codereview.chromium.org/2101022/diff/29001/22003
File media/base/filters.h (right):

http://codereview.chromium.org/2101022/diff/29001/22003#newcode272
media/base/filters.h:272: virtual void
SetFillBufferDoneCallback(FillBufferDoneCallback* callback) {
given the basic operation of setting the callback, lets ditch virtual and have
the getter/setter public

we'll stick to the unix_hacker style:
void set_fill_buffer_done_callback(...)
... fill_buffer_done_callback()

http://codereview.chromium.org/2101022/diff/29001/22003#newcode311
media/base/filters.h:311: virtual void
SetFillBufferDoneCallback(FillBufferDoneCallback* callback) {
ditto

http://codereview.chromium.org/2101022/diff/29001/22004
File media/base/mock_filters.h (right):

http://codereview.chromium.org/2101022/diff/29001/22004#newcode177
media/base/mock_filters.h:177: FillBufferDoneCallback*
fill_buffer_done_callback() {
just make the method public then this stuff goes away

http://codereview.chromium.org/2101022/diff/29001/22007
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/2101022/diff/29001/22007#newcode86
media/filters/audio_renderer_base_unittest.cc:86: // number of asynchronous read
requests sent to |decoder_|.
nit: number -> Number

http://codereview.chromium.org/2101022/diff/29001/22007#newcode87
media/filters/audio_renderer_base_unittest.cc:87: unsigned int pending_reads_;
nit: unsigned int -> size_t (corresponds with the now-removed std::deque and
kMaxQueueSize constant)

http://codereview.chromium.org/2101022/diff/29001/22009
File media/filters/decoder_base_unittest.cc (right):

http://codereview.chromium.org/2101022/diff/29001/22009#newcode54
media/filters/decoder_base_unittest.cc:54: public:
instead of doing this I would change MockDecoderReadCallback to have the
matching callback signature (i.e., OnReadComplete)

the nice part about using callbacks is we don't care if we have a renderer or
not.. all we need is to plug in a callback on the decoder

http://codereview.chromium.org/2101022/diff/29001/22009#newcode133
media/filters/decoder_base_unittest.cc:133: scoped_refptr<MockRenderer> renderer
= new MockRenderer(decoder);
then we can replace this with the "StrictMock<MockDecoderReadCallback>
read_callback" below and set the permanent callback on decoder here

[jiesun, 2010-05-26 17:32:24]

done.
I am going to commit it when bot goes green

http://codereview.chromium.org/2101022/diff/29001/22003
File media/base/filters.h (right):

http://codereview.chromium.org/2101022/diff/29001/22003#newcode272
media/base/filters.h:272: virtual void
SetFillBufferDoneCallback(FillBufferDoneCallback* callback) {
On 2010/05/26 07:41:15, scherkus wrote:
> given the basic operation of setting the callback, lets ditch virtual and have
> the getter/setter public
> 
> we'll stick to the unix_hacker style:
> void set_fill_buffer_done_callback(...)
> ... fill_buffer_done_callback()

Done.

http://codereview.chromium.org/2101022/diff/29001/22003#newcode311
media/base/filters.h:311: virtual void
SetFillBufferDoneCallback(FillBufferDoneCallback* callback) {
On 2010/05/26 07:41:15, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/2101022/diff/29001/22004
File media/base/mock_filters.h (right):

http://codereview.chromium.org/2101022/diff/29001/22004#newcode177
media/base/mock_filters.h:177: FillBufferDoneCallback*
fill_buffer_done_callback() {
On 2010/05/26 07:41:15, scherkus wrote:
> just make the method public then this stuff goes away

Done.

http://codereview.chromium.org/2101022/diff/29001/22007
File media/filters/audio_renderer_base_unittest.cc (right):

http://codereview.chromium.org/2101022/diff/29001/22007#newcode86
media/filters/audio_renderer_base_unittest.cc:86: // number of asynchronous read
requests sent to |decoder_|.
On 2010/05/26 07:41:15, scherkus wrote:
> nit: number -> Number

Done.

http://codereview.chromium.org/2101022/diff/29001/22007#newcode87
media/filters/audio_renderer_base_unittest.cc:87: unsigned int pending_reads_;
On 2010/05/26 07:41:15, scherkus wrote:
> nit: unsigned int -> size_t (corresponds with the now-removed std::deque and
> kMaxQueueSize constant)

Done.

http://codereview.chromium.org/2101022/diff/29001/22009
File media/filters/decoder_base_unittest.cc (right):

http://codereview.chromium.org/2101022/diff/29001/22009#newcode133
media/filters/decoder_base_unittest.cc:133: scoped_refptr<MockRenderer> renderer
= new MockRenderer(decoder);
On 2010/05/26 07:41:15, scherkus wrote:
> then we can replace this with the "StrictMock<MockDecoderReadCallback>
> read_callback" below and set the permanent callback on decoder here

Done.

[Alpha Left Google, 2010-05-27 18:50:12]

On 2010/05/26 17:32:24, jiesun wrote:
> done.
> I am going to commit it when bot goes green
> 
> http://codereview.chromium.org/2101022/diff/29001/22003
> File media/base/filters.h (right):
> 
> http://codereview.chromium.org/2101022/diff/29001/22003#newcode272
> media/base/filters.h:272: virtual void
> SetFillBufferDoneCallback(FillBufferDoneCallback* callback) {
> On 2010/05/26 07:41:15, scherkus wrote:
> > given the basic operation of setting the callback, lets ditch virtual and
have
> > the getter/setter public
> > 
> > we'll stick to the unix_hacker style:
> > void set_fill_buffer_done_callback(...)
> > ... fill_buffer_done_callback()
> 
> Done.
> 
> http://codereview.chromium.org/2101022/diff/29001/22003#newcode311
> media/base/filters.h:311: virtual void
> SetFillBufferDoneCallback(FillBufferDoneCallback* callback) {
> On 2010/05/26 07:41:15, scherkus wrote:
> > ditto
> 
> Done.
> 
> http://codereview.chromium.org/2101022/diff/29001/22004
> File media/base/mock_filters.h (right):
> 
> http://codereview.chromium.org/2101022/diff/29001/22004#newcode177
> media/base/mock_filters.h:177: FillBufferDoneCallback*
> fill_buffer_done_callback() {
> On 2010/05/26 07:41:15, scherkus wrote:
> > just make the method public then this stuff goes away
> 
> Done.
> 
> http://codereview.chromium.org/2101022/diff/29001/22007
> File media/filters/audio_renderer_base_unittest.cc (right):
> 
> http://codereview.chromium.org/2101022/diff/29001/22007#newcode86
> media/filters/audio_renderer_base_unittest.cc:86: // number of asynchronous
read
> requests sent to |decoder_|.
> On 2010/05/26 07:41:15, scherkus wrote:
> > nit: number -> Number
> 
> Done.
> 
> http://codereview.chromium.org/2101022/diff/29001/22007#newcode87
> media/filters/audio_renderer_base_unittest.cc:87: unsigned int pending_reads_;
> On 2010/05/26 07:41:15, scherkus wrote:
> > nit: unsigned int -> size_t (corresponds with the now-removed std::deque and
> > kMaxQueueSize constant)
> 
> Done.
> 
> http://codereview.chromium.org/2101022/diff/29001/22009
> File media/filters/decoder_base_unittest.cc (right):
> 
> http://codereview.chromium.org/2101022/diff/29001/22009#newcode133
> media/filters/decoder_base_unittest.cc:133: scoped_refptr<MockRenderer>
renderer
> = new MockRenderer(decoder);
> On 2010/05/26 07:41:15, scherkus wrote:
> > then we can replace this with the "StrictMock<MockDecoderReadCallback>
> > read_callback" below and set the permanent callback on decoder here
> 
> Done.

LGTM. Should still wait for reviewer to give the final LG.