Support swap damage rect using eglSwapBuffersWithDamageKHR

Support swap damage rect using eglSwapBuffersWithDamageKHR

Note: semantics of this extension are different than PostSubBuffer.
Damage rect expresses area of change between current back and front
buffers.

Immediate motivation is to save power during video playback with
overlays, where the GL buffer typically changes very little.

For now, a switch is required to enable support.

BUG=internal b/31264129
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/24abcc8c9a3c991401d40bda546287c90942706e
Cr-Commit-Position: refs/heads/master@{#417730}

[halliwell, 2016-09-02 16:59:04]

Description was changed from

==========
Support partial swap using eglSwapBuffersWithDamageKHR

Note: in general, eglSwapBuffersWithDamageKHR has slightly different
semantics; the damage rect should be the difference between the
previous and current frame damage.

However, on some platforms the current frame's damage rect is enough
(e.g. on Cast HDMI devices, the entire screen is composed at HDMI
refresh rate), so we can make use of this path.  Opt-in switch
prevents it being used on platforms where it does the wrong thing.

BUG=internal b/31264129
==========

to

==========
Support partial swap using eglSwapBuffersWithDamageKHR

Note: in general, eglSwapBuffersWithDamageKHR has slightly different
semantics; the damage rect should be the difference between the
previous and current frame damage.

However, on some platforms the current frame's damage rect is enough
(e.g. on Cast HDMI devices, the entire screen is composed at HDMI
refresh rate), so we can make use of this path.  Opt-in switch
prevents it being used on platforms where it does the wrong thing.

BUG=internal b/31264129
==========

[halliwell, 2016-09-02 16:59:04]

halliwell@chromium.org changed reviewers:
+ kbr@chromium.org, piman@chromium.org

[piman, 2016-09-02 18:25:47]

https://codereview.chromium.org/2302393002/diff/1/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/2302393002/diff/1/ui/gl/gl_surface_egl.cc#new...
ui/gl/gl_surface_egl.cc:885: DCHECK(supports_post_sub_buffer_ ||
supports_swap_buffer_with_damage_);
Well, the semantics of eglPostSubBufferNV and of eglSwapBuffersWithDamageKHR are
very different.

eglPostSubBufferNV only requires you to write the area that's referenced in the
rect. The rest of the backbuffer can be undefined. The driver may for example
implement it as a blit from back to front, but can't implement it as a trivial
flip.
eglSwapBuffersWithDamageKHR requires you to have the entire area correctly
defined, either because you know it already was (e.g. using buffer age, or with
EGL_SWAP_BEHAVIOR == EGL_BUFFER_PRESERVED) or by redrawing it. The driver may
implement it as a flip - but the damage is important for a compositor so that it
knows, in turn, what to re-composite.

So I don't think we should pretend to implement PostSubBuffer with
eglSwapBuffersWithDamageKHR - if we tell the compositor PostSubBuffer is
supported, it will not write the area outside of the damage region, and so we
may flip corrupted bits.

Instead we should introduce a new entrypoint that has the semantics of
SwapBuffersWithDamage - or possibly just augment SwapBuffers to have an optional
damage rect, and ensure the compositor does write the whole buffer, but still
tracks the damage rect and passes it along.

[halliwell, 2016-09-07 03:10:57]

Description was changed from

==========
Support partial swap using eglSwapBuffersWithDamageKHR

Note: in general, eglSwapBuffersWithDamageKHR has slightly different
semantics; the damage rect should be the difference between the
previous and current frame damage.

However, on some platforms the current frame's damage rect is enough
(e.g. on Cast HDMI devices, the entire screen is composed at HDMI
refresh rate), so we can make use of this path.  Opt-in switch
prevents it being used on platforms where it does the wrong thing.

BUG=internal b/31264129
==========

to

==========
Support partial swap using eglSwapBuffersWithDamageKHR

Note: in general, eglSwapBuffersWithDamageKHR has slightly different
semantics; the damage rect should be the difference between the
previous and current frame damage.

However, on some platforms the current frame's damage rect is enough
(e.g. on Cast HDMI devices, the entire screen is composed at HDMI
refresh rate), so we can make use of this path.  Opt-in switch
prevents it being used on platforms where it does the wrong thing.

BUG=internal b/31264129
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[halliwell, 2016-09-07 03:13:23]

On 2016/09/02 18:25:47, piman wrote:
> https://codereview.chromium.org/2302393002/diff/1/ui/gl/gl_surface_egl.cc
> File ui/gl/gl_surface_egl.cc (right):
> 
>
https://codereview.chromium.org/2302393002/diff/1/ui/gl/gl_surface_egl.cc#new...
> ui/gl/gl_surface_egl.cc:885: DCHECK(supports_post_sub_buffer_ ||
> supports_swap_buffer_with_damage_);
> Well, the semantics of eglPostSubBufferNV and of eglSwapBuffersWithDamageKHR
are
> very different.
> 
> eglPostSubBufferNV only requires you to write the area that's referenced in
the
> rect. The rest of the backbuffer can be undefined. The driver may for example
> implement it as a blit from back to front, but can't implement it as a trivial
> flip.
> eglSwapBuffersWithDamageKHR requires you to have the entire area correctly
> defined, either because you know it already was (e.g. using buffer age, or
with
> EGL_SWAP_BEHAVIOR == EGL_BUFFER_PRESERVED) or by redrawing it. The driver may
> implement it as a flip - but the damage is important for a compositor so that
it
> knows, in turn, what to re-composite.
> 
> So I don't think we should pretend to implement PostSubBuffer with
> eglSwapBuffersWithDamageKHR - if we tell the compositor PostSubBuffer is
> supported, it will not write the area outside of the damage region, and so we
> may flip corrupted bits.
> 
> Instead we should introduce a new entrypoint that has the semantics of
> SwapBuffersWithDamage - or possibly just augment SwapBuffers to have an
optional
> damage rect, and ensure the compositor does write the whole buffer, but still
> tracks the damage rect and passes it along.

I tried adding a new entry point SwapBuffersWithDamage.  Let me know what you
think.
Obviously further CLs would be needed to compute the rect and decide when to use
it in the compositor output surface.

[halliwell, 2016-09-07 03:17:07]

https://codereview.chromium.org/2302393002/diff/1/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/2302393002/diff/1/ui/gl/gl_surface_egl.cc#new...
ui/gl/gl_surface_egl.cc:885: DCHECK(supports_post_sub_buffer_ ||
supports_swap_buffer_with_damage_);
On 2016/09/02 18:25:47, piman wrote:
> Well, the semantics of eglPostSubBufferNV and of eglSwapBuffersWithDamageKHR
are
> very different.
> 
> eglPostSubBufferNV only requires you to write the area that's referenced in
the
> rect. The rest of the backbuffer can be undefined. The driver may for example
> implement it as a blit from back to front, but can't implement it as a trivial
> flip.
> eglSwapBuffersWithDamageKHR requires you to have the entire area correctly
> defined, either because you know it already was (e.g. using buffer age, or
with
> EGL_SWAP_BEHAVIOR == EGL_BUFFER_PRESERVED) or by redrawing it. The driver may
> implement it as a flip - but the damage is important for a compositor so that
it
> knows, in turn, what to re-composite.
> 
> So I don't think we should pretend to implement PostSubBuffer with
> eglSwapBuffersWithDamageKHR - if we tell the compositor PostSubBuffer is
> supported, it will not write the area outside of the damage region, and so we
> may flip corrupted bits.
> 
> Instead we should introduce a new entrypoint that has the semantics of
> SwapBuffersWithDamage - or possibly just augment SwapBuffers to have an
optional
> damage rect, and ensure the compositor does write the whole buffer, but still
> tracks the damage rect and passes it along.

And yes, I understood the difference between this and PostSubBufferNV ... I was
really just being lazy in reusing the plumbing, and patching it up by computing
the damage rect differently :)

