Bug 30960

Summary: [CAIRO] shadow support for Canvas and SVG
Product: WebKit Reporter: Dirk Schulze <krit>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, gns, otte
Priority: P2    
Version: 525.x (Safari 3.1)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 26102    
Attachments:
Description Flags
shadow support for Cairo
simon.fraser: review-
add GCFilter to the win build system
eric: review-
Update patch for Simon Fraser and Eric Seidel's comments.
simon.fraser: review+, abarth: commit-queue-
Update to improve performance, correct security hole. ap: review+, ap: commit-queue-

Description Dirk Schulze 2009-10-30 12:26:31 PDT
Cairo needs shadow support for Canvas and SVG.
Comment 1 Dirk Schulze 2009-10-30 12:56:02 PDT
Created attachment 42227 [details]
shadow support for Cairo

Canvas/SVG shadow support for Cairo
Comment 2 Brent Fulgham 2009-10-30 16:54:10 PDT
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.
Comment 3 Dirk Schulze 2009-10-31 00:28:49 PDT
(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).
Comment 4 Dirk Schulze 2009-10-31 14:35:44 PDT
Created attachment 42260 [details]
add GCFilter to the win build system

This should add GraphicsContextFilter to the windows build system.
Comment 5 Simon Fraser (smfr) 2009-11-06 13:04:03 PST
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.
Comment 6 Eric Seidel (no email) 2009-11-10 13:59:39 PST
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-
Comment 7 Brent Fulgham 2009-11-12 12:14:45 PST
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.
Comment 8 Benjamin Otte 2009-11-12 12:55:18 PST
> +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 ?
Comment 9 Simon Fraser (smfr) 2009-11-12 15:22:12 PST
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.
Comment 10 Brent Fulgham 2009-11-12 15:40:20 PST
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.
Comment 11 Brent Fulgham 2009-11-12 15:48:07 PST
(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.
Comment 12 Adam Barth 2009-11-12 18:25:25 PST
Comment on attachment 43089 [details]
Update patch for Simon Fraser and Eric Seidel's comments.

Brent to land this manually.
Comment 13 Brent Fulgham 2009-11-12 21:37:26 PST
(In reply to comment #12)
> (From update of attachment 43089 [details])
> Brent to land this manually.

Landed in http://trac.webkit.org/changeset/50920
Comment 14 Brent Fulgham 2009-11-12 21:39:22 PST
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.
Comment 15 Benjamin Otte 2009-11-12 23:40:54 PST
(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.
Comment 16 Dirk Schulze 2009-11-13 00:35:05 PST
(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.
Comment 17 Gustavo Noronha (kov) 2009-11-13 02:18:24 PST
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.
Comment 18 Gustavo Noronha (kov) 2009-11-13 07:17:03 PST
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.
Comment 19 Brent Fulgham 2009-11-13 10:20:25 PST
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.
Comment 20 Alexey Proskuryakov 2009-11-13 10:43:07 PST
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
Comment 21 Dirk Schulze 2009-11-13 10:50:49 PST
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.
Comment 22 Brent Fulgham 2009-11-13 11:31:36 PST
Landed in http://trac.webkit.org/changeset/50956.

Dirk to follow up with final patch to correct the alpha blending regression.
Comment 23 Dirk Schulze 2009-11-13 13:06:43 PST
Landed last fix to pass LayoutTests r50963.
Comment 24 Brent Fulgham 2009-11-13 14:30:50 PST
(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.
Comment 25 Dirk Schulze 2009-11-13 23:12:45 PST
The offset example should be blank. It's just ff that uses stroke_extends at the wrong place.