Implement new RPDQ copy logic for CALayer promotion.

Implement new RPDQ copy logic for CALayer promotion.

RPDQ promotion isn't enabled yet. That will be done in a follow up CL.

The new logic heavily reuses the existing RPDQ drawing logic, but uses a custom
drawing context instead of a DrawingFrame and RenderPass.

BUG=581526
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/d9a09cd1eb5cb9bab507ec2841726c02b316260d
Cr-Commit-Position: refs/heads/master@{#410767}

[erikchen, 2016-08-03 17:16:12]

Description was changed from

==========
Implement new RPDQ copy logic for CALayer promotion.

RPDQ promotion isn't enabled yet. That will be done in a follow up CL.

The new logic heavily reuses the existing RPDQ drawing logic, but uses a custom
drawing context instead of a DrawingFrame and RenderPass.

BUG=581526
==========

to

==========
Implement new RPDQ copy logic for CALayer promotion.

RPDQ promotion isn't enabled yet. That will be done in a follow up CL.

The new logic heavily reuses the existing RPDQ drawing logic, but uses a custom
drawing context instead of a DrawingFrame and RenderPass.

BUG=581526
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[erikchen, 2016-08-03 17:19:58]

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

[erikchen, 2016-08-03 17:20:13]

erikchen@chromium.org changed reviewers:
+ ccameron@chromium.org

[erikchen, 2016-08-03 17:20:13]

ccameron: Please review.

[commit-bot: I haz the power, 2016-08-03 17:20: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, 2016-08-03 17:57:21]

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

[commit-bot: I haz the power, 2016-08-03 17:57:21]

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/...)

[erikchen, 2016-08-03 20:36:22]

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

[commit-bot: I haz the power, 2016-08-03 20:36: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, 2016-08-03 21:36:43]

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

[commit-bot: I haz the power, 2016-08-03 21:36:44]

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

[erikchen, 2016-08-03 21:51:00]

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

[commit-bot: I haz the power, 2016-08-03 21:51:40]

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

[erikchen, 2016-08-03 21:58:11]

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

[commit-bot: I haz the power, 2016-08-03 21:58:35]

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

[ccameron, 2016-08-04 01:46:01]

Some remarks -- whiteboard time would solve all of this, I'm sure.

https://codereview.chromium.org/2205133003/diff/100001/cc/output/ca_layer_ove...
File cc/output/ca_layer_overlay.cc (right):

https://codereview.chromium.org/2205133003/diff/100001/cc/output/ca_layer_ove...
cc/output/ca_layer_overlay.cc:307: return *this;
This can just be =default then, right?

https://codereview.chromium.org/2205133003/diff/100001/cc/output/ca_layer_ove...
File cc/output/ca_layer_overlay.h (right):

https://codereview.chromium.org/2205133003/diff/100001/cc/output/ca_layer_ove...
cc/output/ca_layer_overlay.h:73: linked_ptr<RenderPassDrawQuad> rpdq;
unique_ptr, or raw pointer. I'd prefer a raw ptr into the DrawQuadList, and just
not clear the DrawQuadList.

This could be a raw ptr if we were to not clear the DrawQuad list at
https://cs.chromium.org/chromium/src/cc/output/overlay_processor.cc?rcl=0&l=55

(And there's no good reason to clear the DrawQuad list there -- we make the
damage rect be empty, which makes us skip drawing the root render pass at 
https://cs.chromium.org/chromium/src/cc/output/direct_renderer.cc?rcl=1470251...
)

https://codereview.chromium.org/2205133003/diff/100001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2205133003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3785: frame);
Rather than updating the CALayerOverlay parameters here, I think we should just
have CopyRenderPassDrawQuadToOverlayResource be responsible for calling
gl_->ScheduleCALayerCHROMIUM and gl_->ScheduleCALayerSharedStateCHROMIUM.

It's not that much code, and it's easier to duplicate than to try to keep in one
package.

https://codereview.chromium.org/2205133003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:4024: DrawRPDQ(params);
I'll need to understand this a bit better, but, my understanding that this is an
optimized version of:
1. make temp_fbo be the size of the whole backbuffer
2. make *resource also be the size of the whole backbuffer
3. schedule a CALayer that is the size of the screen

We just try to make temp_fbo and *resource be only the size of the image of the
quad on the screen.

What about having this plow forward as usual, but, in DrawRPDQ, add an extra
argument saying "oh, but add a translation matrix to make the image of the quad
be at 0,0? And add an appropriate call to glViewport (probably don't bother with
the rest of the viewport update code).

