Bug 28362 - SVG Filter feComposite implementation is missing
Summary: SVG Filter feComposite implementation is missing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 68469 26389
  Show dependency treegraph
 
Reported: 2009-08-16 12:41 PDT by Dirk Schulze
Modified: 2014-05-12 05:54 PDT (History)
2 users (show)

See Also:


Attachments
feComposite implementation (10.74 KB, patch)
2009-08-16 13:40 PDT, Dirk Schulze
zimmermann: review-
Details | Formatted Diff | Diff
feComposite implementation (9.17 KB, patch)
2009-08-19 06:41 PDT, Dirk Schulze
zimmermann: review+
zimmermann: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2009-08-16 12:41:20 PDT
SVG Filter feComposite implementation is missing.
Comment 1 Dirk Schulze 2009-08-16 13:40:27 PDT
Created attachment 34934 [details]
feComposite implementation

feComposite implementation
Comment 2 Nikolas Zimmermann 2009-08-18 11:52:41 PDT
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.
Comment 3 Dirk Schulze 2009-08-19 06:41:25 PDT
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.
Comment 4 Nikolas Zimmermann 2009-08-22 18:53:06 PDT
Comment on attachment 35118 [details]
feComposite implementation

r+, some comments, before landing;

> +                  2008 Dirk Schulze <krit@webkit.org>
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.
Comment 5 Eric Seidel (no email) 2009-09-08 12:42:55 PDT
Ping?  It's been a couple weeks w/ no comments from Dirk.  I assume he's just busy...
Comment 6 Dirk Schulze 2009-09-09 00:15:55 PDT
(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.
Comment 7 Eric Seidel (no email) 2009-09-21 18:02:12 PDT
The patch still applies cleanly. :)  But we're still waiting on Krit. :(
Comment 8 Dirk Schulze 2009-09-29 13:40:30 PDT
You're impatient Eric ;-) But you're right. Landed in r48896. Sorry for the delay.