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

Issue 827670 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Apr 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug



Sign in to add a comment

Regression: Colors are wrong on macOS

Project Member Reported by ccameron@chromium.org, Mar 30 2018

Issue description

See attached screenshot -- everything in Chrome (except for solid color quads) is way too dark.

You are probably looking for a change made after 534723 (known good), but no later than 534739 (first known bad).
CHANGELOG URL:  https://chromium.googlesource.com/chromium/src/+log/0a656e429f0454c7ff10ce9eaa1d655aed94fd7c..f9230a78b254ec916650e745904207743195d864
Suspecting r534726.

This affects only
- the CARender path
- with GPU raster disabled
- and a non-sRGB color profiles

The hard part about testing this is that it requires
- the native screen capture test system (to test CARenderer)
- a non-sRGB color profile
And there is all sorts of raciness to setting non-sRGB color profiles and taking screenshots (we now just force the bots into an sRGB color profile in perpetuity rather than dynamically setting it).

But we can at least add a test to ensure that whatever call is missing be added back.
 
color.png
420 KB View Download
Just a heads up, M66 Stable cut is on April 12th, 10 days away. This issue is marked as RB-Stable for 66. Please make sure to address this issue prior to stable cut. Thanks! 
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 3 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3cf3eea9fbe8973db19894c1a008f7dcbbe4458d

commit 3cf3eea9fbe8973db19894c1a008f7dcbbe4458d
Author: Christopher Cameron <ccameron@chromium.org>
Date: Tue Apr 03 23:49:55 2018

cc: Add missed GpuMemoryBuffer:SetColorSpace call

This call is necessary to set the color space property on the backing
IOSurface.

It may be that we should change this API to require specifying a color
space a GpuMemoryBuffer creation time. That would be an artifical
constraint, but would make bugs of this sort less likely.

This needs a merge to M66, so I'm not considering it in this patch.

Bug:  827670 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: If5ecdc5a8efcefcf1fcbc9c8a230cd716005ac2e
Reviewed-on: https://chromium-review.googlesource.com/988826
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547872}
[modify] https://crrev.com/3cf3eea9fbe8973db19894c1a008f7dcbbe4458d/cc/raster/zero_copy_raster_buffer_provider.cc

Cc: krajshree@chromium.org
Labels: Needs-Feedback
Unable to reproduce the issue on OS-Mac using chrome reported version #67.0.3384.0.
Also below few set up is required to proceed with testing as per comment #0 which is not available at TE-end,
- the native screen capture test system (to test CARenderer)
- a non-sRGB color profile

Hence, unable to verify the issue on latest chrome version #67.0.3388.0. Attached a screenshot for reference.

ccameron@ - Could you please help us in verifying the issue using the chrome version having the fix.

Thanks...!!

827670.png
444 KB View Download
Labels: Merge-Request-66
Verified in 67.0.3388.0, requesting merge.
Project Member

Comment 7 by sheriffbot@chromium.org, Apr 4 2018

Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: We are only 12 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-66 Merge-Approved-66
Approving merge for M66. Branch:3359
Project Member

Comment 9 by sheriffbot@chromium.org, Apr 9 2018

Cc: abdulsyed@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Reminder: Please note that M66 Stable is only 7 days away. This bug has been marked as ReleaseBlock Stable for M66. So please take a look and appropriately address this bug. 
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 10 2018

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e05988a56a4dbd51722b631df32b1e45515e5549

commit e05988a56a4dbd51722b631df32b1e45515e5549
Author: Christopher Cameron <ccameron@chromium.org>
Date: Tue Apr 10 00:24:11 2018

cc: Add missed GpuMemoryBuffer:SetColorSpace call

This call is necessary to set the color space property on the backing
IOSurface.

It may be that we should change this API to require specifying a color
space a GpuMemoryBuffer creation time. That would be an artifical
constraint, but would make bugs of this sort less likely.

This needs a merge to M66, so I'm not considering it in this patch.

TBR=ccameron@chromium.org

(cherry picked from commit 3cf3eea9fbe8973db19894c1a008f7dcbbe4458d)

Bug:  827670 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: If5ecdc5a8efcefcf1fcbc9c8a230cd716005ac2e
Reviewed-on: https://chromium-review.googlesource.com/988826
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#547872}
Reviewed-on: https://chromium-review.googlesource.com/1003776
Reviewed-by: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#639}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/e05988a56a4dbd51722b631df32b1e45515e5549/cc/raster/zero_copy_raster_buffer_provider.cc

Comment 12 Deleted

ccameron@, can you please help us to verify this fix on today's M66 Beta RC#66.0.3359.106 (Chrome binaries storage: https://goto.google.com/jaxqt) ?

Thank you in-advance!
Status: Verified (was: Assigned)
Marking as verified.
ccameron@, thank you so much!

Sign in to add a comment