Introduce VideoFrameStream, the video frame provider in media pipeline.

Introduce VideoFrameStream, the video frame provider in media pipeline.

VideoFrameStream wraps DemuxerStreams and VideoDecoders to provide VideoFrames
to it's client (e.g. VideoRendererBase). It hides details of the interraction
between DemuxerStream and VideoDecoder. To it's client, VideoFrameStream works
as a reliable VideoFrame source which, after initialization, simply returns
VideoFrames until end-of-stream (EOS).

BUG=141788
TEST=none


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190181

[xhwang, 2013-03-21 01:06:31]

FYI, Patch set 1 actually never calls VideoDecoder::Reset()/Stop() during a
pending read. It only does so after the read callback is fired. This is probably
something we want to do later so that VideoDecoder implementation can be greatly
simplified.

Patch set 2 removes this restriction and is compatible with what we are doing
now. I had to change VideoRendererBaseTest a bit to make sure Stop() callback is
always fired after the read/reset callback, which it should be.

This CL is ready for review now. PTAL!

[xhwang, 2013-03-21 01:07:21]

As always, I'll add tests to cover the new class if the idea looks good.

[xhwang, 2013-03-21 01:10:17]

https://codereview.chromium.org/12818004/diff/2001/media/filters/video_frame_...
File media/filters/video_frame_stream.h (right):

https://codereview.chromium.org/12818004/diff/2001/media/filters/video_frame_...
media/filters/video_frame_stream.h:54: void ReadFrame(const
VideoDecoder::ReadCB& read_cb);
I didn't use Read() on purpose because in the next CL VFS will implement
DemuxerStream which has an API Read().

[scherkus (not reviewing), 2013-03-22 01:02:40]

