Add callback when queueing frame on Surface to create backpressure.

Add callback when queueing frame on Surface to create backpressure.

When a frame is queued on a surface, a callback can queued that will be called when that surface is used to draw a frame. This can be used to create backpressure on renderers or the browser compositor.

BUG=

Committed: https://crrev.com/878e9535344a864efec4c5137e3b1363f911f378
Cr-Commit-Position: refs/heads/master@{#291605}

[jbauman, 2014-08-20 23:36:13]

jamesr, ping

[jamesr, 2014-08-22 06:00:12]

sorry for delay, lgtm

should we be returning unused resources on this callback as well? not sure if
we'd ever want to return resources outside of this time

https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface.cc#n...
cc/surfaces/surface.cc:45: void Surface::AddDrawCallback(const base::Closure&
closure) {
nit: this is called 'callback' other places, which makes slightly more sense

https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface.cc#n...
cc/surfaces/surface.cc:53: it->Run();
if one of the callbacks manipulates our callback list or destroys us we'll be a
sad puppy. a slightly safer way to write this loop is:

typedef std::vector<base::Closure> CallbackList;

CallbackList to_run;
draw_callbacks_.swap(to_run);
for (CallbackList::iterator it = to_run.begin(); it != to_run.end(); ++it) {
  it->Run();
}

this also means that if a draw callback queues another callback the new one will
fire on the next draw, not this one, which is probably what they want if they're
queing frames as soon as they get an ACK

https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface.cc#n...
cc/surfaces/surface.cc:55: draw_callbacks_.clear();
if a surface is referenced in multiple displays the callback will fire when the
first one draws, which is *probably* what we want to happen in most cases but
might be surprising. something to keep in mind...

https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface.h#ne...
cc/surfaces/surface.h:46: std::vector<base::Closure> draw_callbacks_;
a surface can only have one active frame at any point in time (it can have more
queued, but not active), so why does it need a vector of draw callbacks?

https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface_fact...
File cc/surfaces/surface_factory.cc (right):

https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface_fact...
cc/surfaces/surface_factory.cc:44: if (!callback.is_null())
can we instead just pass this as a parameter to QueueFrame() since the callback
is logically tied to that frame? means that the null check will be inside
Surface::QueueFrame but that seems fine

https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface_fact...
File cc/surfaces/surface_factory.h (right):

https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface_fact...
cc/surfaces/surface_factory.h:41: void SubmitFrame(SurfaceId surface_id,
can you document when the callback fires?

[jamesr, 2014-08-22 06:00:35]

I guess we'd also want to return resources if we decided we were going to ditch
a pile of surfaces without drawing them

[jbauman, 2014-08-23 00:02:01]

On 2014/08/22 06:00:12, jamesr wrote:
> sorry for delay, lgtm
> 
> should we be returning unused resources on this callback as well? not sure if
> we'd ever want to return resources outside of this time

I think we generally want to return unused resources when a new surface is
queued, not when the surface is drawn.
> 
> https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface.cc
> File cc/surfaces/surface.cc (right):
> 
>
https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface.cc#n...
> cc/surfaces/surface.cc:45: void Surface::AddDrawCallback(const base::Closure&
> closure) {
> nit: this is called 'callback' other places, which makes slightly more sense

Sounds good.
> 
>
https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface.cc#n...
> cc/surfaces/surface.cc:53: it->Run();
> if one of the callbacks manipulates our callback list or destroys us we'll be
a
> sad puppy. a slightly safer way to write this loop is:
> 
> typedef std::vector<base::Closure> CallbackList;
> 
> CallbackList to_run;
> draw_callbacks_.swap(to_run);
> for (CallbackList::iterator it = to_run.begin(); it != to_run.end(); ++it) {
>   it->Run();
> }
> 
> this also means that if a draw callback queues another callback the new one
will
> fire on the next draw, not this one, which is probably what they want if
they're
> queing frames as soon as they get an ACK

Good idea.
> 
>
https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface.cc#n...
> cc/surfaces/surface.cc:55: draw_callbacks_.clear();
> if a surface is referenced in multiple displays the callback will fire when
the
> first one draws, which is *probably* what we want to happen in most cases but
> might be surprising. something to keep in mind...

Yeah, firing the callback when the first one draws seems best for rate-limiting
purposes, but I can imagine some cases where that would be a bit odd.
> 
> https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface.h
> File cc/surfaces/surface.h (right):
> 
>
https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface.h#ne...
> cc/surfaces/surface.h:46: std::vector<base::Closure> draw_callbacks_;
> a surface can only have one active frame at any point in time (it can have
more
> queued, but not active), so why does it need a vector of draw callbacks?

I was thinking of queuing up (and throwing away) multiple frames, but I guess
the draw callback shouldn't be queued up in this case.
> 
>
https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface_fact...
> File cc/surfaces/surface_factory.cc (right):
> 
>
https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface_fact...
> cc/surfaces/surface_factory.cc:44: if (!callback.is_null())
> can we instead just pass this as a parameter to QueueFrame() since the
callback
> is logically tied to that frame? means that the null check will be inside
> Surface::QueueFrame but that seems fine

Makes sense.
> 
>
https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface_fact...
> File cc/surfaces/surface_factory.h (right):
> 
>
https://codereview.chromium.org/465673003/diff/40001/cc/surfaces/surface_fact...
> cc/surfaces/surface_factory.h:41: void SubmitFrame(SurfaceId surface_id,
> can you document when the callback fires?

[chromium-reviews, 2014-08-23 00:03:22]

On Fri, Aug 22, 2014 at 8:02 PM, <jbauman@chromium.org> wrote:

> On 2014/08/22 06:00:12, jamesr wrote:
>
>> sorry for delay, lgtm
>>
>
>  should we be returning unused resources on this callback as well? not
>> sure if
>> we'd ever want to return resources outside of this time
>>
>
> I think we generally want to return unused resources when a new surface is
> queued, not when the surface is drawn.


Ya, except for resources held in an overlay.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jbauman, 2014-08-23 00:22:59]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-08-23 04:56:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbauman@chromium.org/465673003/60001

[commit-bot: I haz the power, 2014-08-23 22:10:30]

Committed patchset #4 (60001) as 6d328221f3f428755cb104eb80ffb85eb585e78c

[commit-bot: I haz the power, 2014-09-10 02:31:57]

Patchset 4 (id:??) landed as
https://crrev.com/878e9535344a864efec4c5137e3b1363f911f378
Cr-Commit-Position: refs/heads/master@{#291605}