[piman, 2016-09-07 17:19:41]

Thanks, this is better, because we don't encode the client behavior on the
service side, which increases coupling and makes it hard to reason about and
maintain.

A few things but otherwise good to go I think.

https://codereview.chromium.org/2302393002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_handlers.cc
(right):

https://codereview.chromium.org/2302393002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_handlers.cc:1450: const
void* cmd_data) {
nit: you probably need a rebase because this is volatile now.

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

https://codereview.chromium.org/2302393002/diff/20001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:897: // inverted, but the PostSubBuffer rectangle is
still measured from the
nit: PostSubBuffer -> damage ?

https://codereview.chromium.org/2302393002/diff/20001/ui/ozone/platform/cast/...
File ui/ozone/platform/cast/gl_surface_cast.h (right):

https://codereview.chromium.org/2302393002/diff/20001/ui/ozone/platform/cast/...
ui/ozone/platform/cast/gl_surface_cast.h:26: gfx::SwapResult PostSubBuffer(int
x, int y, int width, int height) override;
Did you mean to implement SwapBuffersWithDamage?

[halliwell, 2016-09-08 04:21:34]

Also added to Capabilities (later CL will need some equivalent of
GLRenderer::CanPartialSwap).

https://codereview.chromium.org/2302393002/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_handlers.cc
(right):

https://codereview.chromium.org/2302393002/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_passthrough_handlers.cc:1450: const
void* cmd_data) {
On 2016/09/07 17:19:41, piman wrote:
> nit: you probably need a rebase because this is volatile now.

Done.

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

https://codereview.chromium.org/2302393002/diff/20001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:897: // inverted, but the PostSubBuffer rectangle is
still measured from the
On 2016/09/07 17:19:41, piman wrote:
> nit: PostSubBuffer -> damage ?

Done.

https://codereview.chromium.org/2302393002/diff/20001/ui/ozone/platform/cast/...
File ui/ozone/platform/cast/gl_surface_cast.h (right):

https://codereview.chromium.org/2302393002/diff/20001/ui/ozone/platform/cast/...
ui/ozone/platform/cast/gl_surface_cast.h:26: gfx::SwapResult PostSubBuffer(int
x, int y, int width, int height) override;
On 2016/09/07 17:19:41, piman wrote:
> Did you mean to implement SwapBuffersWithDamage?

Yes, forgot to update this.

[halliwell, 2016-09-08 04:21:45]

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

[commit-bot: I haz the power, 2016-09-08 04:22:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-09-08 04:27:11]

lgtm

https://codereview.chromium.org/2302393002/diff/80001/gpu/command_buffer/comm...
File gpu/command_buffer/common/capabilities.h (right):

https://codereview.chromium.org/2302393002/diff/80001/gpu/command_buffer/comm...
gpu/command_buffer/common/capabilities.h:161: bool swap_buffers_with_damage;
nit: maybe move this around l.118 together with post_sub_buffer?

[commit-bot: I haz the power, 2016-09-08 05:43:54]

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

[commit-bot: I haz the power, 2016-09-08 05:43:55]

Dry run: This issue passed the CQ dry run.

[halliwell, 2016-09-08 13:18:31]

Description was changed from

==========
Support partial swap using eglSwapBuffersWithDamageKHR

Note: in general, eglSwapBuffersWithDamageKHR has slightly different
semantics; the damage rect should be the difference between the
previous and current frame damage.

However, on some platforms the current frame's damage rect is enough
(e.g. on Cast HDMI devices, the entire screen is composed at HDMI
refresh rate), so we can make use of this path.  Opt-in switch
prevents it being used on platforms where it does the wrong thing.

BUG=internal b/31264129
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Support swap damage rect using eglSwapBuffersWithDamageKHR

Note: semantics of this extension are different than PostSubBuffer.
Damage rect expresses area of change between current back and front
buffers.

Immediate motivation is to save power during video playback with
overlays, where the GL buffer typically changes very little.

For now, a switch is required to enable support.

BUG=internal b/31264129
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[halliwell, 2016-09-08 13:18:31]

halliwell@chromium.org changed reviewers:
+ wfh@chromium.org

[halliwell, 2016-09-08 13:19:00]

+wfh for gpu/ipc change

https://codereview.chromium.org/2302393002/diff/80001/gpu/command_buffer/comm...
File gpu/command_buffer/common/capabilities.h (right):

https://codereview.chromium.org/2302393002/diff/80001/gpu/command_buffer/comm...
gpu/command_buffer/common/capabilities.h:161: bool swap_buffers_with_damage;
On 2016/09/08 04:27:11, piman wrote:
> nit: maybe move this around l.118 together with post_sub_buffer?

Done.

[halliwell, 2016-09-09 03:46:20]

halliwell@chromium.org changed reviewers:
+ meacer@chromium.org

[halliwell, 2016-09-09 03:47:41]

On 2016/09/08 13:19:00, halliwell wrote:
> +wfh for gpu/ipc change
> 
>
https://codereview.chromium.org/2302393002/diff/80001/gpu/command_buffer/comm...
> File gpu/command_buffer/common/capabilities.h (right):
> 
>
https://codereview.chromium.org/2302393002/diff/80001/gpu/command_buffer/comm...
> gpu/command_buffer/common/capabilities.h:161: bool swap_buffers_with_damage;
> On 2016/09/08 04:27:11, piman wrote:
> > nit: maybe move this around l.118 together with post_sub_buffer?
> 
> Done.

Friendly nudge for wfh/meacer to look at gpu/ipc change, thanks :)

