Connect the new video rendering path to the compositor.

Connect the new video rendering path to the compositor.

Hooks up the new VideoRendererAlgorithm to the VideoRendererImpl and it
to the VideoRendererSink (VideoFrameCompositor) which finally talks to
the VideoFrameProviderClientImpl!

All of this is behind a new flag: --enable-new-video-renderer for right
now. We'll slowly ramp it up based on crash and YouTube metrics.  It is
also available via chrome://flags for our YT friends to test immediately.

The playback experience is significantly smoother, dropped frame counts
are significantly more accurate, power consumption is much lower in
background tabs, and slightly lower in the foreground!

Smoothness scores (windows):

New:
24: Smoothness: 98.750000, Freezing: 100.000000, ~Dropped: 29 / 749 (3.871829%)
30: Smoothness: 99.330000, Freezing: 100.000000, ~Dropped: 0 / 900 (0.000000%)
60: Smoothness: 99.280000, Freezing: 99.670000, ~Dropped: 6 / 1800 (0.333333%)

Old:
24: Smoothness: 92.640000, Freezing: 100.000000, ~Dropped: 29 / 749 (3.871829%)
30: Smoothness: 68.490000, Freezing: 99.440000, ~Dropped: 5 / 900 (0.555556%)
60: Smoothness: 99.280000, Freezing: 99.670000, ~Dropped: 6 / 1800 (0.333333%)

See http://xorax.sea/barcode/results_win/ for further details.

Power consumption metrics on OSX for 1080p VP9 playback averaged over 10 runs:
New:
energy_consumption_mwh: 38.994639mWh
idle_wakeups_total: 1459.600000count
cpu_utilization (browser): 4.752003%
cpu_utilization (gpu): 7.791677%
cpu_utilization (renderer): 74.691065%

Old:
energy_consumption_mwh: 39.114139mWh
idle_wakeups_total: 1947.900000count
cpu_utilization (browser): 5.125488%
cpu_utilization (gpu): 8.624919%
cpu_utilization (renderer): 84.009566%

New Background:
energy_consumption_mwh: 31.402967mWh
idle_wakeups_total: 195.500000count
cpu_utilization (browser): 1.594367%
cpu_utilization (gpu): 0.154281%
cpu_utilization (renderer): 49.920668%

Old Background:
energy_consumption_mwh: 42.224069mWh
idle_wakeups_total: 522.200000count
cpu_utilization (browser): 1.629945%
cpu_utilization (gpu): 0.155414%
cpu_utilization (renderer): 63.333668%

See http://xorax.sea/barcode/results_osx/ for the full run details. I wouldn't
trust the absolute values for cpu or energy consumption, but the relative
difference should be meaningful.

This clamps background canvas updates from video to once every 250ms,
but in practice I couldn't tell the difference when tab switching in
a debug build.

BUG=386551, 396803, 438680, 438766, 439548, 460190
TEST=manual playback, unit tests
TBR=asvitkine

