Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

For updates to the DirectComposition layer tree to be in sync with
updates to the texture contents we need to put Chrome's backbuffer into
an IDCompositionSurface instead of an IDXGISwapChain2. That way updates
to the surface only happen upon commit of the visual tree.

Unfortunately this adds some complication, in that Chrome must signal
what region will be rendered to before actually rendering to it. All
draws must have a certain offset applied to them within the output
texture, though that's not yet implemented in this patch.

This patch just allocates an IDCompositionSurface and renders to it. A
new GL command (SetDrawRectangleCHROMIUM) must be given before drawing
to the surface.

BUG=678800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;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

Review-Url: https://codereview.chromium.org/2646243002
Cr-Commit-Position: refs/heads/master@{#454119}
Committed: https://chromium.googlesource.com/chromium/src/+/2a2ad220b1c964f4fc348a8118ae9b91c43aa50e

[jbauman, 2017-01-21 03:11:21]

Description was changed from

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

This allocates an IDCompositionSurface and renders to it. A new GL
command (SetDrawRectangleCHROMIUM) must be given before drawing to the
surface. The update offset isn't implemented yet, so rendered contents
may be shifted incorrectly.

BUG=678800
==========

to

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

This allocates an IDCompositionSurface and renders to it. A new GL
command (SetDrawRectangleCHROMIUM) must be given before drawing to the
surface. The update offset isn't implemented yet, so rendered contents
may be shifted incorrectly.

BUG=678800
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
==========

[jbauman, 2017-01-21 03:11:21]

Description was changed from

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

This allocates an IDCompositionSurface and renders to it. A new GL
command (SetDrawRectangleCHROMIUM) must be given before drawing to the
surface. The update offset isn't implemented yet, so rendered contents
may be shifted incorrectly.

BUG=678800
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

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

This allocates an IDCompositionSurface and renders to it. A new GL
command (SetDrawRectangleCHROMIUM) must be given before drawing to the
surface. The update offset isn't implemented yet, so rendered contents
may be shifted incorrectly.

BUG=678800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[jbauman, 2017-01-21 03:11:28]

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

[commit-bot: I haz the power, 2017-01-21 03:11:48]

Dry run: 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, 2017-01-21 04:22:27]

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

[commit-bot: I haz the power, 2017-01-21 04:22:27]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[jbauman, 2017-01-24 01:19:09]

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

[commit-bot: I haz the power, 2017-01-24 01:19:54]

Dry run: 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, 2017-01-24 03:05:59]

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

[commit-bot: I haz the power, 2017-01-24 03:06:02]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[jbauman, 2017-01-24 04:22:18]

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

[commit-bot: I haz the power, 2017-01-24 04:22:39]

Dry run: 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, 2017-01-24 05:49:40]

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

[commit-bot: I haz the power, 2017-01-24 05:49:41]

Dry run: This issue passed the CQ dry run.

[jbauman, 2017-01-24 23:38:20]

Description was changed from

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

This allocates an IDCompositionSurface and renders to it. A new GL
command (SetDrawRectangleCHROMIUM) must be given before drawing to the
surface. The update offset isn't implemented yet, so rendered contents
may be shifted incorrectly.

BUG=678800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

This allocates an IDCompositionSurface and renders to it. A new GL
command (SetDrawRectangleCHROMIUM) must be given before drawing to the
surface. The update offset isn't implemented yet, so rendered contents
may be shifted incorrectly.

BUG=678800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[jbauman, 2017-01-24 23:38:40]

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

[commit-bot: I haz the power, 2017-01-24 23:39:41]

Dry run: 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, 2017-01-25 00:24:43]

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

[commit-bot: I haz the power, 2017-01-25 00:24:44]

Dry run: This issue passed the CQ dry run.

[jbauman, 2017-01-25 00:41:29]

Description was changed from

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

This allocates an IDCompositionSurface and renders to it. A new GL
command (SetDrawRectangleCHROMIUM) must be given before drawing to the
surface. The update offset isn't implemented yet, so rendered contents
may be shifted incorrectly.

BUG=678800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

For updates to the DirectComposition layer tree to be in sync with updates to
the texture contents we need to put Chrome's backbuffer into an
IDCompositionSurface instead of an IDXGISwapChain2. That way updates to the
surface only happen upon commit.

Unfortunately this adds some complication, in that Chrome must signal what error
will be rendered to before actually rendering to it. All draws must have a
certain offset applied to them within the output texture, though that's not yet
implemented in this patch.

This patch just allocates an IDCompositionSurface and renders to it. A new GL
command (SetDrawRectangleCHROMIUM) must be given before drawing to the
surface.

BUG=678800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[jbauman, 2017-01-25 00:41:43]

jbauman@chromium.org changed reviewers:
+ sunnyps@chromium.org

[jbauman, 2017-01-25 00:41:44]

Sorry, this is a pretty big patch, but thanks for looking at least at the
DirectComposition parts of it.

[sunnyps, 2017-01-28 01:33:59]

Most comments are nits except those about more error checking on the client e.g.
one call to set draw rect per swap, rect should be a subrect of surface and rect
should equal surface size after resize.

https://codereview.chromium.org/2646243002/diff/40001/cc/output/direct_render...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2646243002/diff/40001/cc/output/direct_render...
cc/output/direct_renderer.cc:581: frame->current_texture = NULL;
nit: nullptr

https://codereview.chromium.org/2646243002/diff/40001/cc/output/direct_render...
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/2646243002/diff/40001/cc/output/direct_render...
cc/output/direct_renderer.h:188: bool allow_empty_swap_;
nit: can you initialize these too?

