Prime the landing pad for the new video rendering pipeline.

Prime the landing pad for the new video rendering pipeline.

This is not a functional change, it only updates the interfaces and
call sites in preparation for switching to a vsync based video
rendering pipeline.

Some notes:
- Plumbs a VideoRendererSink into the the rendering pipeline; similar to
how we have an AudioRendererSink.
- A couple VideoRendererSink mocks are introduced which will be short
lived. Like audio, we will need fakes which can pump consumption tasks.
- The "PaintCB" callback has been temporarily placed on the new sink
interface such that in the field experiments can be run comparing the
performance of the video rendering approaches.
- Finally nukes Player_X11 since setting up a vsync renderer just for
unused tool code isn't worth the effort.
- Since compositor callbacks may stop due to visibility changes, the
new VideoRendererImpl will use a countdown timer to pump video playback
as frames expire; expired frames will not count as dropped.
- Since canvas/WebGL requires frame updates in the background a new
method has been added to VideoFrameCompositor to return the current
frame if it was updated with 250ms, or to request a new one and return
the updated one.

Subsequent work:
- sunnyps@ will be switching VideoFrameProviderClientImpl over to using
a BeginFrameObserver, which will ultimately drive the Render() callbacks.
- dalecurtis@ will land the VideoRendererAlgorithm which powers the new
rendering pipeline.

BUG=439548
TEST=everything works as is.

Committed: https://crrev.com/45a3c93f745eabf6c1b1cbdac87ed4350a919e76
Cr-Commit-Position: refs/heads/master@{#325306}

[DaleCurtis, 2015-04-02 22:30:10]

Patchset #2 (id:20001) has been deleted

[DaleCurtis, 2015-04-02 22:30:17]

Patchset #1 (id:1) has been deleted

[DaleCurtis, 2015-04-02 22:31:35]

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

[DaleCurtis, 2015-04-02 22:31:36]

Hi guys, please review the base work for the new video rendering pathway.

[DaleCurtis, 2015-04-02 22:31:52]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-03 00:55:04]

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

[DaleCurtis, 2015-04-03 00:58:37]

(just a CQ dry run)

[xhwang, 2015-04-03 07:00:14]

Thanks! A lot of questions trying to understand the design better :)

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:57: // Called by the client on a regular
interval. Returns true if a new frame is
nit: s/is/will be/ ?

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:59: // presentation interval [|deadline_min|,
|deadline_max|].
So I assume the "interval" will be the vsync interval? Is it possible that there
will be >1 frames available during this "interval"? In that case, should only
the newest frame be returned by GetCurrentFrame(), and all other frames be
dropped?

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:61: base::TimeTicks deadline_max) = 0;
bikeshedding on naming: This really is something like UpdateCurrentFrame(). It
doesn't "Render" anything, and it doesn't even retrieve the current frame....

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:70: virtual void PutCurrentFrame() = 0;
Here we have GetCurrentFrame/PutCurrentFrame. Then in the sink we have Render()
to return the frame. Is there any reason for the difference? Is it because
GetCurrentFrame() is also needed by
WebMediaPlayerImpl::GetCurrentFrameFromCompositor()?

https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:74: // visibility. It will not start any
Render() calls before notifying the
What about using web canvas to manipulate video? In that case, is the "output
layer" visible? If the Client doesn't issue Render(), the canvas won't be able
to get new frames...

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
File media/base/video_renderer_sink.h (right):

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
media/base/video_renderer_sink.h:25: // presentation interval [|deadline_min|,
|deadline_max|].  Returns NULL if
ditto: What if there are multiple frames in that interval? Shall the newest
frame be returned?

Also, will the interval be contiguous (no overlapping and no gap)?

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
media/base/video_renderer_sink.h:35: bool is_visible) = 0;
VideoFrameProvider has a separate SetVisible(bool visible) method. Is there any
reason for this difference?

https://codereview.chromium.org/1053113002/diff/60001/media/tools/player_x11/...
File media/tools/player_x11/data_source_logger.h (left):

https://codereview.chromium.org/1053113002/diff/60001/media/tools/player_x11/...
media/tools/player_x11/data_source_logger.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
woo-hoo!

[DaleCurtis, 2015-04-03 17:14:53]

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:59: // presentation interval [|deadline_min|,
|deadline_max|].
On 2015/04/03 07:00:13, xhwang wrote:
> So I assume the "interval" will be the vsync interval? Is it possible that
there
> will be >1 frames available during this "interval"? In that case, should only
> the newest frame be returned by GetCurrentFrame(), and all other frames be
> dropped?

Yup, you can see what the mostly complete algorithm looks like here:
https://codereview.chromium.org/1021943002/ -- I still need to put it through
testing paces and resolve some TODOs, but the general strategy is finalized.

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:61: base::TimeTicks deadline_max) = 0;
On 2015/04/03 07:00:13, xhwang wrote:
> bikeshedding on naming: This really is something like UpdateCurrentFrame(). It
> doesn't "Render" anything, and it doesn't even retrieve the current frame....

Yeah... I went back and forth on this: Either keep consistency with the sink
interface or with today's methods. Your comment has convinced me this should
likely be UpdateCurrentFrame, so I'll change it :)

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:70: virtual void PutCurrentFrame() = 0;
On 2015/04/03 07:00:13, xhwang wrote:
> Here we have GetCurrentFrame/PutCurrentFrame. Then in the sink we have
Render()
> to return the frame. Is there any reason for the difference? Is it because
> GetCurrentFrame() is also needed by
> WebMediaPlayerImpl::GetCurrentFrameFromCompositor()?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...

The compositor is what I'd call a two-phase process, it first "claims" the frame
via GetCurrentFrame() and then may or may not composite it later. If it does
composite the frame, PutCurrentFrame will be called.

The sink will receive an OnFrameDropped() call for new frames vended via
VRS::Render() which either don't get a GetCurrentFrame() or don't get a
PutCurrentFrame() before the next VRS::Render() call.

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:74: // visibility. It will not start any
Render() calls before notifying the
On 2015/04/03 07:00:13, xhwang wrote:
> What about using web canvas to manipulate video? In that case, is the "output
> layer" visible? If the Client doesn't issue Render(), the canvas won't be able
> to get new frames...

Sunny, can you clarify on this? It's my expectation that we'll only stop
receiving vsync callbacks when the entire tab goes into the background, but not
if the tab is in the foreground but the VideoLayer is invisible.

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
File media/base/video_renderer_sink.h (right):

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
media/base/video_renderer_sink.h:25: // presentation interval [|deadline_min|,
|deadline_max|].  Returns NULL if
On 2015/04/03 07:00:13, xhwang wrote:
> ditto: What if there are multiple frames in that interval? Shall the newest
> frame be returned?
> 
> Also, will the interval be contiguous (no overlapping and no gap)?

It's expected to be contiguous and monotonically increasing. The current
algorithm increasing the display count of the last rendered frame for missed
intervals.

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
media/base/video_renderer_sink.h:35: bool is_visible) = 0;
On 2015/04/03 07:00:13, xhwang wrote:
> VideoFrameProvider has a separate SetVisible(bool visible) method. Is there
any
> reason for this difference?

