Add FakeVideoDecoder.

Add FakeVideoDecoder.

The FakeVideoDecoder simulates a typical video decoder that could have some
decoding delays (to simulate out-of-order decoding). Since it's fake,
it does not care about what the input is. It always generate output frames
successfully but with a frame delay (specified in the ctor).

All callbacks passed to FakeVideoDecoder can be held if desired. This gives the
test class a chance to do some operation during pending callbacks.

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

[xhwang, 2013-05-09 18:46:53]

This CL is a little crazier than I thought when I need to make the callbacks
holdable. Please take a look at my approach and any comments are welcome!

PTAL

[scherkus (not reviewing), 2013-05-09 20:07:09]

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_demuxer_st...
File media/filters/fake_demuxer_stream.h (right):

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_demuxer_st...
media/filters/fake_demuxer_stream.h:75: bool hold_next_read_;
can this use your new CallbackHolder?

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
File media/filters/fake_video_decoder.cc (right):

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
media/filters/fake_video_decoder.cc:45: LOG(INFO) << "Decoded frames dropped
during reinitialization.";
does this have to be LOG(INFO)?

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
File media/filters/fake_video_decoder.h (right):

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
media/filters/fake_video_decoder.h:60: template <typename CB> class
CallbackHolder {
this is complex enough that I feel it should warrant a separate file + docs w/
example usage + simple unit tests

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
media/filters/fake_video_decoder.h:85: // Runs or holds the callback as
specified by |hold_next_callback_|.
s/hold_next_callback_/hold_callback_/

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
media/filters/fake_video_decoder.h:87: void Run() {
I don't like how CH emulates the regular API but does things very differently
under the hood. For example:

regular_cb_.Run();  // Looks like a permanent callback.
holder_cb_.Run();  // Might hold. If runs, it resets the cb and is_null() = true

Also if hold_callback_ == true, is it correct that is_null() == false after
Run() is called?

Perhaps some of this can be solved with better function names :)

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
media/filters/fake_video_decoder.h:90: held_cb_ =
base::Bind(ResetAndReturn(&original_cb_));
nit: base::Bind() not needed

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
media/filters/fake_video_decoder.h:104: void Release() {
RunHeldCallback()?

[xhwang, 2013-05-22 04:29:42]

Comments mostly done. Please take another look!

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_demuxer_st...
File media/filters/fake_demuxer_stream.h (right):

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_demuxer_st...
media/filters/fake_demuxer_stream.h:75: bool hold_next_read_;
On 2013/05/09 20:07:09, scherkus wrote:
> can this use your new CallbackHolder?

The only reason I am not using that is I want Reset() to return kAborted. I'd
like to keep this for now and think more about it.

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
File media/filters/fake_video_decoder.cc (right):

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
media/filters/fake_video_decoder.cc:45: LOG(INFO) << "Decoded frames dropped
during reinitialization.";
On 2013/05/09 20:07:09, scherkus wrote:
> does this have to be LOG(INFO)?

Done.

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
File media/filters/fake_video_decoder.h (right):

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
media/filters/fake_video_decoder.h:60: template <typename CB> class
CallbackHolder {
On 2013/05/09 20:07:09, scherkus wrote:
> this is complex enough that I feel it should warrant a separate file + docs w/
> example usage + simple unit tests

Done.

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
media/filters/fake_video_decoder.h:85: // Runs or holds the callback as
specified by |hold_next_callback_|.
On 2013/05/09 20:07:09, scherkus wrote:
> s/hold_next_callback_/hold_callback_/

Done.

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
media/filters/fake_video_decoder.h:87: void Run() {
On 2013/05/09 20:07:09, scherkus wrote:
> I don't like how CH emulates the regular API but does things very differently
> under the hood. For example:
> 
> regular_cb_.Run();  // Looks like a permanent callback.
> holder_cb_.Run();  // Might hold. If runs, it resets the cb and is_null() =
true
> 
> Also if hold_callback_ == true, is it correct that is_null() == false after
> Run() is called?
> 
> Perhaps some of this can be solved with better function names :)

Done.

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
media/filters/fake_video_decoder.h:90: held_cb_ =
base::Bind(ResetAndReturn(&original_cb_));
On 2013/05/09 20:07:09, scherkus wrote:
> nit: base::Bind() not needed

Done.

https://codereview.chromium.org/15085011/diff/1/media/filters/fake_video_deco...
media/filters/fake_video_decoder.h:104: void Release() {
On 2013/05/09 20:07:09, scherkus wrote:
> RunHeldCallback()?

Done.

[scherkus (not reviewing), 2013-05-28 15:56:56]

lgtm w/ nits

https://codereview.chromium.org/15085011/diff/21001/media/base/callback_holder.h
File media/base/callback_holder.h (right):

https://codereview.chromium.org/15085011/diff/21001/media/base/callback_holde...
media/base/callback_holder.h:84: };
DISALLOW ?

https://codereview.chromium.org/15085011/diff/21001/media/base/callback_holde...
File media/base/callback_holder_unittest.cc (right):

https://codereview.chromium.org/15085011/diff/21001/media/base/callback_holde...
media/base/callback_holder_unittest.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
new file: 2013

https://codereview.chromium.org/15085011/diff/21001/media/base/callback_holde...
media/base/callback_holder_unittest.cc:5: #include
"media/base/callback_holder.h"
being the unit test this doesn't have to be first (in other words, remove blank
line below)

https://codereview.chromium.org/15085011/diff/21001/media/filters/fake_demuxe...
File media/filters/fake_demuxer_stream_unittest.cc (right):

https://codereview.chromium.org/15085011/diff/21001/media/filters/fake_demuxe...
media/filters/fake_demuxer_stream_unittest.cc:103: while (1) {
for (;;) {} ?

https://codereview.chromium.org/15085011/diff/21001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/15085011/diff/21001/media/media.gyp#newcode328
media/media.gyp:328: 'filters/fake_video_decoder.h',
hmm.. shouldn't fake classes + callback holder only get compiled in the
media_unittests target?

we can likely use some reorganization of media testing files

[xhwang, 2013-05-28 19:34:09]

https://codereview.chromium.org/15085011/diff/21001/media/base/callback_holder.h
File media/base/callback_holder.h (right):

https://codereview.chromium.org/15085011/diff/21001/media/base/callback_holde...
media/base/callback_holder.h:84: };
On 2013/05/28 15:56:57, scherkus wrote:
> DISALLOW ?

Done.

https://codereview.chromium.org/15085011/diff/21001/media/base/callback_holde...
File media/base/callback_holder_unittest.cc (right):

https://codereview.chromium.org/15085011/diff/21001/media/base/callback_holde...
media/base/callback_holder_unittest.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/05/28 15:56:57, scherkus wrote:
> new file: 2013

Done.

https://codereview.chromium.org/15085011/diff/21001/media/base/callback_holde...
media/base/callback_holder_unittest.cc:5: #include
"media/base/callback_holder.h"
On 2013/05/28 15:56:57, scherkus wrote:
> being the unit test this doesn't have to be first (in other words, remove
blank
> line below)

Done.

https://codereview.chromium.org/15085011/diff/21001/media/filters/fake_demuxe...
File media/filters/fake_demuxer_stream_unittest.cc (right):

https://codereview.chromium.org/15085011/diff/21001/media/filters/fake_demuxe...
media/filters/fake_demuxer_stream_unittest.cc:103: while (1) {
On 2013/05/28 15:56:57, scherkus wrote:
> for (;;) {} ?

Done.

https://codereview.chromium.org/15085011/diff/21001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/15085011/diff/21001/media/media.gyp#newcode328
media/media.gyp:328: 'filters/fake_video_decoder.h',
On 2013/05/28 15:56:57, scherkus wrote:
> hmm.. shouldn't fake classes + callback holder only get compiled in the
> media_unittests target?
> 
> we can likely use some reorganization of media testing files

Done.

[commit-bot: I haz the power, 2013-05-28 19:34:23]

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

[commit-bot: I haz the power, 2013-05-28 19:58:47]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-28 20:03:23]

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

[commit-bot: I haz the power, 2013-05-28 20:24:31]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-28 21:39:54]

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

[commit-bot: I haz the power, 2013-05-28 23:41:27]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-28 23:46:37]

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

[commit-bot: I haz the power, 2013-05-29 06:52:30]

Change committed as 202789