[meacer, 2016-09-09 19:16:44]

On 2016/09/09 03:47:41, halliwell wrote:
> On 2016/09/08 13:19:00, halliwell wrote:
> > +wfh for gpu/ipc change
> > 
> >
>
https://codereview.chromium.org/2302393002/diff/80001/gpu/command_buffer/comm...
> > File gpu/command_buffer/common/capabilities.h (right):
> > 
> >
>
https://codereview.chromium.org/2302393002/diff/80001/gpu/command_buffer/comm...
> > gpu/command_buffer/common/capabilities.h:161: bool swap_buffers_with_damage;
> > On 2016/09/08 04:27:11, piman wrote:
> > > nit: maybe move this around l.118 together with post_sub_buffer?
> > 
> > Done.
> 
> Friendly nudge for wfh/meacer to look at gpu/ipc change, thanks :)

Lgtm

[halliwell, 2016-09-09 20:46:29]

The CQ bit was checked by halliwell@chromium.org

[halliwell, 2016-09-09 20:46:29]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2302393002/#ps120001
(title: "Update GLSurfaceAdapter")

[commit-bot: I haz the power, 2016-09-09 20:47:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 22:09:22]

Description was changed from

==========
Support swap damage rect using eglSwapBuffersWithDamageKHR

Note: semantics of this extension are different than PostSubBuffer.
Damage rect expresses area of change between current back and front
buffers.