I was hoping to avoid having to have the VideoRendererImpl pump callbacks to its
own Render() method, but now I'm not sure. Let me explain my thought process and
hopefully you can help reach the best decision :)

Visibility seems a blink/cc level concept, so I'm still not 100% happy with this
flag as it almost feels like a layer violation... but we need some way to not
count drop frames when render intervals expand to say 100ms vs the typical 16ms
vsync interval. If we can find another way to ignore dropped frames for
invisibility, we can drop this (which would be the best solution I think). Today
Pipeline owns the pipeline statistics, which makes it tricky to deal with the
incorrect reporting.

It also seems cleanest to have VideoFrameCompositor own a FakeVideoRendererSink
which it uses as a proxy to pump Render() callbacks to the VideoRendererImpl
when visibility is lost; even though VideoFrameCompositor or VideoRendererImpl
could own a FakeVideoRendererSink which it would use to pump callbacks to.

All this lead me to having VFC pump Render() callbacks with visibility=false
such that frame rendering is done correctly (i.e. frames dropped and requested
as normal), but dropped frames just aren't counted. Since VRI doesn't seem to
need this value outside of during Render() I put it on that method.

[xhwang, 2015-04-03 19:18:27]

Here's are my thoughts. Let me know if you want to chat offline ;)

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:70: virtual void PutCurrentFrame() = 0;
On 2015/04/03 17:14:52, DaleCurtis wrote:
> On 2015/04/03 07:00:13, xhwang wrote:
> > Here we have GetCurrentFrame/PutCurrentFrame. Then in the sink we have
> Render()
> > to return the frame. Is there any reason for the difference? Is it because
> > GetCurrentFrame() is also needed by
> > WebMediaPlayerImpl::GetCurrentFrameFromCompositor()?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...
> 
> The compositor is what I'd call a two-phase process, it first "claims" the
frame
> via GetCurrentFrame() and then may or may not composite it later. If it does
> composite the frame, PutCurrentFrame will be called.
> 
> The sink will receive an OnFrameDropped() call for new frames vended via
> VRS::Render() which either don't get a GetCurrentFrame() or don't get a
> PutCurrentFrame() before the next VRS::Render() call.

Let me make sure I understand "dropped frame" count correctly, a frame is
dropped when:
1, A frame is already not the newest when Render() is called, or
2, A frame is promoted to "current_frame" in Render(), but GetCurrentFrame() is
not called before the next Render() call, or
3, A frame is returned by GetCurrentFrame(), but PutCurrentFrame() is not called
before the next Render() call.

Is this accurate? 

BTW, based on the comment, PutCurrentFrame() really should be something like
CurrentFrameRendered()...

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:74: // visibility. It will not start any
Render() calls before notifying the
On 2015/04/03 17:14:52, DaleCurtis wrote:
> On 2015/04/03 07:00:13, xhwang wrote:
> > What about using web canvas to manipulate video? In that case, is the
"output
> > layer" visible? If the Client doesn't issue Render(), the canvas won't be
able
> > to get new frames...
> 
> Sunny, can you clarify on this? It's my expectation that we'll only stop
> receiving vsync callbacks when the entire tab goes into the background, but
not
> if the tab is in the foreground but the VideoLayer is invisible.

What if the JS app draw the video into a canvas, grab the pixels from the
canvas, and  store them or send them to the server? This has nothing to do with
visibility and it probably *should* work even if the tab is in background or the
whole browser is hidden.

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
File media/base/video_renderer_sink.h (right):

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
media/base/video_renderer_sink.h:25: // presentation interval [|deadline_min|,
|deadline_max|].  Returns NULL if
On 2015/04/03 17:14:52, DaleCurtis wrote:
> On 2015/04/03 07:00:13, xhwang wrote:
> > ditto: What if there are multiple frames in that interval? Shall the newest
> > frame be returned?
> > 
> > Also, will the interval be contiguous (no overlapping and no gap)?
> 
> It's expected to be contiguous and monotonically increasing. The current
> algorithm increasing the display count of the last rendered frame for missed
> intervals.

Yeah, that makes sense. Let's update the comment to be clear about this.

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
media/base/video_renderer_sink.h:35: bool is_visible) = 0;
On 2015/04/03 17:14:52, DaleCurtis wrote:
> On 2015/04/03 07:00:13, xhwang wrote:
> > VideoFrameProvider has a separate SetVisible(bool visible) method. Is there
> any
> > reason for this difference?
> 
> I was hoping to avoid having to have the VideoRendererImpl pump callbacks to
its
> own Render() method, but now I'm not sure. Let me explain my thought process
and
> hopefully you can help reach the best decision :)
> 
> Visibility seems a blink/cc level concept, so I'm still not 100% happy with
this
> flag as it almost feels like a layer violation... but we need some way to not
> count drop frames when render intervals expand to say 100ms vs the typical
16ms
> vsync interval. If we can find another way to ignore dropped frames for
> invisibility, we can drop this (which would be the best solution I think).
Today
> Pipeline owns the pipeline statistics, which makes it tricky to deal with the
> incorrect reporting.

Agreed "visibility" should be a cc layer concept. If "visibility" is false, can
the cc code keep calling GetCurrentFrame() and drop the frame onto the floor
immediately, and then call PutCurrentFrame() to pretend it's rendered? One issue
from this model is that we still need the cc layer to issue Render() calls
periodically even when the tab is in background. Is that a performance concern
you try to solve?

> It also seems cleanest to have VideoFrameCompositor own a
FakeVideoRendererSink
> which it uses as a proxy to pump Render() callbacks to the VideoRendererImpl
> when visibility is lost; even though VideoFrameCompositor or VideoRendererImpl
> could own a FakeVideoRendererSink which it would use to pump callbacks to.

> All this lead me to having VFC pump Render() callbacks with visibility=false
> such that frame rendering is done correctly (i.e. frames dropped and requested
> as normal), but dropped frames just aren't counted. Since VRI doesn't seem to
> need this value outside of during Render() I put it on that method.

Not sure about the details of this plan. But if someone must issue Render()
anyways, why can't we just let the cc code to do it. (See comment above.)

One thing to note is that when the Render() interval is large (e.g. 200ms),
VideoRenderImpl should drop decoded frame immediately if it's behind the last
Render() call interval. Otherwise, since we only buffer 4 frames and stop
buffering more, we could always lag behind (e.g. drop 4 frames for every
Render() call).

