Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843723003:
Make exo::Surface set device_scale_factor. (Closed)
DescriptionMake exo::Surface set device_scale_factor.
exo::Surface wasn't setting the device_scale_factor in CompositorFrames.
This was triggering a DCHECK when the SurfaceInfo for new cc::Surfaces
have device_scale_factor == 0.
BUG=b/37654593
Review-Url: https://codereview.chromium.org/2843723003
Cr-Commit-Position: refs/heads/master@{#468076}
Committed: https://chromium.googlesource.com/chromium/src/+/32dd23efdd4e557eddca40dc0f5bbb8c4e2ab5cc
Patch Set 1 #
Total comments: 4
Patch Set 2 : Get device_scale_factor from aura::Window. #
Total comments: 3
Messages
Total messages: 15 (5 generated)
kylechar@chromium.org changed reviewers: + reveman@chromium.org
https://codereview.chromium.org/2843723003/diff/1/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2843723003/diff/1/components/exo/surface.cc#n... components/exo/surface.cc:850: frame.metadata.device_scale_factor = state_.buffer_scale; I don't think this is correct as setting the viewport is an alternative way to describe the resolution of the buffer. buffer_scale just describes the size of the buffer compared to the DSF sent to the client here: https://cs.chromium.org/chromium/src/components/exo/wayland/server.cc?l=1141 I think DSF used here should always match the value we send to the client. How is this working today by not setting this value? Can we unit test this?
https://codereview.chromium.org/2843723003/diff/1/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2843723003/diff/1/components/exo/surface.cc#n... components/exo/surface.cc:850: frame.metadata.device_scale_factor = state_.buffer_scale; On 2017/04/25 20:56:17, reveman wrote: > I think DSF used here should always match the value we send to the client. Okay, that mostly makes sense to me and I misunderstood what what |buffer_scale| was being used for. Are you saying that this is the device_scale_factor that is sent to all clients? https://cs.chromium.org/chromium/src/components/exo/wayland/server.cc?l=1141 What about clients who's windows are not on the primary display? The device_scale_factor could be different between displays? >How is this working today by not setting this value? Can we unit test this? CompositorFrameMetadata::device_scale_factor could be used by anything after this step in cc::Surface/cc::Display pipeline. I think the only thing that is guaranteed is that when SurfaceManager::SurfaceCreated() runs it sends out that device_scale_factor as part of the SurfaceInfo for any cc::SurfaceObservers to see. I think it's safe to say that exo isn't using the CompositorFrameMetadata::device_scale_factor and doesn't have any cc::SurfaceObservers that use SurfaceInfo::device_scale_factor right now. Since it's unused it doesn't cause problems. We were using SurfaceInfo::device_scale_factor for something in mus and added a DCHECK that is's not zero after tracking down a bug related to that (not exo related). That DCHECK also ended up catching that it's not set here.
https://codereview.chromium.org/2843723003/diff/1/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2843723003/diff/1/components/exo/surface.cc#n... components/exo/surface.cc:850: frame.metadata.device_scale_factor = state_.buffer_scale; On 2017/04/26 at 13:56:52, kylechar wrote: > On 2017/04/25 20:56:17, reveman wrote: > > I think DSF used here should always match the value we send to the client. > > Okay, that mostly makes sense to me and I misunderstood what what |buffer_scale| was being used for. > > Are you saying that this is the device_scale_factor that is sent to all clients? > > https://cs.chromium.org/chromium/src/components/exo/wayland/server.cc?l=1141 > > What about clients who's windows are not on the primary display? The device_scale_factor could be different between displays? We always send the primary scale right now. Real multi display support for wayland clients is still missing. > > >How is this working today by not setting this value? Can we unit test this? > > CompositorFrameMetadata::device_scale_factor could be used by anything after this step in cc::Surface/cc::Display pipeline. I think the only thing that is guaranteed is that when SurfaceManager::SurfaceCreated() runs it sends out that device_scale_factor as part of the SurfaceInfo for any cc::SurfaceObservers to see. > > I think it's safe to say that exo isn't using the CompositorFrameMetadata::device_scale_factor and doesn't have any cc::SurfaceObservers that use SurfaceInfo::device_scale_factor right now. Since it's unused it doesn't cause problems. > > We were using SurfaceInfo::device_scale_factor for something in mus and added a DCHECK that is's not zero after tracking down a bug related to that (not exo related). That DCHECK also ended up catching that it's not set here. Thanks for explaining. That makes sense. Probably best to just send the DSF of the display that the surface is currently associated with for now.
https://codereview.chromium.org/2843723003/diff/1/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2843723003/diff/1/components/exo/surface.cc#n... components/exo/surface.cc:850: frame.metadata.device_scale_factor = state_.buffer_scale; On 2017/04/26 17:48:42, reveman wrote: > Thanks for explaining. That makes sense. Probably best to just send the DSF of > the display that the surface is currently associated with for now. Done.
one quick question, otherwise lgtm. if we have a bug for this then please update BUG= https://codereview.chromium.org/2843723003/diff/20001/components/exo/surface.h File components/exo/surface.h (right): https://codereview.chromium.org/2843723003/diff/20001/components/exo/surface.... components/exo/surface.h:321: float device_scale_factor_ = 1.0f; Did you verify that this is updated as the |windiow_| is added to the window hierarchy?
Description was changed from ========== Make exo::Surface set device_scale_factor. exo::Surface wasn't setting the device_scale_factor in CompositorFrames. This was triggering a DCHECK when the SurfaceInfo for new cc::Surfaces have device_scale_factor == 0. BUG=none ========== to ========== Make exo::Surface set device_scale_factor. exo::Surface wasn't setting the device_scale_factor in CompositorFrames. This was triggering a DCHECK when the SurfaceInfo for new cc::Surfaces have device_scale_factor == 0. BUG=b/37654593 ==========
I added the b/ bug since it looks like that is fine. https://codereview.chromium.org/2843723003/diff/20001/components/exo/surface.h File components/exo/surface.h (right): https://codereview.chromium.org/2843723003/diff/20001/components/exo/surface.... components/exo/surface.h:321: float device_scale_factor_ = 1.0f; On 2017/04/27 21:01:30, reveman wrote: > Did you verify that this is updated as the |windiow_| is added to the window > hierarchy? It gets updated when the ui::Layer with |window_| as it's delegate gets added to the LayerTree. It looks like UpdateSurface() can get called once or twice before that happens, but since the ui::Layer isn't in the tree yet I don't think those CompositorFrames would be embedded anywhere? Does exo use ui::SurfaceLayers to embed content or is something special going on with those fullscreen exo::Surfaces?
https://codereview.chromium.org/2843723003/diff/20001/components/exo/surface.h File components/exo/surface.h (right): https://codereview.chromium.org/2843723003/diff/20001/components/exo/surface.... components/exo/surface.h:321: float device_scale_factor_ = 1.0f; On 2017/04/28 at 18:02:16, kylechar wrote: > On 2017/04/27 21:01:30, reveman wrote: > > Did you verify that this is updated as the |windiow_| is added to the window > > hierarchy? > > It gets updated when the ui::Layer with |window_| as it's delegate gets added to the LayerTree. It looks like UpdateSurface() can get called once or twice before that happens, but since the ui::Layer isn't in the tree yet I don't think those CompositorFrames would be embedded anywhere? Sounds good. UpdateSurface calls before delegate is added to the tree should not produce any contents so we should be fine. > > Does exo use ui::SurfaceLayers to embed content or is something special going on with those fullscreen exo::Surfaces? We always use ui::SurfaceLayers afaik.
The CQ bit was checked by kylechar@chromium.org
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": 20001, "attempt_start_ts": 1493403421115680,
"parent_rev": "ce7ea8b13892b4b9564f3c8af96ebccc24f4d0fb", "commit_rev":
"32dd23efdd4e557eddca40dc0f5bbb8c4e2ab5cc"}
Message was sent while issue was closed.
Description was changed from ========== Make exo::Surface set device_scale_factor. exo::Surface wasn't setting the device_scale_factor in CompositorFrames. This was triggering a DCHECK when the SurfaceInfo for new cc::Surfaces have device_scale_factor == 0. BUG=b/37654593 ========== to ========== Make exo::Surface set device_scale_factor. exo::Surface wasn't setting the device_scale_factor in CompositorFrames. This was triggering a DCHECK when the SurfaceInfo for new cc::Surfaces have device_scale_factor == 0. BUG=b/37654593 Review-Url: https://codereview.chromium.org/2843723003 Cr-Commit-Position: refs/heads/master@{#468076} Committed: https://chromium.googlesource.com/chromium/src/+/32dd23efdd4e557eddca40dc0f5b... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/32dd23efdd4e557eddca40dc0f5b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