Committed: https://crrev.com/f28f0e50dd756f424541a734cfa8fc5f8542c57a
Cr-Commit-Position: refs/heads/master@{#327889}

[DaleCurtis, 2015-04-24 21:20:27]

dalecurtis@chromium.org changed reviewers:
+ brianderson@chromium.org, sunnyps@chromium.org, xhwang@chromium.org

[DaleCurtis, 2015-04-24 21:20:28]

Here's the final piece of the puzzle (modulo a 100% roll out that is)!

I still need to update all the unittests, but this is enough to give you an idea
of what it all looks like.

[miu, 2015-04-24 22:35:34]

Drive-by comment:

https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
File cc/layers/video_frame_provider_client_impl.cc (right):

https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
cc/layers/video_frame_provider_client_impl.cc:160:
!provider_->UpdateCurrentFrame(args.frame_time + args.interval,
Looks like this math is saying the frame will be delayed by one compositing
interval before being displayed.  Will this affect AV sync?

[DaleCurtis, 2015-04-24 22:43:18]

https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
File cc/layers/video_frame_provider_client_impl.cc (right):

https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
cc/layers/video_frame_provider_client_impl.cc:160:
!provider_->UpdateCurrentFrame(args.frame_time + args.interval,
On 2015/04/24 22:35:34, miu wrote:
> Looks like this math is saying the frame will be delayed by one compositing
> interval before being displayed.  Will this affect AV sync?

No, provided these values are when the frame is displayed onscreen, they are the
same values which will be used to choose the frame that best fits av sync.

briananderson, sunnyps: Can you confirm that we do want to use frame_time +
interval here and just frame_time?

[sunnyps, 2015-04-24 23:18:12]

https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
File cc/layers/video_frame_provider_client_impl.cc (right):

https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
cc/layers/video_frame_provider_client_impl.cc:27: stopped_(true) {
Let's not use the stopped_ variable for keeping track of when to start/stop
rendering. The stopped_ variable is intended to be true when the
VideoFrameProviderClientImpl has been torn down.

https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
cc/layers/video_frame_provider_client_impl.cc:115: TRACE_EVENT0("cc",
"VideoFrameProviderClientImpl::StartRendering");
Might want to do a DCHECK(stopped_) here.

https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
cc/layers/video_frame_provider_client_impl.cc:160:
!provider_->UpdateCurrentFrame(args.frame_time + args.interval,
On 2015/04/24 22:43:18, DaleCurtis wrote:
> On 2015/04/24 22:35:34, miu wrote:
> > Looks like this math is saying the frame will be delayed by one compositing
> > interval before being displayed.  Will this affect AV sync?
> 
> No, provided these values are when the frame is displayed onscreen, they are
the
> same values which will be used to choose the frame that best fits av sync.
> 
> briananderson, sunnyps: Can you confirm that we do want to use frame_time +
> interval here and just frame_time?

frame_time + interval is the time the frame would be displayed.

However I think we should use (frame_time, frame_time + interval) here because
we should make a best effort for rendering the frame on the next vsync (i.e.
frame_time + interval) after it's target timestamp.

[DaleCurtis, 2015-04-24 23:35:01]

https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
File cc/layers/video_frame_provider_client_impl.cc (right):

https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
cc/layers/video_frame_provider_client_impl.cc:160:
!provider_->UpdateCurrentFrame(args.frame_time + args.interval,
On 2015/04/24 23:18:11, sunnyps wrote:
> On 2015/04/24 22:43:18, DaleCurtis wrote:
> > On 2015/04/24 22:35:34, miu wrote:
> > > Looks like this math is saying the frame will be delayed by one
compositing
> > > interval before being displayed.  Will this affect AV sync?
> > 
> > No, provided these values are when the frame is displayed onscreen, they are
> the
> > same values which will be used to choose the frame that best fits av sync.
> > 
> > briananderson, sunnyps: Can you confirm that we do want to use frame_time +
> > interval here and just frame_time?
> 
> frame_time + interval is the time the frame would be displayed.
> 
> However I think we should use (frame_time, frame_time + interval) here because
> we should make a best effort for rendering the frame on the next vsync (i.e.
> frame_time + interval) after it's target timestamp.

Can you elaborate? It sounds like you're saying mutually exclusive things. :)
For the purposes of audio video sync we need to know exactly when the frame is
displayed; which seems to be frame_time+interval.

[sunnyps, 2015-04-24 23:38:21]

On 2015/04/24 23:35:01, DaleCurtis wrote:
>
https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
> File cc/layers/video_frame_provider_client_impl.cc (right):
> 
>
https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
> cc/layers/video_frame_provider_client_impl.cc:160:
> !provider_->UpdateCurrentFrame(args.frame_time + args.interval,
> On 2015/04/24 23:18:11, sunnyps wrote:
> > On 2015/04/24 22:43:18, DaleCurtis wrote:
> > > On 2015/04/24 22:35:34, miu wrote:
> > > > Looks like this math is saying the frame will be delayed by one
> compositing
> > > > interval before being displayed.  Will this affect AV sync?
> > > 
> > > No, provided these values are when the frame is displayed onscreen, they
are
> > the
> > > same values which will be used to choose the frame that best fits av sync.
> > > 
> > > briananderson, sunnyps: Can you confirm that we do want to use frame_time
+
> > > interval here and just frame_time?
> > 
> > frame_time + interval is the time the frame would be displayed.
> > 
> > However I think we should use (frame_time, frame_time + interval) here
because
> > we should make a best effort for rendering the frame on the next vsync (i.e.
> > frame_time + interval) after it's target timestamp.
> 
> Can you elaborate? It sounds like you're saying mutually exclusive things. :)
> For the purposes of audio video sync we need to know exactly when the frame is
> displayed; which seems to be frame_time+interval.

Let's say we have a video frame with a target timestamp = 8 ms and frame_time =
0ms, interval = 16ms. Should we render the video frame in this frame interval? I
think the answer is yes because the target timestamp is in the range
(frame_time, frame_time + interval).

[DaleCurtis, 2015-04-24 23:48:10]

On 2015/04/24 23:38:21, sunnyps wrote:
> On 2015/04/24 23:35:01, DaleCurtis wrote:
> >
>
https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
> > File cc/layers/video_frame_provider_client_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
> > cc/layers/video_frame_provider_client_impl.cc:160:
> > !provider_->UpdateCurrentFrame(args.frame_time + args.interval,
> > On 2015/04/24 23:18:11, sunnyps wrote:
> > > On 2015/04/24 22:43:18, DaleCurtis wrote:
> > > > On 2015/04/24 22:35:34, miu wrote:
> > > > > Looks like this math is saying the frame will be delayed by one
> > compositing
> > > > > interval before being displayed.  Will this affect AV sync?
> > > > 
> > > > No, provided these values are when the frame is displayed onscreen, they
> are
> > > the
> > > > same values which will be used to choose the frame that best fits av
sync.
> > > > 
> > > > briananderson, sunnyps: Can you confirm that we do want to use
frame_time
> +
> > > > interval here and just frame_time?
> > > 
> > > frame_time + interval is the time the frame would be displayed.
> > > 
> > > However I think we should use (frame_time, frame_time + interval) here
> because
> > > we should make a best effort for rendering the frame on the next vsync
(i.e.
> > > frame_time + interval) after it's target timestamp.
> > 
> > Can you elaborate? It sounds like you're saying mutually exclusive things.
:)
> > For the purposes of audio video sync we need to know exactly when the frame
is
> > displayed; which seems to be frame_time+interval.
> 
> Let's say we have a video frame with a target timestamp = 8 ms and frame_time
=
> 0ms, interval = 16ms. Should we render the video frame in this frame interval?
I
> think the answer is yes because the target timestamp is in the range
> (frame_time, frame_time + interval).

I think you've answered my question above then, which is that the current code
is incorrect -- I think we must have had a miscommunication along the way :)
frame_time+interval is _NOT_ the right starting value we want. We definitely do
want to display frames which are between (0, 16ms) in your example.

[DaleCurtis, 2015-04-24 23:49:31]

On 2015/04/24 23:48:10, DaleCurtis wrote:
> On 2015/04/24 23:38:21, sunnyps wrote:
> > On 2015/04/24 23:35:01, DaleCurtis wrote:
> > >
> >
>
https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
> > > File cc/layers/video_frame_provider_client_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
> > > cc/layers/video_frame_provider_client_impl.cc:160:
> > > !provider_->UpdateCurrentFrame(args.frame_time + args.interval,
> > > On 2015/04/24 23:18:11, sunnyps wrote:
> > > > On 2015/04/24 22:43:18, DaleCurtis wrote:
> > > > > On 2015/04/24 22:35:34, miu wrote:
> > > > > > Looks like this math is saying the frame will be delayed by one
> > > compositing
> > > > > > interval before being displayed.  Will this affect AV sync?
> > > > > 
> > > > > No, provided these values are when the frame is displayed onscreen,
they
> > are
> > > > the
> > > > > same values which will be used to choose the frame that best fits av
> sync.
> > > > > 
> > > > > briananderson, sunnyps: Can you confirm that we do want to use
> frame_time
> > +
> > > > > interval here and just frame_time?
> > > > 
> > > > frame_time + interval is the time the frame would be displayed.
> > > > 
> > > > However I think we should use (frame_time, frame_time + interval) here
> > because
> > > > we should make a best effort for rendering the frame on the next vsync
> (i.e.
> > > > frame_time + interval) after it's target timestamp.
> > > 
> > > Can you elaborate? It sounds like you're saying mutually exclusive things.
> :)
> > > For the purposes of audio video sync we need to know exactly when the
frame
> is
> > > displayed; which seems to be frame_time+interval.
> > 
> > Let's say we have a video frame with a target timestamp = 8 ms and
frame_time
> =
> > 0ms, interval = 16ms. Should we render the video frame in this frame
interval?
> I
> > think the answer is yes because the target timestamp is in the range
> > (frame_time, frame_time + interval).
> 
> I think you've answered my question above then, which is that the current code
> is incorrect -- I think we must have had a miscommunication along the way :)
> frame_time+interval is _NOT_ the right starting value we want. We definitely
do
> want to display frames which are between (0, 16ms) in your example.

I guess I'm still a bit confused then why you originally said that any video
frame I give during say (0,16ms) won't be displayed until (frame_time+interval,
frame_time + 2*interval) -- (16, 32) ?

[DaleCurtis, 2015-04-25 00:11:12]

On 2015/04/24 23:49:31, DaleCurtis wrote:
> On 2015/04/24 23:48:10, DaleCurtis wrote:
> > On 2015/04/24 23:38:21, sunnyps wrote:
> > > On 2015/04/24 23:35:01, DaleCurtis wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
> > > > File cc/layers/video_frame_provider_client_impl.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1083383005/diff/1/cc/layers/video_frame_provi...
> > > > cc/layers/video_frame_provider_client_impl.cc:160:
> > > > !provider_->UpdateCurrentFrame(args.frame_time + args.interval,
> > > > On 2015/04/24 23:18:11, sunnyps wrote:
> > > > > On 2015/04/24 22:43:18, DaleCurtis wrote:
> > > > > > On 2015/04/24 22:35:34, miu wrote:
> > > > > > > Looks like this math is saying the frame will be delayed by one
> > > > compositing
> > > > > > > interval before being displayed.  Will this affect AV sync?
> > > > > > 
> > > > > > No, provided these values are when the frame is displayed onscreen,
> they
> > > are
> > > > > the
> > > > > > same values which will be used to choose the frame that best fits av
> > sync.
> > > > > > 
> > > > > > briananderson, sunnyps: Can you confirm that we do want to use
> > frame_time
> > > +
> > > > > > interval here and just frame_time?
> > > > > 
> > > > > frame_time + interval is the time the frame would be displayed.
> > > > > 
> > > > > However I think we should use (frame_time, frame_time + interval) here
> > > because
> > > > > we should make a best effort for rendering the frame on the next vsync
> > (i.e.
> > > > > frame_time + interval) after it's target timestamp.
> > > > 
> > > > Can you elaborate? It sounds like you're saying mutually exclusive
things.
> > :)
> > > > For the purposes of audio video sync we need to know exactly when the
> frame
> > is
> > > > displayed; which seems to be frame_time+interval.
> > > 
> > > Let's say we have a video frame with a target timestamp = 8 ms and
> frame_time
> > =
> > > 0ms, interval = 16ms. Should we render the video frame in this frame
> interval?
> > I
> > > think the answer is yes because the target timestamp is in the range
> > > (frame_time, frame_time + interval).
> > 
> > I think you've answered my question above then, which is that the current
code
> > is incorrect -- I think we must have had a miscommunication along the way :)
> > frame_time+interval is _NOT_ the right starting value we want. We definitely
> do
> > want to display frames which are between (0, 16ms) in your example.
> 
> I guess I'm still a bit confused then why you originally said that any video
> frame I give during say (0,16ms) won't be displayed until
(frame_time+interval,
> frame_time + 2*interval) -- (16, 32) ?

Per chat discussion with Sunny, frame_time is the last vsync interval seen (as
given to the compositor by the OS), because it's the only known (not estimated)
value. We _do_ in fact want to choose frames based on (frame_time + interval,
frame_time + 2*interval) as that's when the frame is estimated to be displayed
on screen.

[DaleCurtis, 2015-04-25 23:10:47]

The latest patch set has all unit tests updated and passing when run with
--enable-new-video-renderer.

PTAL. Feature freeze for M44 is this Friday, May 1st, so please try to get your
feedback and (hopefully) approvals to me as soon as possible :)

[xhwang, 2015-04-28 18:09:27]

On 2015/04/25 23:10:47, DaleCurtis wrote:
> The latest patch set has all unit tests updated and passing when run with
> --enable-new-video-renderer.
> 
> PTAL. Feature freeze for M44 is this Friday, May 1st, so please try to get
your
> feedback and (hopefully) approvals to me as soon as possible :)

Is it possible to split this into multiple smaller CLs? For example, you can
have NullVideoSink in one CL, VRI+Test in one CL, then hooking everything up in
a third CL. That'll make the reviewing process much easier.

[DaleCurtis, 2015-04-28 18:17:58]

Hmm, I'd rather not and can't for most of it; tests will fail without the
majority of these changes. More so than my previous CLs everything in this CL is
intimately tied together. NullVideoSink is purely test code, I don't think it
warrants its own CL relative to the rest of this.

[xhwang, 2015-04-28 18:30:23]

On 2015/04/28 18:17:58, DaleCurtis wrote:
> Hmm, I'd rather not and can't for most of it; tests will fail without the
> majority of these changes. More so than my previous CLs everything in this CL
is
> intimately tied together. NullVideoSink is purely test code, I don't think it
> warrants its own CL relative to the rest of this.

hmm, without looking at the details, I wonder why changes in VRI cannot be in
one standalone CL. It's behind a flag, the current implementations are
empty/notimplemented, and the tests use a NullVideoSink. Nobody is using the new
path, and the old path should still work, right? I guess I am missing something
here.

NullVideoSink has ~200 lines of new code. If you separate it to one CL, you can
probably land it today, without waiting for the rest of this CL, and the
previous CL. IMHO it'll make check-in faster, and make the review of the rest of
this CL easier...

[DaleCurtis, 2015-04-28 20:28:56]

I've removed NullVideoSink at least, https://codereview.chromium.org/1116473002/

The rest I think should stay together.

[DaleCurtis, 2015-04-29 04:34:08]

Latest patch set has all metrics recorded and passes all non-image layout tests.
 The image ones require beginframes to be pumped, otherwise we never display a
frame.

sunnyps, brianderson: I've sent you an email about that, let me know how we can
get layout tests to use beginframes.

[xhwang, 2015-04-29 06:51:55]

I looked at most of VRI and it looks mostly good. But I definitely miss some
details. Let chat on this tomorrow which would be more effective :)

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:27: static const int
kBackgroundRenderingTimeoutMs = 250;
nit: s/static//

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:165: time_progressing_ = false;
Can we DCHECK these three?

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:580: RestartBackgroundRenderTimer();
It's a bit unclear to me when we should call this. Let's chat on this tomorrow
:)

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:734: // frame duration is unknown nad
we've recieved EOS, fire it once we get down
s/nad/and/

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
File media/renderers/video_renderer_impl.h (right):

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl.h:88: void
SetBackgroundRenderingForTesting(bool enabled, base::TimeDelta timeout);
nit: Add comment, or s/timeout/background_rendering_timeout/ to make it self
explanatory.

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl.h:147: // Return true if there is no more
room for additional buffered frames.
nit: Return_s_

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
File media/renderers/video_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl_unittest.cc:716: testing::Bool());
For better test names, use different prefix for different test params? e.g.