[DaleCurtis, 2015-04-03 19:59:16]

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:70: virtual void PutCurrentFrame() = 0;
On 2015/04/03 19:18:27, xhwang wrote:
> On 2015/04/03 17:14:52, DaleCurtis wrote:
> > On 2015/04/03 07:00:13, xhwang wrote:
> > > Here we have GetCurrentFrame/PutCurrentFrame. Then in the sink we have
> > Render()
> > > to return the frame. Is there any reason for the difference? Is it because
> > > GetCurrentFrame() is also needed by
> > > WebMediaPlayerImpl::GetCurrentFrameFromCompositor()?
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...
> > 
> > The compositor is what I'd call a two-phase process, it first "claims" the
> frame
> > via GetCurrentFrame() and then may or may not composite it later. If it does
> > composite the frame, PutCurrentFrame will be called.
> > 
> > The sink will receive an OnFrameDropped() call for new frames vended via
> > VRS::Render() which either don't get a GetCurrentFrame() or don't get a
> > PutCurrentFrame() before the next VRS::Render() call.
> 
> Let me make sure I understand "dropped frame" count correctly, a frame is
> dropped when:
> 1, A frame is already not the newest when Render() is called, or
> 2, A frame is promoted to "current_frame" in Render(), but GetCurrentFrame()
is
> not called before the next Render() call, or
> 3, A frame is returned by GetCurrentFrame(), but PutCurrentFrame() is not
called
> before the next Render() call.
> 
> Is this accurate? 
> 
> BTW, based on the comment, PutCurrentFrame() really should be something like
> CurrentFrameRendered()...

Yes this is correct. I'd be okay with renaming it. WDYT Sunny?

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:74: // visibility. It will not start any
Render() calls before notifying the
On 2015/04/03 19:18:27, xhwang wrote:
> On 2015/04/03 17:14:52, DaleCurtis wrote:
> > On 2015/04/03 07:00:13, xhwang wrote:
> > > What about using web canvas to manipulate video? In that case, is the
> "output
> > > layer" visible? If the Client doesn't issue Render(), the canvas won't be
> able
> > > to get new frames...
> > 
> > Sunny, can you clarify on this? It's my expectation that we'll only stop
> > receiving vsync callbacks when the entire tab goes into the background, but
> not
> > if the tab is in the foreground but the VideoLayer is invisible.
> 
> What if the JS app draw the video into a canvas, grab the pixels from the
> canvas, and  store them or send them to the server? This has nothing to do
with
> visibility and it probably *should* work even if the tab is in background or
the
> whole browser is hidden.

I think if we update every say 100ms that should be fine. Once the app goes into
the background its timers will also be slowed down; requestAnimationFrame won't
fire and setTimeout, setInterval extend to once a second I think. Perhaps we
could detect paint() calls and switch to a higher frequency update.

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
File media/base/video_renderer_sink.h (right):

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
media/base/video_renderer_sink.h:35: bool is_visible) = 0;
On 2015/04/03 19:18:27, xhwang wrote:
> On 2015/04/03 17:14:52, DaleCurtis wrote:
> > On 2015/04/03 07:00:13, xhwang wrote:
> > > VideoFrameProvider has a separate SetVisible(bool visible) method. Is
there
> > any
> > > reason for this difference?
> > 
> > I was hoping to avoid having to have the VideoRendererImpl pump callbacks to
> its
> > own Render() method, but now I'm not sure. Let me explain my thought process
> and
> > hopefully you can help reach the best decision :)
> > 
> > Visibility seems a blink/cc level concept, so I'm still not 100% happy with
> this
> > flag as it almost feels like a layer violation... but we need some way to
not
> > count drop frames when render intervals expand to say 100ms vs the typical
> 16ms
> > vsync interval. If we can find another way to ignore dropped frames for
> > invisibility, we can drop this (which would be the best solution I think).
> Today
> > Pipeline owns the pipeline statistics, which makes it tricky to deal with
the
> > incorrect reporting.
> 
> Agreed "visibility" should be a cc layer concept. If "visibility" is false,
can
> the cc code keep calling GetCurrentFrame() and drop the frame onto the floor
> immediately, and then call PutCurrentFrame() to pretend it's rendered? One
issue
> from this model is that we still need the cc layer to issue Render() calls
> periodically even when the tab is in background. Is that a performance concern
> you try to solve?

Yes, it's power concern; which is why the cc/ time seems to be actively working
to remove background callbacks like this. The cc/ code couldn't skew the render
interval for power savings without letting VRI know somehow, otherwise we'd
count the frames which weren't picked for the longer interval as dropped; which
doesn't seem correct (and surely our friends who care about dropped frames a lot
won't like this :)

> 
> > It also seems cleanest to have VideoFrameCompositor own a
> FakeVideoRendererSink
> > which it uses as a proxy to pump Render() callbacks to the VideoRendererImpl
> > when visibility is lost; even though VideoFrameCompositor or
VideoRendererImpl
> > could own a FakeVideoRendererSink which it would use to pump callbacks to.
> 
> > All this lead me to having VFC pump Render() callbacks with visibility=false
> > such that frame rendering is done correctly (i.e. frames dropped and
requested
> > as normal), but dropped frames just aren't counted. Since VRI doesn't seem
to
> > need this value outside of during Render() I put it on that method.
> 
> Not sure about the details of this plan. But if someone must issue Render()
> anyways, why can't we just let the cc code to do it. (See comment above.)
> 
> One thing to note is that when the Render() interval is large (e.g. 200ms),
> VideoRenderImpl should drop decoded frame immediately if it's behind the last
> Render() call interval. Otherwise, since we only buffer 4 frames and stop
> buffering more, we could always lag behind (e.g. drop 4 frames for every
> Render() call).

Correct. It was my intent that larger render intervals would cause multiple
frames to drop and then requeue new frames as they exceed their potential
display range; this is why we need the visibility signal to count these
appropriately.

[xhwang, 2015-04-03 21:27:55]

I am trying to give a clear definition of "number of dropped frames". 

How about this:

Number of video frames that are not rendered on screen when the video layer is
visible.

With this, I think passing "visibility" along is fine. But that should only be
used for counting number of dropped frames. The CC layer will still pump
Render() at a frequency it deems appropriate. WDYT?

[DaleCurtis, 2015-04-03 21:52:39]

On 2015/04/03 21:27:55, xhwang wrote:
> I am trying to give a clear definition of "number of dropped frames". 
> 
> How about this:
> 
> Number of video frames that are not rendered on screen when the video layer is
> visible.

Yes, though I'm not 100% clear on if it's when the page is visible or the video
layer is visible. We'll need Sunny/compositing folk to clarify.

> 
> With this, I think passing "visibility" along is fine. But that should only be
> used for counting number of dropped frames. The CC layer will still pump
> Render() at a frequency it deems appropriate. WDYT?

Sunny, you mentioned the compositor team wants to remove this type of background
pump. I don't think we can avoid it for playing media. Do you have a preference
for who does this? Having the CC layer do it is certainly the easiest solution,
but the VFC could also install a fake sink for such purposes.

[sunnyps, 2015-04-03 23:46:29]

On 2015/04/03 21:52:39, DaleCurtis wrote:
> On 2015/04/03 21:27:55, xhwang wrote:
> > I am trying to give a clear definition of "number of dropped frames". 
> > 
> > How about this:
> > 
> > Number of video frames that are not rendered on screen when the video layer
is
> > visible.
> 
> Yes, though I'm not 100% clear on if it's when the page is visible or the
video
> layer is visible. We'll need Sunny/compositing folk to clarify.

The SetVisible signal is meant to be related to the tab being visible or not. It
has nothing to do with video layer visibility.

