Bug 28133

Created attachment 34430 [details] SVG Filter feBlend Implementation This is the implementation of feBlend. It needs premuliplied colors of getImageData (see bug 27933)

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. :)

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.

(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.

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.

Created attachment 34478 [details] SVG Filter feBlend Implementation Ok, I surrender and take return's :-)

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 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?

(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.

Why don't you need to call: 100 filterContext->clearRect(FloatRect(FloatPoint(), resultImage()->size())); before the first drawImage call?

(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.

I take it: GraphicsContext* filterContext = getEffectContext(); is known to create a new image buffer? That's not at all clear from the method naming.

(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.

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 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.

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.

(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

Created attachment 35025 [details] SVG Filter feBlend Implementation Clean patch. I removed pt_BR.po.

Created attachment 35063 [details] SVG Filter feBlend Implementation fixed wrong ref counting

landed in r47456.