[Webkit-unassigned] [Bug 49907] Better result passing in filter primitives

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49907





--- Comment #4 from Dirk Schulze <krit at webkit.org>  2010-11-23 10:01:48 PST ---
(From update of attachment 74657)
View in context: https://bugs.webkit.org/attachment.cgi?id=74657&action=review

Even if I really like the idea, I fear that we and up with 3 image results per FilterEffect. But of course I see the performance win. But we also have to think about hardware acceleration. Some platforms support effects directly and I always saw the software rendering as a fallback if a platform does not support one or more effects. Eg on a Mac we can emulate all effects, on Skia and OpenVG we can at least emulate many effects, while we don't have effects on Qt and Cairo _yet_! At the moment every FilterEffect just has the ImageBuffer and the absolutePaintRect, so that it should be possible to replace some effects by platform effects and use the fallback on other ones. If we use your design, it will be much harder and maybe impossible to use platform effects. And I bet even Cairo and Qt will implement effects sooner or later. Do you have some proposals?

> WebCore/platform/graphics/filters/FEComposite.cpp:150
> +    GraphicsContext* filterContext = resultImage->context();

return if a filterContext does not exist. Make sure thet this->hasResult does not return true.

> WebCore/platform/graphics/filters/FEConvolveMatrix.cpp:396
> +    RefPtr<ImageData> imageData = ImageData::create(absolutePaintRect().width(), absolutePaintRect().height());

Save absolutePaintRect().size() locally.

> WebCore/platform/graphics/filters/FEDisplacementMap.cpp:112
> +    int stride = absolutePaintRect().width() * 4;

is it better to save absolutePaintRect() in a local variable here?

> WebCore/platform/graphics/filters/FEFlood.cpp:72
> +    resultImage->context()->fillRect(FloatRect(FloatPoint(), absolutePaintRect().size()), color, ColorSpaceDeviceRGB);

Make sure that getting the context was successful.

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:209
> +            boxBlur(tmpPixelArray, srcPixelArray, kernelSizeY, dyLeft, dyRight, stride, 4, absolutePaintRect().height(), absolutePaintRect().width(), isAlphaImage());

Save absolutePaintRect() in a local variable

> WebCore/platform/graphics/filters/FEOffset.cpp:91
> +    resultImage->context()->drawImageBuffer(in->asImageBuffer(), ColorSpaceDeviceRGB, drawingRegion);

check for the context first.

> WebCore/platform/graphics/filters/FETile.cpp:83
> +    resultImage->context()->setFillPattern(pattern);

Ditto.

> WebCore/platform/graphics/filters/FETurbulence.cpp:327
> +    if (!absolutePaintRect().width() || !absolutePaintRect().height())

save the size of absolutePaintRect in a local variable

> WebCore/platform/graphics/filters/FilterEffect.cpp:104
> +inline void FilterEffect::copyImageBytes(ImageData* dst, ImageData* src, const IntRect& rect)

just a minor detail, can you switch src an dst, please? :-)

> WebCore/platform/graphics/filters/FilterEffect.cpp:107
> +    ASSERT(dst->width() == rect.width() && dst->height() == rect.height());

don't we have an operator== for IntSize? I'd prefer IntSize(dst->width(), dst->height()) == rect.size() Why don't we have a IntSize for ImageData? Could you take a look at it?

> WebCore/platform/graphics/filters/FilterEffect.cpp:172
> +            m_unmultipliedImageResult = m_imageBufferResult->getUnmultipliedImageData(IntRect(IntPoint(), m_absolutePaintRect.size()));

I guess it really depends on the platform if it is faster to do it manually here. Some platforms just copy the data. Can you take a look at this again please?

> WebCore/platform/graphics/filters/FilterEffect.cpp:198
> +            m_premultipliedImageResult = m_imageBufferResult->getPremultipliedImageData(IntRect(IntPoint(), m_absolutePaintRect.size()));

Ditto.

> WebCore/platform/graphics/filters/SourceGraphic.cpp:59
> +    resultImage->context()->drawImageBuffer(filter->sourceImage(), ColorSpaceDeviceRGB, IntPoint());

I'm really unsure if it can happen that a GC doesn't exist while the ImageBuffer does. Is it absolutely save? Can check this please? You're using this pattern on quite some other places.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.