Partial swap implementation for surfaceless

Partial swap implementation for surfaceless

BUG=404089
Committed: https://crrev.com/043a6152153c4af0d50540add7f2ab9a1f3b4f9e
Cr-Commit-Position: refs/heads/master@{#296198}

[achaulk, 2014-09-17 19:11:07]

achaulk@chromium.org changed reviewers:
+ alexst@chromium.org

[alexst (slow to review), 2014-09-17 20:34:06]

still going through the logic, but some initial notes here.

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
File content/browser/compositor/buffer_queue.h (right):

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.h:64: // |exclude_rect| from
|source_texture| to |texture|
nit: period at the end.

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.h:67: const gfx::Rect& exclude_rect,
can you keep the names consistent with GLHelper as new_damage, old_damage?

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
content/browser/compositor/gpu_process_transport_factory.cc:243: GetGLHelper(),
I don't think this is safe to do. If a context is lost, the helper will get
reset, so you'll be storing a deleted object.

GpuProcessTransportFactory implements ImageTransportFactory, which has a
GetInstance and a getter for GLHelper. We should get the helper directly from
there before each use.

https://codereview.chromium.org/571623003/diff/20001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/571623003/diff/20001/ui/gl/gl_surface_ozone.c...
ui/gl/gl_surface_ozone.cc:141: virtual bool SupportsPostSubBuffer() OVERRIDE {
return true; }
This is shared between surfaceless and surfaced, so I don't think we can just
return true here. It's reasonable to enable it in surfaceless by default, but we
need to check that we are surfaceless here.

https://codereview.chromium.org/571623003/diff/20001/ui/gl/gl_surface_ozone.c...
ui/gl/gl_surface_ozone.cc:145: // actually get the sub-buffer swap.
If we are not expected to be here, we should add NOTREACHED()

[achaulk, 2014-09-17 20:41:57]

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
File content/browser/compositor/buffer_queue.h (right):

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.h:67: const gfx::Rect& exclude_rect,
On 2014/09/17 20:34:05, alexst wrote:
> can you keep the names consistent with GLHelper as new_damage, old_damage? 

Done.

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
content/browser/compositor/gpu_process_transport_factory.cc:243: GetGLHelper(),
On 2014/09/17 20:34:06, alexst wrote:
> I don't think this is safe to do. If a context is lost, the helper will get
> reset, so you'll be storing a deleted object.
> 
> GpuProcessTransportFactory implements ImageTransportFactory, which has a
> GetInstance and a getter for GLHelper. We should get the helper directly from
> there before each use.

Done.

https://codereview.chromium.org/571623003/diff/20001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/571623003/diff/20001/ui/gl/gl_surface_ozone.c...
ui/gl/gl_surface_ozone.cc:141: virtual bool SupportsPostSubBuffer() OVERRIDE {
return true; }
On 2014/09/17 20:34:06, alexst wrote:
> This is shared between surfaceless and surfaced, so I don't think we can just
> return true here. It's reasonable to enable it in surfaceless by default, but
we
> need to check that we are surfaceless here.

No this is the surfaceless class

https://codereview.chromium.org/571623003/diff/20001/ui/gl/gl_surface_ozone.c...
ui/gl/gl_surface_ozone.cc:145: // actually get the sub-buffer swap.
On 2014/09/17 20:34:06, alexst wrote:
> If we are not expected to be here, we should add NOTREACHED()

Done.

[piman, 2014-09-17 20:47:17]

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

[piman, 2014-09-17 20:47:18]

https://codereview.chromium.org/571623003/diff/40001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/571623003/diff/40001/ui/gl/gl_surface_ozone.c...
ui/gl/gl_surface_ozone.cc:146: return false;
drive-by: This is weird. Why not simply have PostSubBuffer call SwapBuffers and
not having to worry about this at a higher level?

Truth is, what we would really want is neither SwapBuffer nor PostSubBuffer but
a new "CommitPlanes" or something. But I don't think we really need this extra
complexity at this point.

[achaulk, 2014-09-17 21:01:08]

https://codereview.chromium.org/571623003/diff/40001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/571623003/diff/40001/ui/gl/gl_surface_ozone.c...
ui/gl/gl_surface_ozone.cc:146: return false;
On 2014/09/17 20:47:18, piman (Very slow to review) wrote:
> drive-by: This is weird. Why not simply have PostSubBuffer call SwapBuffers
and
> not having to worry about this at a higher level?
> 
> Truth is, what we would really want is neither SwapBuffer nor PostSubBuffer
but
> a new "CommitPlanes" or something. But I don't think we really need this extra
> complexity at this point.

That's also another possibility

[alexst (slow to review), 2014-09-18 15:39:16]

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
File content/browser/compositor/buffer_queue.cc (right):

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.cc:76: if
(!damage.Contains(current_surface_.damage)) {
Contract on CopyBufferDamage says "Copies all pixels from |previous_texture|
into |texture| that are inside the region covered by |old_damage| but not part
of |new_damage|."

If I'm reading it right, if old_damage is part of new_damage, it'll be a no-op
and we can skip the check here.

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.cc:83: in_flight_surfaces_.back().damage
= damage;
I think this is redundant with the for loop below since we set damage here then
then again as damage.Union(damage).

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.cc:87: for (size_t i = 0; i <
available_surfaces_.size(); i++)
Can you break this into a separate function SetDamageOnBuffers(const gfx::Rect&
damage) and call it here with the union and below with just damage, these two
for loops are almost identical copies here and below.

https://codereview.chromium.org/571623003/diff/40001/content/browser/composit...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/571623003/diff/40001/content/browser/composit...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:44:
frame->gl_frame_data->sub_buffer_rect = gfx::Rect(frame->gl_frame_data->size);
We have a reflector, which is used for multimon mirroring that copies these as
well. It needs the damage rect too.

I think we can remove this here and implement virtual PostSubBuffer(..) in the
GPU process surface to ignore the rect.

[alexst (slow to review), 2014-09-18 16:08:30]

https://codereview.chromium.org/571623003/diff/40001/ui/gl/gl_surface_ozone.cc
File ui/gl/gl_surface_ozone.cc (right):

https://codereview.chromium.org/571623003/diff/40001/ui/gl/gl_surface_ozone.c...
ui/gl/gl_surface_ozone.cc:146: return false;
On 2014/09/17 21:01:08, achaulk wrote:
> On 2014/09/17 20:47:18, piman (Very slow to review) wrote:
> > drive-by: This is weird. Why not simply have PostSubBuffer call SwapBuffers
> and
> > not having to worry about this at a higher level?
> > 
> > Truth is, what we would really want is neither SwapBuffer nor PostSubBuffer
> but
> > a new "CommitPlanes" or something. But I don't think we really need this
extra
> > complexity at this point.
> 
> That's also another 

Made a wrong comment about ignoring what comes here, we should call SwapBuffers
instead once we propagate damage to the reflector.

[achaulk, 2014-09-18 16:45:03]

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
File content/browser/compositor/buffer_queue.cc (right):

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.cc:76: if
(!damage.Contains(current_surface_.damage)) {
On 2014/09/18 15:39:15, alexst wrote:
> Contract on CopyBufferDamage says "Copies all pixels from |previous_texture|
> into |texture| that are inside the region covered by |old_damage| but not part
> of |new_damage|."
> 
> If I'm reading it right, if old_damage is part of new_damage, it'll be a no-op
> and we can skip the check here.

Sure, but we need it anyway for the below set

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.cc:83: in_flight_surfaces_.back().damage
= damage;
On 2014/09/18 15:39:15, alexst wrote:
> I think this is redundant with the for loop below since we set damage here
then
> then again as damage.Union(damage).

Not entirely, the important bit is that it any existing damage. Though because
of the set, there's no difference between setting it to damage or to zero

https://codereview.chromium.org/571623003/diff/20001/content/browser/composit...
content/browser/compositor/buffer_queue.cc:87: for (size_t i = 0; i <
available_surfaces_.size(); i++)
On 2014/09/18 15:39:15, alexst wrote:
> Can you break this into a separate function SetDamageOnBuffers(const
gfx::Rect&
> damage) and call it here with the union and below with just damage, these two
> for loops are almost identical copies here and below.

Sure

https://codereview.chromium.org/571623003/diff/40001/content/browser/composit...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/571623003/diff/40001/content/browser/composit...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc:44:
frame->gl_frame_data->sub_buffer_rect = gfx::Rect(frame->gl_frame_data->size);
On 2014/09/18 15:39:15, alexst wrote:
> We have a reflector, which is used for multimon mirroring that copies these as
> well. It needs the damage rect too.
> 
> I think we can remove this here and implement virtual PostSubBuffer(..) in the
> GPU process surface to ignore the rect.

Done.

[alexst (slow to review), 2014-09-18 16:57:16]

lgtm with a comment nit.

https://codereview.chromium.org/571623003/diff/80001/content/browser/composit...
File content/browser/compositor/buffer_queue.cc (right):

https://codereview.chromium.org/571623003/diff/80001/content/browser/composit...
content/browser/compositor/buffer_queue.cc:90: // Because we copied everything
except |damage| to this texture,
I think it should be "from this texture"

[achaulk, 2014-09-18 17:27:51]

piman for owners

https://codereview.chromium.org/571623003/diff/80001/content/browser/composit...
File content/browser/compositor/buffer_queue.cc (right):

https://codereview.chromium.org/571623003/diff/80001/content/browser/composit...
content/browser/compositor/buffer_queue.cc:90: // Because we copied everything
except |damage| to this texture,
On 2014/09/18 16:57:16, alexst wrote:
> I think it should be "from this texture"

Sure. That's more consistent

[piman, 2014-09-18 17:40:02]

lgtm

[achaulk, 2014-09-23 15:48:00]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-09-23 15:48:17]

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

[commit-bot: I haz the power, 2014-09-23 16:31:51]

Committed patchset #7 (id:120001) as abdca0f977e8bb0bb8278bf68aff17027446eace

[commit-bot: I haz the power, 2014-09-23 16:32:23]

Patchset 7 (id:??) landed as
https://crrev.com/043a6152153c4af0d50540add7f2ab9a1f3b4f9e
Cr-Commit-Position: refs/heads/master@{#296198}