looking nice!

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
File media/filters/video_frame_stream.cc (right):

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.cc:30: DCHECK(state_ == UNINITIALIZED || state_
== STOPPED);
emit state

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.cc:93: DCHECK(decoder_) << "Reset() without
successfully initialized a decoder.";
not needed -- we'll crash

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.cc:100: // ever hit.
not sure I understand this TODO

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.cc:106: // The stopping process will be
continued after all of the following pending
s/stopping process will be continued/stopping will continue/

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.cc:129: BindToCurrentLoop(closure).Run();
what do you think about using "message_loop_->PostTask(closure)" instead?

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.cc:168: DCHECK(state_ == NORMAL);
EQ

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
File media/filters/video_frame_stream.h (right):

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.h:38: VideoFrameStream(const
scoped_refptr<base::MessageLoopProxy>& message_loop,
note: with https://codereview.chromium.org/12989009/ landed we can likely pass
decoders in here + create VDSelector inside the ctor

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.h:92: // Callback for VideoDecoder::Reset().
nit: I don't think this comment + one below are very helpful

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:68: }
add EXPECT_FALSE() for all the pending cbs?

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:104: EXPECT_CALL(*decoder_,
Read(_))
nit: perhaps ReadFrame, Reset, Stop, etc.. can be implemented by calling the
EnterPendingXXX() + SatisfyPendingXXX() methods?

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:218: MessageLoop message_loop_;
I believe all of these can be private, right?

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:241: INSTANTIATE_TEST_CASE_P(,
VideoFrameStreamTest, testing::Bool());
huh? what's the difference?

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:286: TEST_P(VideoFrameStreamTest,
MultipleReadFramesAndResets) {
what is this testing?

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:310: SatisfyAllPendingCallbacks();
I'm not so sure I like this "satisfy everything" helper method -- any reason why
don't specifically call out which CBs to satisfy?

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:316:
SatisfyAllPendingCallbacksWithInitResult(false);
I think this would be clearer if you had:

SatisfyPendingInitialization(false);
SatisfyPendingStopState();

[xhwang, 2013-03-22 07:23:27]

PTAL

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
File media/filters/video_frame_stream.cc (right):

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.cc:30: DCHECK(state_ == UNINITIALIZED || state_
== STOPPED);
On 2013/03/22 01:02:40, scherkus wrote:
> emit state

Done.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.cc:93: DCHECK(decoder_) << "Reset() without
successfully initialized a decoder.";
On 2013/03/22 01:02:40, scherkus wrote:
> not needed -- we'll crash

Done.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.cc:100: // ever hit.
On 2013/03/22 01:02:40, scherkus wrote:
> not sure I understand this TODO

Updated.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.cc:106: // The stopping process will be
continued after all of the following pending
On 2013/03/22 01:02:40, scherkus wrote:
> s/stopping process will be continued/stopping will continue/

Done.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.cc:129: BindToCurrentLoop(closure).Run();
On 2013/03/22 01:02:40, scherkus wrote:
> what do you think about using "message_loop_->PostTask(closure)" instead?

Done.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.cc:168: DCHECK(state_ == NORMAL);
On 2013/03/22 01:02:40, scherkus wrote:
> EQ

Done.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
File media/filters/video_frame_stream.h (right):

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.h:38: VideoFrameStream(const
scoped_refptr<base::MessageLoopProxy>& message_loop,
On 2013/03/22 01:02:40, scherkus wrote:
> note: with https://codereview.chromium.org/12989009/ landed we can likely pass
> decoders in here + create VDSelector inside the ctor

yep, looking forward to that.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream.h:92: // Callback for VideoDecoder::Reset().
On 2013/03/22 01:02:40, scherkus wrote:
> nit: I don't think this comment + one below are very helpful

Done.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:68: }
On 2013/03/22 01:02:40, scherkus wrote:
> add EXPECT_FALSE() for all the pending cbs?

Done.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:104: EXPECT_CALL(*decoder_,
Read(_))
On 2013/03/22 01:02:40, scherkus wrote:
> nit: perhaps ReadFrame, Reset, Stop, etc.. can be implemented by calling the
> EnterPendingXXX() + SatisfyPendingXXX() methods?

Yes, I thought about that too. Done.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:218: MessageLoop message_loop_;
On 2013/03/22 01:02:40, scherkus wrote:
> I believe all of these can be private, right?

Done.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:241: INSTANTIATE_TEST_CASE_P(,
VideoFrameStreamTest, testing::Bool());
On 2013/03/22 01:02:40, scherkus wrote:
> huh? what's the difference?

We test both encrypted/clear demuxer stream cases. In the former case the
DecryptingDemuxerStream will be created and be part of the REad/Reset/Stop fun.
I want to cover both cases, hence "P" here.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:286: TEST_P(VideoFrameStreamTest,
MultipleReadFramesAndResets) {
On 2013/03/22 01:02:40, scherkus wrote:
> what is this testing?

I want to be a little bit crazy here. But probably that should be covered by
integration testing. Removed.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:310: SatisfyAllPendingCallbacks();
On 2013/03/22 01:02:40, scherkus wrote:
> I'm not so sure I like this "satisfy everything" helper method -- any reason
why
> don't specifically call out which CBs to satisfy?

Done.

https://codereview.chromium.org/12818004/diff/8001/media/filters/video_frame_...
media/filters/video_frame_stream_unittest.cc:316:
SatisfyAllPendingCallbacksWithInitResult(false);
On 2013/03/22 01:02:40, scherkus wrote:
> I think this would be clearer if you had:
> 
> SatisfyPendingInitialization(false);
> SatisfyPendingStopState();

Done.

[scherkus (not reviewing), 2013-03-22 17:23:19]

I just realized this doesn't include my HOFA() patch.

My suggestion would be to add HOFA() to VFS and plumb the value through based on
the currently selected decoder.

Want to rebase + send out an updated CL?

https://codereview.chromium.org/12818004/diff/15001/media/filters/video_frame...
File media/filters/video_frame_stream.cc (right):

https://codereview.chromium.org/12818004/diff/15001/media/filters/video_frame...
media/filters/video_frame_stream.cc:98: // Check if we ever call Stop() twice.
this comment isn't very useful (the DCHECK makes this pretty clear, imo!)

https://codereview.chromium.org/12818004/diff/15001/media/filters/video_frame...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/12818004/diff/15001/media/filters/video_frame...
media/filters/video_frame_stream_unittest.cc:202: INSTANTIATE_TEST_CASE_P(,
VideoFrameStreamTest, testing::Bool());
what do the resulting test names look like? can we instantiate them with a name
like "Encrypted" "Normal" etc?

[xhwang, 2013-03-22 20:43:56]

Rebased and comments resolved/replied. PTAL again.

https://codereview.chromium.org/12818004/diff/15001/media/filters/video_frame...
File media/filters/video_frame_stream.cc (right):

https://codereview.chromium.org/12818004/diff/15001/media/filters/video_frame...
media/filters/video_frame_stream.cc:98: // Check if we ever call Stop() twice.
On 2013/03/22 17:23:19, scherkus wrote:
> this comment isn't very useful (the DCHECK makes this pretty clear, imo!)

Done.

https://codereview.chromium.org/12818004/diff/15001/media/filters/video_frame...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/12818004/diff/15001/media/filters/video_frame...
media/filters/video_frame_stream_unittest.cc:202: INSTANTIATE_TEST_CASE_P(,
VideoFrameStreamTest, testing::Bool());
On 2013/03/22 17:23:19, scherkus wrote:
> what do the resulting test names look like? can we instantiate them with a
name
> like "Encrypted" "Normal" etc?

No, it'll look like 0/1 only. And even if I use char* as parameter, it still
shows 0/1:

https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

[scherkus (not reviewing), 2013-03-22 20:52:40]

lgtm w/ nit

https://codereview.chromium.org/12818004/diff/20001/media/filters/video_frame...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/12818004/diff/20001/media/filters/video_frame...
media/filters/video_frame_stream_unittest.cc:202: INSTANTIATE_TEST_CASE_P(,
VideoFrameStreamTest, testing::Bool());
Check out how I did it for MediaTest:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...

instead of testing::Bool() you instantiate two sets, that way you can use the
first parameter to give a more descriptive name

[xhwang, 2013-03-22 21:09:33]

https://codereview.chromium.org/12818004/diff/20001/media/filters/video_frame...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/12818004/diff/20001/media/filters/video_frame...
media/filters/video_frame_stream_unittest.cc:202: INSTANTIATE_TEST_CASE_P(,
VideoFrameStreamTest, testing::Bool());
On 2013/03/22 20:52:40, scherkus wrote:
> Check out how I did it for MediaTest:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
> 
> instead of testing::Bool() you instantiate two sets, that way you can use the
> first parameter to give a more descriptive name

That's smart :) Done.

[commit-bot: I haz the power, 2013-03-22 23:24:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/12818004/13004

[commit-bot: I haz the power, 2013-03-23 15:35:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/12818004/13004

[commit-bot: I haz the power, 2013-03-24 04:06:46]

Change committed as 190181