Bug 28133

Summary: SVG Filter feBlend implementation missing
Product: WebKit Reporter: Dirk Schulze <krit>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, jeffschiller, oliver
Priority: P2    
Version: 525.x (Safari 3.1)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 27933    
Bug Blocks: 68469, 26389    
Attachments:
Description Flags
SVG Filter feBlend Implementation
none
SVG Filter feBlend Implementation
none
SVG Filter feBlend Implementation
oliver: review-
SVG Filter feBlend Implementation
none
SVG Filter feBlend Implementation
oliver: review+
SVG Filter feBlend Implementation oliver: review+

Description Dirk Schulze 2009-08-09 12:32:49 PDT
SVG Filter feBlend implementation missing
Comment 1 Dirk Schulze 2009-08-09 14:38:51 PDT
Created attachment 34430 [details]
SVG Filter feBlend Implementation

This is the implementation of feBlend. It needs premuliplied colors of getImageData (see bug 27933)
Comment 2 Eric Seidel (no email) 2009-08-09 21:40:45 PDT
Comment on attachment 34430 [details]
SVG Filter feBlend Implementation

You're going to want to mark your unknown/lighten/darken etc as "static" so that compilers know they shouldn't have external linkage.

Seems you should name your arguments here:
 33 typedef void (*BlendType)(double, double&, double, double);

Maybe that should return a double instead of taking a double& for colorB?

Should be "static const BlendType const" so compilers know those jump locations aren't gonna change.
 108     BlendType callEffect[] = {unknown, normal, multiply, screen, darken, lighten};

In general this looks good though!  I think you should get oliver to review this, as his graphics expertise is far better than mine. :)
Comment 3 Eric Seidel (no email) 2009-08-09 21:41:45 PDT
CCing oliver on the off chance he hasn't seen this yet.  Looks good to me (with my above review comments), but he should take a look too.
Comment 4 Dirk Schulze 2009-08-09 23:41:38 PDT
(In reply to comment #2)
> (From update of attachment 34430 [details])

> Maybe that should return a double instead of taking a double& for colorB?

I wanted to avoid a rewrite of values for more speed. The calculation of the new pixel-values happens during the interpretation of the current pixel value and this makes the effect slower but can not be avoided.
Comment 5 Eric Seidel (no email) 2009-08-10 07:34:59 PDT
I have no strong preference the reference vs. the return.  That said, I think it's wrong to change how you would write your code for the compiler w/o testing.  Pre-mature optimization, as they say.

IMO, your first client should be other human readers when you're writing code. :)  I try to write as clear as possible for humans, only tweaking for the second client (the compiler) when the compiler is too dumb to do the right thing.  In this case, I would expect a 64-bit compiler to write smart code in either case.  But never having written a compiler, I tend to be blissfully naive about these things.
Comment 6 Dirk Schulze 2009-08-10 09:30:45 PDT
Created attachment 34478 [details]
SVG Filter feBlend Implementation

Ok, I surrender and take return's :-)
Comment 7 Eric Seidel (no email) 2009-08-10 09:32:47 PDT
Comment on attachment 34478 [details]
SVG Filter feBlend Implementation

Marking as text/plain patch file. (/me points at git-send-bugzilla or bugzilla-tool) :)
Comment 8 Eric Seidel (no email) 2009-08-10 09:43:09 PDT
Comment on attachment 34478 [details]
SVG Filter feBlend Implementation

Sorry, didn't mean for it to sound like something you'd need to "surrender" to. :)

We tend to avoid using PassRefPtr on the stack, as it's error prone.  Instead we use a RefPtr and just call .release() when we need the PassRefPtr out of it.

I'm not sure why grabbing the CanvasPixelArray out of in1/in2 wouldn't be a function:
102     // Get PixelArray of m_in2
 103     filterContext->drawImage(m_in2->resultImage()->image(), calculateDrawingRect(m_in2->subRegion()));
 104     imageData = resultImage()->getImageData(imageRect);
 105     PassRefPtr<CanvasPixelArray> srcPixelArrayB(imageData->data());

imageDataAsPixelArray(m_in2);

I'm also not sure if you're intending to re-use the ImageData pointer from m_in2 during your calculations or not.  Is that intended?  I assume you're trying to avoid allocating a new buffer just to hold the result data?
 125             imageData->data()->set(pixelByteOffset + channel, result);
Can you just write directly to resultImage()?

In general this looks great though.

