Bug 202739 – [GTK][WPE] Renderization of Conic gradients

• Component: New Bugs
• Status: RESOLVED FIXED

[Diego Pino]

[GTK] Implement conic gradient

[Diego Pino]

Created attachment 380525 [details]
Patch

[Diego Pino]

This patch implements painting of a conic-gradient in WebKitGTK.

Cairo lacks a direct primitive, such as `cairo_pattern_create_linear` or `cairo_pattern_create_radial`, to paint a conic gradient. However, it's possible to paint a conic gradient using a mesh gradient (`cairo_pattern_create_mesh`), since mesh gradients can adopt any shape.

To paint a conic gradient using a mesh gradient, what I do is to paint a circle sector for each stop color in the gradient. Each circle sector is defined as a patch (`cairo_mesh_pattern_begin_patch`). A sector is defined by a line from the center and a bezier line until the end of the color stop offset, then back to the center. After that, I give each of the four corners defined the gradient colors of the sector. For the mathematics of bezier lines and how to use them as arcs, check out: https://pomax.github.io/bezierinfo/#circles_cubic.

To make the gradient occupy the whole rect where it's defined, I needed the width of the rect, which I think it's not available at the time the gradient is computed. That lead me to tweak the code that initializes a ConicGradient with data for the radius of the circumference (computed out of the width of the rect). I think this change is not correct. Ideally I'd prefer to figure out the width of the rect in the platform dependent code.

There are several layout tests that are failing. In many of these failures the difference regarding the reference image is a very thin line. I think what it's happening is that the border line between sectors is getting over painted. Possibly there's a mistake on how a sector is painted. I looked into it but I haven't figured it out yet.

[Carlos Alberto Lopez Perez]

Comment on attachment 380525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380525&action=review

> Source/WebCore/css/CSSGradientValue.cpp:1364
> +    float startRadius = sqrt(powf(size.width() - centerPoint.x(), 2) + powf(size.width() - centerPoint.y(), 2));

You can use std::hypot here instead (see bug 198483)

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:62
> +static void
> +add_color_stop_rgba(cairo_pattern_t* gradient, Gradient::ColorStop thisColor, float globalAlpha);
> +
> +static cairo_pattern_t*
> +create_conic(float xo, float yo, float r, float angleRadians,
> +    Gradient::ColorStopVector stops, float globalAlpha);
> +
> +static void
> +conic_sector(cairo_pattern_t* gradient, float cx, float cy, float r, float angleRadians,
> +    Gradient::ColorStop firstColor, Gradient::ColorStop lastColor, float globalAlpha);
> +
> +static Gradient::ColorStop
> +interpolate_color(Gradient::ColorStop firstColor, Gradient::ColorStop lastColor);
> +
> +static void
> +set_corner_color_rgba(cairo_pattern_t* gradient, int id, Gradient::ColorStop thisColor, float globalAlpha);

In webkit code the preferred style is to put the return value of the function and the function header on the same line

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:104
> +static void
> +add_color_stop_rgba(cairo_pattern_t *gradient, Gradient::ColorStop stop, float globalAlpha)

