Bug 51312 - Make ContextShadow code cross-platform
Summary: Make ContextShadow code cross-platform
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks: 51567
  Show dependency treegraph
 
Reported: 2010-12-19 16:04 PST by Simon Fraser (smfr)
Modified: 2011-01-30 16:45 PST (History)
11 users (show)

See Also:


Attachments
Patch (35.16 KB, patch)
2011-01-16 14:14 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (37.60 KB, patch)
2011-01-30 15:55 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (38.27 KB, patch)
2011-01-30 16:37 PST, Simon Fraser (smfr)
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-12-19 16:04:29 PST
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.
Comment 1 Dirk Schulze 2010-12-19 22:59:53 PST
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?
Comment 2 Ariya Hidayat 2010-12-20 08:09:22 PST
Qt does not use ContextShadow::drawRectShadow(), though.
Comment 3 Simon Fraser (smfr) 2010-12-20 08:49:32 PST
I want to use ContextShadow for CG, because CG's shadows are slow, and have an incorrect radius.
Comment 4 Hajime Morrita 2010-12-21 18:55:06 PST
Just for curiosity, will this fix Bug 51386 ?
Comment 5 Ariya Hidayat 2010-12-22 18:41:31 PST
> Just for curiosity, will this fix Bug 51386 ?

Likely not.
Comment 6 Dirk Schulze 2011-01-05 00:27:58 PST
(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.
Comment 7 Simon Fraser (smfr) 2011-01-05 08:34:49 PST
(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.
Comment 8 Dirk Schulze 2011-01-16 14:14:13 PST
Created attachment 79112 [details]
Patch
Comment 9 Dirk Schulze 2011-01-17 00:52:15 PST
(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.
Comment 10 Nikolas Zimmermann 2011-01-17 08:09:00 PST
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....
Comment 11 Dirk Schulze 2011-01-17 09:58:07 PST
(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.
Comment 12 Ariya Hidayat 2011-01-17 10:17:35 PST
> 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.
Comment 13 Simon Fraser (smfr) 2011-01-17 10:20:34 PST
Yeah, I think ContextShadow should be properly class-ified, and maybe given a better name.
Comment 14 WebKit Review Bot 2011-01-17 17:39:11 PST
Attachment 79112 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7545184
Comment 15 Simon Fraser (smfr) 2011-01-30 10:43:12 PST
I'm going to make a new class that platforms can migrate to over time.
Comment 16 Simon Fraser (smfr) 2011-01-30 15:55:04 PST
Created attachment 80605 [details]
Patch
Comment 17 Sam Weinig 2011-01-30 16:12:04 PST
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.
Comment 18 Simon Fraser (smfr) 2011-01-30 16:37:00 PST
Created attachment 80606 [details]
Patch
Comment 19 Simon Fraser (smfr) 2011-01-30 16:45:08 PST
http://trac.webkit.org/changeset/77097