https://codereview.chromium.org/2646243002/diff/40001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2646243002/diff/40001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:87: bool
has_alpha) {
can we add a flag to check if set draw rect after reshape is for the full
surface size?

https://codereview.chromium.org/2646243002/diff/40001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:140: if
(set_draw_rectangle_for_frame_)
can we dcheck/check that rect is a subrect of surface size?

https://codereview.chromium.org/2646243002/diff/40001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:140: if
(set_draw_rectangle_for_frame_)
should we dcheck/check(!set_draw_rectangle_for_frame)?

https://codereview.chromium.org/2646243002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2646243002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3549: !offscreen &&
surface->SupportsSetDrawRectangle();
nit: Won't surface->SupportsSetDrawRectangle only return true if it's offscreen?

https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
File gpu/ipc/service/direct_composition_surface_win.cc (right):

https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
gpu/ipc/service/direct_composition_surface_win.cc:37: current_context &&
current_context->IsCurrent(this_surface);
nit: DCHECK(current_context->IsCurrent(this_surface)) instead?

https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
gpu/ipc/service/direct_composition_surface_win.cc:312: bool
DirectCompositionSurfaceWin::CommitAndClearPendingOverlays() {
nit: Can you move this next to ScheduleOverlayPlane?

https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
gpu/ipc/service/direct_composition_surface_win.cc:317: void
DirectCompositionSurfaceWin::InitializeSurface() {
nit: can you move this and ReleaseDrawTexture next to the other initialization
methods?

https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
gpu/ipc/service/direct_composition_surface_win.cc:343:
DirectCompositionSurfaceWin::~DirectCompositionSurfaceWin() {
nit: can you move this next to the ctor?

https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
File gpu/ipc/service/direct_composition_surface_win.h (right):

https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
gpu/ipc/service/direct_composition_surface_win.h:8: #include <d3d11.h>
nit: group this with <windows.h> or vice-versa.

https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
gpu/ipc/service/direct_composition_surface_win.h:87: EGLSurface
default_surface_;
nit: default_surface_ = 0

https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
gpu/ipc/service/direct_composition_surface_win.h:91: EGLSurface real_surface_;
nit: real_surface_ = 0

https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
gpu/ipc/service/direct_composition_surface_win.h:93: bool first_swap_;
nit: first_swap_ = false

https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
File gpu/ipc/service/direct_composition_surface_win_unittest.cc (right):

https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
gpu/ipc/service/direct_composition_surface_win_unittest.cc:34: using
DirectCompositionSurfaceTest = testing::Test;
nit: you can use TEST instead of TEST_F and not have to do this.

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

https://codereview.chromium.org/2646243002/diff/40001/ui/gl/gl_angle_util_win...
ui/gl/gl_angle_util_win.cc:105: 
nit: can you comment under what conditions the following code is needed?

[jbauman, 2017-02-06 21:32:29]

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

[commit-bot: I haz the power, 2017-02-06 21:33:36]

Dry run: 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, 2017-02-06 21:37:11]

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

[commit-bot: I haz the power, 2017-02-06 21:37:12]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[jbauman, 2017-02-07 00:17:46]

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

[commit-bot: I haz the power, 2017-02-07 00:18:47]

Dry run: 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, 2017-02-07 01:36:13]

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

[commit-bot: I haz the power, 2017-02-07 01:36:14]

Dry run: This issue passed the CQ dry run.

[jbauman, 2017-02-07 22:31:40]

PTAL

On 2017/01/28 01:33:59, sunnyps wrote:
> Most comments are nits except those about more error checking on the client
e.g.
> one call to set draw rect per swap, rect should be a subrect of surface and
rect
> should equal surface size after resize.
> 
>
https://codereview.chromium.org/2646243002/diff/40001/cc/output/direct_render...
> File cc/output/direct_renderer.cc (right):
> 
>
https://codereview.chromium.org/2646243002/diff/40001/cc/output/direct_render...
> cc/output/direct_renderer.cc:581: frame->current_texture = NULL;
> nit: nullptr

Done
> 
>
https://codereview.chromium.org/2646243002/diff/40001/cc/output/direct_render...
> File cc/output/direct_renderer.h (right):
> 
>
https://codereview.chromium.org/2646243002/diff/40001/cc/output/direct_render...
> cc/output/direct_renderer.h:188: bool allow_empty_swap_;
> nit: can you initialize these too?
> 
Done

>
https://codereview.chromium.org/2646243002/diff/40001/content/browser/composi...
> File content/browser/compositor/gpu_browser_compositor_output_surface.cc
> (right):
> 
>
https://codereview.chromium.org/2646243002/diff/40001/content/browser/composi...
> content/browser/compositor/gpu_browser_compositor_output_surface.cc:87: bool
> has_alpha) {
> can we add a flag to check if set draw rect after reshape is for the full
> surface size?

Done
> 
>
https://codereview.chromium.org/2646243002/diff/40001/content/browser/composi...
> content/browser/compositor/gpu_browser_compositor_output_surface.cc:140: if
> (set_draw_rectangle_for_frame_)
> can we dcheck/check that rect is a subrect of surface size?

Done.
> 
>
https://codereview.chromium.org/2646243002/diff/40001/content/browser/composi...
> content/browser/compositor/gpu_browser_compositor_output_surface.cc:140: if
> (set_draw_rectangle_for_frame_)
> should we dcheck/check(!set_draw_rectangle_for_frame)?

No, the DirectRenderer may set this multiple times in the frame, so we should
just ignore it.
> 
>
https://codereview.chromium.org/2646243002/diff/40001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):
> 
>
https://codereview.chromium.org/2646243002/diff/40001/gpu/command_buffer/serv...
> gpu/command_buffer/service/gles2_cmd_decoder.cc:3549: !offscreen &&
> surface->SupportsSetDrawRectangle();
> nit: Won't surface->SupportsSetDrawRectangle only return true if it's
offscreen?

No, offscreen means the GLSurface is ignored and it renders to a different FBO,
so SupportsDrawRectangle should be ignored then.
> 
>
https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
> File gpu/ipc/service/direct_composition_surface_win.cc (right):
> 
>
https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
> gpu/ipc/service/direct_composition_surface_win.cc:37: current_context &&
> current_context->IsCurrent(this_surface);
> nit: DCHECK(current_context->IsCurrent(this_surface)) instead?

This surface isn't necessarily current beforehand. Mainly when it's initially
created, but possibly also otherwise (particularly if I implement the default
surface changing API).
> 
>
https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
> gpu/ipc/service/direct_composition_surface_win.cc:312: bool
> DirectCompositionSurfaceWin::CommitAndClearPendingOverlays() {
> nit: Can you move this next to ScheduleOverlayPlane?

Done.
> 
>
https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
> gpu/ipc/service/direct_composition_surface_win.cc:317: void
> DirectCompositionSurfaceWin::InitializeSurface() {
> nit: can you move this and ReleaseDrawTexture next to the other initialization
> methods?
> 
Done.

>
https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
> gpu/ipc/service/direct_composition_surface_win.cc:343:
> DirectCompositionSurfaceWin::~DirectCompositionSurfaceWin() {
> nit: can you move this next to the ctor?

Done.
> 
>
https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
> File gpu/ipc/service/direct_composition_surface_win.h (right):
> 
>
https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
> gpu/ipc/service/direct_composition_surface_win.h:8: #include <d3d11.h>
> nit: group this with <windows.h> or vice-versa.
> 
Done.

>
https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
> gpu/ipc/service/direct_composition_surface_win.h:87: EGLSurface
> default_surface_;
> nit: default_surface_ = 0
> 
Done.

>
https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
> gpu/ipc/service/direct_composition_surface_win.h:91: EGLSurface real_surface_;
> nit: real_surface_ = 0
> 
Done.
>
https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
> gpu/ipc/service/direct_composition_surface_win.h:93: bool first_swap_;
> nit: first_swap_ = false
> 
Done.

>
https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
> File gpu/ipc/service/direct_composition_surface_win_unittest.cc (right):
> 
>
https://codereview.chromium.org/2646243002/diff/40001/gpu/ipc/service/direct_...
> gpu/ipc/service/direct_composition_surface_win_unittest.cc:34: using
> DirectCompositionSurfaceTest = testing::Test;
> nit: you can use TEST instead of TEST_F and not have to do this.

Done.
> 
>
https://codereview.chromium.org/2646243002/diff/40001/ui/gl/gl_angle_util_win.cc
> File ui/gl/gl_angle_util_win.cc (right):
> 
>
https://codereview.chromium.org/2646243002/diff/40001/ui/gl/gl_angle_util_win...
> ui/gl/gl_angle_util_win.cc:105: 
> nit: can you comment under what conditions the following code is needed?

Done.

[jbauman, 2017-02-14 10:22:54]

sunnyps, ping

[sunnyps, 2017-02-14 22:09:13]

lgtm % nits

https://codereview.chromium.org/2646243002/diff/60001/gpu/ipc/service/direct_...
File gpu/ipc/service/direct_composition_surface_win.h (right):

https://codereview.chromium.org/2646243002/diff/60001/gpu/ipc/service/direct_...
gpu/ipc/service/direct_composition_surface_win.h:91: gfx::Size size_;
nit: size_ = gfx::Size(1, 1)

https://codereview.chromium.org/2646243002/diff/60001/gpu/ipc/service/direct_...
File gpu/ipc/service/direct_composition_surface_win_unittest.cc (right):

https://codereview.chromium.org/2646243002/diff/60001/gpu/ipc/service/direct_...
gpu/ipc/service/direct_composition_surface_win_unittest.cc:7: #include
"base/test/test_message_loop.h"
nit: test_message_loop include is no longer needed

https://codereview.chromium.org/2646243002/diff/80001/content/browser/composi...
File content/browser/compositor/gpu_browser_compositor_output_surface.h (right):

https://codereview.chromium.org/2646243002/diff/80001/content/browser/composi...
content/browser/compositor/gpu_browser_compositor_output_surface.h:87: bool
has_set_draw_rectangle_ = false;
nit: has_set_draw_rectangle_since_last_resize_

[sunnyps, 2017-02-14 22:11:40]

nit: Can you please format the CL description to 80 cols?

nit: The CL description says "Chrome must signal what error will be rendered to
before actually rendering to it." You mean "area will be rendered to" right?

[jbauman, 2017-02-15 00:59:11]

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

[commit-bot: I haz the power, 2017-02-15 01:00:50]

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

[jbauman, 2017-02-15 01:06:35]

Description was changed from

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

For updates to the DirectComposition layer tree to be in sync with updates to
the texture contents we need to put Chrome's backbuffer into an
IDCompositionSurface instead of an IDXGISwapChain2. That way updates to the
surface only happen upon commit.

Unfortunately this adds some complication, in that Chrome must signal what error
will be rendered to before actually rendering to it. All draws must have a
certain offset applied to them within the output texture, though that's not yet
implemented in this patch.

This patch just allocates an IDCompositionSurface and renders to it. A new GL
command (SetDrawRectangleCHROMIUM) must be given before drawing to the
surface.

BUG=678800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

For updates to the DirectComposition layer tree to be in sync with
updates to the texture contents we need to put Chrome's backbuffer into
an IDCompositionSurface instead of an IDXGISwapChain2. That way updates
to the surface only happen upon commit of the visual tree.

Unfortunately this adds some complication, in that Chrome must signal
what region will be rendered to before actually rendering to it. All
draws must have a certain offset applied to them within the output
texture, though that's not yet implemented in this patch.

This patch just allocates an IDCompositionSurface and renders to it. A
new GL command (SetDrawRectangleCHROMIUM) must be given before drawing
to the surface.

BUG=678800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[jbauman, 2017-02-15 01:07:26]

jbauman@chromium.org changed reviewers:
+ danakj@chromium.org, dcheng@chromium.org

[jbauman, 2017-02-15 01:07:27]

danakj@: cc/ OWNERS review
dcheng@: gpu\ipc\common\gpu_command_buffer_traits_multi.h OWNERS review

[commit-bot: I haz the power, 2017-02-15 01:09:22]

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

[commit-bot: I haz the power, 2017-02-15 01:09:23]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jbauman, 2017-02-15 01:45:15]

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

[commit-bot: I haz the power, 2017-02-15 01:46:04]

Dry run: 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, 2017-02-15 02:28:10]

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

[commit-bot: I haz the power, 2017-02-15 02:28:12]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[dcheng, 2017-02-15 06:57:06]

https://codereview.chromium.org/2646243002/diff/120001/content/browser/compos...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/2646243002/diff/120001/content/browser/compos...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:148:
DCHECK(has_set_draw_rectangle_since_last_resize_ ||
Is it OK to DCHECK here? I wonder because
DirectCompositionSurfaceWin::SetDrawRectangle has something similar (but they
are actual if statements).

https://codereview.chromium.org/2646243002/diff/120001/gpu/ipc/service/direct...
File gpu/ipc/service/direct_composition_surface_win.cc (right):

https://codereview.chromium.org/2646243002/diff/120001/gpu/ipc/service/direct...
gpu/ipc/service/direct_composition_surface_win.cc:159: LOG(ERROR) <<
"eglDestroySurface failed with error "
DLOG instead of LOG (this applies elsewhere as well)?

https://codereview.chromium.org/2646243002/diff/120001/gpu/ipc/service/direct...
gpu/ipc/service/direct_composition_surface_win.cc:162: real_surface_ = NULL;
Nit: nullptr

https://codereview.chromium.org/2646243002/diff/120001/gpu/ipc/service/direct...
gpu/ipc/service/direct_composition_surface_win.cc:308: std::vector<EGLint>
pbuffer_attribs;
Nit: consider using an initializer list here.

https://codereview.chromium.org/2646243002/diff/120001/gpu/ipc/service/direct...
File gpu/ipc/service/direct_composition_surface_win_unittest.cc (right):

https://codereview.chromium.org/2646243002/diff/120001/gpu/ipc/service/direct...
gpu/ipc/service/direct_composition_surface_win_unittest.cc:37: LOG(ERROR) << "GL
implementation not using DirectComposition";
Or just FAIL() in a unit test (the return won't be needed anymore either, AFAIK)

https://codereview.chromium.org/2646243002/diff/120001/gpu/ipc/service/direct...
gpu/ipc/service/direct_composition_surface_win_unittest.cc:96:
run_loop.RunUntilIdle();
Or just base::RunLoop().RunUntilIdle() to save a line

https://codereview.chromium.org/2646243002/diff/120001/ui/gl/gl_angle_util_wi...
File ui/gl/gl_angle_util_win.cc (right):

https://codereview.chromium.org/2646243002/diff/120001/ui/gl/gl_angle_util_wi...
ui/gl/gl_angle_util_win.cc:111: typedef HRESULT(WINAPI *
PFN_DCOMPOSITION_CREATE_DEVICE2)(
Nit: prefer using over typedef

[danakj, 2017-02-15 16:01:02]

https://codereview.chromium.org/2646243002/diff/120001/cc/output/direct_rende...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2646243002/diff/120001/cc/output/direct_rende...
cc/output/direct_renderer.cc:92:
context_provider->ContextCapabilities().set_draw_rectangle) {
nit: no {}, or merge this with the conditions above so we only check
|context_provider| once

https://codereview.chromium.org/2646243002/diff/120001/cc/output/direct_rende...
cc/output/direct_renderer.cc:520: bool render_pass_is_clipped =
Can you explain why use_set_draw_rectangle_ implies that we must use a clip rect
even if it contains the surface rect, in a comment here? And how this relates to
the fact that we actually use the damage rect as the draw rectangle?

https://codereview.chromium.org/2646243002/diff/120001/cc/output/direct_rende...
File cc/output/direct_renderer.h (right):

https://codereview.chromium.org/2646243002/diff/120001/cc/output/direct_rende...
cc/output/direct_renderer.h:184: 
nit: no whitespace

https://codereview.chromium.org/2646243002/diff/120001/cc/output/output_surfa...
File cc/output/output_surface.h (right):

https://codereview.chromium.org/2646243002/diff/120001/cc/output/output_surfa...
cc/output/output_surface.h:89: virtual void SetDrawRectangle(const gfx::Rect&
rect) {}
= 0 please

[jbauman, 2017-02-16 03:18:33]

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

[commit-bot: I haz the power, 2017-02-16 03:20:01]

Dry run: 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, 2017-02-16 03:37:29]

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

[commit-bot: I haz the power, 2017-02-16 03:37:30]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jbauman, 2017-02-17 02:28:28]

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

[commit-bot: I haz the power, 2017-02-17 02:29:55]

Dry run: 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, 2017-02-17 02:50:04]

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

[commit-bot: I haz the power, 2017-02-17 02:50:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jbauman, 2017-02-17 21:37:17]

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

[commit-bot: I haz the power, 2017-02-17 21:38:31]

Dry run: 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, 2017-02-17 21:44:15]

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

[commit-bot: I haz the power, 2017-02-17 21:44:17]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jbauman, 2017-02-17 22:50:20]

Description was changed from

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

For updates to the DirectComposition layer tree to be in sync with
updates to the texture contents we need to put Chrome's backbuffer into
an IDCompositionSurface instead of an IDXGISwapChain2. That way updates
to the surface only happen upon commit of the visual tree.

Unfortunately this adds some complication, in that Chrome must signal
what region will be rendered to before actually rendering to it. All
draws must have a certain offset applied to them within the output
texture, though that's not yet implemented in this patch.

This patch just allocates an IDCompositionSurface and renders to it. A
new GL command (SetDrawRectangleCHROMIUM) must be given before drawing
to the surface.

BUG=678800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

For updates to the DirectComposition layer tree to be in sync with
updates to the texture contents we need to put Chrome's backbuffer into
an IDCompositionSurface instead of an IDXGISwapChain2. That way updates
to the surface only happen upon commit of the visual tree.

Unfortunately this adds some complication, in that Chrome must signal
what region will be rendered to before actually rendering to it. All
draws must have a certain offset applied to them within the output
texture, though that's not yet implemented in this patch.

This patch just allocates an IDCompositionSurface and renders to it. A
new GL command (SetDrawRectangleCHROMIUM) must be given before drawing
to the surface.

BUG=678800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;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
==========

[jbauman, 2017-02-17 22:50:27]

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

[commit-bot: I haz the power, 2017-02-17 22:51:47]

Dry run: 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, 2017-02-18 00:55:05]

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