INSTANTIATE_TEST_CASE_P(OldVideoRenderer,
                        VideoRendererImplTest,
                        false);

INSTANTIATE_TEST_CASE_P(NewVideoRenderer,
                        VideoRendererImplTest,
                        true);

[xhwang, 2015-04-29 22:42:03]

I reviewed the whole CL and it looks mostly good. It's really nice to see all
pieces coming together and we have great smoothness and performance.

I'll wait for your update based on our offline discussion to take another look.

Thanks for the great work!

https://codereview.chromium.org/1083383005/diff/80001/media/blink/video_frame...
File media/blink/video_frame_compositor.cc (right):

https://codereview.chromium.org/1083383005/diff/80001/media/blink/video_frame...
media/blink/video_frame_compositor.cc:18: static const int
kStaleFrameThresholdMs = 250;
s/static//

https://codereview.chromium.org/1083383005/diff/80001/media/blink/video_frame...
media/blink/video_frame_compositor.cc:126: last_interval_ = deadline_max -
deadline_min;
We use |last| for most member variables here, but the frame is called
|current_frame_|. I understand the logic behind this, but it seems a bit odd.

https://codereview.chromium.org/1083383005/diff/80001/media/blink/video_frame...
media/blink/video_frame_compositor.cc:169: TRACE_EVENT0("media",
"VideoFrameCompositor::Stop");
nit: TRACE_EVENT0("media", __FUCNTION__) if you prefer, but totally up to you :)

