Bug 30960 – [CAIRO] shadow support for Canvas and SVG

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Schulze]

Cairo needs shadow support for Canvas and SVG.

[Dirk Schulze]

Created attachment 42227 [details]
shadow support for Cairo

Canvas/SVG shadow support for Cairo

[Brent Fulgham]

You will need to add the new GraphicContextFilter.cpp to the Visual Studio (and probably Xcode) projects.  I can help you with this, perhaps as a second patch, since messing with these project files is historically hard to do as a landable patch.

[Dirk Schulze]

(In reply to comment #2)
> You will need to add the new GraphicContextFilter.cpp to the Visual Studio (and
> probably Xcode) projects.  I can help you with this, perhaps as a second patch,
> since messing with these project files is historically hard to do as a landable
> patch.

It would ne neccessary for WinCairo, yes. This can be a follow-up.
XCode and the other build systems don't need GraphicContextFilter.cpp yet, since it is just used to add shadows on Cairo at the moment (but can be added and may be usefull for CSS filters in the future).

[Dirk Schulze]

Created attachment 42260 [details]
add GCFilter to the win build system

This should add GraphicsContextFilter to the windows build system.

[Simon Fraser (smfr)]

Comment on attachment 42227 [details]
shadow support for Cairo

> Index: WebCore/platform/graphics/GraphicsContext.h
> ===================================================================
> +        void createPlatformShadow(PassOwnPtr<ImageBuffer> buffer, Color shadowColor, FloatRect shadowRect, float kernelSize);

You should pass Color and FloatRect as const references.

> Index: WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp
> ===================================================================

> +static inline void drawPathShadow(GraphicsContext* context, GraphicsContextPrivate* gcp, bool fillShadow, bool strokeShadow)
> +{
> +
> +        // calculate the kernel size according to the HTML5 specification

Huh? HTML5 has something to say about filters? Oh, canvas. You should say canvas (but this isn't just called for canvas).


> Index: WebCore/platform/graphics/cairo/ImageCairo.cpp
> ===================================================================

> +    // Draw the shadow
> +#if ENABLE(FILTERS)
> +    IntSize shadowSize;
> +    int shadowBlur;
> +    Color shadowColor;
> +    if (context->getShadow(shadowSize, shadowBlur, shadowColor)) {
> +        // calculate the kernel size according to the HTML5 specification
> +        float kernelSize = (shadowBlur < 8 ? shadowBlur / 2.f : sqrt(shadowBlur * 2.f));
> +        int blurRadius = ceil(kernelSize);
> +        IntSize shadowBufferSize(dstRect.width() + blurRadius * 2, dstRect.height() + blurRadius * 2);
> +        FloatRect shadowRect = FloatRect(dstRect.location(), shadowBufferSize);
> +        shadowRect.move(shadowSize.width() - kernelSize, shadowSize.height() - kernelSize);
> +        shadowColor = colorWithOverrideAlpha(shadowColor.rgb(), (shadowColor.alpha() *  context->getAlpha()) / 255.f);
> +
> +        //draw shadow into a new ImageBuffer
> +        OwnPtr<ImageBuffer> shadowBuffer = ImageBuffer::create(shadowBufferSize);
> +        cairo_t* shadowContext = shadowBuffer->context()->platformContext();
> +        cairo_set_source(shadowContext, pattern);
> +        cairo_translate(shadowContext, -dstRect.x(), -dstRect.y());
> +        cairo_rectangle(shadowContext, 0, 0, dstRect.width(), dstRect.height());
> +        cairo_fill(shadowContext);
> +
> +        context->createPlatformShadow(shadowBuffer.release(), shadowColor, shadowRect, kernelSize);

Can you share this code?

> Index: WebCore/platform/graphics/filters/GraphicsContextFilter.h
> ===================================================================

> +    class GraphicsContextFilter : public Filter {

The name of this class is confusing. There's an SVGFilter, but that ultimately renders into a GraphicsContext. What is special about this one? Should this instead be some kind of helper class that makes it easy to apply some filter to drawing into a GraphicsContext?

> +    public:
> +        static PassRefPtr<GraphicsContextFilter> create();
> +
> +        bool effectBoundingBoxMode() { return false; }

This method should be const, and I prefer to see 'virtual' in all subclasses.

> +        void calculateEffectSubRegion(FilterEffect*) { }
> +        FloatRect filterRegion() { return FloatRect(); }

This should be const (in the base class and all subclasses).

> +        FloatRect sourceImageRect() { return FloatRect(); }

This should be const (in the base class and all subclasses).

It looks OK, but I'd like to hear some thoughts about GraphicsContextFilter.

[Eric Seidel (no email)]

Comment on attachment 42260 [details]
add GCFilter to the win build system

I'm not sure why this patch exists by itself.  It seems it should be part of the other patch.  r-

[Brent Fulgham]

Created attachment 43089 [details]
Update patch for Simon Fraser and Eric Seidel's comments.

Updated original patch:
1.  Updated to build with new ColorSpace data types.
2.  Revised const values per Simon's comments.
3.  Clarified HTML5 comment regarding origins of the shadow calculation.
4.  Renamed GraphicsContextFilter as "ImageBufferFilter", since that more accurately describes its function.
5.  Included the VS project changes in the patch.

[Benjamin Otte]

> +static inline void copyContextProperties(cairo_t* srcCr, cairo_t* dstCr)
> +{
> +    cairo_set_antialias(dstCr, cairo_get_antialias(srcCr));
> +    double dashes, offset;
> +    cairo_get_dash(srcCr, &dashes, &offset);
> +    cairo_set_dash(dstCr, &dashes, cairo_get_dash_count(srcCr), offset);
>
This is wrong. cairo_get_dash() expects to be passed an array that has at least cairo_get_dash_count(srcCr) members. This will write random memory if dash count is > 1.

> +        cairo_stroke_extents(cr, &x0, &y0, &x1, &y1);
>
This is a nitpick, but I guess it wouldn't hurt to do it like this:
if (strokeShadow)
    cairo_stroke_extents(cr, &x0, &y0, &x1, &y1);
else
    cairo_fill_extents(cr, &x0, &y0, &x1, &y1);
Fill extents are smaller and faster to compute, though I guess this gets lost in the noise of actually doing the blur.

> +     static void calculateShadowBufferDimensions(IntSize& shadowBufferSize, FloatRect& shadowRect, float& kernelSize, const FloatRect& sourceRect, const IntSize& shadowSize, int shadowBlur);
>
I dislike this function, because it has an ugly prototype. It has 2 out variables and they even share the size. Also, the code always creates a blur image afterwards and moves it. Wouldn't it be nicer to have a function like createShadowImage() that does all of this and just returns the ImageBuffer ?

[Simon Fraser (smfr)]

Comment on attachment 43089 [details]
Update patch for Simon Fraser and Eric Seidel's comments.

> Index: WebCore/GNUmakefile.am
> ===================================================================
> --- WebCore/GNUmakefile.am	(revision 50863)
> +++ WebCore/GNUmakefile.am	(working copy)
> @@ -2625,6 +2625,8 @@ webcore_sources += \
>  	WebCore/platform/graphics/filters/Filter.h \
>  	WebCore/platform/graphics/filters/FilterEffect.cpp \
>  	WebCore/platform/graphics/filters/FilterEffect.h \
> +	WebCore/platform/graphics/filters/GraphicsContextFilter.cpp \
> +	WebCore/platform/graphics/filters/GraphicsContextFilter.h \

Stale file names.

Should you add ImageBufferFilter to the xcode project as well? It seems generally useful.

> Index: WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp
> ===================================================================


> +        Color fillColor = colorWithOverrideAlpha(context->fillColor().rgb(), context->fillColor().alpha() / 255.f * gcp->state.globalAlpha);

If alpha() returns a float no need to make the 255 literal a float; the compiler will promote it.

> +static inline void drawPathShadow(GraphicsContext* context, GraphicsContextPrivate* gcp, bool fillShadow, bool strokeShadow)
> +{
> +#if ENABLE(FILTERS)
> +    IntSize shadowSize;
> +    int shadowBlur;
> +    Color shadowColor;
> +    if (context->getShadow(shadowSize, shadowBlur, shadowColor)) {
> +        //calculate filter values
> +        cairo_t* cr = context->platformContext();
> +        cairo_path_t* path = cairo_copy_path(cr);
> +        double x0, x1, y0, y1;
> +        cairo_stroke_extents(cr, &x0, &y0, &x1, &y1);
> +        FloatRect rect(x0, y0, x1 - x0, y1 - y0);
> +
> +        IntSize shadowBufferSize;
> +        FloatRect shadowRect;
> +        float kernelSize (0.0);
> +        GraphicsContext::calculateShadowBufferDimensions(shadowBufferSize, shadowRect, kernelSize, rect, shadowSize, shadowBlur);
> +
> +        // create suitably-sized ImageBuffer to hold the shadow
> +        OwnPtr<ImageBuffer> shadowBuffer = ImageBuffer::create(shadowBufferSize);
> +
> +        //draw shadow into a new ImageBuffer
> +        cairo_t* shadowContext = shadowBuffer->context()->platformContext();
> +        copyContextProperties(cr, shadowContext);
> +        cairo_translate(shadowContext, -rect.x() + kernelSize, -rect.y() + kernelSize);
> +        cairo_new_path(shadowContext);
> +        cairo_append_path(shadowContext, path);
> +
> +        if (fillShadow)
> +            setPlatformFill(context, shadowContext, gcp);
> +        if (strokeShadow)
> +            setPlatformStroke(context, shadowContext, gcp);
> +
> +        context->createPlatformShadow(shadowBuffer.release(), shadowColor, shadowRect, kernelSize);
> +    }

This would be cleaner with an early return.

> -void GraphicsContext::setPlatformShadow(IntSize const&, int, Color const&, ColorSpace)
> +void GraphicsContext::setPlatformShadow(IntSize const& size, int, Color const&, ColorSpace)
>  {
> -    notImplemented();
> +    // Cairo doesn't support shadows natively, they are drawn manually in the draw*
> +    // functions
> +
> +    if (m_common->state.shadowsIgnoreTransforms) {
> +        // Meaning that this graphics context is associated with a CanvasRenderingContext

For now, but what if this changes? We may decide that shadows ignore CSS transforms at some point.

> Index: WebCore/platform/graphics/cairo/ImageCairo.cpp
> ===================================================================
> --- WebCore/platform/graphics/cairo/ImageCairo.cpp	(revision 50863)
> +++ WebCore/platform/graphics/cairo/ImageCairo.cpp	(working copy)
> @@ -132,6 +132,30 @@ void BitmapImage::draw(GraphicsContext* 
>      cairo_matrix_t matrix = { scaleX, 0, 0, scaleY, srcRect.x(), srcRect.y() };
>      cairo_pattern_set_matrix(pattern, &matrix);
>  
> +    // Draw the shadow
> +#if ENABLE(FILTERS)
> +    IntSize shadowSize;
> +    int shadowBlur;
> +    Color shadowColor;
> +    if (context->getShadow(shadowSize, shadowBlur, shadowColor)) {
> +        IntSize shadowBufferSize;
> +        FloatRect shadowRect;
> +        float kernelSize (0.0);
> +        GraphicsContext::calculateShadowBufferDimensions(shadowBufferSize, shadowRect, kernelSize, dstRect, shadowSize, shadowBlur);
> +        shadowColor = colorWithOverrideAlpha(shadowColor.rgb(), (shadowColor.alpha() *  context->getAlpha()) / 255.f);
> +
> +        //draw shadow into a new ImageBuffer
> +        OwnPtr<ImageBuffer> shadowBuffer = ImageBuffer::create(shadowBufferSize);
> +        cairo_t* shadowContext = shadowBuffer->context()->platformContext();
> +        cairo_set_source(shadowContext, pattern);
> +        cairo_translate(shadowContext, -dstRect.x(), -dstRect.y());
> +        cairo_rectangle(shadowContext, 0, 0, dstRect.width(), dstRect.height());
> +        cairo_fill(shadowContext);
> +
> +        context->createPlatformShadow(shadowBuffer.release(), shadowColor, shadowRect, kernelSize);
> +    }
> +#endif
> +
>      // Draw the image.
>      cairo_translate(cr, dstRect.x(), dstRect.y());
>      cairo_set_source(cr, pattern);
> Index: WebCore/platform/graphics/filters/Filter.h
> ===================================================================

>          // SVG specific
> +        virtual void calculateEffectSubRegion(FilterEffect*) const = 0;

It's not clear why this can be const. Does it change state in |this|, or the filter effect?

> +        virtual bool effectBoundingBoxMode() const = 0;

I can't figure out what this means from the signature. Bool return? Not your fault, but it seems like this needs an enum for the return value.

> Index: WebCore/platform/graphics/filters/ImageBufferFilter.h
> ===================================================================

> +namespace WebCore {
> +
> +    class ImageBufferFilter : public Filter {

We don't generally indent for namespaces.

[Brent Fulgham]

Since Simon r+'d the change, I'm landing with his minor corrections, and will provide a second patch to address the points you raise.

Thanks for taking a look at it.

[Brent Fulgham]

(In reply to comment #9)
> (From update of attachment 43089 [details])
> > Index: WebCore/GNUmakefile.am
> > +	WebCore/platform/graphics/filters/GraphicsContextFilter.cpp \
> > +	WebCore/platform/graphics/filters/GraphicsContextFilter.h \
> 
> Stale file names.

Corrected.  Good catch!

> Should you add ImageBufferFilter to the xcode project as well? It seems
> generally useful.

Will do in a separate patch.
 
> > +        if (strokeShadow)
> > +            setPlatformStroke(context, shadowContext, gcp);
> > +
> > +        context->createPlatformShadow(shadowBuffer.release(), shadowColor, shadowRect, kernelSize);
> > +    }
> 
> This would be cleaner with an early return.

done.

> > Index: WebCore/platform/graphics/filters/Filter.h
> > ===================================================================
> 
> >          // SVG specific
> > +        virtual void calculateEffectSubRegion(FilterEffect*) const = 0;
> 
> It's not clear why this can be const. Does it change state in |this|, or the
> filter effect?

This function (in the only concrete implementation) modifies the FilterEffect argument, not the internal state of the filter.

> > Index: WebCore/platform/graphics/filters/ImageBufferFilter.h
> > ===================================================================
> 
> > +namespace WebCore {
> > +
> > +    class ImageBufferFilter : public Filter {
> 
> We don't generally indent for namespaces.

Fixed.

Thanks for reviewing this.

[Adam Barth]

Comment on attachment 43089 [details]
Update patch for Simon Fraser and Eric Seidel's comments.

Brent to land this manually.

[Brent Fulgham]

(In reply to comment #12)
> (From update of attachment 43089 [details])
> Brent to land this manually.

Landed in http://trac.webkit.org/changeset/50920

[Brent Fulgham]

Comment on attachment 43089 [details]
Update patch for Simon Fraser and Eric Seidel's comments.

Will be providing one last patch to clear up some loose ends.

[Benjamin Otte]

(In reply to comment #8)
> > +static inline void copyContextProperties(cairo_t* srcCr, cairo_t* dstCr)
> > +{
> > +    cairo_set_antialias(dstCr, cairo_get_antialias(srcCr));
> > +    double dashes, offset;
> > +    cairo_get_dash(srcCr, &dashes, &offset);
> > +    cairo_set_dash(dstCr, &dashes, cairo_get_dash_count(srcCr), offset);
> >
> This is wrong. cairo_get_dash() expects to be passed an array that has at least
> cairo_get_dash_count(srcCr) members. This will write random memory if dash
> count is > 1.
> 
I hope not fixing this (likely remote remote hole) was just an oversight and not related to ignoring comments from non-reviewers.

[Dirk Schulze]

(In reply to comment #15)

> I hope not fixing this (likely remote remote hole) was just an oversight and
> not related to ignoring comments from non-reviewers.

Yes, (In reply to comment #15)
> (In reply to comment #8)
> > > +static inline void copyContextProperties(cairo_t* srcCr, cairo_t* dstCr)
> > > +{
> > > +    cairo_set_antialias(dstCr, cairo_get_antialias(srcCr));
> > > +    double dashes, offset;
> > > +    cairo_get_dash(srcCr, &dashes, &offset);
> > > +    cairo_set_dash(dstCr, &dashes, cairo_get_dash_count(srcCr), offset);
> > >
> > This is wrong. cairo_get_dash() expects to be passed an array that has at least
> > cairo_get_dash_count(srcCr) members. This will write random memory if dash
> > count is > 1.
> > 
> I hope not fixing this (likely remote remote hole) was just an oversight and
> not related to ignoring comments from non-reviewers.

Yes this should be fixed. But Brent said that there will be a follow-up.

[Gustavo Noronha (kov)]

Hey, this change seems to have caused 2 regressions in GTK+, and the test introduced by it also fails:

fast/canvas/canvas-composite-alpha.html
fast/canvas/canvas-shadow.html
fast/canvas/gradient-with-clip.html

Is enabling something what is missing? If we need more time to figure it out, I believe we should revert, and reland when the tests pass.

[Gustavo Noronha (kov)]

For the record, krit has found one of the problems introduced, and patched it in http://trac.webkit.org/changeset/50939. We still need to figure out the fast/canvas/canvas-composite-alpha.html regression.

[Brent Fulgham]

Created attachment 43167 [details]
Update to improve performance, correct security hole.

This update attempts to address Benjamin Otte's comments regarding performance and security in regards to interactions with the Cairo library.

[Alexey Proskuryakov]

Comment on attachment 43167 [details]
Update to improve performance, correct security hole.

> +    cairo_get_dash(srcCr, reinterpret_cast<double*>(dashes.data()), &offset);
> +    cairo_set_dash(dstCr, reinterpret_cast<double*>(dashes.data()), dashCount, offset);

Please remove the unneeded reinterpret_cast.

r=me

[Dirk Schulze]

Can you please try to fix the LayoutTest before landing the next patch. Brent? I also try to catch the bug, but would be better to have a green bot first.

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/50956.

Dirk to follow up with final patch to correct the alpha blending regression.

[Dirk Schulze]

Landed last fix to pass LayoutTests r50963.

[Brent Fulgham]

(In reply to comment #8)
> > +        cairo_stroke_extents(cr, &x0, &y0, &x1, &y1);
> >
> This is a nitpick, but I guess it wouldn't hurt to do it like this:
> if (strokeShadow)
>     cairo_stroke_extents(cr, &x0, &y0, &x1, &y1);
> else
>     cairo_fill_extents(cr, &x0, &y0, &x1, &y1);
> Fill extents are smaller and faster to compute, though I guess this gets lost
> in the noise of actually doing the blur.

I'm concerned that this change was bad.  If I use cairo_stroke_extents, then the "Off Screen" example on http://philip.html5.org/demos/canvas/shadows/various.html shows faint lines around the edges.  If I switch to the above code (which I landed earlier today), the "Off Screen" example is blank.

[Dirk Schulze]

The offset example should be blank. It's just ff that uses stroke_extends at the wrong place.