Enqueuing new frames in a Surface should cause Displays to reaggregate it

Enqueuing new frames in a Surface should cause Displays to reaggregate it

Keep track of which Surfaces were included in a Display last
frame, so if a new frame is enqueued in a Surface the Displays containing it will tell their clients to redraw. This way the browser compositor
doesn't have to commit if only the renderer contents changed.

BUG=

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

[jamesr, 2014-08-07 21:04:06]

https://codereview.chromium.org/432093003/diff/40001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/432093003/diff/40001/cc/surfaces/display.cc#n...
cc/surfaces/display.cc:91: base::MessageLoopProxy::current()->PostTask(
the Display should not self-schedule. tell the client you have damage and let it
figure out if and when to draw

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

https://codereview.chromium.org/432093003/diff/40001/cc/surfaces/display.h#ne...
cc/surfaces/display.h:96: std::set<Surface*> contained_surfaces_;
i think this should be a set of IDs, not raw surface pointers

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

https://codereview.chromium.org/432093003/diff/40001/cc/surfaces/surface.cc#n...
cc/surfaces/surface.cc:18: for (std::set<Display*>::iterator it =
containing_displays_.begin();
if you used IDs then you wouldn't need to do this - the SurfaceManager's
id->surface mapping is your weak pointer

https://codereview.chromium.org/432093003/diff/40001/cc/surfaces/surface.cc#n...
cc/surfaces/surface.cc:48: (*it)->Draw();
i think the service should be keeping track of this, not the individual
surfaces. i.e. the thing that's routing queue frame calls to Surface instances
can say 'ok, for this surface ID is there any display that needs to re-aggregate
as a result of this'?

also saying that a new frame needs a draw isn't entirely correct.  When
receiving a new frame, we need to re-aggregate and then look at the damage to
see if that requires us to draw or not. in many cases we won't actually have to
draw anything new

[piman, 2014-08-07 21:09:33]

https://codereview.chromium.org/432093003/diff/40001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/432093003/diff/40001/cc/surfaces/display.cc#n...
cc/surfaces/display.cc:91: base::MessageLoopProxy::current()->PostTask(
On 2014/08/07 21:04:06, jamesr wrote:
> the Display should not self-schedule. tell the client you have damage and let
it
> figure out if and when to draw

drive-by: also going to MessageLoopProxy::current() would interact poorly with
ccameron's fixes for crbug.com/388005 and enne's STP Scheduler fix

[jbauman, 2014-08-11 15:16:40]

PTAL. Modified to keep track of the SurfaceIds instead, and tell the
DisplayClient when damage has happened. Also the OnscreenDisplayClient now does
the draw on a specific TaskRunner.

[jamesr, 2014-08-11 16:58:03]

Could you explain the 'destroy after commit' logic?  Why would we need to care
about when the commit happens here?

https://codereview.chromium.org/432093003/diff/100001/cc/surfaces/surface_man...
File cc/surfaces/surface_manager.h (right):

https://codereview.chromium.org/432093003/diff/100001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.h:30: class CC_SURFACES_EXPORT SurfaceDamageObserver
{
no export needed, this doesn't have any symbols to export

inner classes are pretty annoying (can't forward declare them) and the double
'Surfaces' in the name SurfaceManager::SurfaceDamageObserver is kind of
annoying. can you make this a standalone interface in its own header?

[jbauman, 2014-08-11 17:20:51]

On 2014/08/11 16:58:03, jamesr wrote:
> Could you explain the 'destroy after commit' logic?  Why would we need to care
> about when the commit happens here?

Until the browser compositor commits and swaps, the last surface it swapped may
still be referencing this surface, so if the surface is destroyed before then
then it'll draw incorrectly. We currently don't have another good way to
determine when the last reference will go away.
> 
>
https://codereview.chromium.org/432093003/diff/100001/cc/surfaces/surface_man...
> File cc/surfaces/surface_manager.h (right):
> 
>
https://codereview.chromium.org/432093003/diff/100001/cc/surfaces/surface_man...
> cc/surfaces/surface_manager.h:30: class CC_SURFACES_EXPORT
SurfaceDamageObserver
> {
> no export needed, this doesn't have any symbols to export
> 
> inner classes are pretty annoying (can't forward declare them) and the double
> 'Surfaces' in the name SurfaceManager::SurfaceDamageObserver is kind of
> annoying. can you make this a standalone interface in its own header?

Ok, fixed.

[jamesr, 2014-08-11 17:25:18]

On 2014/08/11 17:20:51, jbauman wrote:
> On 2014/08/11 16:58:03, jamesr wrote:
> > Could you explain the 'destroy after commit' logic?  Why would we need to
care
> > about when the commit happens here?
> 
> Until the browser compositor commits and swaps, the last surface it swapped
may
> still be referencing this surface, so if the surface is destroyed before then
> then it'll draw incorrectly.

Sorry, why would it draw incorrectly?  What is it holding that would result in a
bad draw?

[jbauman, 2014-08-11 17:29:29]

On 2014/08/11 17:25:18, jamesr wrote:
> On 2014/08/11 17:20:51, jbauman wrote:
> > On 2014/08/11 16:58:03, jamesr wrote:
> > > Could you explain the 'destroy after commit' logic?  Why would we need to
> care
> > > about when the commit happens here?
> > 
> > Until the browser compositor commits and swaps, the last surface it swapped
> may
> > still be referencing this surface, so if the surface is destroyed before
then
> > then it'll draw incorrectly.
> 
> Sorry, why would it draw incorrectly?  What is it holding that would result in
a
> bad draw?

The Display's current Surface contains a reference to the RWHVA's old Surface.
If that Surface is destroyed and the Display tries to draw before the browser
compositor commits and swaps a frame that references the new surface, then it
will draw black instead.

[jamesr, 2014-08-11 20:50:44]

Hm, so this is just about resizing?  I'd rather handle that separately -
resizing is complicated and will probably need some new primitives to work well.
 It doesn't seem to have much to do with the rest of this patch, either.

[jbauman, 2014-08-11 21:47:13]

On 2014/08/11 20:50:44, jamesr wrote:
> Hm, so this is just about resizing?  I'd rather handle that separately -
> resizing is complicated and will probably need some new primitives to work
well.
>  It doesn't seem to have much to do with the rest of this patch, either.

PTAL. Removed that code. It flickers black when resizing, but whatever.

[jamesr, 2014-08-11 22:04:43]

lgtm

https://codereview.chromium.org/432093003/diff/140001/cc/surfaces/surface_man...
File cc/surfaces/surface_manager.h (right):

https://codereview.chromium.org/432093003/diff/140001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.h:8: #include <set>
not needed

https://codereview.chromium.org/432093003/diff/140001/content/browser/composi...
File content/browser/compositor/onscreen_display_client.cc (right):

https://codereview.chromium.org/432093003/diff/140001/content/browser/composi...
content/browser/compositor/onscreen_display_client.cc:42:
task_runner_->PostTask(
could you add some traces here and to ::Draw()? we'll want them

[jbauman, 2014-08-14 18:24:22]

piman, could I get an OWNERS review for the ui/compositor change?

[piman, 2014-08-15 03:01:36]

LGTM for now. I'm not super excited about exposing the compositor's task runner
because that can easily lead to bad patterns, but there's no simple way to avoid
that today - it sort of comes from how the DisplayClient hooks up to the browser
compositor's output surface, which is weird.

[chromium-reviews, 2014-08-15 15:50:02]

On Thu, Aug 14, 2014 at 11:01 PM, <piman@chromium.org> wrote:

> LGTM for now. I'm not super excited about exposing the compositor's task
> runner
> because that can easily lead to bad patterns, but there's no simple way to
> avoid
> that today - it sort of comes from how the DisplayClient hooks up to the
> browser
> compositor's output surface, which is weird.
>

Maybe a scary comment and/or accessor method name, and a TODO to a bug
saying how this is going away and how we'll do it differently?


>
> https://codereview.chromium.org/432093003/
>

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

[jbauman, 2014-08-15 21:40:28]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-08-15 21:43:05]

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

[commit-bot: I haz the power, 2014-08-16 06:07:49]

Committed patchset #10 (180001) as 290103