[commit-bot: I haz the power, 2017-02-18 00:55:06]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[jbauman, 2017-02-22 01:50:09]

PTAL. Should have fixed everything requested in review.

On 2017/02/15 06:57:06, dcheng wrote:
>
https://codereview.chromium.org/2646243002/diff/120001/content/browser/compos...
> File content/browser/compositor/gpu_browser_compositor_output_surface.cc
> (right):
> 
>
https://codereview.chromium.org/2646243002/diff/120001/content/browser/compos...
> content/browser/compositor/gpu_browser_compositor_output_surface.cc:148:
> DCHECK(has_set_draw_rectangle_since_last_resize_ ||
> Is it OK to DCHECK here? I wonder because
> DirectCompositionSurfaceWin::SetDrawRectangle has something similar (but they
> are actual if statements).

DirectCompositionSurfaceWin is in a different process so it has to deal with
multiple clients that may behave badly. Generally we have it create gl errors
(which is what returning false will do), not DCHECK.
GpuBrowserCompositorOutputSurface is in the browser and is a client of that, so
it makes sense to have it DCHECK early.
> 
>
https://codereview.chromium.org/2646243002/diff/120001/gpu/ipc/service/direct...
> File gpu/ipc/service/direct_composition_surface_win_unittest.cc (right):
> 
>
https://codereview.chromium.org/2646243002/diff/120001/gpu/ipc/service/direct...
> gpu/ipc/service/direct_composition_surface_win_unittest.cc:37: LOG(ERROR) <<
"GL
> implementation not using DirectComposition";
> Or just FAIL() in a unit test (the return won't be needed anymore either,
AFAIK)

This isn't actually a failure in the test. Many windows bots can't use
DirectComposition (e.g. on Windows 7 or only have D3D9 hardware) so we want to
just skip the test on them, since otherwise we'd have to figure out some
elaborate mechanism to disable the test there.

[dcheng, 2017-02-22 07:58:02]

On 2017/02/22 01:50:09, jbauman wrote:
> PTAL. Should have fixed everything requested in review.
> 
> On 2017/02/15 06:57:06, dcheng wrote:
> >
>
https://codereview.chromium.org/2646243002/diff/120001/content/browser/compos...
> > File content/browser/compositor/gpu_browser_compositor_output_surface.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2646243002/diff/120001/content/browser/compos...
> > content/browser/compositor/gpu_browser_compositor_output_surface.cc:148:
> > DCHECK(has_set_draw_rectangle_since_last_resize_ ||
> > Is it OK to DCHECK here? I wonder because
> > DirectCompositionSurfaceWin::SetDrawRectangle has something similar (but
they
> > are actual if statements).
> 
> DirectCompositionSurfaceWin is in a different process so it has to deal with
> multiple clients that may behave badly. Generally we have it create gl errors
> (which is what returning false will do), not DCHECK.
> GpuBrowserCompositorOutputSurface is in the browser and is a client of that,
so
> it makes sense to have it DCHECK early.

Presumably, it lives in the GPU process though, right? Even if it's a client of
this service, we still consider the browser process more trusted than the GPU,
so in general, it should not be trusting state machines that are driven by
another process.

> > 
> >
>
https://codereview.chromium.org/2646243002/diff/120001/gpu/ipc/service/direct...
> > File gpu/ipc/service/direct_composition_surface_win_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2646243002/diff/120001/gpu/ipc/service/direct...
> > gpu/ipc/service/direct_composition_surface_win_unittest.cc:37: LOG(ERROR) <<
> "GL
> > implementation not using DirectComposition";
> > Or just FAIL() in a unit test (the return won't be needed anymore either,
> AFAIK)
> 
> This isn't actually a failure in the test. Many windows bots can't use
> DirectComposition (e.g. on Windows 7 or only have D3D9 hardware) so we want to
> just skip the test on them, since otherwise we'd have to figure out some
> elaborate mechanism to disable the test there.

[danakj, 2017-02-22 15:04:08]

Code changes LG but why not unit tests for the cc code?

https://codereview.chromium.org/2646243002/diff/220001/cc/output/direct_rende...
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2646243002/diff/220001/cc/output/direct_rende...
cc/output/direct_renderer.cc:599:
output_surface_->SetDrawRectangle(current_frame()->root_damage_rect);
Is this guaranteed to be the full surface if the surface size changed?

https://codereview.chromium.org/2646243002/diff/220001/content/browser/compos...
File content/browser/compositor/browser_compositor_output_surface.h (right):

https://codereview.chromium.org/2646243002/diff/220001/content/browser/compos...
content/browser/compositor/browser_compositor_output_surface.h:38: void
SetDrawRectangle(const gfx::Rect& draw_rectangle) override;
The GPU subclass overrides this, can you move this empty one to the software
subclass instead so we don't have overrides of a non-abstract method in the GPU
subclass?

[jbauman, 2017-02-23 02:47:15]

On 2017/02/22 07:58:02, dcheng wrote:
> On 2017/02/22 01:50:09, jbauman wrote:
> > PTAL. Should have fixed everything requested in review.
> > 
> > On 2017/02/15 06:57:06, dcheng wrote:
> > >
> >
>
https://codereview.chromium.org/2646243002/diff/120001/content/browser/compos...
> > > File content/browser/compositor/gpu_browser_compositor_output_surface.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2646243002/diff/120001/content/browser/compos...
> > > content/browser/compositor/gpu_browser_compositor_output_surface.cc:148:
> > > DCHECK(has_set_draw_rectangle_since_last_resize_ ||
> > > Is it OK to DCHECK here? I wonder because
> > > DirectCompositionSurfaceWin::SetDrawRectangle has something similar (but
> they
> > > are actual if statements).
> > 
> > DirectCompositionSurfaceWin is in a different process so it has to deal with
> > multiple clients that may behave badly. Generally we have it create gl
errors
> > (which is what returning false will do), not DCHECK.
> > GpuBrowserCompositorOutputSurface is in the browser and is a client of that,
> so
> > it makes sense to have it DCHECK early.
> 
> Presumably, it lives in the GPU process though, right? Even if it's a client
of
> this service, we still consider the browser process more trusted than the GPU,
> so in general, it should not be trusting state machines that are driven by
> another process.

GpuBrowserCompositorOutputSurface is in the browser process. The things the
DCHECK is checking are driven only by the cc's GLRenderer (also in the browser
process), so it doesn't have to trust the state machine in the GPU process.

DirectCompositionSurfaceWin is in the GPU process, so it could trust the output
of the browser process, but it still makes sense to verify things anyway rather
than crash in the DirectComposition library.
> 
> > > 
> > >
> >
>
https://codereview.chromium.org/2646243002/diff/120001/gpu/ipc/service/direct...
> > > File gpu/ipc/service/direct_composition_surface_win_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2646243002/diff/120001/gpu/ipc/service/direct...
> > > gpu/ipc/service/direct_composition_surface_win_unittest.cc:37: LOG(ERROR)
<<
> > "GL
> > > implementation not using DirectComposition";
> > > Or just FAIL() in a unit test (the return won't be needed anymore either,
> > AFAIK)
> > 
> > This isn't actually a failure in the test. Many windows bots can't use
> > DirectComposition (e.g. on Windows 7 or only have D3D9 hardware) so we want
to
> > just skip the test on them, since otherwise we'd have to figure out some
> > elaborate mechanism to disable the test there.

[jbauman, 2017-02-23 02:52:41]

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

[commit-bot: I haz the power, 2017-02-23 02:53:10]

Dry run: 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, 2017-02-23 02:57:17]

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

[commit-bot: I haz the power, 2017-02-23 02:57:18]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[jbauman, 2017-02-23 03:14:26]

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

[commit-bot: I haz the power, 2017-02-23 03:15:00]

Dry run: 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, 2017-02-23 04:25:02]

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