> 
> > 
> > With this, I think passing "visibility" along is fine. But that should only
be
> > used for counting number of dropped frames. The CC layer will still pump
> > Render() at a frequency it deems appropriate. WDYT?
> 
> Sunny, you mentioned the compositor team wants to remove this type of
background
> pump. I don't think we can avoid it for playing media. Do you have a
preference
> for who does this? Having the CC layer do it is certainly the easiest
solution,
> but the VFC could also install a fake sink for such purposes.

I think VFC should install a fake pump - the reason for this is that we *might*
not need to pump at the refresh rate of the display. We could conceivably pump
at the video fps or even lower than that. We don't have that kind of flexibility
in cc. We have a 1 sec background tick when a tab is invisible and that's slated
to be removed soon.

[DaleCurtis, 2015-04-07 00:40:52]

Comments addressed and rebased on top of
https://codereview.chromium.org/1068593003/ which lets us avoid multiple
Start/Stop methods on the compositor interface.

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:57: // Called by the client on a regular
interval. Returns true if a new frame is
On 2015/04/03 07:00:13, xhwang wrote:
> nit: s/is/will be/ ?

Done.

https://codereview.chromium.org/1053113002/diff/60001/cc/layers/video_frame_p...
cc/layers/video_frame_provider.h:61: base::TimeTicks deadline_max) = 0;
On 2015/04/03 17:14:52, DaleCurtis wrote:
> On 2015/04/03 07:00:13, xhwang wrote:
> > bikeshedding on naming: This really is something like UpdateCurrentFrame().
It
> > doesn't "Render" anything, and it doesn't even retrieve the current
frame....
> 
> Yeah... I went back and forth on this: Either keep consistency with the sink
> interface or with today's methods. Your comment has convinced me this should
> likely be UpdateCurrentFrame, so I'll change it :)

Done.

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
File media/base/video_renderer_sink.h (right):

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
media/base/video_renderer_sink.h:25: // presentation interval [|deadline_min|,
|deadline_max|].  Returns NULL if
On 2015/04/03 19:18:27, xhwang wrote:
> On 2015/04/03 17:14:52, DaleCurtis wrote:
> > On 2015/04/03 07:00:13, xhwang wrote:
> > > ditto: What if there are multiple frames in that interval? Shall the
newest
> > > frame be returned?
> > > 
> > > Also, will the interval be contiguous (no overlapping and no gap)?
> > 
> > It's expected to be contiguous and monotonically increasing. The current
> > algorithm increasing the display count of the last rendered frame for missed
> > intervals.
> 
> Yeah, that makes sense. Let's update the comment to be clear about this.

Done.

https://codereview.chromium.org/1053113002/diff/60001/media/base/video_render...
media/base/video_renderer_sink.h:35: bool is_visible) = 0;
On 2015/04/03 19:59:16, DaleCurtis wrote:
> On 2015/04/03 19:18:27, xhwang wrote:
> > On 2015/04/03 17:14:52, DaleCurtis wrote:
> > > On 2015/04/03 07:00:13, xhwang wrote:
> > > > VideoFrameProvider has a separate SetVisible(bool visible) method. Is
> there
> > > any
> > > > reason for this difference?
> > > 
> > > I was hoping to avoid having to have the VideoRendererImpl pump callbacks
to
> > its
> > > own Render() method, but now I'm not sure. Let me explain my thought
process
> > and
> > > hopefully you can help reach the best decision :)
> > > 
> > > Visibility seems a blink/cc level concept, so I'm still not 100% happy
with
> > this
> > > flag as it almost feels like a layer violation... but we need some way to
> not
> > > count drop frames when render intervals expand to say 100ms vs the typical
> > 16ms
> > > vsync interval. If we can find another way to ignore dropped frames for
> > > invisibility, we can drop this (which would be the best solution I think).
> > Today
> > > Pipeline owns the pipeline statistics, which makes it tricky to deal with
> the
> > > incorrect reporting.
> > 
> > Agreed "visibility" should be a cc layer concept. If "visibility" is false,
> can
> > the cc code keep calling GetCurrentFrame() and drop the frame onto the floor
> > immediately, and then call PutCurrentFrame() to pretend it's rendered? One
> issue
> > from this model is that we still need the cc layer to issue Render() calls
> > periodically even when the tab is in background. Is that a performance
concern
> > you try to solve?
> 
> Yes, it's power concern; which is why the cc/ time seems to be actively
working
> to remove background callbacks like this. The cc/ code couldn't skew the
render
> interval for power savings without letting VRI know somehow, otherwise we'd
> count the frames which weren't picked for the longer interval as dropped;
which
> doesn't seem correct (and surely our friends who care about dropped frames a
lot
> won't like this :)
> 
> > 
> > > It also seems cleanest to have VideoFrameCompositor own a
> > FakeVideoRendererSink
> > > which it uses as a proxy to pump Render() callbacks to the
VideoRendererImpl
> > > when visibility is lost; even though VideoFrameCompositor or
> VideoRendererImpl
> > > could own a FakeVideoRendererSink which it would use to pump callbacks to.
> > 
> > > All this lead me to having VFC pump Render() callbacks with
visibility=false
> > > such that frame rendering is done correctly (i.e. frames dropped and
> requested
> > > as normal), but dropped frames just aren't counted. Since VRI doesn't seem
> to
> > > need this value outside of during Render() I put it on that method.
> > 
> > Not sure about the details of this plan. But if someone must issue Render()
> > anyways, why can't we just let the cc code to do it. (See comment above.)
> > 
> > One thing to note is that when the Render() interval is large (e.g. 200ms),
> > VideoRenderImpl should drop decoded frame immediately if it's behind the
last
> > Render() call interval. Otherwise, since we only buffer 4 frames and stop
> > buffering more, we could always lag behind (e.g. drop 4 frames for every
> > Render() call).
> 
> Correct. It was my intent that larger render intervals would cause multiple
> frames to drop and then requeue new frames as they exceed their potential
> display range; this is why we need the visibility signal to count these
> appropriately.
> 

As discussed via chat, these have been replaced with OnRenderResumed() and
OnRenderSuspended(). We'll have the VideoRendererImpl itself pump the fake
callbacks based on knowledge of the frame durations.

[DaleCurtis, 2015-04-07 19:58:45]

https://codereview.chromium.org/1053113002/diff/80001/media/base/video_render...
File media/base/video_renderer_sink.h (right):

https://codereview.chromium.org/1053113002/diff/80001/media/base/video_render...
media/base/video_renderer_sink.h:30: virtual scoped_refptr<VideoFrame>
Render(base::TimeTicks deadline_min,
Since VideoFrameCompositor will need to pump and the actual compositor will one
day want to know the next frame TimeTick estimate. I'm going to add back the
"base::TimeTicks* next_frame_time_hint" which the fake pump will use to
determine the best pump rate.

https://codereview.chromium.org/1053113002/diff/80001/media/base/video_render...
media/base/video_renderer_sink.h:37: // Render() callbacks may be suspended for
compositing reasons.  There will
As discussed offline, these don't work since VideoFrameCompositor (or someone
above it) needs to know the current frame -- so it needs to be the one pumping.
I'm going to drop these in favor of going back to the "visible" boolean which is
passed to Render().

[xhwang, 2015-04-08 04:45:13]