Why don't you need to clear the filterContext before drawing m_in?
Comment 9 Dirk Schulze 2009-08-10 10:15:34 PDT
(In reply to comment #8)
> (From update of attachment 34478 [details])
> Sorry, didn't mean for it to sound like something you'd need to "surrender" to.
> :)
> 
> We tend to avoid using PassRefPtr on the stack, as it's error prone.  Instead
> we use a RefPtr and just call .release() when we need the PassRefPtr out of it.
> 
> I'm not sure why grabbing the CanvasPixelArray out of in1/in2 wouldn't be a
> function:
> 102     // Get PixelArray of m_in2
>  103     filterContext->drawImage(m_in2->resultImage()->image(),
> calculateDrawingRect(m_in2->subRegion()));
>  104     imageData = resultImage()->getImageData(imageRect);
>  105     PassRefPtr<CanvasPixelArray> srcPixelArrayB(imageData->data());
> 
> imageDataAsPixelArray(m_in2);
> 
> I'm also not sure if you're intending to re-use the ImageData pointer from
> m_in2 during your calculations or not.  Is that intended?  I assume you're
> trying to avoid allocating a new buffer just to hold the result data?
>  125             imageData->data()->set(pixelByteOffset + channel, result);

Yes, I try to avoid creating a new temporary ImageBuffer. Thats why I clear the context before creating the second PixelArray with the same context of resultImage.

> Why don't you need to clear the filterContext before drawing m_in?
It's not needed to clear the context a second time, because we just need the context to get the Pixelarrays according to our current filterEffect-ImageBuffer (size, startingpoint etc.) and ovewrite everything with  imageData->data()->set(...). A second clear will coast more time, since most graphic librarys make use of composite operators to do it and this is slow, at least for cairo (and, like I explained before, it's not sensible). 

> Can you just write directly to resultImage()?
As far as I know, pixel manipulation of ImageBuffers is just possible with the indirection of ImageData.
Comment 10 Eric Seidel (no email) 2009-08-12 10:22:12 PDT
Why don't you need to call:
 100     filterContext->clearRect(FloatRect(FloatPoint(), resultImage()->size()));

before the first drawImage call?
Comment 11 Dirk Schulze 2009-08-12 10:38:33 PDT
(In reply to comment #10)
> Why don't you need to call:
>  100     filterContext->clearRect(FloatRect(FloatPoint(),
> resultImage()->size()));
> 
> before the first drawImage call?

a new ImageBuffer should always be transparent black.Or am I wrong? Not sure, but it's the transparent black for cg, cairo, qt.
Comment 12 Eric Seidel (no email) 2009-08-12 10:41:28 PDT
I take it:
GraphicsContext* filterContext = getEffectContext();
is known to create a new image buffer?  That's not at all clear from the method naming.
Comment 13 Dirk Schulze 2009-08-12 11:36:29 PDT
(In reply to comment #12)
> I take it:
> GraphicsContext* filterContext = getEffectContext();
> is known to create a new image buffer?  That's not at all clear from the method
> naming.

hm, right. We can rename it to getEffectContextFromNewImageBuffer() in another patch.
Comment 14 Dirk Schulze 2009-08-12 14:14:35 PDT
Created attachment 34689 [details]
SVG Filter feBlend Implementation

Many more optimizations. No doubles for colors (now unsigned char). No drawing operations on GraphicsContext. Pick up PixelArray directly.
Comment 15 Oliver Hunt 2009-08-13 14:28:00 PDT
Comment on attachment 34689 [details]
SVG Filter feBlend Implementation

You may as well add unchecked get functions to ByteArray, eg.
unsigned char ByteArray::get(int index) {
    ASSERT(index<size);
    return data[index];
}

You also don't need to update CanvasPixelArray -- just the underlying bytearray implementation.
Comment 16 Dirk Schulze 2009-08-15 11:39:52 PDT
Created attachment 34901 [details]
SVG Filter feBlend Implementation

I talked with olliej. I added a unsigned char setter and a getter that returns an unsigned char to ByteArray.h. It was necessary to add the callers to CanvasPixelArray too, since ImageData doesn't give a ByteArray, but a CanvasPixelArray back.
Comment 17 Dirk Schulze 2009-08-16 23:51:45 PDT
(In reply to comment #16)
> Created an attachment (id=34901) [details]
> SVG Filter feBlend Implementation

WebKit/gtk/po/pt_BR.po is not part of the patch
Comment 18 Dirk Schulze 2009-08-18 00:21:51 PDT
Created attachment 35025 [details]
SVG Filter feBlend Implementation

Clean patch. I removed pt_BR.po.
Comment 19 Dirk Schulze 2009-08-18 12:45:57 PDT
Created attachment 35063 [details]
SVG Filter feBlend Implementation

fixed wrong ref counting
Comment 20 Dirk Schulze 2009-08-18 14:38:29 PDT
landed in r47456.