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.
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?
Qt does not use ContextShadow::drawRectShadow(), though.
I want to use ContextShadow for CG, because CG's shadows are slow, and have an incorrect radius.
Just for curiosity, will this fix Bug 51386 ?
> Just for curiosity, will this fix Bug 51386 ? Likely not.
(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.
(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.
Created attachment 79112 [details] Patch
(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 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....
(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.
> 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.
Yeah, I think ContextShadow should be properly class-ified, and maybe given a better name.
Attachment 79112 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7545184
I'm going to make a new class that platforms can migrate to over time.
Created attachment 80605 [details] Patch
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.
Created attachment 80606 [details] Patch
http://trac.webkit.org/changeset/77097