https://codereview.chromium.org/1083383005/diff/80001/media/blink/video_frame...
File media/blink/video_frame_compositor_unittest.cc (right):

https://codereview.chromium.org/1083383005/diff/80001/media/blink/video_frame...
media/blink/video_frame_compositor_unittest.cc:108: 
nit: no need for extra line?

[DaleCurtis, 2015-04-30 03:47:02]

Patchset #6 (id:100001) has been deleted

[DaleCurtis, 2015-04-30 03:49:37]

This addresses all comments, but doesn't fix image based layout tests; since
this is behind a flag, that's not a blocker for landing this CL.

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:27: static const int
kBackgroundRenderingTimeoutMs = 250;
On 2015/04/29 06:51:54, xhwang wrote:
> nit: s/static//

Done.

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:165: time_progressing_ = false;
On 2015/04/29 06:51:55, xhwang wrote:
> Can we DCHECK these three?

Done.

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:580: RestartBackgroundRenderTimer();
On 2015/04/29 06:51:54, xhwang wrote:
> It's a bit unclear to me when we should call this. Let's chat on this tomorrow
> :)

Per offline discussion replaced with a repeating timer which checks a last known
render time.

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl.cc:734: // frame duration is unknown nad
we've recieved EOS, fire it once we get down
On 2015/04/29 06:51:54, xhwang wrote:
> s/nad/and/