[commit-bot: I haz the power, 2017-02-23 04:25:03]

Dry run: This issue passed the CQ dry run.

[jbauman, 2017-02-23 04:32:20]

PTAL.

On 2017/02/22 15:04:08, danakj wrote:
> Code changes LG but why not unit tests for the cc code?

Added a cc test.
> 
>
https://codereview.chromium.org/2646243002/diff/220001/cc/output/direct_rende...
> File cc/output/direct_renderer.cc (right):
> 
>
https://codereview.chromium.org/2646243002/diff/220001/cc/output/direct_rende...
> cc/output/direct_renderer.cc:599:
> output_surface_->SetDrawRectangle(current_frame()->root_damage_rect);
> Is this guaranteed to be the full surface if the surface size changed?

Yeah, otherwise the content outside the damage rect is undefined, which is bad.
> 
>
https://codereview.chromium.org/2646243002/diff/220001/content/browser/compos...
> File content/browser/compositor/browser_compositor_output_surface.h (right):
> 
>
https://codereview.chromium.org/2646243002/diff/220001/content/browser/compos...
> content/browser/compositor/browser_compositor_output_surface.h:38: void
> SetDrawRectangle(const gfx::Rect& draw_rectangle) override;
> The GPU subclass overrides this, can you move this empty one to the software
> subclass instead so we don't have overrides of a non-abstract method in the
GPU
> subclass?