Do you want to rebase this CL on top of the other one before I take another
look? Or do you want me to take a look as is? Thanks!

[DaleCurtis, 2015-04-08 17:17:28]

I'm waiting for the discussion on https://codereview.chromium.org/1039533002/ to
resolve so we know if we need to support a fake pump w/ a "don't drop frames"
notification.

[DaleCurtis, 2015-04-13 18:29:27]

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

[DaleCurtis, 2015-04-13 18:29:27]

PTAL. I've removed all visibility related code. Now we'll use xhwang@'s
suggestion and have a countdown timer which will pump the media loop when vsync
callbacks stop. For canvas, WebGL users we'll update the frame if it's older
than 250ms.

I believe this is now ready to land if everyone is happy with the interfaces :)

[sunnyps, 2015-04-13 18:43:49]

On 2015/04/13 18:29:27, DaleCurtis wrote:
> PTAL. I've removed all visibility related code. Now we'll use xhwang@'s
> suggestion and have a countdown timer which will pump the media loop when
vsync
> callbacks stop. For canvas, WebGL users we'll update the frame if it's older
> than 250ms.
> 
> I believe this is now ready to land if everyone is happy with the interfaces
:)

I have one question: Are StartRendering/StopRendering calls tied to video being
resumed/paused?

[DaleCurtis, 2015-04-13 18:45:47]

On 2015/04/13 18:43:49, sunnyps wrote:
> On 2015/04/13 18:29:27, DaleCurtis wrote:
> > PTAL. I've removed all visibility related code. Now we'll use xhwang@'s
> > suggestion and have a countdown timer which will pump the media loop when
> vsync
> > callbacks stop. For canvas, WebGL users we'll update the frame if it's older
> > than 250ms.
> > 
> > I believe this is now ready to land if everyone is happy with the interfaces
> :)
> 
> I have one question: Are StartRendering/StopRendering calls tied to video
being
> resumed/paused?

Yes, if playback_rate == 0 or the video is paused, we'll ensure StopRendering()
is called via VideoRendererSink::Stop().

[sunnyps, 2015-04-13 18:56:54]

On 2015/04/13 18:45:47, DaleCurtis wrote:
> On 2015/04/13 18:43:49, sunnyps wrote:
> > On 2015/04/13 18:29:27, DaleCurtis wrote:
> > > PTAL. I've removed all visibility related code. Now we'll use xhwang@'s
> > > suggestion and have a countdown timer which will pump the media loop when
> > vsync
> > > callbacks stop. For canvas, WebGL users we'll update the frame if it's
older
> > > than 250ms.
> > > 
> > > I believe this is now ready to land if everyone is happy with the
interfaces
> > :)
> > 
> > I have one question: Are StartRendering/StopRendering calls tied to video
> being
> > resumed/paused?
> 
> Yes, if playback_rate == 0 or the video is paused, we'll ensure
StopRendering()
> is called via VideoRendererSink::Stop().

The video renderer -> cc interface LGTM. You'll have to get a cc video OWNER
(e.g. danakj@) to sign off on the cc parts.

[DaleCurtis, 2015-04-13 19:57:51]

Thanks. I added Brian previously; should I add enne@ or danakj@ then too?

[xhwang, 2015-04-13 20:50:30]

The interface looks good in general. I just have a lot of nits...

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:30: // StopUsingProvider() may be called from
any thread. The client should
This is inconsistent with the previous comment that:

"if the Client implementation calls GetCurrentFrame()/PutCurrentFrame() from one
thread, the provider must ensure that all client methods are called from that
thread (typically the compositor thread)".

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:36: // StopRendering() returns. Clients should
handle redundant calls.
Maybe add an example to explain why these are needed, e.g. during Pause(),
nothing needs to be rendered...

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:40: // Notifies the provider's client that a
call to GetCurrentFrame() will
nit: s/provider's//?

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:43: // BeginFrameObserver based approach.
http://crbug.com/336733
What's the relationship b/w this and the return value of UpdateCurrentFrame()
below?

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:59: // the presentation interval
[|deadline_min|, |deadline_max|].
nit to double check: Are you sure it's [] instead of (] or [)?

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:63: // Returns the last frame that was acquired
during UpdateCurrentFrame(). A
nit: "acquired" is not mentioned in the documentation for UpdateCurrentFrame().

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:70: virtual void PutCurrentFrame() = 0;
nit: This naming is really confusing. It probably should be something like
OnFrameRendered(). This can be addressed later though.

https://codereview.chromium.org/1053113002/diff/100001/media/base/video_rende...
File media/base/video_renderer_sink.h (right):

https://codereview.chromium.org/1053113002/diff/100001/media/base/video_rende...
media/base/video_renderer_sink.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
2015

https://codereview.chromium.org/1053113002/diff/100001/media/base/video_rende...
media/base/video_renderer_sink.h:8: #include <vector>
not used?

https://codereview.chromium.org/1053113002/diff/100001/media/base/video_rende...
media/base/video_renderer_sink.h:25: // presentation interval [|deadline_min|,
|deadline_max|].  Returns NULL if
nit: ditto on [] instead of (] or [)

https://codereview.chromium.org/1053113002/diff/100001/media/base/video_rende...
media/base/video_renderer_sink.h:30: virtual scoped_refptr<VideoFrame>
Render(base::TimeTicks deadline_min,
Add include for scoped_refptr and TimeTicks

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

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
media/blink/video_frame_compositor.cc:115: base::Unretained(this)));
Hmm, wondering why having both lock and post... If you bind callback_ and
rendering_ here, can you just drop the lock?

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
media/blink/video_frame_compositor.cc:126: base::Unretained(this)));
ditto

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
File media/blink/video_frame_compositor.h (right):

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
media/blink/video_frame_compositor.h:30: // This class is responsible for
detected frames dropped by the compositor after
s/detected/detecting, hmm actually "signaling"?

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
media/blink/video_frame_compositor.h:32: // PutCurrentFrame() before the next
UpdateCurrentFrame() call.
By doing so, we are signaling frame-not-dropped... Needs a bit polish here.

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
media/blink/video_frame_compositor.h:43: // |compositor_task_runner| is the task
runner on which this class will live.
Clarify what this means. There are still methods called on the media thread.
Also, |this| can be constructed on another thread as well.

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
media/blink/video_frame_compositor.h:68: scoped_refptr<VideoFrame>
GetCurrentFrameAndUpdateIfStale();
Add a comment about why we need this? e.g. canvas, which can help justify the
choice of 250ms.

Also, try not to hardcode the 250ms here?

[brianderson, 2015-04-13 21:16:29]

cc lgtm.

I'm a bit hazy on the WebGL + not visible details. In particular, is
GetCurrentFrameAndUpdateIfStale going to drop video to 4fps?

I like how this paves the way for the old and new code paths to co-exist for a
little. Definitely looking forward to the followup patches!

[brianderson, 2015-04-13 21:17:50]

brianderson@chromium.org changed reviewers:
+ enne@chromium.org

[brianderson, 2015-04-13 21:17:51]

+enne, since they were interested in taking a look.

[DaleCurtis, 2015-04-13 22:10:47]

