exo: Select cursor capture scale at run time

exo: Select cursor capture scale at run time

On low-DPI displays, the pointer surface was being scaled up using
bilinear filtering, resulting in a slightly blurry cursor. This CL
sets the capture scale to the largest DSF instead of a constant.

BUG=730843
TEST=minnie,caroline: Cursor is sharp on all displays.

Review-Url: https://codereview.chromium.org/2933133002
Cr-Commit-Position: refs/heads/master@{#481738}
Committed: https://chromium.googlesource.com/chromium/src/+/18ec3b79f56fe38260ada3658872ffacdb652f22

[Dominik Laskowski, 2017-06-12 15:53:21]

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

[commit-bot: I haz the power, 2017-06-12 15:53:45]

Dry run: CQ is trying da patch.

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

[Dominik Laskowski, 2017-06-12 16:00:18]

domlaskowski@chromium.org changed reviewers:
+ reveman@chromium.org

[Dominik Laskowski, 2017-06-12 16:00:19]

PTAL.

[reveman, 2017-06-12 16:08:33]

Can we keep this simpler and just modify the existing code to use the
highest-DPI display scale at the time of capture instead 2.0 but not worry about
it changing. Do we have critical use-cases that this wouldn't solve?

[Dominik Laskowski, 2017-06-12 16:33:36]

Do you mean replacing |capture_display_id_| with a stateless GetCaptureDisplay
function? I don't think that would simplify much, e.g. we would still need to
reparent the pointer surface on display reconfiguration.

[commit-bot: I haz the power, 2017-06-12 16:38:28]

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

[commit-bot: I haz the power, 2017-06-12 16:38:29]

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

[reveman, 2017-06-12 18:30:07]

On 2017/06/12 at 16:33:36, domlaskowski wrote:
> Do you mean replacing |capture_display_id_| with a stateless GetCaptureDisplay
function? I don't think that would simplify much, e.g. we would still need to
reparent the pointer surface on display reconfiguration.

No. I mean a simple change to Pointer::CaptureCursor that just iterate over all
displays to find a suitable scale instead of using kCursorCaptureScale.

[Dominik Laskowski, 2017-06-12 19:47:00]

Right, that's what GetCaptureDisplay would do, because that logic is needed in
several places, i.e. OnDisplayConfigurationChanged and
{Set,Capture,Update}Cursor. The pointer surface must be parented to the correct
display for the scale to make sense.

[Dominik Laskowski, 2017-06-12 19:54:17]

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

[commit-bot: I haz the power, 2017-06-12 19:55:02]

Dry run: CQ is trying da patch.

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

[reveman, 2017-06-12 19:55:57]

On 2017/06/12 at 19:47:00, domlaskowski wrote:
> Right, that's what GetCaptureDisplay would do, because that logic is needed in
several places, i.e. OnDisplayConfigurationChanged and
{Set,Capture,Update}Cursor. The pointer surface must be parented to the correct
display for the scale to make sense.

I'm suggesting that we don't handle all those cases. I'm suggesting that we just
make it a bit smarter than now by taking the current set of displays into
account whenever we do a capture and don't worry about display configuration
changes.

I'm failing to see why the pointer surface needs to be reparented. We're not
doing that now but we're still able to capture with a specific scale all the
time.

[Dominik Laskowski, 2017-06-12 20:05:16]

On 2017/06/12 19:55:57, reveman wrote:
> I'm failing to see why the pointer surface needs to be reparented. We're not
> doing that now but we're still able to capture with a specific scale all the
> time.

The client incorrectly forced a surface change on pointer enter. The previous
code relied on that to keep the surface parented to the primary display.

[reveman, 2017-06-12 20:08:22]

On 2017/06/12 at 20:05:16, domlaskowski wrote:
> On 2017/06/12 19:55:57, reveman wrote:
> > I'm failing to see why the pointer surface needs to be reparented. We're not
> > doing that now but we're still able to capture with a specific scale all the
> > time.
> 
> The client incorrectly forced a surface change on pointer enter. The previous
code relied on that to keep the surface parented to the primary display.

I'm not following. Please explain.

[Dominik Laskowski, 2017-06-12 20:18:09]

The client called wl_pointer::set_cursor twice on pointer enter, i.e. resetting
then restoring the cursor. The second call would reparent the surface to the
latest primary display, ensuring that the surface is always scaled relative to
the primary display.

[reveman, 2017-06-12 20:55:52]

On 2017/06/12 at 20:18:09, domlaskowski wrote:
> The client called wl_pointer::set_cursor twice on pointer enter, i.e.
resetting then restoring the cursor. The second call would reparent the surface
to the latest primary display, ensuring that the surface is always scaled
relative to the primary display.

Ok, can we start by fixing that scaling logic without any of these other changes
first?

[Dominik Laskowski, 2017-06-12 21:52:36]

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

[commit-bot: I haz the power, 2017-06-12 21:53:26]

Dry run: CQ is trying da patch.

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

[Dominik Laskowski, 2017-06-12 21:56:04]

On 2017/06/12 20:55:52, reveman wrote:
> Ok, can we start by fixing that scaling logic without any of these other
changes
> first?

Rebased on https://codereview.chromium.org/2934953002/

[commit-bot: I haz the power, 2017-06-12 22:31:58]

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

[commit-bot: I haz the power, 2017-06-12 22:31:58]

Dry run: This issue passed the CQ dry run.

[Dominik Laskowski, 2017-06-13 21:37:51]

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

[commit-bot: I haz the power, 2017-06-13 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...

[reveman, 2017-06-13 22:10:04]

Can we keep using the primary display for capture and just adjust the capture
scale?

[commit-bot: I haz the power, 2017-06-13 22:20:14]

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

[commit-bot: I haz the power, 2017-06-13 22:20:15]

Dry run: This issue passed the CQ dry run.

[Dominik Laskowski, 2017-06-13 22:24:02]

On 2017/06/13 22:10:04, reveman wrote:
> Can we keep using the primary display for capture and just adjust the capture
> scale?

No, because the cursor would be blurry with low-DPI primary and high-DPI
secondary displays.

[reveman, 2017-06-14 01:46:41]

On 2017/06/13 at 22:24:02, domlaskowski wrote:
> On 2017/06/13 22:10:04, reveman wrote:
> > Can we keep using the primary display for capture and just adjust the
capture
> > scale?
> 
> No, because the cursor would be blurry with low-DPI primary and high-DPI
secondary displays.

I'm failing to see why if we compensate for the difference in scale. E.g. if
secondary display is 2x then we capture with 2.0 scale on primary display that
is 1x.

[Dominik Laskowski, 2017-06-19 20:13:17]

On 2017/06/14 01:46:41, reveman wrote:
> I'm failing to see why if we compensate for the difference in scale. E.g. if
> secondary display is 2x then we capture with 2.0 scale on primary display that
> is 1x.

You're right, and it simplifies the code a bit. However, PTAL at the dependent
CL [1] first. That wrinkle means we would need |capture_scale_| and
|capture_ratio_|, as opposed to just |capture_display_id_|. What do you prefer?

[1] https://codereview.chromium.org/2944063002/

[reveman, 2017-06-19 20:22:06]

On 2017/06/19 at 20:13:17, domlaskowski wrote:
> On 2017/06/14 01:46:41, reveman wrote:
> > I'm failing to see why if we compensate for the difference in scale. E.g. if
> > secondary display is 2x then we capture with 2.0 scale on primary display
that
> > is 1x.
> 
> You're right, and it simplifies the code a bit. However, PTAL at the dependent
CL [1] first. That wrinkle means we would need |capture_scale_| and
|capture_ratio_|, as opposed to just |capture_display_id_|. What do you prefer?

capture_scale_/ratio_ sounds OK but please remained me why these members are
needed as part of the updated patch.

> 
> [1] https://codereview.chromium.org/2944063002/

[Dominik Laskowski, 2017-06-19 21:37:10]

Description was changed from

==========
exo: Capture cursor on highest-DPI display

When captured on a low-DPI display, the pointer surface was being scaled
up using bilinear filtering, resulting in a slightly blurry cursor. This
CL captures the cursor on the display with the largest DSF.

BUG=730843
TEST=minnie,caroline: Cursor is sharp on all displays.
==========

to

==========
exo: Select cursor capture scale at run time

On low-DPI displays, the pointer surface was being scaled up using
bilinear filtering, resulting in a slightly blurry cursor. This CL
sets the capture scale to the largest DSF instead of a constant.

BUG=730843
TEST=minnie,caroline: Cursor is sharp on all displays.
==========

[Dominik Laskowski, 2017-06-19 21:37:35]

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

[commit-bot: I haz the power, 2017-06-19 21:38:04]

Dry run: CQ is trying da patch.

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

[Dominik Laskowski, 2017-06-19 21:40:43]

PTAL. Will rebase dependent CL shortly, and justify |capture_ratio_| there.

[commit-bot: I haz the power, 2017-06-19 22:41:42]

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

[commit-bot: I haz the power, 2017-06-19 22:41:43]

Dry run: This issue passed the CQ dry run.

[Dominik Laskowski, 2017-06-19 23:02:10]

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

[commit-bot: I haz the power, 2017-06-19 23:02:46]

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-06-20 00:32:16]

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

[commit-bot: I haz the power, 2017-06-20 00:32:17]

Dry run: This issue passed the CQ dry run.

[Dominik Laskowski, 2017-06-22 21:06:48]

Ping.

[reveman, 2017-06-22 21:24:01]

lgtm with nits

https://codereview.chromium.org/2933133002/diff/120001/components/exo/pointer.cc
File components/exo/pointer.cc (right):

https://codereview.chromium.org/2933133002/diff/120001/components/exo/pointer...
components/exo/pointer.cc:7: #include <algorithm>
nit: remove if not needed

https://codereview.chromium.org/2933133002/diff/120001/components/exo/pointer...
components/exo/pointer.cc:80: OnDisplayConfigurationChanged();
nit: don't call this here. see comment below

https://codereview.chromium.org/2933133002/diff/120001/components/exo/pointer...
components/exo/pointer.cc:277: }
nit: please move this logic into a GetCaptureScale() helper function in
anonymous namespace so you can do capture_scale_(GetCaptureScale()) in ctor
initialization list instead of calling OnDisplayConfigurationChanged from there

