preparation for recycling buffer, patch 2

preparation for recycling buffer, patch 2

1. add ProvidesBuffer in Filter interface, not used yet.
2. add Flush stage in pipeline. not used. Render's pause work is moved to Renderer's flush().
3. merge decoder_base with ffmpeg_video_decoder. because it is shared by audio.

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

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

[Alpha Left Google, 2010-07-28 20:15:55]

Thanks for separating the patch.

This looks mostly good. I have a few questions and suggestions.

http://codereview.chromium.org/3030013/diff/4001/5003
File media/base/pipeline_impl.cc (left):

http://codereview.chromium.org/3030013/diff/4001/5003#oldcode57
media/base/pipeline_impl.cc:57: }
This is just cleanup right? I don't see this method used anywhere else.

http://codereview.chromium.org/3030013/diff/4001/5005
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/3030013/diff/4001/5005#newcode123
media/filters/ffmpeg_video_decode_engine.cc:123:
empty_this_buffer_callback_->Run(buffer);
I'm confused why we have this. This is returning the output buffer as-is.
Shouldn't we queue input buffer from emptythisbuffer() and do decode frame in
FillThisBuffer()?

http://codereview.chromium.org/3030013/diff/4001/5006
File media/filters/ffmpeg_video_decode_engine.h (right):

http://codereview.chromium.org/3030013/diff/4001/5006#newcode36
media/filters/ffmpeg_video_decode_engine.h:36: virtual void Seek(Task* done_cb);
What is this method used for? It is called seek but only accepts a task..
shouldn't flush enough? Please refresh me if this is in our previous discussion.