Immediate motivation is to save power during video playback with
overlays, where the GL buffer typically changes very little.

For now, a switch is required to enable support.

BUG=internal b/31264129
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Support swap damage rect using eglSwapBuffersWithDamageKHR

Note: semantics of this extension are different than PostSubBuffer.
Damage rect expresses area of change between current back and front
buffers.

Immediate motivation is to save power during video playback with
overlays, where the GL buffer typically changes very little.

For now, a switch is required to enable support.

BUG=internal b/31264129
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-09-09 22:09:24]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-09 22:12:48]

Description was changed from

==========
Support swap damage rect using eglSwapBuffersWithDamageKHR

Note: semantics of this extension are different than PostSubBuffer.
Damage rect expresses area of change between current back and front
buffers.

Immediate motivation is to save power during video playback with
overlays, where the GL buffer typically changes very little.

For now, a switch is required to enable support.

BUG=internal b/31264129
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Support swap damage rect using eglSwapBuffersWithDamageKHR

Note: semantics of this extension are different than PostSubBuffer.
Damage rect expresses area of change between current back and front
buffers.

Immediate motivation is to save power during video playback with
overlays, where the GL buffer typically changes very little.

For now, a switch is required to enable support.

BUG=internal b/31264129
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/24abcc8c9a3c991401d40bda546287c90942706e
Cr-Commit-Position: refs/heads/master@{#417730}
==========

[commit-bot: I haz the power, 2016-09-09 22:12:50]

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