Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779823002:
exo: Send DSF of internal display to ARC (Closed)
|
Created:
3 years, 9 months ago by Dominik Laskowski Modified:
3 years, 6 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionexo: Send DSF of internal display to ARC
Since the client-side DPI in ARC is configured at build time based on
the internal display, the client must scale the cursor by the DSF of
the internal (not the primary) display to convert from pixels to DIPs.
BUG=642894
TEST=caroline: Cursor scale is correct for each display/resolution.
Review-Url: https://codereview.chromium.org/2779823002
Cr-Commit-Position: refs/heads/master@{#476423}
Committed: https://chromium.googlesource.com/chromium/src/+/9b40ae314fa3842121e01e231b3ea72d04c07ef2
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebase properly #
Total comments: 4
Patch Set 4 : Rebase and update comment #Patch Set 5 : Rebase #Patch Set 6 : Send is_internal flag instead #
Total comments: 2
Patch Set 7 : Rebase #Patch Set 8 : Document is_internal #
Messages
Total messages: 34 (12 generated)
The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
domlaskowski@chromium.org changed reviewers: + oshima@chromium.org, reveman@chromium.org
PTAL.
lgtm with a question https://codereview.chromium.org/2779823002/diff/40001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2779823002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2133: // TODO(domlaskowski): Send real DSFs once client selects DPI using DSF. I don't understand this TODO. Can you link to a bug or better explain what the plan is?
https://codereview.chromium.org/2779823002/diff/40001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2779823002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2139: .device_scale_factor(); This value may change (as primary display may change), but Android need to keep using the same value correct? Is it the above comment?
https://codereview.chromium.org/2779823002/diff/40001/components/exo/wayland/... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2779823002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2133: // TODO(domlaskowski): Send real DSFs once client selects DPI using DSF. On 2017/03/28 07:12:21, reveman wrote: > I don't understand this TODO. Can you link to a bug or better explain what the > plan is? Updated comment. https://codereview.chromium.org/2779823002/diff/40001/components/exo/wayland/... components/exo/wayland/server.cc:2139: .device_scale_factor(); On 2017/03/28 12:46:55, oshima wrote: > This value may change (as primary display may change), but Android need to keep > using the same value correct? Is it the above comment? On a device without an internal display, the cursor scale might be wrong if the primary DSF does not match the density configured at build time. The density should be selected at run time based on the primary DSF.
So this just seems to replace one workaround with another. Would it be better to add an is_internal argument to send_workspace event?
On 2017/03/30 08:52:15, reveman wrote: > So this just seems to replace one workaround with another. Would it be better to > add an is_internal argument to send_workspace event? I agree that this is sort of another workaround, although the edge case is much narrower. I had offline chat with Dominik, and I'm fine with this for now, (lbtm) but I'll leave the final decision to you.
On 2017/03/30 18:22:07, oshima wrote: > On 2017/03/30 08:52:15, reveman wrote: > > So this just seems to replace one workaround with another. Would it be better > to > > add an is_internal argument to send_workspace event? > > I agree that this is sort of another workaround, although the edge case is much > narrower. > I had offline chat with Dominik, and I'm fine with this for now, (lbtm) > but I'll leave the final decision to you. oops typo. lgtm
what's the benefit of replacing the old workaround with this workaround?
On 2017/03/30 18:36:09, reveman wrote: > what's the benefit of replacing the old workaround with this workaround? Correctness. The cursor size sent by the client should be in DIPs, so it should be scaled by the fixed internal DSF. Currently, it's scaled by the DSF of the primary display mode, which depends on the DPI and resolution of the primary display. Also, the new workaround combined with [1] fixes hotspot scaling, and is isolated to remote shell clients. [1] https://codereview.chromium.org/2780623002/ On 2017/03/30 08:52:15, reveman wrote: > Would it be better to add an is_internal argument to send_workspace event? I don't mind moving this to the client, but we'd be changing the protocol for the sake of a workaround, i.e. we don't otherwise need that information.
On 2017/03/30 at 21:14:26, domlaskowski wrote: > On 2017/03/30 18:36:09, reveman wrote: > > what's the benefit of replacing the old workaround with this workaround? > > Correctness. The cursor size sent by the client should be in DIPs, so it should be scaled by the fixed internal DSF. Currently, it's scaled by the DSF of the primary display mode, which depends on the DPI and resolution of the primary display. Also, the new workaround combined with [1] fixes hotspot scaling, and is isolated to remote shell clients. Sorry, I'm still failing to understand how you intend this to work. Cursor surface size is in DIPs and DIPs are defined by the DSF we send in wl_output events. We're still sending the primary dsf in wl_output events so that is still what the client is expected to take into account, arc++ or otherwise. Is cursor scale broken for all wayland clients or just arc++? > > [1] https://codereview.chromium.org/2780623002/ > > > On 2017/03/30 08:52:15, reveman wrote: > > Would it be better to add an is_internal argument to send_workspace event? > > I don't mind moving this to the client, but we'd be changing the protocol for the sake of a workaround, i.e. we don't otherwise need that information.
On 2017/04/04 13:10:28, reveman wrote: > Sorry, I'm still failing to understand how you intend this to work. Cursor > surface size is in DIPs and DIPs are defined by the DSF we send in wl_output > events. We're still sending the primary dsf in wl_output events so that is still > what the client is expected to take into account, arc++ or otherwise. Is cursor > scale broken for all wayland clients or just arc++? Just ARC, which does not use wl_output. ARC content is rendered at a single constant UI scale based on the density of the internal display, and surfaces are scaled to match the DSF of their parent display. The cursor surface is a special case, because it should be scaled to the same physical size on all displays. See the corresponding client-side CL: http://ag/2058936/
On 2017/05/03 at 18:08:33, domlaskowski wrote: > On 2017/04/04 13:10:28, reveman wrote: > > Sorry, I'm still failing to understand how you intend this to work. Cursor > > surface size is in DIPs and DIPs are defined by the DSF we send in wl_output > > events. We're still sending the primary dsf in wl_output events so that is still > > what the client is expected to take into account, arc++ or otherwise. Is cursor > > scale broken for all wayland clients or just arc++? > > Just ARC, which does not use wl_output. ARC content is rendered at a single constant UI scale based on the density of the internal display, and surfaces are scaled to match the DSF of their parent display. The cursor surface is a special case, because it should be scaled to the same physical size on all displays. > > See the corresponding client-side CL: http://ag/2058936/ First step is making sure non-arc clients that use wl_output will always have the cursor rendered at the correct scale. Is that the case today? Then if needed arc should take wl_output into account.
On 2017/05/03 19:27:47, reveman wrote: > First step is making sure non-arc clients that use wl_output will always have > the cursor rendered at the correct scale. Is that the case today? Then if needed > arc should take wl_output into account. As discussed offline, high-DPI scaling does not adhere to the Wayland spec for non-ARC clients: 1) Only the primary display is exposed as a wl_output. 2) The buffer scale does not take the wl_output scale into account. 3) The viewport incorrectly takes precedence over the buffer scale. For ARC, in addition to fixing the above bugs, we need to: 1) Extend wl_output instead of sending workspaces via remote-shell events. 2) Enable the client to select density at run time: http://crbug.com/706645 This transition will go smoother once we align the protocols for server-driven window management. Could we live with this workaround in the meantime?
On 2017/05/08 at 17:03:02, domlaskowski wrote: > On 2017/05/03 19:27:47, reveman wrote: > > First step is making sure non-arc clients that use wl_output will always have > > the cursor rendered at the correct scale. Is that the case today? Then if needed > > arc should take wl_output into account. > > As discussed offline, high-DPI scaling does not adhere to the Wayland spec for non-ARC clients: > > 1) Only the primary display is exposed as a wl_output. Correct, but not really a problem from spec pov. Just exposing one display using the wl_output interface is a valid implementation. We should improve this and expose all displays though. > 2) The buffer scale does not take the wl_output scale into account. Not sure what you mean by this. Are you saying that our surface implementation doesn't take DSF into account correctly? > 3) The viewport incorrectly takes precedence over the buffer scale. We should fix that if spec says something else. Please file a bug and include the relevant part of the spec in the report. > > For ARC, in addition to fixing the above bugs, we need to: > > 1) Extend wl_output instead of sending workspaces via remote-shell events. What's the reason we need to do this? > 2) Enable the client to select density at run time: http://crbug.com/706645 Right. That's all on the client side. > > This transition will go smoother once we align the protocols for server-driven window management. Could we live with this workaround in the meantime? Sure, maybe we should prefer internal display DSF instead of primary for wl_output as well?
lgtm
On 2017/05/08 17:24:53, reveman wrote: > Not sure what you mean by this. Are you saying that our surface implementation > doesn't take DSF into account correctly? > > We should fix that if spec says something else. Please file a bug and include > the relevant part of the spec in the report. Never mind, I misread the spec. The scaling code is correct. On 2017/05/08 17:24:53, reveman wrote: > What's the reason we need to do this? To remove remote-shell in favor of an aura-shell extending xdg-shell, though wl_output should be sufficient if we move all window management to the server. On 2017/05/08 17:24:53, reveman wrote: > Sure, maybe we should prefer internal display DSF instead of primary for > wl_output as well? The internal DSF is just a workaround for the build-time density in ARC, so it should be isolated to remote-shell.
PTAL. On second thought, the client needs to know about internal vs. external displays for the DisplayManager API.
Description was changed from ========== exo: Send DSF of internal display to ARC Since the client-side DPI in ARC is configured at build time based on the internal display, the client must scale the cursor by the DSF of the internal (not the primary) display to convert from pixels to DIPs. This CL removes the need for the workaround in [1], which is specific to ARC. [1] https://crrev.com/f79ede40658ecab4a6c72b6e51c456d68dab2ad0 BUG=642894 TEST=caroline: Cursor scale is correct for each display/resolution. ========== to ========== exo: Send DSF of internal display to ARC Since the client-side DPI in ARC is configured at build time based on the internal display, the client must scale the cursor by the DSF of the internal (not the primary) display to convert from pixels to DIPs. BUG=642894 TEST=caroline: Cursor scale is correct for each display/resolution. ==========
https://codereview.chromium.org/2779823002/diff/100001/third_party/wayland-pr... File third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml (right): https://codereview.chromium.org/2779823002/diff/100001/third_party/wayland-pr... third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml:162: <arg name="is_internal" type="uint"/> why don't we need a version bump for this?
https://codereview.chromium.org/2779823002/diff/100001/third_party/wayland-pr... File third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml (right): https://codereview.chromium.org/2779823002/diff/100001/third_party/wayland-pr... third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml:162: <arg name="is_internal" type="uint"/> On 2017/05/10 20:24:45, reveman wrote: > why don't we need a version bump for this? Because no client depends on that version yet.
The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PS8 just adds a comment requested by lpique.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by domlaskowski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2779823002/#ps140001 (title: "Document is_internal")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496349535340690,
"parent_rev": "1170df977565218f6fcb64b0ddba2d07daf9e8be", "commit_rev":
"9b40ae314fa3842121e01e231b3ea72d04c07ef2"}
Message was sent while issue was closed.
Description was changed from ========== exo: Send DSF of internal display to ARC Since the client-side DPI in ARC is configured at build time based on the internal display, the client must scale the cursor by the DSF of the internal (not the primary) display to convert from pixels to DIPs. BUG=642894 TEST=caroline: Cursor scale is correct for each display/resolution. ========== to ========== exo: Send DSF of internal display to ARC Since the client-side DPI in ARC is configured at build time based on the internal display, the client must scale the cursor by the DSF of the internal (not the primary) display to convert from pixels to DIPs. BUG=642894 TEST=caroline: Cursor scale is correct for each display/resolution. Review-Url: https://codereview.chromium.org/2779823002 Cr-Commit-Position: refs/heads/master@{#476423} Committed: https://chromium.googlesource.com/chromium/src/+/9b40ae314fa3842121e01e231b3e... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/9b40ae314fa3842121e01e231b3e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