On 2015/04/13 21:16:29, brianderson wrote:
> cc lgtm.
> 
> I'm a bit hazy on the WebGL + not visible details. In particular, is
> GetCurrentFrameAndUpdateIfStale going to drop video to 4fps?

Yes, the HTML media spec guarantees a minimum of 250ms between timeupdates
events, so I'm stretching a bit to use that limitation here, but it seems
reasonable. We can tweak it based on feedback pretty easily. Considering web
applications would have to draw into the canvas using a setTimeout, setInterval,
or requestAnimationFrame which would also be on reduced timers while in the
background, I don't think it'll be a practical problem.

> 
> I like how this paves the way for the old and new code paths to co-exist for a
> little. Definitely looking forward to the followup patches!

[DaleCurtis, 2015-04-13 23:15:04]

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

[DaleCurtis, 2015-04-13 23:15:06]

PTAL. Comments addressed.

+dpranke for BUILD.gn and build/gn_migration.gypi removals.

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:30: // StopUsingProvider() may be called from
any thread. The client should
On 2015/04/13 20:50:29, xhwang wrote:
> This is inconsistent with the previous comment that:
> 
> "if the Client implementation calls GetCurrentFrame()/PutCurrentFrame() from
one
> thread, the provider must ensure that all client methods are called from that
> thread (typically the compositor thread)".

Looks like the current comment is correct, StopUsingProvider() is called from
multiple threads in the MediaStream implementation. I've updated the class level
comment to exclude StopUsingProvider().

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:36: // StopRendering() returns. Clients should
handle redundant calls.
On 2015/04/13 20:50:28, xhwang wrote:
> Maybe add an example to explain why these are needed, e.g. during Pause(),
> nothing needs to be rendered...

Done.

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:40: // Notifies the provider's client that a
call to GetCurrentFrame() will
On 2015/04/13 20:50:29, xhwang wrote:
> nit: s/provider's//?

Done.

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:43: // BeginFrameObserver based approach.
http://crbug.com/336733
On 2015/04/13 20:50:29, xhwang wrote:
> What's the relationship b/w this and the return value of UpdateCurrentFrame()
> below?

Nothing, it's only for the old path. UpdateCurrentFrame() will not be called for
the old path; instead the VideoFrameProvider will call DidReceiveFrame() and the
client will call GetCurrentFrame() (as it does today).

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:59: // the presentation interval
[|deadline_min|, |deadline_max|].
On 2015/04/13 20:50:28, xhwang wrote:
> nit to double check: Are you sure it's [] instead of (] or [)?

Double checked with Sunny, it's [].

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:63: // Returns the last frame that was acquired
during UpdateCurrentFrame(). A
On 2015/04/13 20:50:29, xhwang wrote:
> nit: "acquired" is not mentioned in the documentation for
UpdateCurrentFrame().

I've reworded the text to refer to calling UpdateCurrentFrame().

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:70: virtual void PutCurrentFrame() = 0;
On 2015/04/13 20:50:29, xhwang wrote:
> nit: This naming is really confusing. It probably should be something like
> OnFrameRendered(). This can be addressed later though.

Unfortunately it doesn't necessary mean rendered either since the compositor may
drop the buffer on the way to the gpu process. I agree it's a bit confusing
though; for this patch lets just keep the current name.

https://codereview.chromium.org/1053113002/diff/100001/media/base/video_rende...
File media/base/video_renderer_sink.h (right):

https://codereview.chromium.org/1053113002/diff/100001/media/base/video_rende...
media/base/video_renderer_sink.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
On 2015/04/13 20:50:29, xhwang wrote:
> 2015

Done.

https://codereview.chromium.org/1053113002/diff/100001/media/base/video_rende...
media/base/video_renderer_sink.h:8: #include <vector>
On 2015/04/13 20:50:29, xhwang wrote:
> not used?

Done.

https://codereview.chromium.org/1053113002/diff/100001/media/base/video_rende...
media/base/video_renderer_sink.h:25: // presentation interval [|deadline_min|,
|deadline_max|].  Returns NULL if
On 2015/04/13 20:50:29, xhwang wrote:
> nit: ditto on [] instead of (] or [)

Acknowledged.

https://codereview.chromium.org/1053113002/diff/100001/media/base/video_rende...
media/base/video_renderer_sink.h:30: virtual scoped_refptr<VideoFrame>
Render(base::TimeTicks deadline_min,
On 2015/04/13 20:50:29, xhwang wrote:
> Add include for scoped_refptr and TimeTicks

Done.

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

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
media/blink/video_frame_compositor.cc:115: base::Unretained(this)));
On 2015/04/13 20:50:29, xhwang wrote:
> Hmm, wondering why having both lock and post... If you bind callback_ and
> rendering_ here, can you just drop the lock?

Because we need to ensure that callback_ is cleared before this function
returns, or an outstanding tick of the compositor could fire the callback before
this function returns.

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
media/blink/video_frame_compositor.cc:126: base::Unretained(this)));
On 2015/04/13 20:50:29, xhwang wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
File media/blink/video_frame_compositor.h (right):

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
media/blink/video_frame_compositor.h:30: // This class is responsible for
detected frames dropped by the compositor after
On 2015/04/13 20:50:29, xhwang wrote:
> s/detected/detecting, hmm actually "signaling"?

Done.

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
media/blink/video_frame_compositor.h:32: // PutCurrentFrame() before the next
UpdateCurrentFrame() call.
On 2015/04/13 20:50:29, xhwang wrote:
> By doing so, we are signaling frame-not-dropped... Needs a bit polish here.

Done.

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
media/blink/video_frame_compositor.h:43: // |compositor_task_runner| is the task
runner on which this class will live.
On 2015/04/13 20:50:29, xhwang wrote:
> Clarify what this means. There are still methods called on the media thread.
> Also, |this| can be constructed on another thread as well.

Clarified which methods are called where.

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
media/blink/video_frame_compositor.h:68: scoped_refptr<VideoFrame>
GetCurrentFrameAndUpdateIfStale();
On 2015/04/13 20:50:29, xhwang wrote:
> Add a comment about why we need this? e.g. canvas, which can help justify the
> choice of 250ms.
> 
> Also, try not to hardcode the 250ms here?

Done.

[Dirk Pranke, 2015-04-13 23:19:07]

BUILD.gn, build/gn_migration.gypi changes lgtm.

[xhwang, 2015-04-14 00:29:52]

Thanks! LGTM

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:59: // the presentation interval
[|deadline_min|, |deadline_max|].
On 2015/04/13 23:15:06, DaleCurtis wrote:
> On 2015/04/13 20:50:28, xhwang wrote:
> > nit to double check: Are you sure it's [] instead of (] or [)?
> 
> Double checked with Sunny, it's [].

Okay, but a bit surprised... I assume cc code will make sure there's no
overlaps...

https://codereview.chromium.org/1053113002/diff/100001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:70: virtual void PutCurrentFrame() = 0;
On 2015/04/13 23:15:06, DaleCurtis wrote:
> On 2015/04/13 20:50:29, xhwang wrote:
> > nit: This naming is really confusing. It probably should be something like
> > OnFrameRendered(). This can be addressed later though.
> 
> Unfortunately it doesn't necessary mean rendered either since the compositor
may
> drop the buffer on the way to the gpu process. I agree it's a bit confusing
> though; for this patch lets just keep the current name.

Acknowledged.

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

https://codereview.chromium.org/1053113002/diff/100001/media/blink/video_fram...
media/blink/video_frame_compositor.cc:115: base::Unretained(this)));
On 2015/04/13 23:15:06, DaleCurtis wrote:
> On 2015/04/13 20:50:29, xhwang wrote:
> > Hmm, wondering why having both lock and post... If you bind callback_ and
> > rendering_ here, can you just drop the lock?
> 
> Because we need to ensure that callback_ is cleared before this function
> returns, or an outstanding tick of the compositor could fire the callback
before
> this function returns.

Thanks for the explanation! Maybe worth some comment here. It's not obvious now
since the callback_ is not used in UpdateCurrentFrame() yet.

[DaleCurtis, 2015-04-14 16:25:45]

enne: Did you want to review as well?

[danakj, 2015-04-14 16:46:16]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-04-14 16:46:18]

https://codereview.chromium.org/1053113002/diff/120001/cc/layers/video_frame_...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1053113002/diff/120001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:35: // Notifies the client that it should start
or stop making regular Render()
What is this Render() you refer to here? There is no such thing on the
interface.

[DaleCurtis, 2015-04-14 17:01:06]

https://codereview.chromium.org/1053113002/diff/120001/cc/layers/video_frame_...
File cc/layers/video_frame_provider.h (right):

https://codereview.chromium.org/1053113002/diff/120001/cc/layers/video_frame_...
cc/layers/video_frame_provider.h:35: // Notifies the client that it should start
or stop making regular Render()
On 2015/04/14 16:46:17, danakj (away until april 19) wrote:
> What is this Render() you refer to here? There is no such thing on the
> interface.

Whoops, should be UpdateCurrentFrame() since we renamed it.

[DaleCurtis, 2015-04-14 17:12:52]

(comments updated)

[DaleCurtis, 2015-04-14 22:30:41]

Spoke with enne via chat, she isn't planning to review, but it "... lgtm in a
handwavy sense." I'm going to go ahead and CQ, if anyone has some outstanding
comments, I'll address them in a follow up.

Thanks for the reviews!

[DaleCurtis, 2015-04-14 22:31:02]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-04-14 22:31:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
sunnyps@chromium.org, brianderson@chromium.org, dpranke@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1053113002/#ps140001
(title: "Fix comments.")

[commit-bot: I haz the power, 2015-04-14 22:31:12]

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

[commit-bot: I haz the power, 2015-04-14 22:36:56]

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

[commit-bot: I haz the power, 2015-04-14 22:36:58]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[DaleCurtis, 2015-04-14 22:55:36]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-04-14 22:55:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, brianderson@chromium.org, sunnyps@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1053113002/#ps160001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-04-14 22:55:49]

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

[commit-bot: I haz the power, 2015-04-14 23:15:53]

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

[commit-bot: I haz the power, 2015-04-14 23:15:55]

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

[DaleCurtis, 2015-04-14 23:53:41]

xhwang: PTAL, I forgot to update the mojo interfaces.

[xhwang, 2015-04-15 00:12:19]

still lgtm % one nit question

https://codereview.chromium.org/1053113002/diff/180001/media/mojo/services/mo...
File media/mojo/services/mojo_renderer_service.h (right):

https://codereview.chromium.org/1053113002/diff/180001/media/mojo/services/mo...
media/mojo/services/mojo_renderer_service.h:93: scoped_ptr<VideoRendererSink>
video_renderer_sink_;
ARS is ref-counted and VRS is not. Is there a plan to address this asymmetry?
Sorry for my asymmetriphobia :)

[DaleCurtis, 2015-04-15 00:13:56]

https://codereview.chromium.org/1053113002/diff/180001/media/mojo/services/mo...
File media/mojo/services/mojo_renderer_service.h (right):

https://codereview.chromium.org/1053113002/diff/180001/media/mojo/services/mo...
media/mojo/services/mojo_renderer_service.h:93: scoped_ptr<VideoRendererSink>
video_renderer_sink_;
On 2015/04/15 00:12:19, xhwang wrote:
> ARS is ref-counted and VRS is not. Is there a plan to address this asymmetry?
> Sorry for my asymmetriphobia :)

It'd be nice to make ARS non-ref-counted, but that's a major yak shave since
it's used all over. I don't have plans to do this. I just want to make sure we
start with the right model for VideoRendererSink :)

[DaleCurtis, 2015-04-15 00:14:06]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-04-15 00:14:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, brianderson@chromium.org, sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/1053113002/#ps180001
(title: "Fix mojo, wcompile errors.")

[commit-bot: I haz the power, 2015-04-15 00:14:28]

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

[xhwang, 2015-04-15 00:17:06]

https://codereview.chromium.org/1053113002/diff/180001/media/mojo/services/mo...
File media/mojo/services/mojo_renderer_service.h (right):

https://codereview.chromium.org/1053113002/diff/180001/media/mojo/services/mo...
media/mojo/services/mojo_renderer_service.h:93: scoped_ptr<VideoRendererSink>
video_renderer_sink_;
On 2015/04/15 00:13:56, DaleCurtis wrote:
> On 2015/04/15 00:12:19, xhwang wrote:
> > ARS is ref-counted and VRS is not. Is there a plan to address this
asymmetry?
> > Sorry for my asymmetriphobia :)
> 
> It'd be nice to make ARS non-ref-counted, but that's a major yak shave since
> it's used all over. I don't have plans to do this. I just want to make sure we
> start with the right model for VideoRendererSink :)

Acknowledged.

[commit-bot: I haz the power, 2015-04-15 00:57:30]

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

[commit-bot: I haz the power, 2015-04-15 00:57:32]

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

[DaleCurtis, 2015-04-15 01:43:33]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-04-15 01:43:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, brianderson@chromium.org, sunnyps@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1053113002/#ps200001
(title: "Fix unittest.")

[commit-bot: I haz the power, 2015-04-15 01:43:54]

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

[commit-bot: I haz the power, 2015-04-15 04:03:46]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2015-04-15 04:04:52]

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

[gunsch, 2015-04-15 05:53:20]

gunsch@chromium.org changed reviewers:
+ gunsch@chromium.org

[gunsch, 2015-04-15 05:53:21]

+CC some of Cast team