same here

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:117
> +static cairo_pattern_t*
> +create_conic(float xo, float yo, float r, float angleRadians,

same here

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:142
> +static void
> +conic_sector(cairo_pattern_t *gradient, float cx, float cy, float r, float angleRadians,

and here

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:180
> +static void
> +set_corner_color_rgba(cairo_pattern_t* gradient, int id, Gradient::ColorStop stop, float globalAlpha)

here also

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:193
> +static Gradient::ColorStop
> +interpolate_color(Gradient::ColorStop from, Gradient::ColorStop to)

and here

[Diego Pino]

Created attachment 380777 [details]
Patch

[Diego Pino]

I didn't read the previous comment until I pushed a new patch. I will change the name of the functions in a new patch.

I have fixed most of the failing tests. There were two kind of issues:

- Wrong positioning of some sectors due to making all the sectors of the circle share a common center point. It's necessary to compute a different center for each sector depending on its position in the circle.

- Some gradient colors do not match. This might be due to how Cairo computes gradient colors or might be a problem in the current code. When a conic gradient has only two colors, I need to compute a middle stop color because it's not possible to paint a whole circle with a single Bezier line. Perhaps the middle color I compute is not the same as a graphics library would compute, thus the difference. In any case, the difference is minimal (0.01%) and I think the test should be marked as passed somehow. I manage to create a new expected tests for one of these failing tests, but the other two are non trivial so I added the tests to GTK/TestExpectations as ImageFailure.

[Diego Pino]

Created attachment 380822 [details]
Patch

[Diego Pino]

* Addressed comments about function naming convention.
* Fixed the remaining two failing tests. To paint a conic gradient of two colors accurately it's necessary to draw the cone using 4 bezier lines (5 color stops).

I think the patch is ready now. All conic tests are passing. I think however the code to figure out the radius of the cone should be moved somehow to platform dependent code.

[Carlos Alberto Lopez Perez]

Comment on attachment 380822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380822&action=review

> Source/WebCore/css/CSSGradientValue.cpp:1364
> +    float startRadius = sqrt(powf(size.width() - centerPoint.x(), 2) + powf(size.width() - centerPoint.y(), 2));

Not sure if you missed my previous comment, but you can use std::hypot here (see bug 198483)

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:137
> +static void
> +addConicSector(cairo_pattern_t *gradient, float cx, float cy, float r, float angleRadians,
> +    Gradient::ColorStop from, Gradient::ColorStop to, float globalAlpha)

Please fix the style here (return value in same line than function declaration)

> Source/cmake/OptionsGTK.cmake:165
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSS_CONIC_GRADIENTS PRIVATE ON)

The implementation looks generic enough to work also on the WPE port. Is that the case? Then why not enable it there as well?

> LayoutTests/ChangeLog:6
> +        [GTK] Implement conic gradient
> +        https://bugs.webkit.org/show_bug.cgi?id=202739
> +
> +        * platform/gtk/fast/gradients/conic-gradient-alpha-expected.html: Added.

Other than adding the new expectation you have to unskip this test for platform GTK. For that add the line below to the file LayoutTests/platform/gtk/TestExpectations

fast/gradients/conic-gradient-alpha.html [ Pass ]

[Diego Pino]

Created attachment 380880 [details]
Patch

[Diego Pino]

Updated patch.

Some of Lea Verou's conic gradient examples (http://leaverou.github.io/conic-gradient/) were not working. It was necessary to add a check for the first and last stop colors offset. If the offset is not zero or one respectively, it's then necessary to add extra stops with offset equals to zero and one.

[Diego Pino]

Created attachment 384570 [details]
Patch

[Diego Pino]

Created attachment 384580 [details]
Patch

[Diego Pino]

Created attachment 384591 [details]
Patch

[Diego Pino]

Created attachment 385117 [details]
Patch

[Diego Pino]

I've tackled all the issues commented in the last review:

- Style issues.
- Use std::hypot.
- Add platform specific test to TestExpections with flag PASS.
- Enable feature for WPE.

[Carlos Alberto Lopez Perez]

Comment on attachment 385117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385117&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by Carlos Alberto Lopez Perez.

Please remove this and leave the "reviewed by nobody oops", I still haven't reviewed this with an r+ :)

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:59
> +static void addColorStopRGBA(cairo_pattern_t* gradient, Gradient::ColorStop thisColor, float globalAlpha);
> +
> +#if PLATFORM(GTK) || PLATFORM(WPE)
> +
> +typedef struct point_t {
> +    double x, y;
> +} point_t;
> +
> +static void addConicSector(cairo_pattern_t* gradient, float cx, float cy, float r, float angleRadians,
> +    Gradient::ColorStop firstColor, Gradient::ColorStop lastColor, float globalAlpha);
> +static cairo_pattern_t* createConic(float xo, float yo, float r, float angleRadians,
> +    Gradient::ColorStopVector stops, float globalAlpha);
> +static Gradient::ColorStop interpolateColorStop(Gradient::ColorStop firstColor, Gradient::ColorStop lastColor);
> +static void setCornerColorRGBA(cairo_pattern_t* gradient, int id, Gradient::ColorStop thisColor, float globalAlpha);
> +
> +#endif
> +

This should be moved to Gradient.h

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:76
> +#if PLATFORM(GTK) || PLATFORM(WPE)
> +        [&] (const ConicData& data)  -> cairo_pattern_t* {
> +            return createConic(data.point0.x(), data.point0.y(), data.startRadius, data.angleRadians, stops(), globalAlpha);
> +#else
>          [&] (const ConicData&)  -> cairo_pattern_t* {
>              // FIXME: implement conic gradient rendering.
>              return nullptr;
> +#endif

Is something on this implementation that is GTK/WPE specific and won't work on other platforms using Cairo? Otherwise, maybe its a good idea to not ifdef the implementation?

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:117
> +#if PLATFORM(GTK) || PLATFORM(WPE)

Ditto

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:120
> +static cairo_pattern_t* createConic(float xo, float yo, float r, float angleRadians,
> +    Gradient::ColorStopVector stops, float globalAlpha)

This can be written in one line. In webkit style code is ok to write the function header in one very long line instead of breaking it in several

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:156
> +static void addConicSector(cairo_pattern_t *gradient, float cx, float cy, float r, float angleRadians,
> +    Gradient::ColorStop from, Gradient::ColorStop to, float globalAlpha)

Ditto.

[Diego Pino]

Created attachment 385556 [details]
Patch

[Diego Pino]

Amended messaged "Reviewed by" and reformatting of function declarations.

Regarding the other two issues:

  - The functions declaration at the beginning of GradientCairo are only meant to be used in this file, that's why I declared them as static (they're not meant to be exported). From my point of view, they don't belong to a header file because they're static functions. In fact, the reason why I declared these functions in first place was to allow calling them at any point in the code (otherwise functions can only call functions that were implemented before). If this is not a common practice in the WebKit codebase I can remove these declarations and rearrange the order of the functions.

  - About the guards for GTK and WPE, the reason is that in previous patches all EWS were passing except WinCairo. It failed on a compilation error. I looked into the log (https://ews-build.webkit.org/#/builders/12/builds/11280) but I couldn't figure out what was the issue. Eventually I decided to wrap the relevant code with #ifdefs for GTK and WPE.

[Carlos Alberto Lopez Perez]

BTW, the Mac/iOS EWS have failed to build this last upload.

(In reply to Diego Pino from comment #18)
> Amended messaged "Reviewed by" and reformatting of function declarations.
> 
> Regarding the other two issues:
> 
>   - The functions declaration at the beginning of GradientCairo are only
> meant to be used in this file, that's why I declared them as static (they're
> not meant to be exported). From my point of view, they don't belong to a
> header file because they're static functions. In fact, the reason why I
> declared these functions in first place was to allow calling them at any
> point in the code (otherwise functions can only call functions that were
> implemented before). If this is not a common practice in the WebKit codebase
> I can remove these declarations and rearrange the order of the functions.
> 

Yes, in that case rearranging the order is preferred I think


>   - About the guards for GTK and WPE, the reason is that in previous patches
> all EWS were passing except WinCairo. It failed on a compilation error. I
> looked into the log
> (https://ews-build.webkit.org/#/builders/12/builds/11280) but I couldn't
> figure out what was the issue. Eventually I decided to wrap the relevant
> code with #ifdefs for GTK and WPE.

Ok

[Diego Pino]

Created attachment 385759 [details]
Patch

[Diego Pino]

Thanks Carlos for your patience and multiple reviews.

I updated the code to get rid of the static declarations at the beginning of `GradientCairo.cpp` and rearranged the code accordingly.

The reason why the iOS/Mac ports started to fail was that there has been a major refactoring in `Gradient.h`. My patch added an additional piece of information (the radius) to ConicData (Gradient.h). However, this data is not part of the spec (https://www.w3.org/TR/css-images-4/#conic-gradients).

Thus, I have removed in the last update the computation of the radius in `Gradient.h`. However, the GTK/WPE ports need a radius to compute the conic gradient (or at least the width and height of the area in where to paint). The Mac port passes a rect when computing the gradient so that information is implicit, but the GTK/WPE ports simply pass an alpha color. So, I had a dilemma I either change the GTK/WPE API of `createPlatformGradient` or I use a large value for the radius which will later be clipped. I opted for the latter solution.

[Carlos Alberto Lopez Perez]

Comment on attachment 385759 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385759&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by nobody oops.

This should be:
"Reviewed by NOBODY (OOPS!)."
Otherwise the tooling will get confused

> LayoutTests/ChangeLog:6
> +        Reviewed by nobody oops.

Ditto

> ChangeLog:6
> +        Reviewed by nobody oops.

Ditto

[Diego Pino]

Created attachment 385788 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 385788 [details]
Patch

r=me. thanks!

[WebKit Commit Bot]

Comment on attachment 385788 [details]
Patch

Clearing flags on attachment: 385788

Committed r253685: <https://trac.webkit.org/changeset/253685>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58043610>