Summary: | REGRESSION(r286765): [Cairo][GTK][WPE] Various SVG tests that use filters fail | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Arcady Goldmints-Orlov <crzwdjk> | ||||||||||||||
Component: | SVG | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, don.olmstead, ews-watchlist, heycam, Hironori.Fujii, sabouhallawa, ujwal.koneru, webkit-bug-importer, zimmermann | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 268537 | ||||||||||||||||
Attachments: |
|
Description
Arcady Goldmints-Orlov
2021-12-09 18:29:44 PST
Created attachment 446651 [details]
Actual test result
Actual test result. Note the lighter shade of green.
WinCairo is also failing the tests. Created attachment 459746 [details]
WIP patch
Disabling DESTINATION_COLOR_SPACE_LINEAR_SRGB can fix the issue.
Created attachment 459747 [details]
Patch
Comment on attachment 459747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459747&action=review Thanks for checking this! > Source/WTF/wtf/PlatformEnable.h:252 > +#define ENABLE_DESTINATION_COLOR_SPACE_LINEAR_SRGB 0 That's the right move, one shouldn't assume all graphics backends support color spaces. *sigh* what ever happened to 'https://core.ac.uk/download/pdf/14701464.pdf' ? > Source/WebCore/platform/graphics/cairo/ImageBufferCairoBackend.cpp:65 > +#if ENABLE(DESTINATION_COLOR_SPACE_LINEAR_SRGB) Isn't the intention to assure that the call sites, don't pass LinearSRGB in the Cairo case? One example from an existing call-site: #if ENABLE(DESTINATION_COLOR_SPACE_LINEAR_SRGB) auto colorSpace = DestinationColorSpace::LinearSRGB(); #else auto colorSpace = DestinationColorSpace::SRGB(); #endif auto sourceGraphic = context->createScaledImageBuffer(filterData->drawingRegion, filterScale, colorSpace, filterData->filter->renderingMode()); ... If so, we should assert that the color spaces are not LinearSRGB fo the destination if !ENABLE(DESTINTAION_COLOR_SPACE_LINEAR_SRGB). At least that's how I understand the current code. Comment on attachment 459747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459747&action=review >> Source/WTF/wtf/PlatformEnable.h:252 >> +#define ENABLE_DESTINATION_COLOR_SPACE_LINEAR_SRGB 0 > > That's the right move, one shouldn't assume all graphics backends support color spaces. > *sigh* what ever happened to 'https://core.ac.uk/download/pdf/14701464.pdf' ? Quickly searching through Cairo ML archive, I found that Adrian Johnson suggested color space support ten years ago, based on the Andrea Canciani work. But, I found out no progress. [cairo] Color space API https://lists.cairographics.org/archives/cairo/2012-July/023353.html >> Source/WebCore/platform/graphics/cairo/ImageBufferCairoBackend.cpp:65 >> +#if ENABLE(DESTINATION_COLOR_SPACE_LINEAR_SRGB) > > Isn't the intention to assure that the call sites, don't pass LinearSRGB in the Cairo case? > > One example from an existing call-site: > > #if ENABLE(DESTINATION_COLOR_SPACE_LINEAR_SRGB) > auto colorSpace = DestinationColorSpace::LinearSRGB(); > #else > auto colorSpace = DestinationColorSpace::SRGB(); > #endif > > auto sourceGraphic = context->createScaledImageBuffer(filterData->drawingRegion, filterScale, colorSpace, filterData->filter->renderingMode()); > > ... > > If so, we should assert that the color spaces are not LinearSRGB fo the destination if !ENABLE(DESTINTAION_COLOR_SPACE_LINEAR_SRGB). > At least that's how I understand the current code. Agreed. Will fix. Created attachment 459775 [details]
Patch
Created attachment 459878 [details]
Patch
Comment on attachment 459878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459878&action=review r=me with feedback > Source/WTF/wtf/PlatformEnable.h:248 > -#define ENABLE_DESTINATION_COLOR_SPACE_LINEAR_SRGB 1 > +#define ENABLE_DESTINATION_COLOR_SPACE_LINEAR_SRGB 0 My only concern here is that this might affect Apple ports. Could we do #if USE(CAIRO) #define ENABLE_DESTINATION_COLOR_SPACE_LINEAR_SRGB 0 #else #define ENABLE_DESTINATION_COLOR_SPACE_LINEAR_SRGB 1 #endif (In reply to Don Olmstead from comment #9) > Comment on attachment 459878 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=459878&action=review > > r=me with feedback > > > Source/WTF/wtf/PlatformEnable.h:248 > > -#define ENABLE_DESTINATION_COLOR_SPACE_LINEAR_SRGB 1 > > +#define ENABLE_DESTINATION_COLOR_SPACE_LINEAR_SRGB 0 > > My only concern here is that this might affect Apple ports. > > Could we do > > #if USE(CAIRO) > #define ENABLE_DESTINATION_COLOR_SPACE_LINEAR_SRGB 0 > #else > #define ENABLE_DESTINATION_COLOR_SPACE_LINEAR_SRGB 1 > #endif Fujii mentioned that these values were already present in port specific PlatformEnable headers so I'm fine with landing as is then. Comment on attachment 459878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=459878&action=review >>> Source/WTF/wtf/PlatformEnable.h:248 >>> +#define ENABLE_DESTINATION_COLOR_SPACE_LINEAR_SRGB 0 >> >> My only concern here is that this might affect Apple ports. >> >> Could we do >> >> #if USE(CAIRO) >> #define ENABLE_DESTINATION_COLOR_SPACE_LINEAR_SRGB 0 >> #else >> #define ENABLE_DESTINATION_COLOR_SPACE_LINEAR_SRGB 1 >> #endif > > Fujii mentioned that these values were already present in port specific PlatformEnable headers so I'm fine with landing as is then. And, this matches with how above ENABLE_DESTINATION_COLOR_SPACE_DISPLAY_P3 is defined. Committed r295144 (251234@main): <https://commits.webkit.org/251234@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 459878 [details]. |