sync compositor: Set display viewport for software draw

sync compositor: Set display viewport for software draw

After switching to using cc::Display for software draw, this fixes the
case where the viewport is not at the origin of the surface.

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

Committed: https://crrev.com/4dae52297b5786ca794317adf11ca39468ee47a6
Cr-Commit-Position: refs/heads/master@{#408288}

[boliu, 2016-07-27 21:15:36]

Description was changed from

==========
sync compositor: Set display viewport for software draw

After switching to using cc::Display for software draw, this fixes the
case where the viewport is not at the origin of the surface.

BUG=631276
==========

to

==========
sync compositor: Set display viewport for software draw

After switching to using cc::Display for software draw, this fixes the
case where the viewport is not at the origin of the surface.

BUG=631276
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[boliu, 2016-07-27 21:24:21]

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

[boliu, 2016-07-27 21:24:22]

ptal, somewhat more involved than I thought, need to plumb both the correct
surface size (which is not necessarily the frame size), and the correct viewport

I was curious why hardware_renderer doesn't need SetExternalViewport, turns out
that's handled by having another root surface, which handles both the
"viewport", and applying the transform. software draws don't have a matrix to
apply, but could use another surface to apply the viewport as well I guess, but
seems overkill.

After this one lands, I want to explore removing viewport and clip from
LTHI::OnDraw.

[danakj, 2016-07-27 21:28:36]

https://codereview.chromium.org/2187563006/diff/1/content/renderer/android/sy...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2187563006/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_output_surface.cc:338:
surface_size_ = gfx::Size(canvas->getBaseLayerSize().width(),
Ahh. Does it need to be a member on SCOS still too or can it be a local var
here?

https://codereview.chromium.org/2187563006/diff/1/content/renderer/android/sy...
File content/renderer/android/synchronous_compositor_output_surface.h (right):

https://codereview.chromium.org/2187563006/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_output_surface.h:162:
SoftwareOutputSurface* software_output_surface_;  // Owned by |display_|.
= nullptr here instead of in constructor?

[danakj, 2016-07-27 21:28:41]

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

[commit-bot: I haz the power, 2016-07-27 21:28:53]

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-07-27 21:28:54]

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

[commit-bot: I haz the power, 2016-07-27 21:28:55]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[boliu, 2016-07-27 21:43:04]

https://codereview.chromium.org/2187563006/diff/1/content/renderer/android/sy...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2187563006/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_output_surface.cc:338:
surface_size_ = gfx::Size(canvas->getBaseLayerSize().width(),
On 2016/07/27 21:28:36, danakj wrote:
> Ahh. Does it need to be a member on SCOS still too or can it be a local var
> here?

Hmm... in practice, no, because DelegatingRenderer doesn't use this at all, so
don't need to set it in DemandDrawHw either.

But if only remove it here, then it makes no sense for SCOS to have the surface
size from hardware draw, even if it's not used.

Long term, it would be good to like have cc::Display use a separate
OutputSurface type, and only have all this resize logic on that type.

In this CL? Umm, keep it just to be consistent with everything else?

https://codereview.chromium.org/2187563006/diff/1/content/renderer/android/sy...
File content/renderer/android/synchronous_compositor_output_surface.h (right):

https://codereview.chromium.org/2187563006/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_output_surface.h:162:
SoftwareOutputSurface* software_output_surface_;  // Owned by |display_|.
On 2016/07/27 21:28:36, danakj wrote:
> = nullptr here instead of in constructor?

Done.

[danakj, 2016-07-27 21:49:53]

LGTM

https://codereview.chromium.org/2187563006/diff/1/content/renderer/android/sy...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2187563006/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_output_surface.cc:338:
surface_size_ = gfx::Size(canvas->getBaseLayerSize().width(),
On 2016/07/27 21:43:04, boliu wrote:
> On 2016/07/27 21:28:36, danakj wrote:
> > Ahh. Does it need to be a member on SCOS still too or can it be a local var
> > here?
> 
> Hmm... in practice, no, because DelegatingRenderer doesn't use this at all, so
> don't need to set it in DemandDrawHw either.
> 
> But if only remove it here, then it makes no sense for SCOS to have the
surface
> size from hardware draw, even if it's not used.
> 
> Long term, it would be good to like have cc::Display use a separate
> OutputSurface type, and only have all this resize logic on that type.

I'm working toward that right now.

> In this CL? Umm, keep it just to be consistent with everything else?

If you make it different now with comments it'll make it easier for me to split
the two APIs apart and understand what needs to be on the LTHI's API vs the
Display's API. Or at least put some TODOs/comments explaining. But I'd prefer to
not set things that won't be used.

[boliu, 2016-07-27 22:00:51]

https://codereview.chromium.org/2187563006/diff/1/content/renderer/android/sy...
File content/renderer/android/synchronous_compositor_output_surface.cc (right):

https://codereview.chromium.org/2187563006/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_output_surface.cc:338:
surface_size_ = gfx::Size(canvas->getBaseLayerSize().width(),
On 2016/07/27 21:49:53, danakj wrote:
> On 2016/07/27 21:43:04, boliu wrote:
> > On 2016/07/27 21:28:36, danakj wrote:
> > > Ahh. Does it need to be a member on SCOS still too or can it be a local
var
> > > here?
> > 
> > Hmm... in practice, no, because DelegatingRenderer doesn't use this at all,
so
> > don't need to set it in DemandDrawHw either.
> > 
> > But if only remove it here, then it makes no sense for SCOS to have the
> surface
> > size from hardware draw, even if it's not used.
> > 
> > Long term, it would be good to like have cc::Display use a separate
> > OutputSurface type, and only have all this resize logic on that type.
> 
> I'm working toward that right now.
> 
> > In this CL? Umm, keep it just to be consistent with everything else?
> 
> If you make it different now with comments it'll make it easier for me to
split
> the two APIs apart and understand what needs to be on the LTHI's API vs the
> Display's API. Or at least put some TODOs/comments explaining. But I'd prefer
to
> not set things that won't be used.

Ok, not setting SCOS::surface_size_ anywhere, and it will remain empty. That's
consistent too :p

[boliu, 2016-07-27 22:15:02]

The CQ bit was checked by boliu@chromium.org

[boliu, 2016-07-27 22:15:03]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2187563006/#ps40001
(title: "don't set surface_size_")

[commit-bot: I haz the power, 2016-07-27 22:15:24]

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-07-27 22:15:25]

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

[commit-bot: I haz the power, 2016-07-27 22:15:26]

Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[danakj, 2016-07-27 22:18:04]

Nice, LGTM

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

https://codereview.chromium.org/2187563006/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:339: gfx::Size
surface_size(canvas->getBaseLayerSize().width(),
nit: You could do
s_o_s_->SetSurfaceSize(gfx::SkISizeToSize(canvas->getBaseLayerSize()));

[boliu, 2016-07-27 22:22:50]

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

https://codereview.chromium.org/2187563006/diff/40001/content/renderer/androi...
content/renderer/android/synchronous_compositor_output_surface.cc:339: gfx::Size
surface_size(canvas->getBaseLayerSize().width(),
On 2016/07/27 22:18:03, danakj wrote:
> nit: You could do
> s_o_s_->SetSurfaceSize(gfx::SkISizeToSize(canvas->getBaseLayerSize()));

Done.

[boliu, 2016-07-27 22:29:40]

The CQ bit was checked by boliu@chromium.org

[boliu, 2016-07-27 22:29:40]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2187563006/#ps60001
(title: "SkISizeToSize")

[commit-bot: I haz the power, 2016-07-27 22:30:07]

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-07-27 22:30:08]

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

[commit-bot: I haz the power, 2016-07-27 22:30:10]

Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[boliu, 2016-07-27 22:31:22]

On 2016/07/27 22:30:10, commit-bot: I haz the power wrote:
> Your CL was about to rely on recently removed CQ feature(s):
>  * master.tryserver.blink builder linux_blink_rel was renamed to
> linux_precise_blink_rel (http://crbug.com/590036)

Hmm, I think cq is actually rejecting this due to ^^^, should fix the presubmit.
Or maybe I just need to sync

[boliu, 2016-07-27 22:31:37]

Description was changed from

==========
sync compositor: Set display viewport for software draw

After switching to using cc::Display for software draw, this fixes the
case where the viewport is not at the origin of the surface.

BUG=631276
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
sync compositor: Set display viewport for software draw

After switching to using cc::Display for software draw, this fixes the
case where the viewport is not at the origin of the surface.

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

[boliu, 2016-07-27 22:31:47]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2016-07-27 22:32:04]

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

[danakj, 2016-07-27 22:36:57]

On 2016/07/27 22:31:22, boliu wrote:
> On 2016/07/27 22:30:10, commit-bot: I haz the power wrote:
> > Your CL was about to rely on recently removed CQ feature(s):
> >  * master.tryserver.blink builder linux_blink_rel was renamed to
> > linux_precise_blink_rel (http://crbug.com/590036)
> 
> Hmm, I think cq is actually rejecting this due to ^^^, should fix the
presubmit.
> Or maybe I just need to sync

Codesearch doesn't show it yet, but I think new patches should work:
https://codereview.chromium.org/2183323002/diff/1/cc/PRESUBMIT.py

[commit-bot: I haz the power, 2016-07-28 00:10:42]

Description was changed from

==========
sync compositor: Set display viewport for software draw

After switching to using cc::Display for software draw, this fixes the
case where the viewport is not at the origin of the surface.

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

to

==========
sync compositor: Set display viewport for software draw

After switching to using cc::Display for software draw, this fixes the
case where the viewport is not at the origin of the surface.

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

[commit-bot: I haz the power, 2016-07-28 00:10:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-28 00:14:09]

Description was changed from

==========
sync compositor: Set display viewport for software draw

After switching to using cc::Display for software draw, this fixes the
case where the viewport is not at the origin of the surface.

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

to

==========
sync compositor: Set display viewport for software draw

After switching to using cc::Display for software draw, this fixes the
case where the viewport is not at the origin of the surface.

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

Committed: https://crrev.com/4dae52297b5786ca794317adf11ca39468ee47a6
Cr-Commit-Position: refs/heads/master@{#408288}
==========

[commit-bot: I haz the power, 2016-07-28 00:14:10]

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