LibWeb+LibGfx: Canvas2D filters

[ananas-dev]

This is the scaffolding of filters for CanvasRenderingContext2D.
To make this possible I had to introduce breaking changes to the Painter api of LibGfx which I'll describe below:

• I added a new Gfx::Filter object which represents the variant of possible filters
• Skia paints are now built using the list of filters in addition to the paint style

I modified the painter's api to accept filters:

void stroke_path(Gfx::Path const&, Gfx::PaintStyle const&, ReadonlySpan<Gfx::Filter>, float thickness, float global_alpha);
void fill_path(Gfx::Path const&, Gfx::PaintStyle const&, ReadonlySpan<Gfx::Filter>, float global_alpha, Gfx::WindingRule);

I kept the other versions of stroke_path and fill_path for backwards compatibility with TinyVG because filters are not implemented there yet, it's also useful for drawing shadows with less overhead.

[shlyakpavel]

Btw might conflict a bit with #2717

[shlyakpavel]

Ok, I see now.
Let's address TinyVG filters later in another PR if required. Current solution doesn't look nice and is local to Qt Chrome, but it seems to work at least

ladybird/UI/Qt/TVGIconEngine.cpp

Lines 38 to 49 in ea469fb

	for (auto const& filter : m_filters) {
	if (filter->mode() == mode) {
	for (int y = 0; y < bitmap->height(); ++y) {
	for (int x = 0; x < bitmap->width(); ++x) {
	auto original_color = bitmap->get_pixel(x, y);
	auto filtered_color = filter->function()(original_color);
	bitmap->set_pixel(x, y, filtered_color);
	}
	}
	break;
	}
	}

[ananas-dev]

Ok, I see now. Let's address TinyVG filters later in another PR if required. Current solution doesn't look nice and is local to Qt Chrome, but it seems to work at least

ladybird/UI/Qt/TVGIconEngine.cpp

Lines 38 to 49 in ea469fb

	for (auto const& filter : m_filters) {
	if (filter->mode() == mode) {
	for (int y = 0; y < bitmap->height(); ++y) {
	for (int x = 0; x < bitmap->width(); ++x) {
	auto original_color = bitmap->get_pixel(x, y);
	auto filtered_color = filter->function()(original_color);
	bitmap->set_pixel(x, y, filtered_color);
	}
	}
	break;
	}
	}

Yeah I think its better to address it in another PR to prevent this one from being too large, it should be relatively straightforward to implement on top of this pr though

[shlyakpavel]

diff --git a/Libraries/LibWeb/HTML/CanvasRenderingContext2D.cpp b/Libraries/LibWeb/HTML/CanvasRenderingContext2D.cpp
index 72fc773daa..54b17b52fb 100644
--- a/Libraries/LibWeb/HTML/CanvasRenderingContext2D.cpp
+++ b/Libraries/LibWeb/HTML/CanvasRenderingContext2D.cpp
@@ -303,8 +303,12 @@ void CanvasRenderingContext2D::stroke_internal(Gfx::Path const& path)
     auto& state = drawing_state();

     // FIXME: Honor state's line_cap, line_join, miter_limit, dash_list, and line_dash_offset.
-    painter->stroke_path(path, state.stroke_style.to_gfx_paint_style(), state.filters, state.line_width, state.global_alpha);
-
+    // painter->stroke_path(path, state.stroke_style.to_gfx_paint_style(), state.filters, state.line_width, state.global_alpha);
+    if (auto color = state.stroke_style.as_color(); color.has_value()) {
+        painter->stroke_path(path, color->with_opacity(state.global_alpha), state.line_width);
+    } else {
+        painter->stroke_path(path, state.stroke_style.to_gfx_paint_style(), state.filters, state.line_width, state.global_alpha);
+    }
     did_draw(path.bounding_box());
 }

@@ -339,8 +343,13 @@ void CanvasRenderingContext2D::fill_internal(Gfx::Path const& path, Gfx::Winding
     auto path_to_fill = path;
     path_to_fill.close_all_subpaths();
     auto& state = this->drawing_state();
-    painter->fill_path(path_to_fill, state.fill_style.to_gfx_paint_style(), state.filters, state.global_alpha, winding_rule);
-
+    //painter->fill_path(path_to_fill, state.fill_style.to_gfx_paint_style(), state.filters, state.global_alpha, winding_rule);
+    // Restore previous logic:
+    if (auto color = state.fill_style.as_color(); color.has_value()) {
+        painter->fill_path(path_to_fill, color->with_opacity(state.global_alpha), winding_rule);
+    } else {
+        painter->fill_path(path_to_fill, state.fill_style.to_gfx_paint_style(), state.filters, state.global_alpha, winding_rule);
+    }
     did_draw(path_to_fill.bounding_box());
 }

Fixes the test on Mac

[shlyakpavel]

@ananas-dev this looks weird to me as well, but I'm 100% positive that helps

[shlyakpavel]

@ananas-dev

