color: Approximated color spaces are not communicated to the renderer process |
||||||||||
Issue descriptionSuppose we have an ICC profile that is approximated by a gfx::ColorSpace. The gfx::ColorSpace will - have internal types of PrimaryID::ICC_BASED and TransferID::ICC_BASED - have an icc_profile_id pointing back to the original ICC profile We will pass the gfx::ColorSpace to the renderer process in content::ScreenInfo. - note that we do not pass the corresponding ICC profile to the renderer We then pass gfx::ColorSpace::GetParametricApproximation to the compositor for raster. Inside gfx::ColorSpace::GetParametricApproximation, we try to look up the original ICC profile, fail, and throw up our hands and say "let's raster to sRGB". As a result, a WCG monitor with a transfer function that we can't accurately approximate will end up being rastered in sRGB.
,
Sep 18 2017
,
Sep 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/64a55ad8cf4f8601c5424de21e6900becd890640 commit 64a55ad8cf4f8601c5424de21e6900becd890640 Author: Christopher Cameron <ccameron@chromium.org> Date: Thu Sep 21 05:31:22 2017 Send ICCProfile for output display to renderer Two motivations for this: 1. On Mac, this ensures that the ICCProfile sent to IOSurfaces (in the renderer) will match the ICCProfile of the display exactly, ensuring that the WindowServer will not need to do any color conversion. 2. On all platforms, this fixes a bug wherein GetRasterColorSpace would fail to find the ICCProfile that it is approximating, and so it would inappropriately return sRGB instead of a wide color gamut space. Fix some tests, and add more tests: Remove the hard-coded ICC profile IDs that were used for test color spaces outside of layout tests since they aren't needed, and were masking bugs in ID tracking. Add unit tests for the ICC profile cache. The ICC profile cache is messy and should be removed entirely, but these tests will at least alert us to changes in behavior, until we remove it entirely (see crbug.com/766736 ). R=hubbe, dcheng TBR=avi (content/) Bug: 764352 Change-Id: Id41c8d8dc2c262a0ecc3f7aa25dcc70f334e7667 Reviewed-on: https://chromium-review.googlesource.com/672154 Commit-Queue: ccameron chromium <ccameron@chromium.org> Reviewed-by: Fredrik Hubinette <hubbe@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#503362} [modify] https://crrev.com/64a55ad8cf4f8601c5424de21e6900becd890640/content/browser/web_contents/web_contents_view_aura.cc [modify] https://crrev.com/64a55ad8cf4f8601c5424de21e6900becd890640/content/browser/web_contents/web_contents_view_mac.mm [modify] https://crrev.com/64a55ad8cf4f8601c5424de21e6900becd890640/content/common/view_messages.h [modify] https://crrev.com/64a55ad8cf4f8601c5424de21e6900becd890640/content/public/common/screen_info.h [modify] https://crrev.com/64a55ad8cf4f8601c5424de21e6900becd890640/third_party/WebKit/Source/platform/graphics/ColorSpaceGamut.cpp [modify] https://crrev.com/64a55ad8cf4f8601c5424de21e6900becd890640/ui/gfx/color_space.cc [modify] https://crrev.com/64a55ad8cf4f8601c5424de21e6900becd890640/ui/gfx/color_space.h [modify] https://crrev.com/64a55ad8cf4f8601c5424de21e6900becd890640/ui/gfx/icc_profile.cc [modify] https://crrev.com/64a55ad8cf4f8601c5424de21e6900becd890640/ui/gfx/icc_profile.h [modify] https://crrev.com/64a55ad8cf4f8601c5424de21e6900becd890640/ui/gfx/icc_profile_unittest.cc [modify] https://crrev.com/64a55ad8cf4f8601c5424de21e6900becd890640/ui/gfx/test/icc_profiles.cc
,
Sep 25 2017
,
Sep 25 2017
This bug requires manual review: M62 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), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 25 2017
Can you please confirm how safe this merge is overall, why it's needed for M62 vs waiting for M63, and if this is well tested in Canary? Since the fix is quite large, I'm worried about taking this late in M62.
,
Sep 25 2017
This bug was a regression in 61 that was caught late in the release cycle. The fix is low risk -- most of the code change is to add testing to a path that was missing testing in 61.
,
Sep 26 2017
Great thanks - approving merge to M62. Branch:3202
,
Sep 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/128e878aa51877cbd9cc569f4e5549ee4a59c13a commit 128e878aa51877cbd9cc569f4e5549ee4a59c13a Author: Christopher Cameron <ccameron@chromium.org> Date: Tue Sep 26 18:36:15 2017 Send ICCProfile for output display to renderer Two motivations for this: 1. On Mac, this ensures that the ICCProfile sent to IOSurfaces (in the renderer) will match the ICCProfile of the display exactly, ensuring that the WindowServer will not need to do any color conversion. 2. On all platforms, this fixes a bug wherein GetRasterColorSpace would fail to find the ICCProfile that it is approximating, and so it would inappropriately return sRGB instead of a wide color gamut space. Fix some tests, and add more tests: Remove the hard-coded ICC profile IDs that were used for test color spaces outside of layout tests since they aren't needed, and were masking bugs in ID tracking. Add unit tests for the ICC profile cache. The ICC profile cache is messy and should be removed entirely, but these tests will at least alert us to changes in behavior, until we remove it entirely (see crbug.com/766736 ). R=dcheng, hubbe TBR=avi (content/), ccameron@chromium.org (cherry picked from commit 64a55ad8cf4f8601c5424de21e6900becd890640) Bug: 764352 Change-Id: Id41c8d8dc2c262a0ecc3f7aa25dcc70f334e7667 Reviewed-on: https://chromium-review.googlesource.com/672154 Commit-Queue: ccameron chromium <ccameron@chromium.org> Reviewed-by: Fredrik Hubinette <hubbe@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#503362} Reviewed-on: https://chromium-review.googlesource.com/683355 Reviewed-by: ccameron chromium <ccameron@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#452} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/128e878aa51877cbd9cc569f4e5549ee4a59c13a/content/browser/web_contents/web_contents_view_aura.cc [modify] https://crrev.com/128e878aa51877cbd9cc569f4e5549ee4a59c13a/content/browser/web_contents/web_contents_view_mac.mm [modify] https://crrev.com/128e878aa51877cbd9cc569f4e5549ee4a59c13a/content/common/view_messages.h [modify] https://crrev.com/128e878aa51877cbd9cc569f4e5549ee4a59c13a/content/public/common/screen_info.h [modify] https://crrev.com/128e878aa51877cbd9cc569f4e5549ee4a59c13a/third_party/WebKit/Source/platform/graphics/ColorSpaceGamut.cpp [modify] https://crrev.com/128e878aa51877cbd9cc569f4e5549ee4a59c13a/ui/gfx/color_space.cc [modify] https://crrev.com/128e878aa51877cbd9cc569f4e5549ee4a59c13a/ui/gfx/color_space.h [modify] https://crrev.com/128e878aa51877cbd9cc569f4e5549ee4a59c13a/ui/gfx/icc_profile.cc [modify] https://crrev.com/128e878aa51877cbd9cc569f4e5549ee4a59c13a/ui/gfx/icc_profile.h [modify] https://crrev.com/128e878aa51877cbd9cc569f4e5549ee4a59c13a/ui/gfx/icc_profile_unittest.cc [modify] https://crrev.com/128e878aa51877cbd9cc569f4e5549ee4a59c13a/ui/gfx/test/icc_profiles.cc
,
Sep 26 2017
,
Oct 2 2017
,
Oct 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a00ea6e4c800171bed2de8e7bc9ad7d6696b6ab8 commit a00ea6e4c800171bed2de8e7bc9ad7d6696b6ab8 Author: Christopher Cameron <ccameron@chromium.org> Date: Mon Oct 02 00:44:55 2017 Fix bug where GpuMemoryBufferImplIOSurface breaks ICCProfile cache All ICCProfile ids should be generated in the browser process. A bug here caused an id to be generated in the renderer process, resulting in conflicting ids and caches spitting out the wrong data. The issue is that GpuMemoryBufferImplIOSurface::SetColorSpaceForScanout gets the ICCProfile from a ColorSpace, which calls ICCProfile::FromData, which creates a new profile id, potentially conflicting with ids sent from the browser process. Avoid this by having GpuMemoryBufferImplIOSurface query just the profile data directly, avoiding the ICCProfile structure. This area needs cleanup in crbug.com/766736 . Bug: 764352 , (originally filed in 770219) TBR=ccameron@chromium.org (cherry picked from commit 7a8be39db8c7bf5eda180ad00718f7d4d24a75bf) Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I9d2bd6ac4fd24fbdeb7d073bf5f9f6e9b32d00a6 Reviewed-on: https://chromium-review.googlesource.com/692708 Commit-Queue: ccameron chromium <ccameron@chromium.org> Reviewed-by: Fredrik Hubinette <hubbe@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#505491} Reviewed-on: https://chromium-review.googlesource.com/694543 Reviewed-by: ccameron chromium <ccameron@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#530} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/a00ea6e4c800171bed2de8e7bc9ad7d6696b6ab8/gpu/ipc/client/gpu_memory_buffer_impl_io_surface.cc [modify] https://crrev.com/a00ea6e4c800171bed2de8e7bc9ad7d6696b6ab8/ui/gfx/color_space.cc [modify] https://crrev.com/a00ea6e4c800171bed2de8e7bc9ad7d6696b6ab8/ui/gfx/color_space.h
,
Oct 2 2017
,
Oct 23 2017
Issue 762997 has been merged into this issue. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by ccameron@chromium.org
, Sep 12 2017