Bug 51312 – Make ContextShadow code cross-platform

• Component: Platform
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

It would be nice if ContextShadow::drawRectShadow() were cross-platform. ImageBuffer could be used here, but we'd need direct pixel access to avoid having to copy to do blurring (see bug 51297).

Also somewhat related, bug 51309 talks about sharing blurring code between ContextShadow and FEGaussianBlur.

[Dirk Schulze]

Hm, Qt and Cairo already use ContextShadow, why would CG and Skia need ContextShadow? Both have an own implementation? Is the quality not good enough?

[Ariya Hidayat]

Qt does not use ContextShadow::drawRectShadow(), though.

[Simon Fraser (smfr)]

I want to use ContextShadow for CG, because CG's shadows are slow, and have an incorrect radius.

[Hajime Morrita]

Just for curiosity, will this fix Bug 51386 ?

[Ariya Hidayat]

> Just for curiosity, will this fix Bug 51386 ?

Likely not.

[Dirk Schulze]

(In reply to comment #0)
> It would be nice if ContextShadow::drawRectShadow() were cross-platform. ImageBuffer could be used here, but we'd need direct pixel access to avoid having to copy to do blurring (see bug 51297).
> 
> Also somewhat related, bug 51309 talks about sharing blurring code between ContextShadow and FEGaussianBlur.

At least for drawRectShadow() I don't believe that direct pixel access makes a big difference. I bet (you may have checked for your self), that the general case will be 100x100 pixel for the temporary ImageBuffer for blurring operations. The copy operations carry no weight for such a small image. I don't want to prevent you to add direct pixel access, but the first implementation doesn't need to have it.

[Simon Fraser (smfr)]

(In reply to comment #6)
> (In reply to comment #0)
> > It would be nice if ContextShadow::drawRectShadow() were cross-platform. ImageBuffer could be used here, but we'd need direct pixel access to avoid having to copy to do blurring (see bug 51297).
> > 
> > Also somewhat related, bug 51309 talks about sharing blurring code between ContextShadow and FEGaussianBlur.
> 
> At least for drawRectShadow() I don't believe that direct pixel access makes a big difference. I bet (you may have checked for your self), that the general case will be 100x100 pixel for the temporary ImageBuffer for blurring operations. The copy operations carry no weight for such a small image. I don't want to prevent you to add direct pixel access, but the first implementation doesn't need to have it.

I agree that in my measurements the blur cost was much higher than the copy.

[Dirk Schulze]

Created attachment 79112 [details]
Patch

[Dirk Schulze]

(In reply to comment #8)
> Created an attachment (id=79112) [details]
> Patch

The patch just adds ContextShadow to all build systems (including Mac). Added support for horizontal and vertical blurring with different blur radii as well.

[Nikolas Zimmermann]

Comment on attachment 79112 [details]
Patch

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

I'm tempted to r+ this, though the public variables in ContextShadow lead to a r-. Who allowed this to be landed? :-)

> Source/WebCore/platform/graphics/ContextShadow.h:82
> +    ContextShadowType m_type;
>  
>      Color m_color;
> -    int m_blurDistance;
> +    IntSize m_blurDistance;
>      FloatSize m_offset;

public variables?
This needs to be hidden....

[Dirk Schulze]

(In reply to comment #10)
> (From update of attachment 79112 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79112&action=review
> 
> I'm tempted to r+ this, though the public variables in ContextShadow lead to a r-. Who allowed this to be landed? :-)
> 
> > Source/WebCore/platform/graphics/ContextShadow.h:82
> > +    ContextShadowType m_type;
> >  
> >      Color m_color;
> > -    int m_blurDistance;
> > +    IntSize m_blurDistance;
> >      FloatSize m_offset;
> 
> public variables?
> This needs to be hidden....

I thought the same :-) But Simon already opened another bug for cleaning up ContextShadow. It was not my intention to clean up ContextShadow in the first place.

[Ariya Hidayat]

> I'm tempted to r+ this, though the public variables in ContextShadow lead to a r-. Who allowed this to be landed? :-)
> 
> > Source/WebCore/platform/graphics/ContextShadow.h:82
> > +    ContextShadowType m_type;
> >  
> >      Color m_color;
> > -    int m_blurDistance;
> > +    IntSize m_blurDistance;
> >      FloatSize m_offset;
> 
> public variables?
> This needs to be hidden....

Because it was a private, lightweight helper struct for Qt port only. Then it got adopted by everyone.

[Simon Fraser (smfr)]

Yeah, I think ContextShadow should be properly class-ified, and maybe given a better name.

[WebKit Review Bot]

Attachment 79112 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7545184

[Simon Fraser (smfr)]

I'm going to make a new class that platforms can migrate to over time.

[Simon Fraser (smfr)]

Created attachment 80605 [details]
Patch

[Sam Weinig]

Comment on attachment 80605 [details]
Patch

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

> Source/WebCore/platform/graphics/ShadowBlur.cpp:47
> +static inline int roundUpToMultipleOf32(int d)
> +{
> +    return (1 + (d >> 5)) << 5;
> +}

Maybe this should go in MathExtras.h

> Source/WebCore/platform/graphics/ShadowBlur.cpp:77
> +        const double scratchBufferPurgeInterval = 2;

This should probably be a static const.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:112
> +    // See comments in http://webkit.org/b/40793, it seems sensible
> +    // to follow Skia's limit of 128 pixels of blur radius

I think putting the reason would be more helpful than putting the link.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:127
> +    } else {
> +        m_type = SolidShadow;
> +    }

The braces are not needed in this clause.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:132
> +static const int BlurSumShift = 15;
> +static const float GaussianKernelFactor = 3 / 4.f * sqrtf(2 * piFloat);

We don't traditionally capitalize constant names.  I am also not sure if calling sqrtf() for a static const will work in all environments.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:148
> +    int d;

d is really not the most descriptive name. How about diameter (if that is what it is)?

> Source/WebCore/platform/graphics/ShadowBlur.cpp:158
> +    int dmax = d >> 1;
> +    int dmin = dmax - 1 + (d & 1);
> +    if (dmin < 0)
> +        dmin = 0;
> +

Again, not great names.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:166
> +        unsigned char* pixels = imageData;
> +        int stride = (!k) ? 4 : rowStride;
> +        int delta = (!k) ? rowStride : 4;
> +        int jfinal = (!k) ? size.height() : size.width();
> +        int dim = (!k) ? size.width() : size.height();

The parens around !k are not necessary. jfinal should probably be jFinal.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:298
> +    const int frameSize = inflation * 2;
> +    m_sourceRect = IntRect(0, 0, shadowedRect.width() + frameSize, shadowedRect.height() + frameSize);
> +    m_layerOrigin = FloatPoint(layerFloatRect.x(), layerFloatRect.y());
> +    m_layerSize = layerFloatRect.size();
> +
> +    const FloatPoint m_unclippedLayerOrigin = FloatPoint(unclippedLayerRect.x(), unclippedLayerRect.y());
> +    const FloatSize clippedOut = m_unclippedLayerOrigin - m_layerOrigin;
> +
> +    // Set the origin as the top left corner of the scratch image, or, in case there's a clipped
> +    // out region, set the origin accordingly to the full bounding rect's top-left corner.
> +    const float translationX = -shadowedRect.x() + inflation - fabsf(clippedOut.width());
> +    const float translationY = -shadowedRect.y() + inflation - fabsf(clippedOut.height());
> +    m_layerContextTranslation = FloatSize(translationX, translationY);

I don't know if the use of const here is adding anything.

[Simon Fraser (smfr)]

Created attachment 80606 [details]
Patch

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/77097