Done.

[dcheng, 2017-02-23 06:49:16]

ipc lgtm

thanks for the clarifications

[danakj, 2017-02-23 22:54:09]

On Wed, Feb 22, 2017 at 11:32 PM, <jbauman@chromium.org> wrote:

> PTAL.
>
> On 2017/02/22 15:04:08, danakj wrote:
> > Code changes LG but why not unit tests for the cc code?
>
> Added a cc test.
> >
> >
> https://codereview.chromium.org/2646243002/diff/220001/cc/
> output/direct_renderer.cc
> > File cc/output/direct_renderer.cc (right):
> >
> >
> https://codereview.chromium.org/2646243002/diff/220001/cc/
> output/direct_renderer.cc#newcode599
> > cc/output/direct_renderer.cc:599:
> > output_surface_->SetDrawRectangle(current_frame()->root_damage_rect);
> > Is this guaranteed to be the full surface if the surface size changed?
>
> Yeah, otherwise the content outside the damage rect is undefined, which is
> bad.
>

Sorry my question was vague, which left me unclear on your answer. I meant
how confident are you that the damage rect will indeed be the whole surface
after resize? I thought it would just be the delta between the two sizes.


>
> >
> >
> https://codereview.chromium.org/2646243002/diff/220001/
> content/browser/compositor/browser_compositor_output_surface.h
> > File content/browser/compositor/browser_compositor_output_surface.h
> (right):
> >
> >
> https://codereview.chromium.org/2646243002/diff/220001/
> content/browser/compositor/browser_compositor_output_surface.h#newcode38
> > content/browser/compositor/browser_compositor_output_surface.h:38: void
> > SetDrawRectangle(const gfx::Rect& draw_rectangle) override;
> > The GPU subclass overrides this, can you move this empty one to the
> software
> > subclass instead so we don't have overrides of a non-abstract method in
> the
> GPU
> > subclass?
>
> Done.
>
>
> https://codereview.chromium.org/2646243002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jbauman, 2017-02-23 23:34:49]

