cc: Move draw params from SetExternalDrawConstraints to OnDraw

cc: Move draw params from SetExternalDrawConstraints to OnDraw

This is a clean up to move some of the logic spread between
LayerTreeHostImpl and SynchronousCompositorOutputSurface about setting
and saving parameters for synchronous compositor draws.

Move draw parameters to OnDraw. These will be reset before OnDraw
returns. Parameters are overridden permanently is set in
SetExternalDrawConstraints.

BUG=419795
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7097ee5b1f3691d12f425d719d33698bb90261b6
Cr-Commit-Position: refs/heads/master@{#365700}

[boliu, 2015-10-23 18:02:46]

boliu@chromium.org changed reviewers:
+ danakj@chromium.org

[boliu, 2015-10-23 18:02:47]

Early thoughts?

The production code changes are all there. Negative line number so far, but
still need to fix/rewrite all cc_unittests that need to be rewritten..

A few things:
I went overzealous with AutoResets but I think that might be the right thing to
do here. Conceptually the draw parameters are only valid during the draw, and cc
should fall back to its own values otherwise. I checked where they are used and
these are places they are not used that's not in draws:
* update tile priorities
* CanDraw
* SetFullRectLayerDamage
* BuildPropertyTreeForTesting
They all seem find to use chrome values rather that external draw values.

So with that, maybe it's possible to drop all the external_foo_ params in LTHI
too and just use the viewport and whatnot directly. But that assumes the
original values never change inside a OnDraw, which might not be true.


I also moved "damage on first hardware draw after software" to the end of the
software draw, since it's possible to set damage without causing redraw in LTHI.

[danakj, 2015-10-23 18:13:10]

On 2015/10/23 18:02:47, boliu wrote:
> Early thoughts?
> 
> The production code changes are all there. Negative line number so far, but
> still need to fix/rewrite all cc_unittests that need to be rewritten..
> 
> A few things:
> I went overzealous with AutoResets but I think that might be the right thing
to
> do here. Conceptually the draw parameters are only valid during the draw, and
cc
> should fall back to its own values otherwise. I checked where they are used
and
> these are places they are not used that's not in draws:
> * update tile priorities
> * CanDraw
> * SetFullRectLayerDamage
> * BuildPropertyTreeForTesting
> They all seem find to use chrome values rather that external draw values.
> 
> So with that, maybe it's possible to drop all the external_foo_ params in LTHI
> too and just use the viewport and whatnot directly. But that assumes the
> original values never change inside a OnDraw, which might not be true.

The only original value there is the viewport, which only changes in a commit.
That does not occur mid-draw. I think we can then.

> 
> I also moved "damage on first hardware draw after software" to the end of the
> software draw, since it's possible to set damage without causing redraw in
LTHI.

LG to me overall.

https://codereview.chromium.org/1418273002/diff/1/cc/output/output_surface_cl...
File cc/output/output_surface_client.h (right):

https://codereview.chromium.org/1418273002/diff/1/cc/output/output_surface_cl...
cc/output/output_surface_client.h:43: virtual void OnDraw(const gfx::Transform&
transform,
maybe now's a good time to give more context in these parameter names?

https://codereview.chromium.org/1418273002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1418273002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:1513: client_->OnDrawForOutputSurface();
I was picturing resourcelessdraw being passed here as well instead of
OnResourcelessSoftwareDrawStateChanged()

[boliu, 2015-10-23 18:18:20]

kk, will come back when tests are in good shape..

https://codereview.chromium.org/1418273002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1418273002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:1513: client_->OnDrawForOutputSurface();
On 2015/10/23 18:13:10, danakj wrote:
> I was picturing resourcelessdraw being passed here as well instead of
> OnResourcelessSoftwareDrawStateChanged()

Ahh, good point :)

[boliu, 2015-10-23 18:18:32]

Description was changed from

==========
cc: Move draw params from SetExternalDrawConstraints to OnDraw

This is a clean up to move some of the logic spread between
LayerTreeHostImpl and SynchronousCompositorOutputSurface about setting
and saving parameters for synchronous compositor draws.

Move draw parameters to OnDraw. These will be reset before OnDraw
returns. Parameters are overridden permanently is set in
SetExternalDrawConstraints.

WIP: Fix all cc_unittests

BUG=419795
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Move draw params from SetExternalDrawConstraints to OnDraw

This is a clean up to move some of the logic spread between
LayerTreeHostImpl and SynchronousCompositorOutputSurface about setting
and saving parameters for synchronous compositor draws.

Move draw parameters to OnDraw. These will be reset before OnDraw
returns. Parameters are overridden permanently is set in
SetExternalDrawConstraints.

WIP: Fix all cc_unittests

BUG=419795
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[boliu, 2015-10-23 18:18:32]

boliu@chromium.org changed reviewers:
- danakj@chromium.org

[boliu, 2015-11-04 00:27:26]

Description was changed from

==========
cc: Move draw params from SetExternalDrawConstraints to OnDraw

This is a clean up to move some of the logic spread between
LayerTreeHostImpl and SynchronousCompositorOutputSurface about setting
and saving parameters for synchronous compositor draws.

Move draw parameters to OnDraw. These will be reset before OnDraw
returns. Parameters are overridden permanently is set in
SetExternalDrawConstraints.

WIP: Fix all cc_unittests

BUG=419795
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Move draw params from SetExternalDrawConstraints to OnDraw

This is a clean up to move some of the logic spread between
LayerTreeHostImpl and SynchronousCompositorOutputSurface about setting
and saving parameters for synchronous compositor draws.

Move draw parameters to OnDraw. These will be reset before OnDraw
returns. Parameters are overridden permanently is set in
SetExternalDrawConstraints.

BUG=419795
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[boliu, 2015-11-04 00:39:10]

boliu@chromium.org changed reviewers:
+ danakj@chromium.org

[boliu, 2015-11-04 00:39:11]

And I'm back! Tests are fixed. And added a couple of new LTHIUnittests

Two things to note:

external_viewport_ etc are saved instead of being reset at end of OnDraw. I was
too overzealous in the first patch, and noticed this case doesn't work while
writing tests:

Let's say LTHI::device_viewport_size_ is 100x100, and external draw values are
reset at end of OnDraw. Then this sequence doesn't work:
OnDraw(transform=identity, viewport=0,0,50x50, clip=0,0,50x50,
resourceless=false) <- calls SetNeedsRedraw
OnDraw(transform=identity, viewport=0,0,100x100, clip=0,0,100x100,
resourceless=false) <- does not SetNeedsRedraw

This second OnDraw definitely should re-draw the external draw params differ
from the previous OnDraw. But in PS1, the draw would be incorrectly skipped,
since external params patches "internal" params exactly.


Second thing is had two test-only methods:
LTHI::SetResourcelessSoftwareDrawForTesting <- makes fixing LayerTreeTests so
much easier.
LTHI::SetExternalViewporForTesting <- Really needed by layer_tree_pixel_test.cc
only. afaict it really does want to shift the draw viewport

[boliu, 2015-11-06 16:52:46]

ping?

[danakj, 2015-11-24 21:17:30]

danakj@chromium.org changed reviewers:
+ brianderson@chromium.org

[danakj, 2015-11-24 21:17:31]

lgtm

Hey! This is great, thanks for the CL. I have some comments/questions about
testing mostly.

https://codereview.chromium.org/1418273002/diff/80001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/80001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1755:
host_impl_.SetExternalViewporForTesting(viewport);
Why is this needed for this test?

https://codereview.chromium.org/1418273002/diff/80001/cc/test/fake_output_sur...
File cc/test/fake_output_surface.h (right):

https://codereview.chromium.org/1418273002/diff/80001/cc/test/fake_output_sur...
cc/test/fake_output_surface.h:57: static scoped_ptr<FakeOutputSurface>
Create3DSoftware() {
"3d"

Create3dWithResourcelessSoftwareSupport? or something a bit more explainy?

I don't want people using this by default

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1231: void
LayerTreeHostImpl::SetExternalViewporForTesting(
Why aren't callers of this just using SetExternalDrawConstraints?

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1459: SetFullRootLayerDamage();
Can you explain why this and SetNeedsRedraw() still belong in here? Maybe with a
comment too?

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1529: // always swap. Otherwise, need to set
redraw for all changes to draw
all changes => any changes

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (left):

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:99: virtual void
OnResourcelessSoftareDrawStateChanged(
Softare :D

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:301: void SetExternalViewporForTesting(const
gfx::Rect& external_viewport);
"Viewport"

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:348: void SetExternalDrawConstraints(
Now more like SetExternalTilePriority...Constraints?

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:162: last_on_draw_frame_ =
frame.Pass();
std::move (include <utility> too)

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:5737: SetResourcelessSoftwareDraw();
These tests that set resourceless software draw on get weird. Now we're going to
do things like CreatePendingTree with resourceless_software_draw_ enabled, which
will never happen in production cuz it's only true inside OnDraw.

I'd like to delete the SetResourcelessSoftwareDrawForTesting if possible.. any
thoughts how we could do that?

https://codereview.chromium.org/1418273002/diff/80001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/1418273002/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:148:
InvokeComposite(transform, viewport, clip, false);
nit: use a temp var dont pass bool literals to functions

https://codereview.chromium.org/1418273002/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:170:
InvokeComposite(transform, clip, clip, true);
ditto

[danakj, 2015-11-24 21:17:56]

And I hit the lgtm button by accident hah. Anyways...

[boliu, 2015-11-24 23:27:26]

Patchset #7 (id:120001) has been deleted

[boliu, 2015-11-24 23:27:38]

rebase + changes in two patch sets :)

https://codereview.chromium.org/1418273002/diff/80001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/80001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1755:
host_impl_.SetExternalViewporForTesting(viewport);
On 2015/11/24 21:17:30, danakj (behind on reviews) wrote:
> Why is this needed for this test?

Good question..

It's needed for the for the EXPECT_EQ(viewport,
pending_layer_->visible_rect_for_tile_priority()) a few lines below.

And I think this is somewhat expected. visible_rect_for_tile_priority is a bit
of a misnomer I think. It's not derived from
viewport/transform_for_tile_priority; rather it's the last visible_rect
calculated when draw constraints was valid for tile priority (ie not software
draw).

What a mouthful.. :|

Conceptually, it should be the visible rect calculated with
viewport/transform_for_tile_priority. But I guess a second CalcDrawProps is too
expensive?

https://codereview.chromium.org/1418273002/diff/80001/cc/test/fake_output_sur...
File cc/test/fake_output_surface.h (right):

https://codereview.chromium.org/1418273002/diff/80001/cc/test/fake_output_sur...
cc/test/fake_output_surface.h:57: static scoped_ptr<FakeOutputSurface>
Create3DSoftware() {
On 2015/11/24 21:17:30, danakj (behind on reviews) wrote:
> "3d"
> 
> Create3dWithResourcelessSoftwareSupport?

That works :)

> or something a bit more explainy?
> 
> I don't want people using this by default

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1231: void
LayerTreeHostImpl::SetExternalViewporForTesting(
On 2015/11/24 21:17:31, danakj (behind on reviews) wrote:
> Why aren't callers of this just using SetExternalDrawConstraints?

external_viewport_ is set in OnDraw now, so this is needed for tests that calls
CalcDrawProps directly

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1459: SetFullRootLayerDamage();
On 2015/11/24 21:17:31, danakj (behind on reviews) wrote:
> Can you explain why this and SetNeedsRedraw() still belong in here? Maybe with
a
> comment too?

Essentially this replaces SetNeedsRedrawRect from SyncOutputSurface.

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1529: // always swap. Otherwise, need to set
redraw for all changes to draw
On 2015/11/24 21:17:30, danakj (behind on reviews) wrote:
> all changes => any changes

Done.

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:301: void SetExternalViewporForTesting(const
gfx::Rect& external_viewport);
On 2015/11/24 21:17:31, danakj (behind on reviews) wrote:
> "Viewport"

Done.

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.h:348: void SetExternalDrawConstraints(
On 2015/11/24 21:17:31, danakj (behind on reviews) wrote:
> Now more like SetExternalTilePriority...Constraints?

Done

Also dropped the _for_tile_priority part from param names

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:162: last_on_draw_frame_ =
frame.Pass();
On 2015/11/24 21:17:31, danakj (behind on reviews) wrote:
> std::move (include <utility> too)

Done \o/

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:5737: SetResourcelessSoftwareDraw();
On 2015/11/24 21:17:31, danakj (behind on reviews) wrote:
> These tests that set resourceless software draw on get weird. Now we're going
to
> do things like CreatePendingTree with resourceless_software_draw_ enabled,
which
> will never happen in production cuz it's only true inside OnDraw.
> 
> I'd like to delete the SetResourcelessSoftwareDrawForTesting if possible.. any
> thoughts how we could do that?

Err, these tests are weird. They are explicitly drawing while active tree
viewport is invalid, which also can't happen in production either, except with
resourceless software draws.

The software draw is needed only so that the tests can ignore the CanDraw checks
and actually swap.

I added a comment and moved the SetResourcelessSoftwareDrawForTesting lower in
the test. Is that good enough?

https://codereview.chromium.org/1418273002/diff/80001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/1418273002/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:148:
InvokeComposite(transform, viewport, clip, false);
On 2015/11/24 21:17:31, danakj (behind on reviews) wrote:
> nit: use a temp var dont pass bool literals to functions

Done.

https://codereview.chromium.org/1418273002/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:170:
InvokeComposite(transform, clip, clip, true);
On 2015/11/24 21:17:31, danakj (behind on reviews) wrote:
> ditto

Done.

[boliu, 2015-12-07 23:17:31]

back from vacation, and just rebased this patch

ptal?

[boliu, 2015-12-10 16:46:00]

On 2015/12/07 23:17:31, boliu wrote:
> back from vacation, and just rebased this patch
> 
> ptal?

gentle ping?

[danakj, 2015-12-10 22:33:54]

https://codereview.chromium.org/1418273002/diff/80001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/80001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1755:
host_impl_.SetExternalViewporForTesting(viewport);
There's a typo "SetExternalViewporForTesting" -> Viewport

On 2015/11/24 23:27:37, boliu wrote:
> On 2015/11/24 21:17:30, danakj (behind on reviews) wrote:
> > Why is this needed for this test?
> 
> Good question..
> 
> It's needed for the for the EXPECT_EQ(viewport,
> pending_layer_->visible_rect_for_tile_priority()) a few lines below.
> 
> And I think this is somewhat expected. visible_rect_for_tile_priority is a bit
> of a misnomer I think. It's not derived from
> viewport/transform_for_tile_priority; rather it's the last visible_rect
> calculated when draw constraints was valid for tile priority (ie not software
> draw).
> 
> What a mouthful.. :|
> 
> Conceptually, it should be the visible rect calculated with
> viewport/transform_for_tile_priority. But I guess a second CalcDrawProps is
too
> expensive?

So... at least an explanation like this should live as a comment. I'm trying to
parse/apply it all myself..

This sets the external_viewport_ which changes the LTHI::DeviceViewport() which
is used in various places.

The EXPECT below is verifying that DeviceViewport is used for the visible rect
when computing visible_content_rect?

We get a tiling with 4 tiles, two in the DeviceViewport and 2 outside. We check
that the tiles inside the DeviceViewport are required for activation and the
ones outside are not?

So I am not sure that this test is the right place to be testing that setting
external_viewport changes the viewport used for the layer's visible rect.
Testing that it good, but this test is checking requires for activation stuff.
This test could just use SetViewport?

I'd rather a separate test that just does OnDraw and checks visible rects.

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1231: void
LayerTreeHostImpl::SetExternalViewporForTesting(
On 2015/11/24 23:27:38, boliu wrote:
> On 2015/11/24 21:17:31, danakj (behind on reviews) wrote:
> > Why aren't callers of this just using SetExternalDrawConstraints?
> 
> external_viewport_ is set in OnDraw now, so this is needed for tests that
calls
> CalcDrawProps directly

If you call CalcDrawProps you can pass whatever viewport you want, no?

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:5737: SetResourcelessSoftwareDraw();
On 2015/11/24 23:27:38, boliu wrote:
> On 2015/11/24 21:17:31, danakj (behind on reviews) wrote:
> > These tests that set resourceless software draw on get weird. Now we're
going
> to
> > do things like CreatePendingTree with resourceless_software_draw_ enabled,
> which
> > will never happen in production cuz it's only true inside OnDraw.
> > 
> > I'd like to delete the SetResourcelessSoftwareDrawForTesting if possible..
any
> > thoughts how we could do that?
> 
> Err, these tests are weird. They are explicitly drawing while active tree
> viewport is invalid, which also can't happen in production either, except with
> resourceless software draws.
> 
> The software draw is needed only so that the tests can ignore the CanDraw
checks
> and actually swap.
> 
> I added a comment and moved the SetResourcelessSoftwareDrawForTesting lower in
> the test. Is that good enough?

Can these tests call OnDraw instead?

https://codereview.chromium.org/1418273002/diff/180001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/1418273002/diff/180001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.h:305: void SetExternalViewportForTesting(const
gfx::Rect& external_viewport);
I really like all the changes in this patch except the ForTesting methods. They
leave me feeling like the tests that rely on them aren't testing the actual path
that does these things, so what is the point? And the actual paths are missing
that coverage.

I'd really like if we refactored tests to not need these ForTesting methods.
Sorry for being slow, I don't like leaving reviews that are just like *skeptical
face* without a counter proposal of some sort.

This CL does make prod a much better place tho!

https://codereview.chromium.org/1418273002/diff/180001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/180001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:5960: const bool
resourceless_software_draw = true;
if you wanted.. you could just pass |true| directly here now, there's only 1
variable and the name of the method says what it is now

[brianderson, 2015-12-14 20:51:26]

Scheduler changes lgtm.

https://codereview.chromium.org/1418273002/diff/180001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1418273002/diff/180001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1470: const gfx::Transform& transform) {
Would it make sense to DCHECK(using_synchronous_renderer_compositor) here? (And
in LayerTreeHostImpl::OnDraw.)

https://codereview.chromium.org/1418273002/diff/180001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1582: if (resourceless_software_draw) {
I think it would be easier to understand the flow of this method if there was a
higher level if(resourceless_software_draw) {} else {} block. wdyt?

[boliu, 2015-12-15 23:46:45]

Spent afternoon trying to remove the ForTesting methods. Got rid of a few more
uses, but both still remain, with some questions.

Didn't address any other comments.

https://codereview.chromium.org/1418273002/diff/200001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/200001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:1821:
host_impl_.SetExternalViewportForTesting(viewport);
Removed this one. Can check the computed rect instead, which comes from the
viewport for tile properties, not the draw viewport.

https://codereview.chromium.org/1418273002/diff/220001/cc/test/layer_tree_pix...
File cc/test/layer_tree_pixel_test.cc (right):

https://codereview.chromium.org/1418273002/diff/220001/cc/test/layer_tree_pix...
cc/test/layer_tree_pixel_test.cc:77:
impl->SetExternalViewportForTesting(viewport);
This one I'm confused. Not sure where this magic translate(20,10) comes from or
why it's needed.

Removing this only breaks two pixel tests:
ImageScaledBackgroundFilter.ImageFilterScaled_GL
LayerTreeHostFiltersPixelTest.BackgroundFilterBlurOffAxis

Resulting image in both tests differs only by 11 pixels. They visually look the
same, so it's not a shift of the whole image or anything. Should I just
rebaseline these and remove this?

https://codereview.chromium.org/1418273002/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:476: TEST_F(LayerTreeHostImplTest,
ResourcelessDrawWithEmptyViewport) {
Rewrote this test. Previous one was basically testing that resourceless draws
turns off the empty viewport CanDraw check. Rewrote to just do a resourceless
OnDraw and make sure the resulting frame is valid.

https://codereview.chromium.org/1418273002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:6014: SetResourcelessSoftwareDraw(); 
// Needed to draw while viewport invalid.
This and the test below are the only ones that need
SetResourcelessSoftwareDrawForTesting, again, to skip the CanDraw checks due to
invalid viewport.

This requires checking the render pass list immediate after PrepareToDraw, which
is kind of hard since OnDraw is a single call that can't be split up. Best thing
I got like set a lambda to be called by OnDrawForOutputSurface, which is kind of
ugly, and probably involves duplicating a bunch of test code. Thoughts?

[boliu, 2015-12-16 00:00:50]

https://codereview.chromium.org/1418273002/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:6014: SetResourcelessSoftwareDraw(); 
// Needed to draw while viewport invalid.
On 2015/12/15 23:46:45, boliu wrote:
> This and the test below are the only ones that need
> SetResourcelessSoftwareDrawForTesting, again, to skip the CanDraw checks due
to
> invalid viewport.
> 
> This requires checking the render pass list immediate after PrepareToDraw,
which
> is kind of hard since OnDraw is a single call that can't be split up. Best
thing
> I got like set a lambda to be called by OnDrawForOutputSurface, which is kind
of
> ugly, and probably involves duplicating a bunch of test code. Thoughts?

Just found RenderPass::CopyAll, maybe it's easier to just save the last
RenderPassList instead.

[boliu, 2015-12-16 01:17:47]

PTAL

Managed to get rid of SetResourcelessSoftwareDrawForTesting \o/

SetExternalViewportForTesting remains due to my question on the pixel tests.

https://codereview.chromium.org/1418273002/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:6014: SetResourcelessSoftwareDraw(); 
// Needed to draw while viewport invalid.
On 2015/12/16 00:00:50, boliu wrote:
> On 2015/12/15 23:46:45, boliu wrote:
> > This and the test below are the only ones that need
> > SetResourcelessSoftwareDrawForTesting, again, to skip the CanDraw checks due
> to
> > invalid viewport.
> > 
> > This requires checking the render pass list immediate after PrepareToDraw,
> which
> > is kind of hard since OnDraw is a single call that can't be split up. Best
> thing
> > I got like set a lambda to be called by OnDrawForOutputSurface, which is
kind
> of
> > ugly, and probably involves duplicating a bunch of test code. Thoughts?
> 
> Just found RenderPass::CopyAll, maybe it's easier to just save the last
> RenderPassList instead.

Ok, did that. Not too bad.

https://codereview.chromium.org/1418273002/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:2491:
host_impl->SetExternalTilePriorityConstraints(empty_rect, identity);
Also rewrote this test to not need this, but actually do draws with through
Invalidate/OnDraw.

[boliu, 2015-12-16 19:06:42]

Rebaselined pixel tests. Weirdly one more pixel test failing this morning after
rebase.

Looked into Brian's comments, but chose not to do anything on both counts.

PTAL

https://codereview.chromium.org/1418273002/diff/180001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1418273002/diff/180001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1470: const gfx::Transform& transform) {
On 2015/12/14 20:51:26, brianderson wrote:
> Would it make sense to DCHECK(using_synchronous_renderer_compositor) here?
(And
> in LayerTreeHostImpl::OnDraw.)

This works in production, and so probably good idea long term. But this is
causing a bunch of tests to fail. In theory these don't require
using_synchronous_renderer_compositor, since a bunch of tests has its own
LTHIClient implementations. This DCHECK already exists for
Scheduler::OnDrawForOutputSurface so at least tests using real scheduler is ok.
This patch is already pretty big as it is, so didn't change this.

https://codereview.chromium.org/1418273002/diff/180001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1582: if (resourceless_software_draw) {
On 2015/12/14 20:51:26, brianderson wrote:
> I think it would be easier to understand the flow of this method if there was
a
> higher level if(resourceless_software_draw) {} else {} block. wdyt?

Tried this, result below in this comment. Really dislike duplicating a few
lines, so didn't go with this.

void LayerTreeHostImpl::OnDraw(const gfx::Transform& transform,
                               const gfx::Rect& viewport,
                               const gfx::Rect& clip,
                               bool resourceless_software_draw) {
  DCHECK(!resourceless_software_draw_);
  const bool transform_changed = external_transform_ != transform;
  const bool viewport_changed = external_viewport_ != viewport;
  const bool clip_changed = external_clip_ != clip;

  external_transform_ = transform;
  external_viewport_ = viewport;
  external_clip_ = clip;

  if (!resourceless_software_draw) {
    // Set damage viewport if draw parameters changed.
    if (transform_changed || viewport_changed || clip_changed) {
      SetFullRootLayerDamage();
      SetNeedsRedraw();
    }

    // UpdateDrawProperties does not depend on clip.
    if (transform_changed || viewport_changed) {
      active_tree_->set_needs_update_draw_properties();
    }

    client_->OnDrawForOutputSurface(resourceless_software_draw_);
    return;
  }

  {
    base::AutoReset<bool> resourceless_software_draw_reset(
        &resourceless_software_draw_, resourceless_software_draw);

    // For resourceless software draw, always set full damage to ensure draw
    // always swap.
    SetFullRootLayerDamage();
    SetNeedsRedraw();
    active_tree_->set_needs_update_draw_properties();
    client_->OnCanDrawStateChanged(CanDraw());

    client_->OnDrawForOutputSurface(resourceless_software_draw_);
  }

  active_tree_->set_needs_update_draw_properties();
  client_->OnCanDrawStateChanged(CanDraw());
  // This draw may have reset all damage, which would lead to subsequent
  // incorrect hardware draw, so explicitly set damage for next hardware
  // draw as well.
  SetFullRootLayerDamage();
}

[brianderson, 2015-12-16 22:49:25]

Ok. Still lgtm.

[danakj, 2015-12-16 23:57:53]

LGTM

https://codereview.chromium.org/1418273002/diff/220001/cc/test/layer_tree_pix...
File cc/test/layer_tree_pixel_test.cc (right):

https://codereview.chromium.org/1418273002/diff/220001/cc/test/layer_tree_pix...
cc/test/layer_tree_pixel_test.cc:77:
impl->SetExternalViewportForTesting(viewport);
On 2015/12/15 23:46:45, boliu wrote:
> This one I'm confused. Not sure where this magic translate(20,10) comes from
or
> why it's needed.
> 
> Removing this only breaks two pixel tests:
> ImageScaledBackgroundFilter.ImageFilterScaled_GL
> LayerTreeHostFiltersPixelTest.BackgroundFilterBlurOffAxis
> 
> Resulting image in both tests differs only by 11 pixels. They visually look
the
> same, so it's not a shift of the whole image or anything. Should I just
> rebaseline these and remove this?

Hrm, that is very weird. Pretty sure this was meant to move the contents of the
test outputs or something.. well, I think the right thing to test for a pixel
test wrt to this would be to call DrawFrame with a viewport rect of choice
directly with other pixel tests, and not for these layer tree tests, so ok.

https://codereview.chromium.org/1418273002/diff/280001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/280001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:6065: TestEmptyLayerWithOnDraw();
It would be nice to wrap each of these Test calls with EXPECT_SCOPED() to give
an easier backtrace on errors.

[boliu, 2015-12-17 00:14:07]

https://codereview.chromium.org/1418273002/diff/280001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/280001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:6065: TestEmptyLayerWithOnDraw();
On 2015/12/16 23:57:53, danakj (behind on reviews) wrote:
> It would be nice to wrap each of these Test calls with EXPECT_SCOPED() to give
> an easier backtrace on errors.

Done. Copied EXPECT_SCOPED define from scheduler_unittest.cc

[boliu, 2015-12-17 00:14:13]

The CQ bit was checked by boliu@chromium.org

[boliu, 2015-12-17 00:14:13]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/1418273002/#ps300001
(title: "EXPECT_SCOPED")

[danakj, 2015-12-17 00:31:24]

https://codereview.chromium.org/1418273002/diff/280001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1418273002/diff/280001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:6065: TestEmptyLayerWithOnDraw();
On 2015/12/17 00:14:07, boliu wrote:
> On 2015/12/16 23:57:53, danakj (behind on reviews) wrote:
> > It would be nice to wrap each of these Test calls with EXPECT_SCOPED() to
give
> > an easier backtrace on errors.
> 
> Done. Copied EXPECT_SCOPED define from scheduler_unittest.cc

Oh I thot it was a general gtest thing. Well, cool. :)

[commit-bot: I haz the power, 2015-12-17 00:33:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418273002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418273002/300001

[commit-bot: I haz the power, 2015-12-17 03:16:16]

Description was changed from

==========
cc: Move draw params from SetExternalDrawConstraints to OnDraw

This is a clean up to move some of the logic spread between
LayerTreeHostImpl and SynchronousCompositorOutputSurface about setting
and saving parameters for synchronous compositor draws.

Move draw parameters to OnDraw. These will be reset before OnDraw
returns. Parameters are overridden permanently is set in
SetExternalDrawConstraints.

BUG=419795
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Move draw params from SetExternalDrawConstraints to OnDraw

This is a clean up to move some of the logic spread between
LayerTreeHostImpl and SynchronousCompositorOutputSurface about setting
and saving parameters for synchronous compositor draws.

Move draw parameters to OnDraw. These will be reset before OnDraw
returns. Parameters are overridden permanently is set in
SetExternalDrawConstraints.

BUG=419795
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2015-12-17 03:16:17]

Committed patchset #15 (id:300001)

[commit-bot: I haz the power, 2015-12-17 03:17:29]

Description was changed from

==========
cc: Move draw params from SetExternalDrawConstraints to OnDraw

This is a clean up to move some of the logic spread between
LayerTreeHostImpl and SynchronousCompositorOutputSurface about setting
and saving parameters for synchronous compositor draws.

Move draw parameters to OnDraw. These will be reset before OnDraw
returns. Parameters are overridden permanently is set in
SetExternalDrawConstraints.

BUG=419795
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Move draw params from SetExternalDrawConstraints to OnDraw

This is a clean up to move some of the logic spread between
LayerTreeHostImpl and SynchronousCompositorOutputSurface about setting
and saving parameters for synchronous compositor draws.

Move draw parameters to OnDraw. These will be reset before OnDraw
returns. Parameters are overridden permanently is set in
SetExternalDrawConstraints.

BUG=419795
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7097ee5b1f3691d12f425d719d33698bb90261b6
Cr-Commit-Position: refs/heads/master@{#365700}
==========

[commit-bot: I haz the power, 2015-12-17 03:17:30]

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