[Dominik Laskowski, 2017-06-22 22:28:54]

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

[commit-bot: I haz the power, 2017-06-22 22:29:17]

Dry run: CQ is trying da patch.

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

[Dominik Laskowski, 2017-06-22 22:31:14]

https://codereview.chromium.org/2933133002/diff/120001/components/exo/pointer.cc
File components/exo/pointer.cc (right):

https://codereview.chromium.org/2933133002/diff/120001/components/exo/pointer...
components/exo/pointer.cc:7: #include <algorithm>
On 2017/06/22 21:24:01, reveman wrote:
> nit: remove if not needed

Removed along with std::max.

https://codereview.chromium.org/2933133002/diff/120001/components/exo/pointer...
components/exo/pointer.cc:80: OnDisplayConfigurationChanged();
On 2017/06/22 21:24:01, reveman wrote:
> nit: don't call this here. see comment below

Done.

https://codereview.chromium.org/2933133002/diff/120001/components/exo/pointer...
components/exo/pointer.cc:277: }
On 2017/06/22 21:24:01, reveman wrote:
> nit: please move this logic into a GetCaptureScale() helper function in
> anonymous namespace so you can do capture_scale_(GetCaptureScale()) in ctor
> initialization list instead of calling OnDisplayConfigurationChanged from
there

