LibWeb: Apply CSS filters on SVGs #2248

gotlougit · 2024-11-09T11:09:37Z

The code largely mirrors the one in StackingContext::paint() for applying CSS filters to a generic context, with some minor changes.

There's a weird compile issue where matrix_with_scaled_translation() is not being found by the compiler, even though StackingContext::paint() uses it with no issues. PaintableBox also has the relevant header file imported as well, so it is not like it is missing somewhere down in an inherited object either.

Compile log (with matrix_with_scaled_translation):

/usr/bin/ccache /usr/bin/c++ -DENABLE_COMPILETIME_FORMAT_CHECK -DHAVE_PULSEAUDIO=1 -DLibWeb_EXPORTS -DSK_CODEC_DECODES_BMP -DSK_CODEC_DECODES_GIF -DSK_CODEC_DECODES_ICO -DSK_CODEC_DECODES_JPEG -DSK_CODEC_DECODES_PNG -DSK_CODEC_DECODES_RAW -DSK_CODEC_DECODES_WBMP -DSK_CODEC_DECODES_WEBP -DSK_DISABLE_TRACING -DSK_ENABLE_AVX512_OPTS -DSK_ENABLE_PRECOMPILE -DSK_FONTMGR_ANDROID_AVAILABLE -DSK_FONTMGR_FONTCONFIG_AVAILABLE -DSK_FONTMGR_FREETYPE_DIRECTORY_AVAILABLE -DSK_FONTMGR_FREETYPE_EMBEDDED_AVAILABLE -DSK_FONTMGR_FREETYPE_EMPTY_AVAILABLE -DSK_GAMMA_APPLY_TO_A8 -DSK_GANESH -DSK_GL -DSK_HAS_WUFFS_LIBRARY -DSK_R32_SHIFT=16 -DSK_SUPPORT_PDF -DSK_TYPEFACE_FACTORY_FREETYPE -DSK_USE_PERFETTO -DSK_USE_VMA -DSK_VULKAN -DSK_XML -DUSE_FONTCONFIG=1 -DUSE_VULKAN=1 -D_FILE_OFFSET_BITS=64 -I/app -I/app/Userland/Services -I/app/Userland/Libraries -I/app/Build/release/Lagom -I/app/Build/release/Lagom/Userland/Services -I/app/Build/release/Lagom/Userland/Libraries -I/app/Meta/Lagom/../.. -I/app/Meta/Lagom/../../Userland -I/app/Meta/Lagom/../../Userland/Libraries -I/app/Meta/Lagom/../../Userland/Services -I/app/Build/release -isystem /app/Build/release/vcpkg_installed/x64-linux/include -isystem /app/Build/release/vcpkg_installed/x64-linux/include/skia -O2 -g -DNDEBUG -std=c++23 -fPIC -fdiagnostics-color=always -Wall -Wextra -fno-exceptions -ffp-contract=off -Wcast-qual -Wformat=2 -Wimplicit-fallthrough -Wmissing-declarations -Wmissing-field-initializers -Wsuggest-override -Wno-invalid-offsetof -Wno-unknown-warning-option -Wno-unused-command-line-argument -Werror -Wno-expansion-to-defined -Wno-literal-suffix -Wno-dangling-reference -fno-semantic-interposition -fvisibility-inlines-hidden -fstack-protector-strong -fstrict-flex-arrays=2 -Wno-maybe-uninitialized -Wno-shorten-64-to-32 -fsigned-char -ggnu-pubnames -fPIC -O2 -g1 -MD -MT Lagom/Userland/Libraries/LibWeb/CMakeFiles/LibWeb.dir/Painting/StackingContext.cpp.o -MF Lagom/Userland/Libraries/LibWeb/CMakeFiles/LibWeb.dir/Painting/StackingContext.cpp.o.d -o Lagom/Userland/Libraries/LibWeb/CMakeFiles/LibWeb.dir/Painting/StackingContext.cpp.o -c /app/Userland/Libraries/LibWeb/Painting/StackingContext.cpp
/app/Userland/Libraries/LibWeb/Painting/StackingContext.cpp: In static member function 'static void Web::Painting::StackingContext::paint_svg(Web::PaintContext&, const Web::Painting::PaintableBox&, Web::Painting::PaintPhase)':
/app/Userland/Libraries/LibWeb/Painting/StackingContext.cpp:122:23: error: 'matrix_with_scaled_translation' was not declared in this scope
  122 |             .matrix = matrix_with_scaled_translation(transform_matrix, to_device_pixels_scale),
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
At global scope:
cc1plus: note: unrecognized command-line option '-Wno-shorten-64-to-32' may have been intended to silence earlier diagnostics
cc1plus: note: unrecognized command-line option '-Wno-unused-command-line-argument' may have been intended to silence earlier diagnostics
cc1plus: note: unrecognized command-line option '-Wno-unknown-warning-option' may have been intended to silence earlier diagnostics
[1692/2668] Building CXX object Lagom/Userla....dir/ReferrerPolicy/AbstractOperations.cpp.o
ninja: build stopped: subcommand failed.

