New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 764352 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug



Sign in to add a comment

color: Approximated color spaces are not communicated to the renderer process

Project Member Reported by ccameron@chromium.org, Sep 12 2017

Issue description

Suppose 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.
 
Two fixes I have in mind here:

Fix 1: Pass the ICC profile in content::ScreenInfo, if "needed"
  A "needed" here means at least that the gfx::ColorSpace will refer back to it
  B "needed" may also include sending it unconditionally on Mac, since we will be attaching the ICC profile to IOSurfaces, which may need to match the input for power reasons

Fix 2: Pass gfx::ColorSpace::GetParametricApproximation to content::ScreenInfo.

IMO we should do Fix 1 because of reason 1.B -- I'll need to gather power data on this first, though.
Labels: -Pri-3 M-62 Pri-1
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Labels: Merge-Request-62 OS-Linux OS-Mac OS-Windows
Project Member

Comment 5 by sheriffbot@chromium.org, Sep 25 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
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
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. 
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.
Labels: -Merge-Review-62 Merge-Approved-62
Great thanks - approving merge to M62. Branch:3202
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 26 2017

Labels: -merge-approved-62 merge-merged-3202
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

Status: Fixed (was: Assigned)
Status: Assigned (was: Fixed)
Project Member

Comment 12 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
Cc: krajshree@chromium.org ccameron@chromium.org ericrk@chromium.org pbomm...@chromium.org
 Issue 762997  has been merged into this issue.

Sign in to add a comment