Bug 45599 – [Cairo] Generalize ContextShadow from the Qt port and use it for shadow code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

I believe we should fork Qt's ContextShadow implementation and use it for all Cairo shadowing code. Reasons for this:

1. It will better abstract shadowing logic. My initial implementation of this has already  simplified the shadowing code in places like FontCairo.cpp. It will also make it simpler to fix shadow bugs and add full-shadow support in places where it is broken: (see https://bugs.webkit.org/show_bug.cgi?id=45597).

2. Reduce the number of full-surface copies. The SVG filter code requires one surface copy for the input and makes another copy for the output. That's probably one more than we need.

3. It can be alpha-only. webkit-box-shadow, text shadows and canvas shadows are alpha only. The blurring algorithm should be more efficient if it's operating on only one channel.

4. We can support shadows without depending on SVG filters. This is a minor point, but it's a nice thing to have for embedders who want a small library.

Why fork:

The Qt version of ContextShadow is written with fast transposition in mind (Cairo doesn't support this) and four channel images. There is very little extra logic outside of this (calculating the appropriate transformations for the temporary surface, for instance). I'm confidant unforking is possible though, especially if some other port wishes to use this abstraction.

I should have a first-round patch for this early next week.

[Ariya Hidayat]

Can you wait until I land the patch for https://bugs.webkit.org/show_bug.cgi?id=44222 first? The blur code is rewritten to be smaller and faster there and you certainly don't want to redo the generalization.

[Martin Robinson]

Hrm. If you land a generalized version at some time in the future, the cost of switching the implementation should be almost zero. In the Cairo version I've kept the interface to ShadowContext exactly the same as your version. The blur algorithm that I'm using is currently a very naive sliding window type, because the most important optimization for us was to properly handle clipping. This means that when you land your code, there will be a large incentive for us to share it and I don't mind doing that work.

Additionally, we'd also like to expose our tiling optimizations on it as a higher level method. I think having a fork (even if for a short amount of time), will allow us to do this without getting in your way.

[Martin Robinson]

Arriya informed me that his soon-to-be-posted changes to the blur algorithm do not assume fast transposition. I think instead of forking we should probably just generalize ContextShadow. I've posted prelimary patch which does this. I'll prepare it for real once Arriya lands his improvements (an advance version of which this patch uses).

[Martin Robinson]

Created attachment 67620 [details]
Preliminary patch for this issue

[Martin Robinson]

Created attachment 67766 [details]
Generalize ContextShadow and add a (disabled) Cairo implementation

[WebKit Review Bot]

Attachment 67766 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/cairo/CairoUtilities.h:29:  cairo_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/ContextShadow.h:37:  cairo_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/ContextShadow.h:38:  cairo_surface_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Ariya has landed his awesome improvements to the blurring algorithm, so I've posted the first patch for this issue. I've decided to split this into two patches.

1. The first will generalize ContextShadow and add an unused Cairo implementation.
2. The second will activate it for all shadow code and update the baselines.

This should make the first patch easier to review by those unfamiliar with the Cairo code.

[Martin Robinson]

Here is a bug tracking the issue with check-webkit-style: https://bugs.webkit.org/show_bug.cgi?id=45867

[Martin Robinson]

Created attachment 67804 [details]
Patch with Alex's suggestions

[WebKit Review Bot]

Attachment 67804 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/cairo/CairoUtilities.h:29:  cairo_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/ContextShadow.h:38:  cairo_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/ContextShadow.h:39:  cairo_surface_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alejandro G. Castro]

*** Bug 43962 has been marked as a duplicate of this bug. ***

[Dirk Schulze]

Comment on attachment 67804 [details]
Patch with Alex's suggestions

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

Ok, great patch. I would like to see Ariya or someother guy of the Qt folks to look at the qt changes. Otherwise looks good, please fix the style issues. Did not look at the style bot results. r- for the issues mentioned above. If no Qt guy is available, I look at the qt changes again.

> WebCore/platform/graphics/ContextShadow.cpp:33
> +#include "MathExtras.h"
> +#include "Timer.h"

shouldn'T it be <wtf/MathExtras.h>? not sure about timer. Why do you need timer?

> WebCore/platform/graphics/ContextShadow.cpp:57
> -    if (!color.isValid() || !color.alpha()) {
> +    if (!m_color.isValid()) {

Why not chechink for alpha?

> WebCore/platform/graphics/ContextShadow.cpp:76
> +    m_offset = FloatSize(0, 0);

Just FloatSize()

> WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:49
> +// ContextShadow needs a scratch image as the buffer for the blur filter.
> +// Instead of creating and destroying the buffer for every operation,
> +// we create a buffer which will be automatically purged via a timer.
> +class PurgeScratchBufferTimer : public TimerBase {
> +private:

Ah here you need the timer, can the include get removed on th platform independent file?

[Martin Robinson]

Created attachment 68401 [details]
Patch with Dirk's suggestions

[Martin Robinson]

(In reply to comment #12)

Thanks for the review!

> > WebCore/platform/graphics/ContextShadow.cpp:33
> > +#include "MathExtras.h"
> > +#include "Timer.h"
> shouldn'T it be <wtf/MathExtras.h>? not sure about timer. Why do you need timer?

Fixed! Also removed the extra Timer.h.

> > WebCore/platform/graphics/ContextShadow.cpp:57
> > -    if (!color.isValid() || !color.alpha()) {
> > +    if (!m_color.isValid()) {
> Why not chechink for alpha?

I'm not sure where that went. I added it back.

> > WebCore/platform/graphics/ContextShadow.cpp:76
> > +    m_offset = FloatSize(0, 0);
> Just FloatSize()

Fixed!

[Ariya Hidayat]

Comment on attachment 68401 [details]
Patch with Dirk's suggestions

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

Nice stuff :)

> WebCore/platform/graphics/ContextShadow.h:113
> +    void calculateMinimalLayerRect(const FloatRect& layerArea, const IntRect& clipRect);

I guess 'calculateLayerBoundingRect' is less complicated. 'Minimal' there does not add anything.


Otherwise, LGTM.

[Martin Robinson]

(In reply to comment #15)

> I guess 'calculateLayerBoundingRect' is less complicated. 'Minimal' there does not add anything.

Thanks for the review! I'll change this before landing.

[Martin Robinson]

Committed r68145: <http://trac.webkit.org/changeset/68145>