Created attachment 446650 [details] Expected test result After r286765, a number of SVG tests that use filters have started failing, displaying a different shade of green from the expected one.
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].
<rdar://problem/94309303>