WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
234118
REGRESSION(
r286765
): [Cairo][GTK][WPE] Various SVG tests that use filters fail
https://bugs.webkit.org/show_bug.cgi?id=234118
Summary
REGRESSION(r286765): [Cairo][GTK][WPE] Various SVG tests that use filters fail
Arcady Goldmints-Orlov
Reported
2021-12-09 18:29:44 PST
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.
Attachments
Expected test result
(2.81 KB, image/png)
2021-12-09 18:29 PST
,
Arcady Goldmints-Orlov
no flags
Details
Actual test result
(2.81 KB, image/png)
2021-12-09 18:31 PST
,
Arcady Goldmints-Orlov
no flags
Details
WIP patch
(1.58 KB, patch)
2022-05-25 00:28 PDT
,
Fujii Hironori
no flags
Details
Formatted Diff
Diff
Patch
(6.36 KB, patch)
2022-05-25 00:52 PDT
,
Fujii Hironori
no flags
Details
Formatted Diff
Diff
Patch
(7.62 KB, patch)
2022-05-25 17:47 PDT
,
Fujii Hironori
no flags
Details
Formatted Diff
Diff
Patch
(7.73 KB, patch)
2022-05-30 22:43 PDT
,
Fujii Hironori
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Arcady Goldmints-Orlov
Comment 1
2021-12-09 18:31:27 PST
Created
attachment 446651
[details]
Actual test result Actual test result. Note the lighter shade of green.
Fujii Hironori
Comment 2
2022-05-25 00:07:53 PDT
WinCairo is also failing the tests.
Fujii Hironori
Comment 3
2022-05-25 00:28:12 PDT
Created
attachment 459746
[details]
WIP patch Disabling DESTINATION_COLOR_SPACE_LINEAR_SRGB can fix the issue.
Fujii Hironori
Comment 4
2022-05-25 00:52:38 PDT
Created
attachment 459747
[details]
Patch
Nikolas Zimmermann
Comment 5
2022-05-25 02:47:09 PDT
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 6
2022-05-25 13:01:04 PDT
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
Comment 7
2022-05-25 17:47:56 PDT
Created
attachment 459775
[details]
Patch
Fujii Hironori
Comment 8
2022-05-30 22:43:57 PDT
Created
attachment 459878
[details]
Patch
Don Olmstead
Comment 9
2022-06-02 12:57:34 PDT
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
Comment 10
2022-06-02 13:03:16 PDT
(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 11
2022-06-02 13:06:32 PDT
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
Comment 12
2022-06-02 14:06:17 PDT
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
Comment 13
2022-06-02 14:07:32 PDT
<
rdar://problem/94309303
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug