color space layout tests fail in OOPIF mode |
||||
Issue descriptionRun 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.
,
Aug 20
Actually, this is probably all color space tests, including these: virtual/exotic-color-space/images
,
Aug 23
->masonfreed for triage, to see if these are real failures or just bugs in the testing code.
,
Sep 3
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.
,
Sep 7
This CL is in review, and it fixes all virtual/exotic-color-space tests: https://chromium-review.googlesource.com/c/chromium/src/+/1205713 Here are the layout tests from Linux, Mac, Win with the --enable-display-compositor-pixel-dumps flag enabled: https://test-results.appspot.com/data/layout_results/linux-blink-rel/797/layout-test-results/results.html https://test-results.appspot.com/data/layout_results/mac10_13_retina-blink-rel/400/layout-test-results/results.html https://test-results.appspot.com/data/layout_results/win7-blink-rel/791/layout-test-results/results.html I will close this bug once that CL lands.
,
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
,
Sep 10
CL has landed. Closing this bug. |
||||
►
Sign in to add a comment |
||||
Comment 1 by schenney@chromium.org
, Aug 20