Fixes #2015

gmta · 2024-11-09T22:22:35Z

@gotlougit matrix_with_scaled_translation is a static function defined in the .cpp, so it needs to be defined before its first usage. Currently it's defined after ::paint_svg().

Userland/Libraries/LibWeb/Painting/StackingContext.cpp

gotlougit · 2024-11-10T05:11:42Z

Thanks for the review, I had some more follow-up questions.
Thanks @gmta for explaining about the static function definition/declaration issue.

gotlougit · 2024-11-21T07:45:52Z

I ended up creating the ApplyFilters command in a similar manner to what ApplyOpacity does, and this modified method now indeed works to fix the reported SVG filter bug. Hopefully this is more in line with what we need, but I am not as sure about the CSS side of things, would be open to helping fix that one up too.

ladybird-bot · 2024-11-21T07:48:34Z

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

AtkinsSJ · 2024-11-21T12:06:22Z

The Tests/LibWeb/Screenshot/input/css-filter.html test is failing on CI. (No screenshots were dumped, I thought someone fixed that?)

Also, please remove the LibWeb: Apply CSS filters on SVGs and Revert "LibWeb: Apply CSS filters on SVGs" commits from your branch.

gotlougit · 2024-11-21T14:05:20Z

Ran the test manually to get the observed output (resolution is off due to windowing differences)

I think the CSS filters are not being applied properly/potentially being applied twice?

gotlougit · 2024-11-22T13:51:22Z

Seems like the CI now passes properly, due to the fact that StackingContext calls ApplyFilters now instead of doing the filter stuff inside push_stacking_context. This helped keep the same behavior as before and still allows code reuse for implementing SVG filters properly.

gmta · 2024-11-22T13:53:41Z

Libraries/LibWeb/Painting/DisplayListPlayerSkia.cpp

@@ -1103,6 +1077,41 @@ void DisplayListPlayerSkia::apply_opacity(ApplyOpacity const& command)
    canvas.saveLayer(nullptr, &paint);
 }