diff --git a/Libraries/LibGfx/PainterSkia.cpp b/Libraries/LibGfx/PainterSkia.cpp
index c14abf71c0..cb2108d95b 100644
--- a/Libraries/LibGfx/PainterSkia.cpp
+++ b/Libraries/LibGfx/PainterSkia.cpp
@@ -359,7 +359,8 @@ void PainterSkia::stroke_path(Gfx::Path const& path, Gfx::PaintStyle const& pain
     auto sk_path = to_skia_path(path);
     auto paint = to_skia_paint(paint_style, filters);
     paint.setAntiAlias(true);
-    paint.setAlphaf(global_alpha);
+    //paint.setAlphaf(global_alpha);
+    (void)global_alpha;
     paint.setStyle(SkPaint::Style::kStroke_Style);
     paint.setStrokeWidth(thickness);
     impl().canvas()->drawPath(sk_path, paint);
@@ -392,7 +393,8 @@ void PainterSkia::fill_path(Gfx::Path const& path, Gfx::PaintStyle const& paint_
     sk_path.setFillType(to_skia_path_fill_type(winding_rule));
     auto paint = to_skia_paint(paint_style, filters);
     paint.setAntiAlias(true);
-    paint.setAlphaf(global_alpha);
+    //paint.setAlphaf(global_alpha);
+    (void)global_alpha;
     impl().canvas()->drawPath(sk_path, paint);
 }

[ananas-dev]

@shlyakpavel I'm trying desperate stuff but the last difference I see between the 2 code paths is the order of calling of setAntiAlias and setColor, is it possible to try to set the antialias in to_skia_paint before running apply_paint_style and remove subsequent calls to setAntiAlias in fill_path and stroke_path.

[shlyakpavel]

diff --git a/Libraries/LibGfx/PainterSkia.cpp b/Libraries/LibGfx/PainterSkia.cpp
index c14abf71c0..185cc894c7 100644
--- a/Libraries/LibGfx/PainterSkia.cpp
+++ b/Libraries/LibGfx/PainterSkia.cpp
@@ -250,6 +250,7 @@ static SkPaint to_skia_paint(Gfx::PaintStyle const& style, Span<Gfx::Filter> fil
 {
     SkPaint paint;

+    paint.setAntiAlias(true);
     apply_paint_style(paint, style);
     apply_filters(paint, filters);

@@ -341,7 +342,7 @@ void PainterSkia::stroke_path(Gfx::Path const& path, Gfx::Color color, float thi
         return;

     SkPaint paint;
-    paint.setAntiAlias(true);
+    //paint.setAntiAlias(true);
     paint.setMaskFilter(SkMaskFilter::MakeBlur(kNormal_SkBlurStyle, blur_radius / 2));
     paint.setStyle(SkPaint::kStroke_Style);
     paint.setStrokeWidth(thickness);
@@ -358,7 +359,7 @@ void PainterSkia::stroke_path(Gfx::Path const& path, Gfx::PaintStyle const& pain

     auto sk_path = to_skia_path(path);
     auto paint = to_skia_paint(paint_style, filters);
-    paint.setAntiAlias(true);
+    //paint.setAntiAlias(true);
     paint.setAlphaf(global_alpha);
     paint.setStyle(SkPaint::Style::kStroke_Style);
     paint.setStrokeWidth(thickness);

If that's what you mean, it still fails

[shlyakpavel]

diff --git a/Libraries/LibGfx/PainterSkia.cpp b/Libraries/LibGfx/PainterSkia.cpp
index c14abf71c0..0406229e29 100644
--- a/Libraries/LibGfx/PainterSkia.cpp
+++ b/Libraries/LibGfx/PainterSkia.cpp
@@ -6,6 +6,7 @@
  * SPDX-License-Identifier: BSD-2-Clause
  */

+#include "AK/Assertions.h"
 #define AK_DONT_REPLACE_STD

 #include <AK/OwnPtr.h>
@@ -74,7 +75,7 @@ static void apply_paint_style(SkPaint& paint, Gfx::PaintStyle const& style)
     if (is<Gfx::SolidColorPaintStyle>(style)) {
         auto const& solid_color = static_cast<Gfx::SolidColorPaintStyle const&>(style);
         auto color = solid_color.sample_color(Gfx::IntPoint(0, 0));
-
+        VERIFY_NOT_REACHED();
         paint.setColor(to_skia_color(color));
     } else if (is<Gfx::CanvasLinearGradientPaintStyle>(style)) {
         auto const& linear_gradient = static_cast<Gfx::CanvasLinearGradientPaintStyle const&>(style);

That's never reached lol

[shlyakpavel]

What could go wrong

        FillOrStrokeStyle fill_style { Gfx::Color::Black };
        FillOrStrokeStyle stroke_style { Gfx::Color::Black };

[shlyakpavel]

isGfx::SolidColorPaintStyle(style) does not work as expected. It's always false

[shlyakpavel]

Non-maintainer opinion: this looks good overall, with some minor suggestions

[shlyakpavel]

This enum class cannot just get a random non-enum value from unverified source, and compile-time check seems to me to be a more reasonable solution here. I suggest removing this VERIFY_NOT_REACHED default case, so if there's a new enum case not present here, it fails to compile instead of a runtime failure

[ladybird-bot]

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.