http://codereview.chromium.org/3030013/diff/4001/5008
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/3030013/diff/4001/5008#newcode135
media/filters/ffmpeg_video_decoder.cc:135: decode_engine_->Seek(
Is Seek() needed in VideoDecodeEngine? Sounds strange to me that you just call
this method without any parameters.

http://codereview.chromium.org/3030013/diff/4001/5008#newcode258
media/filters/ffmpeg_video_decoder.cc:258:
demuxer_stream_->Read(NewCallback(this, &FFmpegVideoDecoder::OnReadComplete));
If possible I'd just do

NewCallback(decode_engine_, &FFmpegVideoDecodeEngine, EmptyThisBuffer);

http://codereview.chromium.org/3030013/diff/4001/5009
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/3030013/diff/4001/5009#newcode38
media/filters/ffmpeg_video_decoder.h:38: message_loop()->PostTask(FROM_HERE,
You can consider using the pattern:

void Seek(...) {
  if (MessageLoop::current() != message_loop()) {
    message_loop()->PostTask(..., NewRunnableMethod(this, ...::Seek, ...);
    return;
  }

  // Actually doing the seek.
}

http://codereview.chromium.org/3030013/diff/4001/5009#newcode91
media/filters/ffmpeg_video_decoder.h:91: void InitializeTask(DemuxerStream*
demuxer_stream, FilterCallback* callback);
Rename to DoInitialize

http://codereview.chromium.org/3030013/diff/4001/5009#newcode94
media/filters/ffmpeg_video_decoder.h:94: void StopTask(FilterCallback*
callback);
Rename this to DoStop

http://codereview.chromium.org/3030013/diff/4001/5009#newcode97
media/filters/ffmpeg_video_decoder.h:97: void FlushTask(FilterCallback*
callback);
Rename this to DoFlush instead.

http://codereview.chromium.org/3030013/diff/4001/5009#newcode100
media/filters/ffmpeg_video_decoder.h:100: void SeekTask(FilterCallback*
callback);
Rename this to DoSeek instead.

http://codereview.chromium.org/3030013/diff/4001/5009#newcode103
media/filters/ffmpeg_video_decoder.h:103: void OnReadComplete(Buffer* buffer) {
Should move the implementation to .cc file.

http://codereview.chromium.org/3030013/diff/4001/5010
File media/filters/ffmpeg_video_decoder_unittest.cc (left):

http://codereview.chromium.org/3030013/diff/4001/5010#oldcode441
media/filters/ffmpeg_video_decoder_unittest.cc:441: // Decode a fifth time with
an end of stream buffer.  this should
What about these test steps? It's important not to make the test weaker.

http://codereview.chromium.org/3030013/diff/4001/5010#oldcode458
media/filters/ffmpeg_video_decoder_unittest.cc:458:
TEST_F(FFmpegVideoDecoderTest, DoDecode_FinishEnqueuesEmptyFrames) {
And these test cases too.

http://codereview.chromium.org/3030013/diff/4001/5010
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/3030013/diff/4001/5010#newcode71
media/filters/ffmpeg_video_decoder_unittest.cc:71: class MockVideoRendererSink :
public MockVideoRenderer {
I think you can just add the method to MockVideoRenderer so we don't need an
extra class.

http://codereview.chromium.org/3030013/diff/4001/5010#newcode446
media/filters/ffmpeg_video_decoder_unittest.cc:446:
.WillOnce(ReturnFromDemux(decoder_.get(), end_of_stream_buffer_, kTestPts3))
80 chars here.

[jiesun, 2010-07-28 20:34:30]

thanks for review it.
I will address the name problem. but here is initial response to some questions

http://codereview.chromium.org/3030013/diff/4001/5003
File media/base/pipeline_impl.cc (left):

http://codereview.chromium.org/3030013/diff/4001/5003#oldcode57
media/base/pipeline_impl.cc:57: }
On 2010/07/28 20:15:55, Alpha wrote:
> This is just cleanup right? I don't see this method used anywhere else.
> 

yes.

http://codereview.chromium.org/3030013/diff/4001/5005
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/3030013/diff/4001/5005#newcode123
media/filters/ffmpeg_video_decode_engine.cc:123:
empty_this_buffer_callback_->Run(buffer);
this is from original code, original logic is when render read decoder, decoder
send a read to demux, this is 1:1.
now that logic had been moved  from decoder class to engine class. I am fine
with both.

http://codereview.chromium.org/3030013/diff/4001/5006
File media/filters/ffmpeg_video_decode_engine.h (right):

http://codereview.chromium.org/3030013/diff/4001/5006#newcode36
media/filters/ffmpeg_video_decode_engine.h:36: virtual void Seek(Task* done_cb);
true, I believe that for decoder, the usage of seek task is doubtful, that's
because currently we use pull model, and assume that decode engine had the best
knowledge of how many "read" need to be performed. all initial read after a
"seek" is done in seek task.
if we would like to compromise with this design, we could let "decoder" class do
this, then engine interface will be free from seek task.

http://codereview.chromium.org/3030013/diff/4001/5008
File media/filters/ffmpeg_video_decoder.cc (right):

http://codereview.chromium.org/3030013/diff/4001/5008#newcode135
media/filters/ffmpeg_video_decoder.cc:135: decode_engine_->Seek(
the only case we need that is in omx engine... we could modify that ...

http://codereview.chromium.org/3030013/diff/4001/5008#newcode258
media/filters/ffmpeg_video_decoder.cc:258:
demuxer_stream_->Read(NewCallback(this, &FFmpegVideoDecoder::OnReadComplete));
we had to maintain our state in ReadCompleteTask.

http://codereview.chromium.org/3030013/diff/4001/5009
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/3030013/diff/4001/5009#newcode38
media/filters/ffmpeg_video_decoder.h:38: message_loop()->PostTask(FROM_HERE,
great, I will do that.

http://codereview.chromium.org/3030013/diff/4001/5010
File media/filters/ffmpeg_video_decoder_unittest.cc (left):

http://codereview.chromium.org/3030013/diff/4001/5010#oldcode441
media/filters/ffmpeg_video_decoder_unittest.cc:441: // Decode a fifth time with
an end of stream buffer.  this should

because we don't return empty frame now for decoding delays, so it is covered in
new test.

http://codereview.chromium.org/3030013/diff/4001/5010#oldcode458
media/filters/ffmpeg_video_decoder_unittest.cc:458:
TEST_F(FFmpegVideoDecoderTest, DoDecode_FinishEnqueuesEmptyFrames) {
On 2010/07/28 20:15:55, Alpha wrote:
> And these test cases too.
> 
these tests use private mock, which is not applied. and
FinishEnqueuesEmptyFrames is kind of covered by the TestStateTransition.

DoSeek test actually really do not test anything except the state is kNormal
after seek, I could add it back, but I doubt the usefulness of this test. .

[jiesun, 2010-07-29 16:57:38]

please take another look.

Seek() is now empty is just because this is one step of a big patch, in the
final patch, it is used to intiaite reads from decoder ( when decoder are buffer
owner.). Seek() in engine interface is optional, but currently OpenMAX decoder
had some assumption ( reads number <= free input buffers). I think we could
remove it in later patch.

http://codereview.chromium.org/3030013/diff/4001/5009
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/3030013/diff/4001/5009#newcode38
media/filters/ffmpeg_video_decoder.h:38: message_loop()->PostTask(FROM_HERE,
On 2010/07/28 20:15:55, Alpha wrote:
> You can consider using the pattern:
> 
> void Seek(...) {
>   if (MessageLoop::current() != message_loop()) {
>     message_loop()->PostTask(..., NewRunnableMethod(this, ...::Seek, ...);
>     return;
>   }
> 
>   // Actually doing the seek.
> }

Done.

http://codereview.chromium.org/3030013/diff/4001/5009#newcode91
media/filters/ffmpeg_video_decoder.h:91: void InitializeTask(DemuxerStream*
demuxer_stream, FilterCallback* callback);
On 2010/07/28 20:15:55, Alpha wrote:
> Rename to DoInitialize
> 

Done.

http://codereview.chromium.org/3030013/diff/4001/5009#newcode94
media/filters/ffmpeg_video_decoder.h:94: void StopTask(FilterCallback*
callback);
On 2010/07/28 20:15:55, Alpha wrote:
> Rename this to DoStop
> 

Done.

http://codereview.chromium.org/3030013/diff/4001/5009#newcode97
media/filters/ffmpeg_video_decoder.h:97: void FlushTask(FilterCallback*
callback);
On 2010/07/28 20:15:55, Alpha wrote:
> Rename this to DoFlush instead.
> 

Done.

http://codereview.chromium.org/3030013/diff/4001/5009#newcode100
media/filters/ffmpeg_video_decoder.h:100: void SeekTask(FilterCallback*
callback);
On 2010/07/28 20:15:55, Alpha wrote:
> Rename this to DoSeek instead.
> 

Done.

http://codereview.chromium.org/3030013/diff/4001/5009#newcode103
media/filters/ffmpeg_video_decoder.h:103: void OnReadComplete(Buffer* buffer) {
On 2010/07/28 20:15:55, Alpha wrote:
> Should move the implementation to .cc file.
> 

Done.

[Alpha Left Google, 2010-08-05 18:58:28]

Sorry for leaving this too long. LGTM.

[Alpha Left Google, 2010-08-06 02:30:46]

I voiced out my concern for an unnecessary Seek() method in this patch. It's ok
that you commit this as is. But please clean it up in subsequent patches as you
suggested.

[jiesun, 2010-08-06 04:25:10]

actually as the patch title say, it is one step of patch
http://codereview.chromium.org/2992002/show. which is too big for reviewer. that
patch is for recycling buffers and support openmax seek. stop using empty buffer
as read request, flush before stop etc..
you may ask why write so big patch, because it is intertwined, I could not
submit part of it without breaking other code path.
Seek() is actually used currently in video renderer to issue initial read() (
pull model start from here) in current model, renderer is assumed to own
buffers. ( though it using empty buffers) . in that patch, decoder could
provides buffers, so its decoder's responsibilities to issue initial read()... 
seek() is used, 
I  will agree this logic is weired. I will prefer to put this in Filter::Play()
rather than Filter::Seek(), but some video renderer logic will break. ( e.g.
pre-roll state).
Seek() in engine interface probably could be spared when we issue reads from
decoder class. but if you think about it, how much input are guaranteed to push
out a output frame (i.e. maximum decode reorder latency) is unknown to decoder
class, 
if we are able to play safe with a predefined number, then we could use push
model in demuxer to get rid of the initial read for all.
this involve another patch that was turned down by the team month ago.