On 2017/02/23 22:54:09, danakj wrote:
> On Wed, Feb 22, 2017 at 11:32 PM, <mailto:jbauman@chromium.org> wrote:
> 
> > PTAL.
> >
> > On 2017/02/22 15:04:08, danakj wrote:
> > > Code changes LG but why not unit tests for the cc code?
> >
> > Added a cc test.
> > >
> > >
> > https://codereview.chromium.org/2646243002/diff/220001/cc/
> > output/direct_renderer.cc
> > > File cc/output/direct_renderer.cc (right):
> > >
> > >
> > https://codereview.chromium.org/2646243002/diff/220001/cc/
> > output/direct_renderer.cc#newcode599
> > > cc/output/direct_renderer.cc:599:
> > > output_surface_->SetDrawRectangle(current_frame()->root_damage_rect);
> > > Is this guaranteed to be the full surface if the surface size changed?
> >
> > Yeah, otherwise the content outside the damage rect is undefined, which is
> > bad.
> >
> 
> Sorry my question was vague, which left me unclear on your answer. I meant
> how confident are you that the damage rect will indeed be the whole surface
> after resize? I thought it would just be the delta between the two sizes.
> 
I'm very confident about that - LayerTreeHostImpl::SetViewportSize does
SetFullViewportDamage(). Generally you're going to want to do a full relayout of
the window anyway, so that makes sense.