Hi Dale,
 I just saw this because the Chromecast build broke :( I'm hoping to get our
trybots to blocking this week, but obviously they're not yet. This seems like
potentially a pretty big change for us to address, and possibly bigger in the
near future (?).

Is there some documentation as to the design for the "new video rendering
pipeline"? Since Chromecast uses a hardware surface for all rendering, we
basically need to paint a single VideoHole frame (or the equivalent of that) and
then not paint anything else, so maybe that won't be too bad in the new path.

What are the follow-up planned changes to the Renderer interface / how Renderer
implementers should behave?

Thanks.

[gunsch, 2015-04-15 06:01:35]

A revert of this CL (patchset #9 id:200001) has been created in
https://codereview.chromium.org/1052493005/ by gunsch@chromium.org.

The reason for reverting is: Actually, given the complexity of this change (not
a one-line Cast fix) and the fact that we have broken Cast trybots running @
100% right now
(http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/),
I'm going to revert this. I'm working on a brief patch of what I expect would
need to be done for Cast and will upload/mail shortly..

[gunsch, 2015-04-15 06:16:26]

On 2015/04/15 06:01:35, gunsch wrote:
> A revert of this CL (patchset #9 id:200001) has been created in
> https://codereview.chromium.org/1052493005/ by mailto:gunsch@chromium.org.
> 
> The reason for reverting is: Actually, given the complexity of this change
(not
> a one-line Cast fix) and the fact that we have broken Cast trybots running @
> 100% right now
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/),
> I'm going to revert this. I'm working on a brief patch of what I expect would
> need to be done for Cast and will upload/mail shortly..

Follow-up: https://codereview.chromium.org/1088063002 is an initial additional
patch to this CL that should work for Cast --- it compiles and doesn't crash at
runtime (x86) but I can verify on a device tomorrow (we don't have visual output
on x86). It's a mechanical enough change (at least for now, using the old path
in VideoRendererSink) that I don't expect any problems.

[DaleCurtis, 2015-04-15 16:28:24]

On 2015/04/15 05:53:21, gunsch wrote:
> +CC some of Cast team
> 
> Hi Dale,
>  I just saw this because the Chromecast build broke :( I'm hoping to get our
> trybots to blocking this week, but obviously they're not yet. This seems like
> potentially a pretty big change for us to address, and possibly bigger in the
> near future (?).
> 
> Is there some documentation as to the design for the "new video rendering
> pipeline"? Since Chromecast uses a hardware surface for all rendering, we
> basically need to paint a single VideoHole frame (or the equivalent of that)
and
> then not paint anything else, so maybe that won't be too bad in the new path.
> 
> What are the follow-up planned changes to the Renderer interface / how
Renderer
> implementers should behave?
> 
> Thanks.

VideoRendererSink describes the process pretty thoroughly; essentially the
compositor will callback into the sink on a regular interval for VideoFrames. In
your case, you'd just call VRS::Start(), wait for the first Render callback and
then call Stop() again; the compositor will hold onto that single hole frame you
issued. You can repeat the process for a natural size change.

Do I need anything special to build the cast_shell target? If not I can
incorporate your patch (which looks good to me, I've left some comments there)
and reland shortly.

[gunsch, 2015-04-15 16:35:40]

On 2015/04/15 16:28:24, DaleCurtis wrote:
> On 2015/04/15 05:53:21, gunsch wrote:
> > +CC some of Cast team
> > 
> > Hi Dale,
> >  I just saw this because the Chromecast build broke :( I'm hoping to get our
> > trybots to blocking this week, but obviously they're not yet. This seems
like
> > potentially a pretty big change for us to address, and possibly bigger in
the
> > near future (?).
> > 
> > Is there some documentation as to the design for the "new video rendering
> > pipeline"? Since Chromecast uses a hardware surface for all rendering, we
> > basically need to paint a single VideoHole frame (or the equivalent of that)
> and
> > then not paint anything else, so maybe that won't be too bad in the new
path.
> > 
> > What are the follow-up planned changes to the Renderer interface / how
> Renderer
> > implementers should behave?
> > 
> > Thanks.
> 
> VideoRendererSink describes the process pretty thoroughly; essentially the
> compositor will callback into the sink on a regular interval for VideoFrames.
In
> your case, you'd just call VRS::Start(), wait for the first Render callback
and
> then call Stop() again; the compositor will hold onto that single hole frame
you
> issued. You can repeat the process for a natural size change.
> 
> Do I need anything special to build the cast_shell target? If not I can
> incorporate your patch (which looks good to me, I've left some comments there)
> and reland shortly.

You shouldn't need anything special to build cast_shell; it's pretty much just a
Linux build. We have some short docs at 
https://www.chromium.org/developers/how-tos/build-instructions-cast .

Thanks for the comments. I read through this CL more thoroughly; it looks like
it should be a pretty straightforward transition for us to use
VideoRendererSink.

[gunsch, 2015-04-15 16:38:49]

On 2015/04/15 16:35:40, gunsch wrote:
> On 2015/04/15 16:28:24, DaleCurtis wrote:
> > On 2015/04/15 05:53:21, gunsch wrote:
> > > +CC some of Cast team
> > > 
> > > Hi Dale,
> > >  I just saw this because the Chromecast build broke :( I'm hoping to get
our
> > > trybots to blocking this week, but obviously they're not yet. This seems
> like
> > > potentially a pretty big change for us to address, and possibly bigger in
> the
> > > near future (?).
> > > 
> > > Is there some documentation as to the design for the "new video rendering
> > > pipeline"? Since Chromecast uses a hardware surface for all rendering, we
> > > basically need to paint a single VideoHole frame (or the equivalent of
that)
> > and
> > > then not paint anything else, so maybe that won't be too bad in the new
> path.
> > > 
> > > What are the follow-up planned changes to the Renderer interface / how
> > Renderer
> > > implementers should behave?
> > > 
> > > Thanks.
> > 
> > VideoRendererSink describes the process pretty thoroughly; essentially the
> > compositor will callback into the sink on a regular interval for
VideoFrames.
> In
> > your case, you'd just call VRS::Start(), wait for the first Render callback
> and
> > then call Stop() again; the compositor will hold onto that single hole frame
> you
> > issued. You can repeat the process for a natural size change.
> > 
> > Do I need anything special to build the cast_shell target? If not I can
> > incorporate your patch (which looks good to me, I've left some comments
there)
> > and reland shortly.
> 
> You shouldn't need anything special to build cast_shell; it's pretty much just
a
> Linux build. We have some short docs at 
> https://www.chromium.org/developers/how-tos/build-instructions-cast .
> 
> Thanks for the comments. I read through this CL more thoroughly; it looks like
> it should be a pretty straightforward transition for us to use
> VideoRendererSink.

Alternately, you can just run our trybots on your change instead of building it
yourself:

git cl try -m tryserver.chromium.linux -b cast_shell_linux -b cast_shell_android

[DaleCurtis, 2015-04-15 17:41:44]

gunsch: PTAL. I've merged in your patch and sent it off to your trybots.

[gunsch, 2015-04-15 19:32:50]

On 2015/04/15 17:41:44, DaleCurtis wrote:
> gunsch: PTAL. I've merged in your patch and sent it off to your trybots.

Lgtm, thanks!

[DaleCurtis, 2015-04-15 20:03:15]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-04-15 20:03:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, brianderson@chromium.org, sunnyps@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1053113002/#ps180002
(title: "Fix cast.")

[commit-bot: I haz the power, 2015-04-15 20:04:15]

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

[commit-bot: I haz the power, 2015-04-15 21:15:06]

Committed patchset #10 (id:180002)

[commit-bot: I haz the power, 2015-04-15 21:16:13]

Patchset 10 (id:??) landed as
https://crrev.com/45a3c93f745eabf6c1b1cbdac87ed4350a919e76
Cr-Commit-Position: refs/heads/master@{#325306}