Mac: Color spaces set using SetColorSpaceMetadataCHROMIUM have higher power usage |
||||||||
Issue descriptionGPU rasterization allocates textures using TexStorage2DImageCHROMIUM and specifies the color space for the storage with SetColorSpaceMetadataCHROMIUM When we draw these IOSurfaces using CoreAnimation, there is a power "discount" if the IOSurface has the exact same byte-for-byte ICC profile as the monitor it is displayed on. We exploit this via by checking for profiles from ICCProfile::FromCacheMac at [1], which is populated in the browser by ScreenMac at [2] and in the renderer via the IPC at [3] (sort-of-implicitly). But ... the GPU process doesn't have any mechanism for getting these "true ICC profiles", so it just constructs one for the color space sent in SetColorSpaceMetadataCHROMIUM, which isn't byte-for-byte-the-same, and we suffer. [1] https://cs.chromium.org/chromium/src/ui/gfx/mac/io_surface.cc?rcl=2f117ff452c82a39c0ab351dc2d724246f3f98f4&l=214 [2] https://cs.chromium.org/chromium/src/ui/display/mac/screen_mac.mm?rcl=d5c6b8a0ed6b7f29ae3607470762e273da52dc27&l=90 [3] https://cs.chromium.org/chromium/src/content/public/common/screen_info.h?rcl=90c40887181fcfd72fa1b0b7ecc4a510623d7c54&l=32
,
Aug 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/79358bd9739b283d64df5a507dd28fd5826730fd commit 79358bd9739b283d64df5a507dd28fd5826730fd Author: Christopher Cameron <ccameron@chromium.org> Date: Thu Aug 02 04:31:44 2018 Remove need of ICC profiles in the renderer process Change zero-copy to specify IOSurface color profiles using SetColorSpaceMetadataCHROMIUM rather than calling SetColorSpace on the GpuMemoryBuffer directly. The SetColorSpaceMetadataCHROMIUM path ends up calling gl::GLImageIOSurface::SetColorSpace, which uses the gfx::DisplayICCProfiles structure to ensure that we use low power profiles when possible. This removes the need for - gfx::ICCProfile::FromCacheMac - Sending gfx::ICCProfiles from the browser to the renderer in the content::ScreenInfo - Note that content::ScreenInfo::icc_profile was not ever actually read - Sending it over IPC would result it being added to the cache read by gfx::ICCProfile::FromCacheMac - mojo and IPC support of gfx::ICCProfile Bug: 869570 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I814496a5fffc8da38b4b59d7d2cc055dc47cd52b Reviewed-on: https://chromium-review.googlesource.com/1159233 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Cr-Commit-Position: refs/heads/master@{#580066} [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/cc/raster/zero_copy_raster_buffer_provider.cc [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/content/browser/renderer_host/display_util.cc [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/content/common/frame_messages.h [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/content/common/renderer.mojom [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/content/public/common/screen_info.h [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/ui/gfx/icc_profile.cc [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/ui/gfx/icc_profile.h [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/ui/gfx/icc_profile_unittest.cc [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/ui/gfx/ipc/color/gfx_param_traits.cc [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/ui/gfx/ipc/color/gfx_param_traits.h [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/ui/gfx/mac/display_icc_profiles.cc [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/ui/gfx/mac/io_surface.cc [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/ui/gfx/mojo/BUILD.gn [delete] https://crrev.com/a36c7238867b1b3892da685335553beb995b511b/ui/gfx/mojo/icc_profile.mojom [delete] https://crrev.com/a36c7238867b1b3892da685335553beb995b511b/ui/gfx/mojo/icc_profile.typemap [modify] https://crrev.com/79358bd9739b283d64df5a507dd28fd5826730fd/ui/gfx/typemaps.gni
,
Aug 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3c5a7b67521a08cfab124c1644c4b21bb43fa7e3 commit 3c5a7b67521a08cfab124c1644c4b21bb43fa7e3 Author: Christopher Cameron <ccameron@chromium.org> Date: Thu Aug 02 20:32:27 2018 Be robust to race when display is attached in DisplayICCProfiles Consider the sequence: 1. We query the number of displays using CGGetActiveDisplayList and get |display_count| as 0. 2. We allocate a vector |displays| with that size, so it's nullptr. 3. We populate that vector with a call to CGGetActiveDisplayList, but, because |displays| is nullptr, that populates |display_count| with the number of displays on the system. 4. BUT, it so-happens that between steps 1 and 3, we added an active display, so |display_count| is now non-zero. 5. We now index into |displays|, which is empty, and blow up. Early-out if our initial |display_count| is zero to avoid this situation. Bug: 870320, 869570 Change-Id: Ibeec67ce430faccb6e350af3fb335f1770e7b072 Reviewed-on: https://chromium-review.googlesource.com/1160769 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Kenneth Russell <kbr@chromium.org> Reviewed-by: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#580302} [modify] https://crrev.com/3c5a7b67521a08cfab124c1644c4b21bb43fa7e3/ui/gfx/mac/display_icc_profiles.cc
,
Aug 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/947f2fb9755802f60dd3bbe871c852dd79f240b3 commit 947f2fb9755802f60dd3bbe871c852dd79f240b3 Author: Christopher Cameron <ccameron@chromium.org> Date: Wed Aug 08 20:02:01 2018 Correctly set DisplayICCProfiles::needs_update_ Bug: 869570 , 871948 Change-Id: I92312b385c7e38610aa92ad2e25a1b2fa0f192ff Reviewed-on: https://chromium-review.googlesource.com/1166205 Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/heads/master@{#581673} [modify] https://crrev.com/947f2fb9755802f60dd3bbe871c852dd79f240b3/ui/gfx/mac/display_icc_profiles.cc
,
Aug 10
Adding merge request for M69. This bug gets us a fairly big GPU power reduction, which covers up any increases caused by the switch to MacViews.
,
Aug 10
The bug is marked as P3 or Feature. It should not be merged as M69 is in beta. Please contact the approriate milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 10
,
Aug 13
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 13
There are multiple CL listed in this bug. Which CL you're requesting a merge for ? And how safe is the change to merge to M69?
,
Aug 13
I would like to get all 3 of these merged -- there was a bug in the first one, and the second two patches fix that bug. I believe this has had adequate time to bake in Canary (including the last patch) and I'd like to merge to 69 earlier, to give it more time to bake on the branch.
,
Aug 13
Approving merge to M69 branch 3497 based on comment #11. Please merge ASAP. Thank you.
,
Aug 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b90a1fee94bac9c090f568c3dda254298c14247 commit 3b90a1fee94bac9c090f568c3dda254298c14247 Author: Christopher Cameron <ccameron@chromium.org> Date: Mon Aug 13 17:23:04 2018 Mac: Cache monitor ICC profiles in the GPU process When we draw IOSurfaces using CoreAnimation, there is a power "discount" if the IOSurface has the exact same byte-for-byte ICC profile as the monitor it is displayed on. The browser retrieves these ICC profiles via ScreenMac, and the renderer process is sent them via ScreenInfo, but the GPU process does not get them. As a consequence, color spaces assigned to GLImages via SetColorSpaceMetadataCHROMIUM do not match monitors' ICC profiles, and power suffers. Solve this by creating a DisplayICCProfiles class to provide a map from ColorSpace objects to the ICC profile data that created them. Instantiate this in the GPU process for use with by GLImageIOSurface::SetColorSpace. As a follow on to this patch (but not for merge): - Change the zero copy path to use SetColorSpaceMetadataCHROMIUM - Remove ICCProfile::FromCacheMac - Remove sending ICCProfiles via IPC to the renderer (they are currently only used for this purpose) - Make SetColorSpaceMetadataCHROMIUM be the only path that will produce this byte-for-byte ICC profile match (it will be the only one that matters) TBR=ccameron@chromium.org (cherry picked from commit 8768493d87dd889bdf13da5333dce0015e5164af) Bug: 869570 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Change-Id: Ifb0981a97ccbb249b1d0dab1c0aa57e48931ebdb Reviewed-on: https://chromium-review.googlesource.com/1157288 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Kenneth Russell <kbr@chromium.org> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#579946} Reviewed-on: https://chromium-review.googlesource.com/1172942 Reviewed-by: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#565} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/3b90a1fee94bac9c090f568c3dda254298c14247/ui/gfx/BUILD.gn [add] https://crrev.com/3b90a1fee94bac9c090f568c3dda254298c14247/ui/gfx/mac/display_icc_profiles.cc [add] https://crrev.com/3b90a1fee94bac9c090f568c3dda254298c14247/ui/gfx/mac/display_icc_profiles.h [modify] https://crrev.com/3b90a1fee94bac9c090f568c3dda254298c14247/ui/gfx/mac/io_surface.cc [modify] https://crrev.com/3b90a1fee94bac9c090f568c3dda254298c14247/ui/gl/gl_image_io_surface.mm
,
Aug 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/01ed55db949939bf603080e72317f1261206121f commit 01ed55db949939bf603080e72317f1261206121f Author: Christopher Cameron <ccameron@chromium.org> Date: Mon Aug 13 17:25:47 2018 Remove need of ICC profiles in the renderer process Change zero-copy to specify IOSurface color profiles using SetColorSpaceMetadataCHROMIUM rather than calling SetColorSpace on the GpuMemoryBuffer directly. The SetColorSpaceMetadataCHROMIUM path ends up calling gl::GLImageIOSurface::SetColorSpace, which uses the gfx::DisplayICCProfiles structure to ensure that we use low power profiles when possible. This removes the need for - gfx::ICCProfile::FromCacheMac - Sending gfx::ICCProfiles from the browser to the renderer in the content::ScreenInfo - Note that content::ScreenInfo::icc_profile was not ever actually read - Sending it over IPC would result it being added to the cache read by gfx::ICCProfile::FromCacheMac - mojo and IPC support of gfx::ICCProfile TBR=ccameron@chromium.org (cherry picked from commit 79358bd9739b283d64df5a507dd28fd5826730fd) Bug: 869570 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I814496a5fffc8da38b4b59d7d2cc055dc47cd52b Reviewed-on: https://chromium-review.googlesource.com/1159233 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#580066} Reviewed-on: https://chromium-review.googlesource.com/1172946 Reviewed-by: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#566} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/cc/raster/zero_copy_raster_buffer_provider.cc [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/content/browser/renderer_host/display_util.cc [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/content/common/frame_messages.h [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/content/common/renderer.mojom [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/content/public/common/screen_info.h [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/ui/gfx/icc_profile.cc [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/ui/gfx/icc_profile.h [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/ui/gfx/icc_profile_unittest.cc [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/ui/gfx/ipc/color/gfx_param_traits.cc [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/ui/gfx/ipc/color/gfx_param_traits.h [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/ui/gfx/mac/display_icc_profiles.cc [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/ui/gfx/mac/io_surface.cc [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/ui/gfx/mojo/BUILD.gn [delete] https://crrev.com/3b90a1fee94bac9c090f568c3dda254298c14247/ui/gfx/mojo/icc_profile.mojom [delete] https://crrev.com/3b90a1fee94bac9c090f568c3dda254298c14247/ui/gfx/mojo/icc_profile.typemap [modify] https://crrev.com/01ed55db949939bf603080e72317f1261206121f/ui/gfx/typemaps.gni
,
Aug 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c14d522a2465b026cdd4039c76b392644dbd947a commit c14d522a2465b026cdd4039c76b392644dbd947a Author: Christopher Cameron <ccameron@chromium.org> Date: Mon Aug 13 17:28:52 2018 Be robust to race when display is attached in DisplayICCProfiles Consider the sequence: 1. We query the number of displays using CGGetActiveDisplayList and get |display_count| as 0. 2. We allocate a vector |displays| with that size, so it's nullptr. 3. We populate that vector with a call to CGGetActiveDisplayList, but, because |displays| is nullptr, that populates |display_count| with the number of displays on the system. 4. BUT, it so-happens that between steps 1 and 3, we added an active display, so |display_count| is now non-zero. 5. We now index into |displays|, which is empty, and blow up. Early-out if our initial |display_count| is zero to avoid this situation. TBR=ccameron@chromium.org (cherry picked from commit 3c5a7b67521a08cfab124c1644c4b21bb43fa7e3) Bug: 870320, 869570 Change-Id: Ibeec67ce430faccb6e350af3fb335f1770e7b072 Reviewed-on: https://chromium-review.googlesource.com/1160769 Commit-Queue: ccameron <ccameron@chromium.org> Reviewed-by: Kenneth Russell <kbr@chromium.org> Reviewed-by: ccameron <ccameron@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#580302} Reviewed-on: https://chromium-review.googlesource.com/1172948 Cr-Commit-Position: refs/branch-heads/3497@{#567} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/c14d522a2465b026cdd4039c76b392644dbd947a/ui/gfx/mac/display_icc_profiles.cc
,
Aug 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/641c47d1a39dbec86c8d476a19abe312f5001df1 commit 641c47d1a39dbec86c8d476a19abe312f5001df1 Author: Christopher Cameron <ccameron@chromium.org> Date: Mon Aug 13 17:32:02 2018 Correctly set DisplayICCProfiles::needs_update_ TBR=ccameron@chromium.org (cherry picked from commit 947f2fb9755802f60dd3bbe871c852dd79f240b3) Bug: 869570 , 871948 Change-Id: I92312b385c7e38610aa92ad2e25a1b2fa0f192ff Reviewed-on: https://chromium-review.googlesource.com/1166205 Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Commit-Queue: ccameron <ccameron@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#581673} Reviewed-on: https://chromium-review.googlesource.com/1173011 Reviewed-by: ccameron <ccameron@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#568} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/641c47d1a39dbec86c8d476a19abe312f5001df1/ui/gfx/mac/display_icc_profiles.cc
,
Aug 13
No conflicts during merge. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by bugdroid1@chromium.org
, Aug 1