[danakj, 2017-02-23 23:38:51]

LGTM

https://codereview.chromium.org/2646243002/diff/240001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/2646243002/diff/240001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:1866: context->set_have_post_sub_buffer(true);
can you set_support_set_draw_rectangle here instead of using TestContext3d()? I
think this is how things will look on the way to getting rid of TestWGC3D, and
makes it consistently present for the full lifetime

https://codereview.chromium.org/2646243002/diff/240001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:1948: EXPECT_EQ(output_rectangle,
output_surface->last_set_draw_rectangle());
FWIW you could mock and EXPECT the method instead of adding state+getters on
FakeOutputSurface. up to u

https://codereview.chromium.org/2646243002/diff/240001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:1962: TEST_F(GLRendererPartialSwapTest,
SetDrawRectangle) {
nit: maybe name the two test variants a bit more explicitly?
SetDrawRectangle_PartialSwap and SetDrawRectangle_NoPartialSwap?

https://codereview.chromium.org/2646243002/diff/240001/cc/test/fake_output_su...
File cc/test/fake_output_surface.h (right):

https://codereview.chromium.org/2646243002/diff/240001/cc/test/fake_output_su...
cc/test/fake_output_surface.h:105: const gfx::Rect& last_set_draw_rectangle() {
nit: white space after function body

[jbauman, 2017-02-24 01:00:37]

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

[commit-bot: I haz the power, 2017-02-24 01:01:20]

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

[jbauman, 2017-02-24 01:06:49]

jbauman@chromium.org changed reviewers:
+ boliu@chromium.org, fsamuel@chromium.org

[jbauman, 2017-02-24 01:06:50]

boliu@: android-related review
fsamuel@: services/ui/surfaces OWNERS review

[boliu, 2017-02-24 01:11:34]

On 2017/02/24 01:06:50, jbauman wrote:
> boliu@: android-related review

rs lgtm

> fsamuel@: services/ui/surfaces OWNERS review

[Fady Samuel, 2017-02-24 01:12:29]

Hmm Mus doesn't need an implementation of this?

[jbauman, 2017-02-24 01:54:32]

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

[commit-bot: I haz the power, 2017-02-24 01:55:20]

Dry run: 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, 2017-02-24 03:20:32]

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

[commit-bot: I haz the power, 2017-02-24 03:20:33]

Dry run: This issue passed the CQ dry run.

[jbauman, 2017-02-24 22:14:55]

On 2017/02/24 01:12:29, Fady Samuel wrote:
> Hmm Mus doesn't need an implementation of this?

Good point, added an implementation.

[jbauman, 2017-02-28 23:00:30]

fsamuel, PTAL. Thanks.

On 2017/02/24 22:14:55, jbauman wrote:
> On 2017/02/24 01:12:29, Fady Samuel wrote:
> > Hmm Mus doesn't need an implementation of this?
> 
> Good point, added an implementation.

[Fady Samuel, 2017-03-01 18:40:51]

lgtm

[jbauman, 2017-03-01 21:52:02]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2017-03-01 21:52:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
sunnyps@chromium.org, danakj@chromium.org, dcheng@chromium.org,
boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2646243002/#ps300001
(title: "add mus implementation")

[commit-bot: I haz the power, 2017-03-01 21:52:53]

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, 2017-03-01 23:04:15]

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