That would strike me as safer. But I may not understand all of the structure
here.

https://codereview.chromium.org/2205133003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:4044: ca_layer_overlay->shared_state = shared_state;
This doesn't strike me as correct. What if shared_state->transform originally
had a rotational component to it? This would wipe that out.

[erikchen, 2016-08-05 17:12:39]

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

[commit-bot: I haz the power, 2016-08-05 17:13:11]

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

[erikchen, 2016-08-05 17:15:06]

ccameron: PTAL

https://codereview.chromium.org/2205133003/diff/100001/cc/output/ca_layer_ove...
File cc/output/ca_layer_overlay.cc (right):

https://codereview.chromium.org/2205133003/diff/100001/cc/output/ca_layer_ove...
cc/output/ca_layer_overlay.cc:307: return *this;
On 2016/08/04 01:46:01, ccameron wrote:
> This can just be =default then, right?

No longer necessary at all.

https://codereview.chromium.org/2205133003/diff/100001/cc/output/ca_layer_ove...
File cc/output/ca_layer_overlay.h (right):

https://codereview.chromium.org/2205133003/diff/100001/cc/output/ca_layer_ove...
cc/output/ca_layer_overlay.h:73: linked_ptr<RenderPassDrawQuad> rpdq;
On 2016/08/04 01:46:01, ccameron wrote:
> unique_ptr, or raw pointer. I'd prefer a raw ptr into the DrawQuadList, and
just
> not clear the DrawQuadList.
> 
> This could be a raw ptr if we were to not clear the DrawQuad list at
> https://cs.chromium.org/chromium/src/cc/output/overlay_processor.cc?rcl=0&l=55
> 
> (And there's no good reason to clear the DrawQuad list there -- we make the
> damage rect be empty, which makes us skip drawing the root render pass at 
>
https://cs.chromium.org/chromium/src/cc/output/direct_renderer.cc?rcl=1470251...
> )
> 

Done.

https://codereview.chromium.org/2205133003/diff/100001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2205133003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3785: frame);
On 2016/08/04 01:46:01, ccameron wrote:
> Rather than updating the CALayerOverlay parameters here, I think we should
just
> have CopyRenderPassDrawQuadToOverlayResource be responsible for calling
> gl_->ScheduleCALayerCHROMIUM and gl_->ScheduleCALayerSharedStateCHROMIUM.
> 
> It's not that much code, and it's easier to duplicate than to try to keep in
one
> package.

Done.

https://codereview.chromium.org/2205133003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:4024: DrawRPDQ(params);
On 2016/08/04 01:46:01, ccameron wrote:
> I'll need to understand this a bit better, but, my understanding that this is
an
> optimized version of:
> 1. make temp_fbo be the size of the whole backbuffer
> 2. make *resource also be the size of the whole backbuffer
> 3. schedule a CALayer that is the size of the screen
> 
> We just try to make temp_fbo and *resource be only the size of the image of
the
> quad on the screen.
> 
> What about having this plow forward as usual, but, in DrawRPDQ, add an extra
> argument saying "oh, but add a translation matrix to make the image of the
quad
> be at 0,0? And add an appropriate call to glViewport (probably don't bother
with
> the rest of the viewport update code).
> 
> That would strike me as safer. But I may not understand all of the structure
> here.

Your suggestion is sound. The new PS implements your suggestion.

https://codereview.chromium.org/2205133003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:4044: ca_layer_overlay->shared_state = shared_state;
On 2016/08/04 01:46:01, ccameron wrote:
> This doesn't strike me as correct. What if shared_state->transform originally
> had a rotational component to it? This would wipe that out.

Right. This should have been preTranslate.

[commit-bot: I haz the power, 2016-08-05 17:15:28]

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

[commit-bot: I haz the power, 2016-08-05 17:15:29]

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

[erikchen, 2016-08-05 17:17:00]

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

[commit-bot: I haz the power, 2016-08-05 17:17:25]

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, 2016-08-05 17:48:33]

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

[commit-bot: I haz the power, 2016-08-05 17:48:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-08-05 18:22:50]

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

[commit-bot: I haz the power, 2016-08-05 18:23: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, 2016-08-05 19:19:11]

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

[commit-bot: I haz the power, 2016-08-05 19:19:12]

Dry run: This issue passed the CQ dry run.

[ccameron, 2016-08-08 21:36:53]

I was thinking of something even less invasive -- just computing the image of
the quad, and swapping out one of the transforms at the last second.