Done.

[commit-bot: I haz the power, 2017-06-22 23:03:24]

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

[commit-bot: I haz the power, 2017-06-22 23:03:25]

Dry run: This issue passed the CQ dry run.

[Dominik Laskowski, 2017-06-23 00:15:12]

The CQ bit was checked by domlaskowski@chromium.org

[Dominik Laskowski, 2017-06-23 00:15:13]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2933133002/#ps140001
(title: "Address nits")

[commit-bot: I haz the power, 2017-06-23 00:15:31]

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-06-23 00:20:39]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498176912814230,
"parent_rev": "938998bd58a2fabb04556833a298f3c7facd5135", "commit_rev":
"18ec3b79f56fe38260ada3658872ffacdb652f22"}

[commit-bot: I haz the power, 2017-06-23 00:20:54]

Description was changed from

==========
exo: Select cursor capture scale at run time

On low-DPI displays, the pointer surface was being scaled up using
bilinear filtering, resulting in a slightly blurry cursor. This CL
sets the capture scale to the largest DSF instead of a constant.

BUG=730843
TEST=minnie,caroline: Cursor is sharp on all displays.
==========

to

==========
exo: Select cursor capture scale at run time

On low-DPI displays, the pointer surface was being scaled up using
bilinear filtering, resulting in a slightly blurry cursor. This CL
sets the capture scale to the largest DSF instead of a constant.

BUG=730843
TEST=minnie,caroline: Cursor is sharp on all displays.

Review-Url: https://codereview.chromium.org/2933133002
Cr-Commit-Position: refs/heads/master@{#481738}
Committed:
https://chromium.googlesource.com/chromium/src/+/18ec3b79f56fe38260ada3658872...
==========

[commit-bot: I haz the power, 2017-06-23 00:20:57]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/18ec3b79f56fe38260ada3658872...