[commit-bot: I haz the power, 2017-03-01 23:04:19]

Failed to apply patch for
content/browser/compositor/gpu_browser_compositor_output_surface.cc:
While running git apply --index -p1;
  error: patch failed:
content/browser/compositor/gpu_browser_compositor_output_surface.cc:115
  error: content/browser/compositor/gpu_browser_compositor_output_surface.cc:
patch does not apply

Patch:       content/browser/compositor/gpu_browser_compositor_output_surface.cc
Index: content/browser/compositor/gpu_browser_compositor_output_surface.cc
diff --git a/content/browser/compositor/gpu_browser_compositor_output_surface.cc
b/content/browser/compositor/gpu_browser_compositor_output_surface.cc
index
d663726cda9947827ee1887f72fb2562a3b4ffbd..05806558c96aac8cbbe7869fb7ba9da4e1a477e6
100644
--- a/content/browser/compositor/gpu_browser_compositor_output_surface.cc
+++ b/content/browser/compositor/gpu_browser_compositor_output_surface.cc
@@ -96,6 +96,8 @@ void GpuBrowserCompositorOutputSurface::Reshape(
     const gfx::ColorSpace& color_space,
     bool has_alpha,
     bool use_stencil) {
+  size_ = size;
+  has_set_draw_rectangle_since_last_resize_ = false;
   context_provider()->ContextGL()->ResizeCHROMIUM(
       size.width(), size.height(), device_scale_factor, has_alpha);
 }
@@ -115,6 +117,8 @@ void GpuBrowserCompositorOutputSurface::SwapBuffers(
     }
   }
 
+  set_draw_rectangle_for_frame_ = false;
+
   if (frame.sub_buffer_rect) {
     context_provider_->ContextSupport()->PartialSwapBuffers(
         *frame.sub_buffer_rect);
@@ -145,6 +149,19 @@ void
GpuBrowserCompositorOutputSurface::SetSurfaceSuspendedForRecycle(
     bool suspended) {}
 #endif
 
+void GpuBrowserCompositorOutputSurface::SetDrawRectangle(
+    const gfx::Rect& rect) {
+  if (set_draw_rectangle_for_frame_)
+    return;
+  DCHECK(gfx::Rect(size_).Contains(rect));
+  DCHECK(has_set_draw_rectangle_since_last_resize_ ||
+         (gfx::Rect(size_) == rect));
+  set_draw_rectangle_for_frame_ = true;
+  has_set_draw_rectangle_since_last_resize_ = true;
+  context_provider()->ContextGL()->SetDrawRectangleCHROMIUM(
+      rect.x(), rect.y(), rect.width(), rect.height());
+}
+
 gpu::CommandBufferProxyImpl*
 GpuBrowserCompositorOutputSurface::GetCommandBufferProxy() {
   ui::ContextProviderCommandBuffer* provider_command_buffer =

[jbauman, 2017-03-01 23:19:00]

The CQ bit was checked by jbauman@chromium.org

[jbauman, 2017-03-01 23:19:01]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
fsamuel@chromium.org, sunnyps@chromium.org, boliu@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2646243002/#ps320001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-01 23:19:54]

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, 2017-03-02 00:35:18]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1488410340101010,
"parent_rev": "20032a5cd3037ac303100cae6585e279d6908c23", "commit_rev":
"2a2ad220b1c964f4fc348a8118ae9b91c43aa50e"}

[commit-bot: I haz the power, 2017-03-02 00:36:00]

Description was changed from

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

For updates to the DirectComposition layer tree to be in sync with
updates to the texture contents we need to put Chrome's backbuffer into
an IDCompositionSurface instead of an IDXGISwapChain2. That way updates
to the surface only happen upon commit of the visual tree.

Unfortunately this adds some complication, in that Chrome must signal
what region will be rendered to before actually rendering to it. All
draws must have a certain offset applied to them within the output
texture, though that's not yet implemented in this patch.

This patch just allocates an IDCompositionSurface and renders to it. A
new GL command (SetDrawRectangleCHROMIUM) must be given before drawing
to the surface.

BUG=678800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;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

==========
Use IDCompositionSurface to implement DirectCompositionSurfaceWin.

For updates to the DirectComposition layer tree to be in sync with
updates to the texture contents we need to put Chrome's backbuffer into
an IDCompositionSurface instead of an IDXGISwapChain2. That way updates
to the surface only happen upon commit of the visual tree.

Unfortunately this adds some complication, in that Chrome must signal
what region will be rendered to before actually rendering to it. All
draws must have a certain offset applied to them within the output
texture, though that's not yet implemented in this patch.

This patch just allocates an IDCompositionSurface and renders to it. A
new GL command (SetDrawRectangleCHROMIUM) must be given before drawing
to the surface.

BUG=678800
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;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

Review-Url: https://codereview.chromium.org/2646243002
Cr-Commit-Position: refs/heads/master@{#454119}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a2ad220b1c964f4fc348a8118ae...
==========

[commit-bot: I haz the power, 2017-03-02 00:36:03]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/2a2ad220b1c964f4fc348a8118ae...