Done.

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
File media/renderers/video_renderer_impl.h (right):

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl.h:88: void
SetBackgroundRenderingForTesting(bool enabled, base::TimeDelta timeout);
On 2015/04/29 06:51:55, xhwang wrote:
> nit: Add comment, or s/timeout/background_rendering_timeout/ to make it self
> explanatory.

Done.

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl.h:147: // Return true if there is no more
room for additional buffered frames.
On 2015/04/29 06:51:55, xhwang wrote:
> nit: Return_s_

Done.

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
File media/renderers/video_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/1083383005/diff/60001/media/renderers/video_r...
media/renderers/video_renderer_impl_unittest.cc:716: testing::Bool());
On 2015/04/29 06:51:55, xhwang wrote:
> For better test names, use different prefix for different test params? e.g.
> 
> INSTANTIATE_TEST_CASE_P(OldVideoRenderer,
>                         VideoRendererImplTest,
>                         false);
> 
> INSTANTIATE_TEST_CASE_P(NewVideoRenderer,
>                         VideoRendererImplTest,
>                         true);

Done.

https://codereview.chromium.org/1083383005/diff/80001/media/blink/video_frame...
File media/blink/video_frame_compositor.cc (right):

https://codereview.chromium.org/1083383005/diff/80001/media/blink/video_frame...
media/blink/video_frame_compositor.cc:18: static const int
kStaleFrameThresholdMs = 250;
On 2015/04/29 22:42:03, xhwang wrote:
> s/static//