+void DisplayListPlayerSkia::apply_filters(ApplyFilters const& command)
+{
+    if (command.filter.is_none()) {


How does this work when command.opacity < 1 and filters are set? Is the opacity applied twice?

Ladybird seems to report opacity correctly when testing with an SVG and with a <div> with some text inside it (same filter applied to both).

The code duplication can create confusion though. What we could do is get rid of ApplyOpacity by itself and merge that into ApplyFilters, or alternatively, get rid of the duplication here. I guess it depends on the scope of what we'd like to keep as a "filter". I would be open to either approach, although my personal preference would be for the second option.

note: checking the opacity is less than one could probably be promoted to checking for non-trivial filters being applied (i.e. those introducing a substantial visual change, e.g. opacity < 1 or hue ≠ 0° etc.) as an optimisation. not sure if that is the correct moment to update, or whether it is a good place for an optimisation either.

kalenikaliaksandr

Please rearrange changes in commits such that any specific commit does not introduce a regression:

LibWeb: Add ApplyFilters command for both CSS and SVGs
LibWeb: Remove filter from PushStackingContext <-- at this point filters has regressed for stacking contexts
LibWeb: Use apply_filters() in StackingContext

I would expect to see commits in the following order:

move filters application from PushStackingContext into ApplyFiltes
reuse ApplyFilters for SVG

kalenikaliaksandr · 2024-11-22T13:55:49Z

Libraries/LibWeb/Painting/SVGSVGPaintable.cpp

            context.display_list_recorder().apply_opacity(computed_values.opacity());
        }

+        context.display_list_recorder().apply_filters(paintable.computed_values().opacity(), paintable.computed_values().filter());


it seems like we no longer need apply_opacity() here if apply_filters() is responsible for both filters and opacity

This is pretty similar to #2248 (comment), I am open for discussion on what to do next.

Libraries/LibWeb/Painting/PaintableBox.cpp

This helps reuse this code in other areas, such as for filters for SVGs

The opacity is still being set separately by using ApplyOpacity for both CSS and SVG

gotlougit · 2024-11-23T17:41:22Z

I ended up removing the opacity code from the new ApplyFilters command since the paint code seems to call ApplyOpacity. Locally tests pass on my machine, so I think this is a decent enough change.

Psychpsyo · 2024-11-23T21:13:59Z

This made it so that any content drawn after an SVG with a filter is also affected by that filter.
I've PR'd a fix for that here: #2537

kalenikaliaksandr requested changes Nov 9, 2024

View reviewed changes

Userland/Libraries/LibWeb/Painting/StackingContext.cpp Outdated Show resolved Hide resolved

kalenikaliaksandr reviewed Nov 9, 2024

View reviewed changes

Userland/Libraries/LibWeb/Painting/StackingContext.cpp Outdated Show resolved Hide resolved

shlyakpavel mentioned this pull request Nov 15, 2024

Test Ladybird linebender/resvg-test-suite#55

Draft

gotlougit requested review from kalenikaliaksandr and gmta November 21, 2024 07:45

gotlougit force-pushed the apply-css-filters-on-svg branch from 4b2c19b to 1aa6d5b Compare November 21, 2024 07:54

gotlougit requested review from AtkinsSJ, alimpfard and trflynn89 as code owners November 21, 2024 07:54

gotlougit force-pushed the apply-css-filters-on-svg branch 3 times, most recently from 93cc506 to 664f091 Compare November 21, 2024 08:07

gotlougit force-pushed the apply-css-filters-on-svg branch from 664f091 to 2600171 Compare November 21, 2024 14:05

gmta reviewed Nov 22, 2024

View reviewed changes

kalenikaliaksandr requested changes Nov 22, 2024

View reviewed changes

gotlougit added 2 commits November 22, 2024 19:53

LibWeb: Migrate CSS filter application to new ApplyFilters command

9652388

This helps reuse this code in other areas, such as for filters for SVGs

LibWeb: Use ApplyFilters command for SVGs

29cc289

gotlougit force-pushed the apply-css-filters-on-svg branch from 602f847 to 29cc289 Compare November 22, 2024 14:25

LibWeb: Deduplicate opacity code in ApplyFilters

54e8eea

The opacity is still being set separately by using ApplyOpacity for both CSS and SVG

kalenikaliaksandr merged commit ecdb53c into LadybirdBrowser:master Nov 23, 2024
6 checks passed

Psychpsyo mentioned this pull request Nov 23, 2024

LibWeb: Unapply CSS filters after applying them to SVG #2537

Merged

gotlougit deleted the apply-css-filters-on-svg branch November 24, 2024 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LibWeb: Apply CSS filters on SVGs #2248

LibWeb: Apply CSS filters on SVGs #2248

gotlougit commented Nov 9, 2024

gmta commented Nov 9, 2024

gotlougit commented Nov 10, 2024

gotlougit commented Nov 21, 2024

ladybird-bot commented Nov 21, 2024

AtkinsSJ commented Nov 21, 2024

gotlougit commented Nov 21, 2024

gotlougit commented Nov 22, 2024

gmta Nov 22, 2024

gotlougit Nov 22, 2024 •

edited

Loading

konradekk Nov 22, 2024

kalenikaliaksandr left a comment

kalenikaliaksandr Nov 22, 2024

gotlougit Nov 22, 2024

gotlougit commented Nov 23, 2024

Psychpsyo commented Nov 23, 2024

LibWeb: Apply CSS filters on SVGs #2248

LibWeb: Apply CSS filters on SVGs #2248

Conversation

gotlougit commented Nov 9, 2024

gmta commented Nov 9, 2024

gotlougit commented Nov 10, 2024

gotlougit commented Nov 21, 2024

ladybird-bot commented Nov 21, 2024

AtkinsSJ commented Nov 21, 2024

gotlougit commented Nov 21, 2024

gotlougit commented Nov 22, 2024

gmta Nov 22, 2024

Choose a reason for hiding this comment

gotlougit Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

konradekk Nov 22, 2024

Choose a reason for hiding this comment

kalenikaliaksandr left a comment

Choose a reason for hiding this comment

kalenikaliaksandr Nov 22, 2024

Choose a reason for hiding this comment

gotlougit Nov 22, 2024

Choose a reason for hiding this comment

gotlougit commented Nov 23, 2024

Psychpsyo commented Nov 23, 2024

gotlougit Nov 22, 2024 •

edited

Loading