In:
  https://codereview.chromium.org/2224113002
  PS1 is your patch, and PS2 is the change on top of it.

Of note is that it is possible to bind an IOSurface as an FBO target (we do this
for the backbuffer, so I just copied that code in). That should skip the extra
copy.

[ccameron, 2016-08-08 21:48:10]

https://codereview.chromium.org/2205133003/diff/220001/cc/output/ca_layer_ove...
File cc/output/ca_layer_overlay.cc (right):

https://codereview.chromium.org/2205133003/diff/220001/cc/output/ca_layer_ove...
cc/output/ca_layer_overlay.cc:70: CALayerOverlay* ca_layer_overlay) {
Cause we're messing with the 3D transforms, we can only trust this to work when
we're just drawing layers on top of each other -- if we have to do
intersections, etc, in 3D space, we're hosed. So we'll need to fail if
sorting_context_id is 0.

[erikchen, 2016-08-09 00:01:17]

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

[erikchen, 2016-08-09 00:01:28]

ccameron: PTAL

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

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, 2016-08-09 01:12:10]

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

[commit-bot: I haz the power, 2016-08-09 01:12:12]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-08-09 01:43:03]

erikchen@chromium.org changed reviewers:
+ isherman@chromium.org

[erikchen, 2016-08-09 01:43:04]

isherman: Please review tools/metrics/histograms/histograms.xml

[ccameron, 2016-08-09 06:32:15]

lgtm, sorry for the back-and-forth

https://codereview.chromium.org/2205133003/diff/260001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2205133003/diff/260001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3975: ScopedFramebufferDeleter
framebuffer_deleter(temp_fbo, gl_);
Because we don't have any early-outs here, I think it's be just as easy to do an
explicit delete at the end of the function.

https://codereview.chromium.org/2205133003/diff/260001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:4031: 0, 0, new_bounds.width(), new_bounds.height(),
It's vaguely easier to comprehend this if bounds_rect is set to have
new_bounds.x and new_bounds.y, and transform is left un-preTranslated.

(assuming this is right)

[erikchen, 2016-08-09 16:52:13]

https://codereview.chromium.org/2205133003/diff/260001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2205133003/diff/260001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3975: ScopedFramebufferDeleter
framebuffer_deleter(temp_fbo, gl_);
On 2016/08/09 06:32:15, ccameron wrote:
> Because we don't have any early-outs here, I think it's be just as easy to do
an
> explicit delete at the end of the function.

Done.

https://codereview.chromium.org/2205133003/diff/260001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:4031: 0, 0, new_bounds.width(), new_bounds.height(),
On 2016/08/09 06:32:15, ccameron wrote:
> It's vaguely easier to comprehend this if bounds_rect is set to have
> new_bounds.x and new_bounds.y, and transform is left un-preTranslated.
> 
> (assuming this is right)

Done.

[erikchen, 2016-08-09 17:02:16]

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

[commit-bot: I haz the power, 2016-08-09 17:02:59]

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, 2016-08-09 18:10:46]

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

[commit-bot: I haz the power, 2016-08-09 18:10:47]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2016-08-09 19:17:46]

histograms.xml lgtm

[erikchen, 2016-08-09 19:18:33]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-08-09 19:18:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/2205133003/#ps280001
(title: "Comments from ccameron.")

[commit-bot: I haz the power, 2016-08-09 19:19:47]

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-08-09 19:43:06]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-08-09 19:46:24]

Description was changed from

==========
Implement new RPDQ copy logic for CALayer promotion.

RPDQ promotion isn't enabled yet. That will be done in a follow up CL.

The new logic heavily reuses the existing RPDQ drawing logic, but uses a custom
drawing context instead of a DrawingFrame and RenderPass.

BUG=581526
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Implement new RPDQ copy logic for CALayer promotion.

RPDQ promotion isn't enabled yet. That will be done in a follow up CL.

The new logic heavily reuses the existing RPDQ drawing logic, but uses a custom
drawing context instead of a DrawingFrame and RenderPass.

BUG=581526
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/d9a09cd1eb5cb9bab507ec2841726c02b316260d
Cr-Commit-Position: refs/heads/master@{#410767}
==========

[commit-bot: I haz the power, 2016-08-09 19:46:26]

Patchset 15 (id:??) landed as
https://crrev.com/d9a09cd1eb5cb9bab507ec2841726c02b316260d
Cr-Commit-Position: refs/heads/master@{#410767}