Done.

https://codereview.chromium.org/1083383005/diff/80001/media/blink/video_frame...
media/blink/video_frame_compositor.cc:126: last_interval_ = deadline_max -
deadline_min;
On 2015/04/29 22:42:03, xhwang wrote:
> We use |last| for most member variables here, but the frame is called
> |current_frame_|. I understand the logic behind this, but it seems a bit odd.

Yeah it does seem odd, but I think it matches the interface pattern, so I'm
going to leave it as is.

https://codereview.chromium.org/1083383005/diff/80001/media/blink/video_frame...
media/blink/video_frame_compositor.cc:169: TRACE_EVENT0("media",
"VideoFrameCompositor::Stop");
On 2015/04/29 22:42:03, xhwang wrote:
> nit: TRACE_EVENT0("media", __FUCNTION__) if you prefer, but totally up to you
:)

Done.

https://codereview.chromium.org/1083383005/diff/80001/media/blink/video_frame...
File media/blink/video_frame_compositor_unittest.cc (right):

https://codereview.chromium.org/1083383005/diff/80001/media/blink/video_frame...
media/blink/video_frame_compositor_unittest.cc:108: 
On 2015/04/29 22:42:03, xhwang wrote:
> nit: no need for extra line?

Done.

[xhwang, 2015-04-30 16:39:21]

