Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2547243002:
Set device scale factor in CompositorFrame and scale frame size in WS. (Closed)
|
Created:
4 years ago by riajiang Modified:
4 years ago CC:
chromium-reviews, rjkroege, qsr+mojo_chromium.org, danakj+watch_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, jbauman+watch_chromium.org, Aaron Boodman, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, cc-bugs_chromium.org, darin (slow to review), Fady Samuel, kylechar Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet device scale factor in CompositorFrame and scale frame size in WS.
1. Set the correct device scale factor when creating CompositorFrame in
FrameGenerator and MusBrowserCompositorOutputSurface.
2. Scale the frame size by device scale factor from DIP to physical
pixels in WindowServer.
This also solves the first TODO of CL
https://codereview.chromium.org/2447303002/.
BUG=669371
TEST=manual (with --force-device-scale-factor=2)
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Committed: https://crrev.com/e5d57c4bddf4750a2889b2f391635f0dccbef431
Cr-Commit-Position: refs/heads/master@{#439321}
Patch Set 1 #
Total comments: 9
Patch Set 2 : dsf #
Total comments: 8
Patch Set 3 : comments #
Total comments: 1
Patch Set 4 : test #
Messages
Total messages: 40 (20 generated)
Description was changed from ========== Set device scale factor in CompositorFrame and scale frame size in WS. 1. Set the correct device scale factor when creating CompositorFrame in FrameGenerator and MusBrowserCompositorOutputSurface. 2. Scale the frame size by device scale factor from physical pixels to DIP in WindowServer. This also solves the first TODO of CL https://codereview.chromium.org/2447303002/. BUG=669371 ========== to ========== Set device scale factor in CompositorFrame and scale frame size in WS. 1. Set the correct device scale factor when creating CompositorFrame in FrameGenerator and MusBrowserCompositorOutputSurface. 2. Scale the frame size by device scale factor from physical pixels to DIP in WindowServer. This also solves the first TODO of CL https://codereview.chromium.org/2447303002/. BUG=669371 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Set device scale factor in CompositorFrame and scale frame size in WS. 1. Set the correct device scale factor when creating CompositorFrame in FrameGenerator and MusBrowserCompositorOutputSurface. 2. Scale the frame size by device scale factor from physical pixels to DIP in WindowServer. This also solves the first TODO of CL https://codereview.chromium.org/2447303002/. BUG=669371 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Set device scale factor in CompositorFrame and scale frame size in WS. 1. Set the correct device scale factor when creating CompositorFrame in FrameGenerator and MusBrowserCompositorOutputSurface. 2. Scale the frame size by device scale factor from DIP to physical pixels in WindowServer. This also solves the first TODO of CL https://codereview.chromium.org/2447303002/. BUG=669371 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Set device scale factor in CompositorFrame and scale frame size in WS. 1. Set the correct device scale factor when creating CompositorFrame in FrameGenerator and MusBrowserCompositorOutputSurface. 2. Scale the frame size by device scale factor from DIP to physical pixels in WindowServer. This also solves the first TODO of CL https://codereview.chromium.org/2447303002/. BUG=669371 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Set device scale factor in CompositorFrame and scale frame size in WS. 1. Set the correct device scale factor when creating CompositorFrame in FrameGenerator and MusBrowserCompositorOutputSurface. 2. Scale the frame size by device scale factor from DIP to physical pixels in WindowServer. This also solves the first TODO of CL https://codereview.chromium.org/2447303002/. BUG=669371 TEST=manual (with --force-device-scale-factor=2) CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by riajiang@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
riajiang@chromium.org changed reviewers: + danakj@chromium.org, sadrul@chromium.org, sky@chromium.org
Hi! danakj@, could you take a look at content/browser/compositor? sky@, could you take a look at services/ui/ws? Thanks!
https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/frame_genera... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:97: void FrameGenerator::UpdateDeviceScaleFactor(float device_scale_factor) { Generally for trivial functions like this we use unix_hacker_case and inline it in the header, e.g.: void set_device_scale_factor(...) ... https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/window_server.h File services/ui/ws/window_server.h (right): https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/window_serve... services/ui/ws/window_server.h:331: const gfx::Size& frame_size_in_dip, Why does this function take dips?
kylechar@chromium.org changed reviewers: + kylechar@chromium.org
https://codereview.chromium.org/2547243002/diff/1/cc/ipc/display_compositor.m... File cc/ipc/display_compositor.mojom (right): https://codereview.chromium.org/2547243002/diff/1/cc/ipc/display_compositor.m... cc/ipc/display_compositor.mojom:51: gfx.mojom.Size frame_size_in_dip, This is comes via SurfaceManager like so: SurfaceManager::SurfaceCreated() DisplayCompositor::OnSurfaceCreated() DisplayCompositorClient::OnSurfaceCreated() SurfaceManager operates in PP, so frame_size will be in PP. https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/frame_genera... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.h:54: float device_scale_factor); Instead of adding another parameter to the constructor, you could just call frame_generator_.set_device_scale_factor(...) after the constructor? https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/window_server.h File services/ui/ws/window_server.h (right): https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/window_serve... services/ui/ws/window_server.h:331: const gfx::Size& frame_size_in_dip, On 2016/12/03 16:28:30, sky wrote: > Why does this function take dips? +1. This function receives PP values for frame_size.
https://codereview.chromium.org/2547243002/diff/1/content/browser/compositor/... File content/browser/compositor/mus_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2547243002/diff/1/content/browser/compositor/... content/browser/compositor/mus_browser_compositor_output_surface.cc:76: ui_frame.metadata.device_scale_factor = device_scale_factor_; What if the device scale changes? It's not fixed for the life of the output surface. Granted this whole class needs to go away and should not actually ship to users.. but are you okay with this cuz I don't see it documented in TODOs or anything like that.
Since we switched to use the aura-mus client-lib and ash right now is not scaling correctly with the new client-lib, I'm going to work on scaling on the ash side first so that I can test frame_size more accurately. Will re-ping when this CL is ready again. https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/frame_genera... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.cc:97: void FrameGenerator::UpdateDeviceScaleFactor(float device_scale_factor) { On 2016/12/03 16:28:30, sky wrote: > Generally for trivial functions like this we use unix_hacker_case and inline it > in the header, e.g.: > void set_device_scale_factor(...) ... Done. https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/frame_genera... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/frame_genera... services/ui/ws/frame_generator.h:54: float device_scale_factor); On 2016/12/05 14:46:11, kylechar wrote: > Instead of adding another parameter to the constructor, you could just call > frame_generator_.set_device_scale_factor(...) after the constructor? Done.
On 2016/12/09 19:31:44, riajiang wrote: > Since we switched to use the aura-mus client-lib and ash right now is not > scaling correctly with the new client-lib, I'm going to work on scaling on the > ash side first so that I can test frame_size more accurately. Will re-ping when > this CL is ready again. > > https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/frame_genera... > File services/ui/ws/frame_generator.cc (right): > > https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/frame_genera... > services/ui/ws/frame_generator.cc:97: void > FrameGenerator::UpdateDeviceScaleFactor(float device_scale_factor) { > On 2016/12/03 16:28:30, sky wrote: > > Generally for trivial functions like this we use unix_hacker_case and inline > it > > in the header, e.g.: > > void set_device_scale_factor(...) ... > > Done. > > https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/frame_genera... > File services/ui/ws/frame_generator.h (right): > > https://codereview.chromium.org/2547243002/diff/1/services/ui/ws/frame_genera... > services/ui/ws/frame_generator.h:54: float device_scale_factor); > On 2016/12/05 14:46:11, kylechar wrote: > > Instead of adding another parameter to the constructor, you could just call > > frame_generator_.set_device_scale_factor(...) after the constructor? > > Done. After scaling in the new client-lib, frame_size appears to be set correctly in pixels (except for chrome browser, which is still using the old client-lib). I deleted all changes related to frame_size conversion, the rest is to make sure we are initializing device scale factor correctly. Please take another look. Thanks!
After scaling in the new client-lib, frame_size appears to be set correctly in pixels (except for chrome browser, which is still using the old client-lib). I deleted all changes related to frame_size conversion, the rest is to make sure we are initializing device scale factor correctly. Please take another look. Thanks! https://codereview.chromium.org/2547243002/diff/1/content/browser/compositor/... File content/browser/compositor/mus_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2547243002/diff/1/content/browser/compositor/... content/browser/compositor/mus_browser_compositor_output_surface.cc:76: ui_frame.metadata.device_scale_factor = device_scale_factor_; On 2016/12/06 22:44:27, danakj_OOO_and_slow wrote: > What if the device scale changes? It's not fixed for the life of the output > surface. Granted this whole class needs to go away and should not actually ship > to users.. but are you okay with this cuz I don't see it documented in TODOs or > anything like that. Changed to get the device_scale_factor from the display for window_ (aura::Window) which should be updated when dsf changes.
https://codereview.chromium.org/2547243002/diff/20001/services/ui/surfaces/di... File services/ui/surfaces/display_compositor.cc (right): https://codereview.chromium.org/2547243002/diff/20001/services/ui/surfaces/di... services/ui/surfaces/display_compositor.cc:229: DCHECK(device_scale_factor); DCHECK_GT(device_scale_factor, 0.0f) maybe so it's clear it's a float?
https://codereview.chromium.org/2547243002/diff/20001/services/ui/surfaces/di... File services/ui/surfaces/display_compositor.cc (right): https://codereview.chromium.org/2547243002/diff/20001/services/ui/surfaces/di... services/ui/surfaces/display_compositor.cc:229: DCHECK(device_scale_factor); DCHECK_GT(device_scale_factor, 0.0f) maybe so it's clear it's a float?
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2547243002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2547243002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:125: float device_scale_factor_; drive-by: Initialize this maybe to 1.0f? https://codereview.chromium.org/2547243002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2547243002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:88: init_params.metrics.device_scale_factor); I would actually make the device_scale_factor a param on the constructor.
content/b/c/ lgtm https://codereview.chromium.org/2547243002/diff/20001/content/browser/composi... File content/browser/compositor/mus_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2547243002/diff/20001/content/browser/composi... content/browser/compositor/mus_browser_compositor_output_surface.cc:77: ui_frame.metadata.device_scale_factor = display::Screen::GetScreen() I would normally ask that you plumb this in more directly, but I cant justify investing much in this impl of OutputSurface which we'll delete ASAP.
https://codereview.chromium.org/2547243002/diff/20001/content/browser/composi... File content/browser/compositor/mus_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2547243002/diff/20001/content/browser/composi... content/browser/compositor/mus_browser_compositor_output_surface.cc:77: ui_frame.metadata.device_scale_factor = display::Screen::GetScreen() On 2016/12/15 18:58:02, danakj_OOO_and_slow wrote: > I would normally ask that you plumb this in more directly, but I cant justify > investing much in this impl of OutputSurface which we'll delete ASAP. +1
https://codereview.chromium.org/2547243002/diff/20001/services/ui/surfaces/di... File services/ui/surfaces/display_compositor.cc (right): https://codereview.chromium.org/2547243002/diff/20001/services/ui/surfaces/di... services/ui/surfaces/display_compositor.cc:229: DCHECK(device_scale_factor); On 2016/12/15 18:25:03, kylechar wrote: > DCHECK_GT(device_scale_factor, 0.0f) maybe so it's clear it's a float? Done. https://codereview.chromium.org/2547243002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator.h (right): https://codereview.chromium.org/2547243002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator.h:125: float device_scale_factor_; On 2016/12/15 18:27:51, Fady Samuel wrote: > drive-by: Initialize this maybe to 1.0f? Done. https://codereview.chromium.org/2547243002/diff/20001/services/ui/ws/frame_ge... File services/ui/ws/frame_generator_unittest.cc (right): https://codereview.chromium.org/2547243002/diff/20001/services/ui/ws/frame_ge... services/ui/ws/frame_generator_unittest.cc:88: init_params.metrics.device_scale_factor); On 2016/12/15 18:27:51, Fady Samuel wrote: > I would actually make the device_scale_factor a param on the constructor. We are also updating DSF in PlatformDisplayDefault::UpdateViewportMetrics which is calling this set_..., does it make sense to just have this one setter and use this when initializing DSF as well?
lgtm
lgtm
lgtm
LGTM https://codereview.chromium.org/2547243002/diff/40001/ui/display/screen_base.cc File ui/display/screen_base.cc (right): https://codereview.chromium.org/2547243002/diff/40001/ui/display/screen_base.... ui/display/screen_base.cc:42: // TODO(riajiang): Implement this for multi-displays either here or in ScreenMus may end up using ScreenAsh. But we'll straighten that out later.
The CQ bit was checked by riajiang@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, fsamuel@chromium.org, kylechar@chromium.org, sadrul@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2547243002/#ps60001 (title: "test")
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": 60001, "attempt_start_ts": 1481943827204830,
"parent_rev": "b5018314605a66dd849c2ab40bb3a3d4937f9c43", "commit_rev":
"1261aec2d6acd8a56f834159e29af0b9e957936c"}
Message was sent while issue was closed.
Description was changed from ========== Set device scale factor in CompositorFrame and scale frame size in WS. 1. Set the correct device scale factor when creating CompositorFrame in FrameGenerator and MusBrowserCompositorOutputSurface. 2. Scale the frame size by device scale factor from DIP to physical pixels in WindowServer. This also solves the first TODO of CL https://codereview.chromium.org/2447303002/. BUG=669371 TEST=manual (with --force-device-scale-factor=2) CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Set device scale factor in CompositorFrame and scale frame size in WS. 1. Set the correct device scale factor when creating CompositorFrame in FrameGenerator and MusBrowserCompositorOutputSurface. 2. Scale the frame size by device scale factor from DIP to physical pixels in WindowServer. This also solves the first TODO of CL https://codereview.chromium.org/2447303002/. BUG=669371 TEST=manual (with --force-device-scale-factor=2) CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2547243002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Set device scale factor in CompositorFrame and scale frame size in WS. 1. Set the correct device scale factor when creating CompositorFrame in FrameGenerator and MusBrowserCompositorOutputSurface. 2. Scale the frame size by device scale factor from DIP to physical pixels in WindowServer. This also solves the first TODO of CL https://codereview.chromium.org/2447303002/. BUG=669371 TEST=manual (with --force-device-scale-factor=2) CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2547243002 ========== to ========== Set device scale factor in CompositorFrame and scale frame size in WS. 1. Set the correct device scale factor when creating CompositorFrame in FrameGenerator and MusBrowserCompositorOutputSurface. 2. Scale the frame size by device scale factor from DIP to physical pixels in WindowServer. This also solves the first TODO of CL https://codereview.chromium.org/2447303002/. BUG=669371 TEST=manual (with --force-device-scale-factor=2) CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/e5d57c4bddf4750a2889b2f391635f0dccbef431 Cr-Commit-Position: refs/heads/master@{#439321} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e5d57c4bddf4750a2889b2f391635f0dccbef431 Cr-Commit-Position: refs/heads/master@{#439321}
Message was sent while issue was closed.
Patchset #5 (id:80001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
