Introduce NullVideoSink for test classes.

Introduce NullVideoSink for test classes.

Similar to NullAudioSink, but I decided not to use FakeAudioWorker
as a generic fake worker since it adds more complexity then we need
for the video renderer sink interface.  I also didn't want to add
pause support or stop callback support to it.

BUG=473424
TEST=new unittest, forthcoming pipeline changes.

Committed: https://crrev.com/6523d600339aad86efcf1c238152e833c248dae5
Cr-Commit-Position: refs/heads/master@{#327450}

[DaleCurtis, 2015-04-28 20:26:36]

dalecurtis@chromium.org changed reviewers:
+ xhwang@chromium.org

[xhwang, 2015-04-28 23:12:07]

Looking pretty good. I just have a few comments.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink.cc
File media/base/null_video_sink.cc (right):

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.cc:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
2015

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.cc:56: DVLOG(2) << "NullVideoSink::Render(" <<
current_render_time_.ToInternalValue()
s/Render/CallRender? This doesn't necessarily trigger a callback_->Render()
call...

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.cc:81: delay += interval_ * (-delay / interval_ + 1);
IIUIC, this translates to:
   delay += interval_ * (-delay / interval_) + interval_;
=> new_delay = delay + interval_ * (-delay / interval_) + interval_;
=> new_delay = delay - delay + interval_ ;
=> new_delay = interval_

Is this what you want? It doesn't seem to give us the next interval starting
from current_render_time_...

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink.h
File media/base/null_video_sink.h (right):

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.h:27: // called back to back by repeated post tasks.
Optionally, if specified,
s/called/be called/

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.h:42: void pause_callbacks(base::TimeTicks
pause_util) {
s/pause_callbacks/PauseRenderCallbacks/?

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.h:46: void
set_tick_clock_for_testing(base::TickClock* tick_clock) {
Put comments here since this is the public interface.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.h:46: void
set_tick_clock_for_testing(base::TickClock* tick_clock) {
Use SetTickClockForTesting() since this is not strictly a simple setter (due to
the ForTesting part). Also, it's more consistent with other instances of this:

https://code.google.com/p/chromium/codesearch#search/&q=SetTickClockForTestin...

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.h:50: void set_stop_callback(const base::Closure&
stop_cb) {
set_stop_cb? Also add comment. It's not clear unless I look at the comment above
|stop_cb_|.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.h:55: // Task that periodically calls Render() to
consume audio data.
s/audio/video

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.h:79: // If specified, used instead of
base::TimeTicks::Now().
This comment only applies to |tick_clock_|, right?

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink_...
File media/base/null_video_sink_unittest.cc (right):

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink_...
media/base/null_video_sink_unittest.cc:44: }
style nit: These are not getters and should use normal function name CamelCase
style.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink_...
media/base/null_video_sink_unittest.cc:53: return new_sink;
.Pass()?

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink_...
media/base/null_video_sink_unittest.cc:155: 
Is it interesting to test the case where Render() takes some time that we have
to skip a render interval?

[DaleCurtis, 2015-04-29 00:54:08]

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink.cc
File media/base/null_video_sink.cc (right):

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.cc:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2015/04/28 23:12:06, xhwang wrote:
> 2015

Done.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.cc:56: DVLOG(2) << "NullVideoSink::Render(" <<
current_render_time_.ToInternalValue()
On 2015/04/28 23:12:06, xhwang wrote:
> s/Render/CallRender? This doesn't necessarily trigger a callback_->Render()
> call...

Just going to remove, it was really for debugging anyways.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.cc:81: delay += interval_ * (-delay / interval_ + 1);
On 2015/04/28 23:12:06, xhwang wrote:
> IIUIC, this translates to:
>    delay += interval_ * (-delay / interval_) + interval_;
> => new_delay = delay + interval_ * (-delay / interval_) + interval_;
> => new_delay = delay - delay + interval_ ;
> => new_delay = interval_
> 
> Is this what you want? It doesn't seem to give us the next interval starting
> from current_render_time_...

That's not quite true since this is integer math and the parenthesis force
(-delay/interval_) to be computed first. Consider:

interval = 10
now = 12;
delay == (10 - 12) == -2
=> delay += 10 * (2 / 10 + 1)
=> delay += 10 * (0 + 1)
=> new_delay = -2 + 10 = 8

now = 22;
delay == (10 - 22) == -12
=> delay += 10 * (12 / 10 + 1)
=> delay += 10 * (1 + 1)
=> new_delay = -12 + 2 * 10 == 8

I think this is the same as "delay = interval - (-delay % interval)" if you
prefer that instead.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink.h
File media/base/null_video_sink.h (right):

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.h:27: // called back to back by repeated post tasks.
Optionally, if specified,
On 2015/04/28 23:12:06, xhwang wrote:
> s/called/be called/

Done.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.h:42: void pause_callbacks(base::TimeTicks
pause_util) {
On 2015/04/28 23:12:06, xhwang wrote:
> s/pause_callbacks/PauseRenderCallbacks/?

Done.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.h:46: void
set_tick_clock_for_testing(base::TickClock* tick_clock) {
On 2015/04/28 23:12:06, xhwang wrote:
> Put comments here since this is the public interface.

That doesn't apply to this case, the reason being that in the VRI tests we need
a single tick clock instance to be used by both NullVideoSink and VRI, both
can't have ownership of the tick clock :)

As such, this is a raw pointer and can be a hacker_style() method with a simple
setter.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.h:50: void set_stop_callback(const base::Closure&
stop_cb) {
On 2015/04/28 23:12:06, xhwang wrote:
> set_stop_cb? Also add comment. It's not clear unless I look at the comment
above
> |stop_cb_|.

I prefer it spelled out for a method name. Added a comment though.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.h:55: // Task that periodically calls Render() to
consume audio data.
On 2015/04/28 23:12:06, xhwang wrote:
> s/audio/video

Done.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.h:79: // If specified, used instead of
base::TimeTicks::Now().
On 2015/04/28 23:12:06, xhwang wrote:
> This comment only applies to |tick_clock_|, right?

Done.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink_...
File media/base/null_video_sink_unittest.cc (right):

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink_...
media/base/null_video_sink_unittest.cc:44: }
On 2015/04/28 23:12:07, xhwang wrote:
> style nit: These are not getters and should use normal function name CamelCase
> style.

Removed since they were only in one test.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink_...
media/base/null_video_sink_unittest.cc:53: return new_sink;
On 2015/04/28 23:12:06, xhwang wrote:
> .Pass()?

Not necessary in this case, it's automatically moved.

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink_...
media/base/null_video_sink_unittest.cc:155: 
On 2015/04/28 23:12:06, xhwang wrote:
> Is it interesting to test the case where Render() takes some time that we have
> to skip a render interval?

Not particularly in this case, it was more important for audio. It's also hard
to test given performance tolerances on the bot. I.e. we'd end up using a sleep
during Render() to stall the callback, but who knows what the result of that
will be on a bot. It could sleep for 1 cycle or many.

[xhwang, 2015-04-29 03:49:13]

LGTM % comments

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink.cc
File media/base/null_video_sink.cc (right):

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.cc:81: delay += interval_ * (-delay / interval_ + 1);
On 2015/04/29 00:54:07, DaleCurtis wrote:
> On 2015/04/28 23:12:06, xhwang wrote:
> > IIUIC, this translates to:
> >    delay += interval_ * (-delay / interval_) + interval_;
> > => new_delay = delay + interval_ * (-delay / interval_) + interval_;
> > => new_delay = delay - delay + interval_ ;
> > => new_delay = interval_
> > 
> > Is this what you want? It doesn't seem to give us the next interval starting
> > from current_render_time_...
> 
> That's not quite true since this is integer math and the parenthesis force
> (-delay/interval_) to be computed first. Consider:
> 
> interval = 10
> now = 12;
> delay == (10 - 12) == -2
> => delay += 10 * (2 / 10 + 1)
> => delay += 10 * (0 + 1)
> => new_delay = -2 + 10 = 8
> 
> now = 22;
> delay == (10 - 22) == -12
> => delay += 10 * (12 / 10 + 1)
> => delay += 10 * (1 + 1)
> => new_delay = -12 + 2 * 10 == 8
> 
> I think this is the same as "delay = interval - (-delay % interval)" if you
> prefer that instead.

Ah, I see. Sorry for my ignorance :)

Just an extra comment on the edge case:

now = 20;
delay == (10 - 20) == -10
=> delay += 10 * (10 / 10 + 1)
=> delay += 10 * (1 + 1)
=> new_delay = -10 + 2 * 10 == 10

But shouldn't the new_delay be 0 in this case?

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink_...
File media/base/null_video_sink_unittest.cc (right):

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink_...
media/base/null_video_sink_unittest.cc:53: return new_sink;
On 2015/04/29 00:54:07, DaleCurtis wrote:
> On 2015/04/28 23:12:06, xhwang wrote:
> > .Pass()?
> 
> Not necessary in this case, it's automatically moved.

Cool. TIL!

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink_...
media/base/null_video_sink_unittest.cc:155: 
On 2015/04/29 00:54:07, DaleCurtis wrote:
> On 2015/04/28 23:12:06, xhwang wrote:
> > Is it interesting to test the case where Render() takes some time that we
have
> > to skip a render interval?
> 
> Not particularly in this case, it was more important for audio. It's also hard
> to test given performance tolerances on the bot. I.e. we'd end up using a
sleep
> during Render() to stall the callback, but who knows what the result of that
> will be on a bot. It could sleep for 1 cycle or many.

Can you just call tick_clock_.Advance() in Render() to simulate the case that
Render() stalled callback?

https://codereview.chromium.org/1116473002/diff/20001/media/base/null_video_s...
File media/base/null_video_sink.h (right):

https://codereview.chromium.org/1116473002/diff/20001/media/base/null_video_s...
media/base/null_video_sink.h:42: void pause_render_callbacks(base::TimeTicks
pause_util) {
Here and below: based on the style guide:
1. Only accessors and mutators can use hacker style, and
2. Accessors and mutators (get and set functions) should match the name of the
variable they are getting and setting.

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names

These three functions are not following the style guide.

[DaleCurtis, 2015-04-29 04:26:48]

Thanks for review! We can discuss style tomorrow :)

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink.cc
File media/base/null_video_sink.cc (right):

https://codereview.chromium.org/1116473002/diff/1/media/base/null_video_sink....
media/base/null_video_sink.cc:81: delay += interval_ * (-delay / interval_ + 1);
On 2015/04/29 03:49:13, xhwang wrote:
> On 2015/04/29 00:54:07, DaleCurtis wrote:
> > On 2015/04/28 23:12:06, xhwang wrote:
> > > IIUIC, this translates to:
> > >    delay += interval_ * (-delay / interval_) + interval_;
> > > => new_delay = delay + interval_ * (-delay / interval_) + interval_;
> > > => new_delay = delay - delay + interval_ ;
> > > => new_delay = interval_
> > > 
> > > Is this what you want? It doesn't seem to give us the next interval
starting
> > > from current_render_time_...
> > 
> > That's not quite true since this is integer math and the parenthesis force
> > (-delay/interval_) to be computed first. Consider:
> > 
> > interval = 10
> > now = 12;
> > delay == (10 - 12) == -2
> > => delay += 10 * (2 / 10 + 1)
> > => delay += 10 * (0 + 1)
> > => new_delay = -2 + 10 = 8
> > 
> > now = 22;
> > delay == (10 - 22) == -12
> > => delay += 10 * (12 / 10 + 1)
> > => delay += 10 * (1 + 1)
> > => new_delay = -12 + 2 * 10 == 8
> > 
> > I think this is the same as "delay = interval - (-delay % interval)" if you
> > prefer that instead.
> 
> Ah, I see. Sorry for my ignorance :)
> 
> Just an extra comment on the edge case:
> 
> now = 20;
> delay == (10 - 20) == -10
> => delay += 10 * (10 / 10 + 1)
> => delay += 10 * (1 + 1)
> => new_delay = -10 + 2 * 10 == 10
> 
> But shouldn't the new_delay be 0 in this case?

It could go either way, since delay is 0, we may not be able to run right away
if other tasks are queued, so we have previously taken the safe route to wait
until the next callback.

https://codereview.chromium.org/1116473002/diff/20001/media/base/null_video_s...
File media/base/null_video_sink.h (right):

https://codereview.chromium.org/1116473002/diff/20001/media/base/null_video_s...
media/base/null_video_sink.h:42: void pause_render_callbacks(base::TimeTicks
pause_util) {
On 2015/04/29 03:49:13, xhwang wrote:
> Here and below: based on the style guide:
> 1. Only accessors and mutators can use hacker style, and
> 2. Accessors and mutators (get and set functions) should match the name of the
> variable they are getting and setting.
> 
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names
> 
> These three functions are not following the style guide.

Disagree especially on the middle one, for_testing() is very much an allowed
exception which we use all over the place. The last one I'll go ahead and
rename, and the first I'll move into a method as a hedge, but I believe these
are all okay as is.

Note: "You may also use lowercase letters for other very short inlined
functions. For example if a function were so cheap you would not cache the value
if you were calling it in a loop, then lowercase naming would be acceptable."

[DaleCurtis, 2015-04-29 04:27:09]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-04-29 04:27:10]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1116473002/#ps40001
(title: "Comments.")

[commit-bot: I haz the power, 2015-04-29 04:27:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1116473002/40001

[xhwang, 2015-04-29 05:20:19]

https://codereview.chromium.org/1116473002/diff/20001/media/base/null_video_s...
File media/base/null_video_sink.h (right):

https://codereview.chromium.org/1116473002/diff/20001/media/base/null_video_s...
media/base/null_video_sink.h:42: void pause_render_callbacks(base::TimeTicks
pause_util) {
On 2015/04/29 04:26:48, DaleCurtis wrote:
> On 2015/04/29 03:49:13, xhwang wrote:
> > Here and below: based on the style guide:
> > 1. Only accessors and mutators can use hacker style, and
> > 2. Accessors and mutators (get and set functions) should match the name of
the
> > variable they are getting and setting.
> > 
> >
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names
> > 
> > These three functions are not following the style guide.
> 
> Disagree especially on the middle one, for_testing() is very much an allowed
> exception which we use all over the place. The last one I'll go ahead and
> rename, and the first I'll move into a method as a hedge, but I believe these
> are all okay as is.
> 
> Note: "You may also use lowercase letters for other very short inlined
> functions. For example if a function were so cheap you would not cache the
value
> if you were calling it in a loop, then lowercase naming would be acceptable."

I missed this text. Thanks for pointing that out!

[commit-bot: I haz the power, 2015-04-29 08:31:15]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-29 08:32:15]

Patchset 3 (id:??) landed as
https://crrev.com/6523d600339aad86efcf1c238152e833c248dae5
Cr-Commit-Position: refs/heads/master@{#327450}