Bug 234118 – REGRESSION(r286765): [Cairo][GTK][WPE] Various SVG tests that use filters fail

• Component: SVG
• Status: RESOLVED FIXED

[Arcady Goldmints-Orlov]

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.

[Arcady Goldmints-Orlov]

Created attachment 446651 [details]
Actual test result

Actual test result. Note the lighter shade of green.

[Fujii Hironori]

WinCairo is also failing the tests.

[Fujii Hironori]

Created attachment 459746 [details]
WIP patch

Disabling DESTINATION_COLOR_SPACE_LINEAR_SRGB can fix the issue.

[Fujii Hironori]

Created attachment 459747 [details]
Patch

[Nikolas Zimmermann]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 459775 [details]
Patch

[Fujii Hironori]

Created attachment 459878 [details]
Patch

[Don Olmstead]

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

[Don Olmstead]

(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.

[Fujii Hironori]

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.

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/94309303>