lgtm with a few comments. Thanks!

https://codereview.chromium.org/1083383005/diff/120001/media/blink/video_fram...
File media/blink/video_frame_compositor.cc (right):

https://codereview.chromium.org/1083383005/diff/120001/media/blink/video_fram...
media/blink/video_frame_compositor.cc:122: LOG(ERROR) << "Got begin frame
callback... but had no callback...";
In what case can this happen? Should this be a DCHECK?

https://codereview.chromium.org/1083383005/diff/120001/media/blink/video_fram...
media/blink/video_frame_compositor.cc:197: client_->DidReceiveFrame();
There's a chance that we didn't update the |current_frame_| (see l.204). In that
case should we still call DidReceiveFrame()?

https://codereview.chromium.org/1083383005/diff/120001/media/renderers/video_...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1083383005/diff/120001/media/renderers/video_...
media/renderers/video_renderer_impl.cc:558: frames_decoded_++;
Can we move this out of "if" and then drop it from PaintNextReadyFrame_Locked()
and DropNextReadyFrame_Locked()?

https://codereview.chromium.org/1083383005/diff/120001/media/renderers/video_...
media/renderers/video_renderer_impl.cc:656: // unrendered frames.
This comment doesn't apply to l.657, which needs a separate comment.

https://codereview.chromium.org/1083383005/diff/120001/media/renderers/video_...
File media/renderers/video_renderer_impl.h (right):

https://codereview.chromium.org/1083383005/diff/120001/media/renderers/video_...
media/renderers/video_renderer_impl.h:89: // amount of time to wait after the
last Render() call beforing starting the
s/beforing/before

https://codereview.chromium.org/1083383005/diff/120001/media/renderers/video_...
media/renderers/video_renderer_impl.h:143: void RestartBackgroundRenderTimer();
This is now only started once after we start the sink. Maybe just
"StartBackgr...."?

[DaleCurtis, 2015-04-30 18:06:00]

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

[DaleCurtis, 2015-04-30 18:06:01]

+creis for the chrome://flags and switch propagation changes:

chrome/app/generated_resources.grd
chrome/browser/about_flags.cc
content/browser/renderer_host/render_process_host_impl.cc

https://codereview.chromium.org/1083383005/diff/120001/media/blink/video_fram...
File media/blink/video_frame_compositor.cc (right):

