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

Issue 875942 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 667551



Sign in to add a comment

color space layout tests fail in OOPIF mode

Project Member Reported by schenney@chromium.org, Aug 20

Issue description

Run a try job with this patch and look at the failures, or look at the existing try job runs.

https://chromium-review.googlesource.com/c/chromium/src/+/1075891/3

Three tests in the hdr directory fail with black screens, rather than images:

 hdr/color-jpeg-with-color-profile.html
 hdr/color-profile-video.html
 hdr/video-canvas-alpha.html

Investigate and fix so we can get Layout Test coverage for OOPIF.

Guessing at a component.
 
Blocking: 667551
Summary: color space layout tests fail in OOPIF mode (was: hdr layout tests fail in OOPIF mode)
Actually, this is probably all color space tests, including these:

virtual/exotic-color-space/images
Owner: masonfreed@chromium.org
Status: Assigned (was: Untriaged)
->masonfreed for triage, to see if these are real failures or just bugs in the
testing code.
Ok, as best as I can tell, I think this is a bug in the test. The exotic-color-space tests all set the --force-color-profile flag to color-spin-gamma24, which has rotated primaries and a 2.4 gamma. The effect of that should be (and is, when run in Chrome) to display all images with rotated colors and a different gamma curve. However, the current set of "expected" test results for all exotic-color-space tests appear NOT color rotated. They look just like the input images, with no transformation applied. The way that currently happens is that the layout test pixel dump code creates a new set of surfaces to render to, which have a hard-coded sRGB color space. Those surfaces do not honor the color-spin-gamma24 color profile. Which is why the output looks non-rotated/transformed. However, when the --enable-display-compositor-pixel-dump flag is ALSO enabled (for OOPIF mode testing), the output is captured from the browser process and the actual compositing surface, which is correctly set with the color-spin color space. THAT output looks transformed/rotated, as I would expect it to be. It exactly matches what a full Chrome browser produces when --force-color-profile flag=color-spin-gamma24 is specified on the command line. And it matches what I would expect to happen when this flag is in use: transformation of the output.

Given the above, my preference would be to re-baseline these tests to match the new output. However, I only have two days of experience with this color-space codebase (and the whole concept of color-space / ICC / CIEXYZ / etc). So I need to discuss with ccameron to see where I'm wrong. Another potential todo item would be to find where the pixel dump surfaces are hard-coded for sRGB (or more properly, where they AREN'T set to the input color space) and have them instead honor the --force-color-profile setting. Even in that case, we will still need to re-baseline the tests.

If I'm correct about the above, it would seem that only the final displayed content (as captured by the --enable-display-compositor-pixel-dump capture) matters, and the intermediate transformation steps, such as the rendering color space, don't really matter. The only time they would is if one of those transforms crushes the gamut, resulting in a clipped color space output. If all of that makes sense, it would seem that we should add a layout test with a very wide gamut (ProPhoto RGB?) input image, and make sure the entire image makes it through rendering and display without gamut crushing. Perhaps such a test already exists, and I didn't see it. Part of the problem here might be that the actual pixel capture for testing stores the capture in an SkBitmap with an sRGB color space, hard-coded. So this might require more code on the capture side to capture custom color-space images for testing.

Ok, having said all of that, I need someone (ccameron) to poke holes in my logic. To discuss Tuesday.
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 10

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

commit c71f15ab1ace78c7efeeeda9f8552b4af9db2877
Author: Mason Freed <masonfreed@chromium.org>
Date: Mon Sep 10 16:53:32 2018

[CI] Adding a force-raster-color-profile flag, to get OOPIF mode tests to pass

With this change, there are now two color-profile related switches:
 --force-color-profile - forces the DISPLAY color profile to the desired setting.
                         Note that the name of the flag didn't change, for
                         backwards-compatibility. But it now refers to the
                         display surface only.
 --force-raster-color-profile - forces the rasterization to happen in the
                                provided color space.

The exotic-color-space virtual test suite now sets the raster color profile to
the ColorSpin profile, and the display color profile to sRGB. All other tests
(by default) set both color profiles to sRGB.


Bug:  875942 ,  667551 
Change-Id: I66445cea5ed8dcc259a8f0bda2141350bcf66458
Reviewed-on: https://chromium-review.googlesource.com/1205713
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589948}
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/chrome/browser/about_flags.cc
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/chrome/test/base/in_process_browser_test.cc
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/content/public/test/browser_test_base.cc
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/content/shell/app/shell_main_delegate.cc
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/content/test/layouttest_support.cc
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/third_party/WebKit/LayoutTests/VirtualTestSuites
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/ui/android/display_android_manager.cc
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/ui/display/display.cc
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/ui/display/display.h
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/ui/display/display_switches.cc
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/ui/display/display_switches.h
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/ui/display/mac/screen_mac.mm
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/ui/display/win/screen_win.cc
[modify] https://crrev.com/c71f15ab1ace78c7efeeeda9f8552b4af9db2877/ui/views/widget/desktop_aura/desktop_screen_x11.cc

Status: Fixed (was: Assigned)
CL has landed. Closing this bug.

Sign in to add a comment