Bug 28362 – SVG Filter feComposite implementation is missing

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dirk Schulze]

SVG Filter feComposite implementation is missing.

[Dirk Schulze]

Created attachment 34934 [details]
feComposite implementation

feComposite implementation

[Nikolas Zimmermann]

Comment on attachment 34934 [details]
feComposite implementation

r- for various issues:


> +#if !PLATFORM(CG) && !PLATFORM(SKIA)
> +void inline operatorIn(const PassRefPtr<CanvasPixelArray>& srcPixelArrayA, PassRefPtr<CanvasPixelArray>& srcPixelArrayB)
s/void inline/inline void/
Pass CanvasPixelArray pointers here.

>  {
> +    for (unsigned pixelOffset = 0; pixelOffset < srcPixelArrayA->length(); pixelOffset++) {
s/pixelOffset++/++pixelOffset/

> +        unsigned pixelByteOffset = pixelOffset * 4;
> +        unsigned char alphaA = srcPixelArrayA->get(pixelByteOffset + 3);
> +        unsigned char alphaB = srcPixelArrayB->get(pixelByteOffset + 3);
> +
> +        unsigned char resultB = (alphaA * alphaB) / 255;
> +        srcPixelArrayB->set(pixelByteOffset + 3, resultB);
> +    }
> +}
> +#endif

It should be made more clear why CG/Skia support clipToImageBuffer, where the rest doesn't.

> + void inline arithmetic(const PassRefPtr<CanvasPixelArray>& srcPixelArrayA, PassRefPtr<CanvasPixelArray>& srcPixelArrayB,
s/void inline/inline void/
Pass CanvasPixelArray pointers here.

> +    for (unsigned pixelOffset = 0; pixelOffset < srcPixelArrayA->length(); pixelOffset++) {
s/pixelOffset++/++pixelOffset/

> +        unsigned pixelByteOffset = pixelOffset * 4;
> +        for (unsigned channel = 0; channel < 4; channel++) {
s/channel++/++channel/

> +#if PLATFORM(CG) || PLATFORM(SKIA)
> +                filterContext->save();
> +                filterContext->clipToImageBuffer(calculateDrawingRect(m_in2->subRegion()), m_in2->resultImage());
> +                filterContext->drawImage(m_in->resultImage()->image(), calculateDrawingRect(m_in->subRegion()));
> +                filterContext->restore();
> +#else
> +                IntRect effectADrawingRect = calculateDrawingIntRect(m_in2->subRegion());
> +                PassRefPtr<CanvasPixelArray> srcPixelArrayA(m_in2->resultImage()->getPremultipliedImageData(effectADrawingRect)->data());
> +
> +                IntRect effectBDrawingRect = calculateDrawingIntRect(m_in->subRegion());
> +                PassRefPtr<ImageData> imageData(m_in->resultImage()->getPremultipliedImageData(effectBDrawingRect));
> +                PassRefPtr<CanvasPixelArray> srcPixelArrayB(imageData->data());
> +
> +                operatorIn(srcPixelArrayA, srcPixelArrayB);
> +                resultImage()->putPremultipliedImageData(imageData.get(), IntRect(IntPoint(), resultImage()->size()), IntPoint());
> +#endif

This code should be encapsulated in a helper function, to avoid spreading CG/Skia defines.
Don't use "PassRefPtr<CanvasPixelArray> = ...". As the name indicates, it should only be used when passing around RefPtrs, which is not the case here. Also imageData->data() returns a plain pointer, no need for you to ref/deref it.

There's also a style issue in the FECOMPOSITE_OPERATOR_ARITHMETIC case, you're indenting too much there.

[Dirk Schulze]

Created attachment 35118 [details]
feComposite implementation

I didn't use the platform switch this time. Gtk and Qt have to support clipToImageBuffer anyway to get support for SVG Masking and some CSS3 related stuff. There are already bug reports for clipToImageBuffer and patches for it (just need clean-up or more improvement). 
I fixed the sytle issues and the ref counting.

[Nikolas Zimmermann]

Comment on attachment 35118 [details]
feComposite implementation

r+, some comments, before landing;

> +                  2008 Dirk Schulze <[e-mail]>
2009.

> +inline void arithmetic(const RefPtr<CanvasPixelArray>& srcPixelArrayA, CanvasPixelArray*& srcPixelArrayB,
> +                       const float& k1, const float& k2, const float& k3, const float& k4)
s/const float&/float/. We don't use this style in WebKit, even though it's correct, there's no gain.


> +    FloatRect srcRect = FloatRect(0, 0, -1, -1);
Use (0.0f, 0.0f, -1.0f, -1.0f) to be consistent with other callsites.

> +    switch (m_type) {
> +        case FECOMPOSITE_OPERATOR_OVER:
> +        case FECOMPOSITE_OPERATOR_IN:
> +        case FECOMPOSITE_OPERATOR_OUT:
> +        case FECOMPOSITE_OPERATOR_ATOP:
> +        case FECOMPOSITE_OPERATOR_XOR:
> +        case FECOMPOSITE_OPERATOR_ARITHMETIC: {
Indention, case & switch should be aligned.

[Eric Seidel (no email)]

Ping?  It's been a couple weeks w/ no comments from Dirk.  I assume he's just busy...

[Dirk Schulze]

(In reply to comment #5)
> Ping?  It's been a couple weeks w/ no comments from Dirk.  I assume he's just
> busy...

Yes thats true. I'm busy with exams :-( 
I hope that I can create the test results and upload the patch with the corrections at the end of this week.

[Eric Seidel (no email)]

The patch still applies cleanly. :)  But we're still waiting on Krit. :(

[Dirk Schulze]

You're impatient Eric ;-) But you're right. Landed in r48896. Sorry for the delay.