https://codereview.chromium.org/1083383005/diff/120001/media/blink/video_fram...
media/blink/video_frame_compositor.cc:122: LOG(ERROR) << "Got begin frame
callback... but had no callback...";
On 2015/04/30 16:39:20, xhwang wrote:
> In what case can this happen? Should this be a DCHECK?

Left over debugging from when I was experimenting with delayed BeginFrame event
stoppage to fix layout tests.

https://codereview.chromium.org/1083383005/diff/120001/media/blink/video_fram...
media/blink/video_frame_compositor.cc:197: client_->DidReceiveFrame();
On 2015/04/30 16:39:20, xhwang wrote:
> There's a chance that we didn't update the |current_frame_| (see l.204). In
that
> case should we still call DidReceiveFrame()?

Whoops yes, rolled this into ProcessNewFrame() and fixed the Stop() case as
well.

https://codereview.chromium.org/1083383005/diff/120001/media/renderers/video_...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1083383005/diff/120001/media/renderers/video_...
media/renderers/video_renderer_impl.cc:558: frames_decoded_++;
On 2015/04/30 16:39:21, xhwang wrote:
> Can we move this out of "if" and then drop it from
PaintNextReadyFrame_Locked()
> and DropNextReadyFrame_Locked()?

Done.

https://codereview.chromium.org/1083383005/diff/120001/media/renderers/video_...
media/renderers/video_renderer_impl.cc:656: // unrendered frames.
On 2015/04/30 16:39:20, xhwang wrote:
> This comment doesn't apply to l.657, which needs a separate comment.

Done.

https://codereview.chromium.org/1083383005/diff/120001/media/renderers/video_...
File media/renderers/video_renderer_impl.h (right):

https://codereview.chromium.org/1083383005/diff/120001/media/renderers/video_...
media/renderers/video_renderer_impl.h:89: // amount of time to wait after the
last Render() call beforing starting the
On 2015/04/30 16:39:21, xhwang wrote:
> s/beforing/before

Done.

https://codereview.chromium.org/1083383005/diff/120001/media/renderers/video_...
media/renderers/video_renderer_impl.h:143: void RestartBackgroundRenderTimer();
On 2015/04/30 16:39:21, xhwang wrote:
> This is now only started once after we start the sink. Maybe just
> "StartBackgr...."?

Done.

[Charlie Reis, 2015-04-30 20:18:53]

LGTM for:
chrome/app/generated_resources.grd
chrome/browser/about_flags.cc
content/browser/renderer_host/render_process_host_impl.cc

[brianderson, 2015-04-30 22:15:32]

cc lgtm

[DaleCurtis, 2015-04-30 23:49:49]

Thanks for review everyone, I'll CQ after
https://codereview.chromium.org/1021943002/ lands later tonight.

[DaleCurtis, 2015-05-01 02:22:51]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-05-01 02:22:51]

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/1083383005/#ps160001
(title: "Fix only EOS failures.")

[commit-bot: I haz the power, 2015-05-01 02:23:02]

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

[commit-bot: I haz the power, 2015-05-01 03:22:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-01 03:22:18]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DaleCurtis, 2015-05-01 06:03:04]

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

[DaleCurtis, 2015-05-01 06:03:48]

tbr=asvitkine for corresponding LoginCustomFlags entry to histograms.xml

[DaleCurtis, 2015-05-01 06:03:58]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-05-01 06:03:58]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
creis@chromium.org, brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/1083383005/#ps180001
(title: "Update LoginCustomFlags histogram.")

[commit-bot: I haz the power, 2015-05-01 06:04:09]

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

[commit-bot: I haz the power, 2015-05-01 07:08:36]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2015-05-01 07:09:37]

Patchset 9 (id:??) landed as
https://crrev.com/f28f0e50dd756f424541a734cfa8fc5f8542c57a
Cr-Commit-Position: refs/heads/master@{#327889}

[Alexei Svitkine (slow), 2015-05-01 15:10:53]

lgtm