Bug 49907

Created attachment 74548 [details] concept

In general I think this is a great idea! But I couldn't look into detail. Reading your patch tis evening. But the idea sounds great.

Created attachment 74657 [details] updated concept Working on small tests (hopefully mostly finished). Would like to hear your opinion.

Comment on attachment 74657 [details] updated concept 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.

> 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. I have no strong opinion here. I know most filters (light, turbulence, gaussian blur, morphology, convolve matrix, displacement) will never be hw accelerated except the simplest ones (offset, merge). And yeah, we can have 3 results for each filters, although the filter itself only creates one, and the others are created on demand. But you should decide whether it is worth to work on this project. That is why I submit conceptual patches, since if this is not a good idea, at least I don't have to spend full days working on it.

(In reply to comment #5) > > 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. > > I have no strong opinion here. I know most filters (light, turbulence, gaussian blur, morphology, convolve matrix, displacement) will never be hw accelerated except the simplest ones (offset, merge). And yeah, we can have 3 results for each filters, although the filter itself only creates one, and the others are created on demand. But you should decide whether it is worth to work on this project. That is why I submit conceptual patches, since if this is not a good idea, at least I don't have to spend full days working on it. Yes, thanks for the great proposal, but maybe others should also comment on this bug. To the hwa. I know of OpenVG that they support fast filter effects for gaussian blur and convolve matrix, not sure how they calculate it. But I wouldn't say for sure that they can't be faster with hwa :-) And I'm pretty sure that these filter can be a lot faster on a GPU because they are very good to parallelize. Adding Simon and Oliver, since hwa is not my field of knowledge.

If we ever want to support hw accelerators, they would require a special code path (specialized apply functions), and we need to write specialized code for them. Thus, this could be a sw rendering (SMP accelerated) code path, which could be enabled by compiler directives. The 3 new functions can fallback to the original ImageBuffer based implementations when we don't want to use these buffers in a mixed (sh/hw accelerated) environment.

(In reply to comment #7) > If we ever want to support hw accelerators, they would require a special code path (specialized apply functions), and we need to write specialized code for them. Thus, this could be a sw rendering (SMP accelerated) code path, which could be enabled by compiler directives. The 3 new functions can fallback to the original ImageBuffer based implementations when we don't want to use these buffers in a mixed (sh/hw accelerated) environment. Talked about this topic with zherzeg on IRC. It should still be possible to combine HW and SW rendering if needed. The ImageBuffer is always accessible like it is right now.

> > 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. Thinking about this. Is it possible, that the image is allocated, but the context is not? Anyway, I would add this check to FilterEffect::createImageBufferResult() (All filters, which allocates an imageBuffer will use the context!) An ASSERT(!hasResult()) added to all create.*Result functions.

Created attachment 74848 [details] patch This patch is tested and working (On mac leopard with -p and --tolerance 0).

Comment on attachment 74848 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=74848&action=review The style bot is red, did you check the style manually on your machine? > WebCore/platform/graphics/filters/FEGaussianBlur.cpp:179 > + in->asPremultipliedImage(resultImage, effectDrawingRect); Can you explain how this works? the resultImage is possibly larger than the sourceImage, in every direction. How do you copy the data to the resultImage? > WebCore/platform/graphics/filters/FEGaussianBlur.cpp:192 > + RefPtr<ImageData> tmpImageData = ImageData::create(absolutePaintRect().width(), absolutePaintRect().height()); > ByteArray* tmpPixelArray = tmpImageData->data()->data(); > > - int stride = 4 * imageRect.width(); > + IntSize paintSize = absolutePaintRect().size(); Can you move the intsize some line above and replace the absolutePainRect() calls? > WebCore/platform/graphics/filters/FETile.cpp:84 > + resultImage->context()->setFillPattern(pattern); > + resultImage->context()->fillRect(FloatRect(FloatPoint(), absolutePaintRect().size())); save context in a local var before using it here. > WebCore/platform/graphics/filters/FilterEffect.cpp:78 > + // This function is allowed to return with NULL. I think this sentence is unnecessary. > WebCore/platform/graphics/filters/FilterEffect.cpp:94 > + PassRefPtr<ImageData> imageData = ImageData::create(rect.width(), rect.height()); You have to take the ownership first: RefPtr<ImageData> > WebCore/platform/graphics/filters/FilterEffect.cpp:101 > + PassRefPtr<ImageData> imageData = ImageData::create(rect.width(), rect.height()); Ditto. > WebCore/platform/graphics/filters/FilterEffect.cpp:223 > + if (!m_imageBufferResult->context()) { > + // All filter effects, which allocates an imageBufferResult > + // will use its context. If we would not destroy the image here > + // the filter would return a black rectangle, which is not desired. > + // (I would prefer an ASSERT here, since I think the context must be exists) > + m_imageBufferResult.clear(); > + } I agree. ;)

Created attachment 74995 [details] patch 2 Fixed everything.

> Can you explain how this works? the resultImage is possibly larger than the sourceImage, in every direction. How do you copy the data to the resultImage? Same way as get/put pre/unmultiplied data-s. If the rect is bigger than the source area, it fills with 0 first (transparent black pixels).

> The style bot is red, did you check the style manually on your machine? No idea about this. It did not report an error, and now it is green... Perhaps an internal error?

Comment on attachment 74995 [details] patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=74995&action=review I love the general idea of this patch! Some comments below, Dirk might have additional ones: > WebCore/ChangeLog:8 > + SVN filter primitives can use the output of other filters. The s/SVN/SVG/ > WebCore/ChangeLog:11 > + primitive result was converted to image buffer before, which s/result was/results were/, s/buffer/buffers/ > WebCore/ChangeLog:15 > + this patch. All apply() methods updated according to this new s/updated/are updated/ > WebCore/platform/graphics/filters/FEBlend.cpp:-135 > - resultImage()->putPremultipliedImageData(imageData.get(), imageRect, IntPoint()); Ah, this is correct, as you're using the resultImage instead of creating a new one. > WebCore/platform/graphics/filters/FEComponentTransfer.cpp:172 > + in->asUnmultipliedImage(imageData, drawingRect); Hm, why is the return value ignored? (Ignore this, I was confused as there are two variants of asUnmultipliedImage, this one returns void.... see below) > WebCore/platform/graphics/filters/FilterEffect.cpp:84 > + m_imageBufferResult = ImageBuffer::create(m_absolutePaintRect.size(), ColorSpaceLinearRGB); > + IntRect dst(IntPoint(), m_absolutePaintRect.size()); line 84: s/dst/destinationRect/ > WebCore/platform/graphics/filters/FilterEffect.cpp:96 > + return imageData; A call to .release() is missing here! > WebCore/platform/graphics/filters/FilterEffect.cpp:103 > + return imageData; Ditto. > WebCore/platform/graphics/filters/FilterEffect.cpp:106 > +inline void FilterEffect::copyImageBytes(ImageData* src, ImageData* dst, const IntRect& rect) Avoid abbrevations, while it is convienent here, I'd prefer source and destination. > WebCore/platform/graphics/filters/FilterEffect.cpp:113 > + int originx = rect.x(); I'd prefer originX or xOrigin here. > WebCore/platform/graphics/filters/FilterEffect.cpp:114 > + int destx = 0; Same here, destX or xDest (I'd prefer the latter ;-) > WebCore/platform/graphics/filters/FilterEffect.cpp:119 > + int endx = rect.x() + rect.width(); rect.right() ? > WebCore/platform/graphics/filters/FilterEffect.cpp:129 > + int endy = rect.y() + rect.height(); rect.bottom() ? > WebCore/platform/graphics/filters/FilterEffect.cpp:137 > + int dstScanline = rect.width() * 4; > + int srcScanline = m_absolutePaintRect.width() * 4; > + unsigned char *dstPixel = dst->data()->data()->data() + (((desty * rect.width()) + destx) * 4); > + unsigned char *srcPixel = src->data()->data()->data() + (((originy * m_absolutePaintRect.width()) + originx) * 4); unsigned char* destinationPixel, and one useless pair of braces at the end. Same for sourcePixel. dstScanline should be "destinationScanline", same for srcScanline. > WebCore/platform/graphics/filters/FilterEffect.cpp:143 > + originy++; I'd prefer ++originy. > WebCore/platform/graphics/filters/FilterEffect.cpp:151 > + if (!m_unmultipliedImageResult) { Can you rewrite this with a early return for m_unmultipliedImageResult=true. > WebCore/platform/graphics/filters/FilterEffect.cpp:159 > + unsigned char* src = m_premultipliedImageResult->data()->data()->data(); > + unsigned char* dst = m_unmultipliedImageResult->data()->data()->data(); > + unsigned char* end = src + (m_absolutePaintRect.width() * m_absolutePaintRect.height() * 4); Again, src -> source, dst -> destination etc. > WebCore/platform/graphics/filters/FilterEffect.cpp:184 > + if (!m_premultipliedImageResult) { Same comments as abobe, early return style please, and variable renamings. > WebCore/platform/graphics/filters/FilterEffect.h:58 > + PassRefPtr<ImageData> asUnmultipliedImage(const IntRect&); > + PassRefPtr<ImageData> asPremultipliedImage(const IntRect&); > + void asUnmultipliedImage(ImageData* dst, const IntRect&); > + void asPremultipliedImage(ImageData* dst, const IntRect&); The naming of these methods is misleading, I stared at 172 in->asUnmultipliedImage(imageData, drawingRect); before, wondering why you ignore the returned PassRefPtr<ImageData> until I realized there are two variants of the same method, one which returns void and the other with the PassRefPtr. Can you rename the last two methods, to something like "convertToXXXImage" or maybe even a better name.. Also the "dst" parameter name is not needed. > WebCore/platform/graphics/filters/FilterEffect.h:132 > + inline void copyImageBytes(ImageData* src, ImageData* dst, const IntRect&); s/src/source/ s/dst/destination/

Comment on attachment 74995 [details] patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=74995&action=review Great patch. Just some minor notes. Could you write a manual perf test which shows the affect of your changes please? You can place the test in WebCore/manual-test. If it could measure the timings it would be a great tool for performance patches in the future, but it doesn't need it. Beth wrote a perf test for SVG clipping. It is placed there too. Maybe you can take a look at it. > WebCore/platform/graphics/filters/FilterEffect.cpp:25 > + IIRC we don't add newlines here. >> WebCore/platform/graphics/filters/FilterEffect.cpp:106 >> +inline void FilterEffect::copyImageBytes(ImageData* src, ImageData* dst, const IntRect& rect) > > Avoid abbrevations, while it is convienent here, I'd prefer source and destination. I guess you're following the style in ImageBuffer*.cpp, but maybe Niko is right here. > WebCore/platform/graphics/filters/FilterEffect.cpp:110 > + if (rect.x() < 0 || rect.y() < 0 || (rect.x() + rect.width()) > m_absolutePaintRect.width() || (rect.y() + rect.height()) > m_absolutePaintRect.height()) (rect.x() + rect.width()) See Nikos comment about right(), the same for the vertical position, just bottom() >> WebCore/platform/graphics/filters/FilterEffect.cpp:159 >> + unsigned char* end = src + (m_absolutePaintRect.width() * m_absolutePaintRect.height() * 4); > > Again, src -> source, dst -> destination etc. Would mean to write copyImageBytes(m_unmultipliedImageResult.get(), dst, rect); twice. Bu maybe it's still better to read. So...

> > WebCore/platform/graphics/filters/FilterEffect.h:58 > > + PassRefPtr<ImageData> asUnmultipliedImage(const IntRect&); > > + PassRefPtr<ImageData> asPremultipliedImage(const IntRect&); > > + void asUnmultipliedImage(ImageData* dst, const IntRect&); > > + void asPremultipliedImage(ImageData* dst, const IntRect&); Renamed to copyUnmultipliedImage and copyPremultipliedImage. About the renamings, bottoms(), etc.: As Krit said, they all come from the GraphicsContext (platform dependent) implementation, and perhaps they should be renamed there as well (they probably copied the mac code). Unfortunately there are lots of platforms.

(In reply to comment #17) > > > WebCore/platform/graphics/filters/FilterEffect.h:58 > > > + PassRefPtr<ImageData> asUnmultipliedImage(const IntRect&); > > > + PassRefPtr<ImageData> asPremultipliedImage(const IntRect&); > > > + void asUnmultipliedImage(ImageData* dst, const IntRect&); > > > + void asPremultipliedImage(ImageData* dst, const IntRect&); > > Renamed to copyUnmultipliedImage and copyPremultipliedImage. > > About the renamings, bottoms(), etc.: As Krit said, they all come from the GraphicsContext (platform dependent) implementation, and perhaps they should be renamed there as well (they probably copied the mac code). Unfortunately there are lots of platforms. Not your problem now. Just fix it for your patch and just for SVG. If you want, you can change the style in ImageBuffer* and GraphicsContext* in another patch afterwards.

Created attachment 75679 [details] patch 3 Added a nice manual test. 73% speedup with my patch (on a mac mini).

Comment on attachment 75679 [details] patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=75679&action=review The patch looks great, still some notes from me. > WebCore/platform/graphics/filters/FEBlend.cpp:92 > + if (hasResult()) > + return; You should mention this in the changelog, that you do not create the same filter effect result twice. > WebCore/platform/graphics/filters/FETurbulence.cpp:329 > + if (!absolutePaintRect().width() || !absolutePaintRect().height()) do we allow negative width or height? If not you could use isEmpty() here. > WebCore/platform/graphics/filters/FilterEffect.cpp:130 > + int xOrigin = rect.x(); > + int xDest = 0; > + if (xOrigin < 0) { > + xDest = -xOrigin; > + xOrigin = 0; > + } > + int xEnd = rect.right(); > + if (xEnd > m_absolutePaintRect.width()) > + xEnd = m_absolutePaintRect.width(); > + > + int yOrigin = rect.y(); > + int yDest = 0; > + if (yOrigin < 0) { > + yDest = -yOrigin; > + yOrigin = 0; > + } > + int yEnd = rect.bottom(); > + if (yEnd > m_absolutePaintRect.height()) > + yEnd = m_absolutePaintRect.height(); I wounder if we could use the intersect functionality of FloatRect instead doing it ourself here.

> I wounder if we could use the intersect functionality of FloatRect instead doing it ourself here. Don't think so as we need the info 'how' the intersection happens.

Created attachment 75793 [details] patch 4 Minor changes. Hopefully this is the last iteration.

Attachment 75793 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75793 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75793 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75793 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 75793 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.

Sorry, I reviewed the patch a while ago, somehow the review was not commited and I didn't check it afterwards :-(

Comment on attachment 75793 [details] patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=75793&action=review r=me. Sorry for the delay. Just ping me, write me a mail if you are waiting more than a week. > WebCore/ChangeLog:18 > + Filters do not generate their results twice (or more), when ..twice (or more) anymore. ...

> r=me. Sorry for the delay. Just ping me, write me a mail if you are waiting more than a week. No poblem. I thought you were busy, and I had other task to do anyway.

Landed in r73894 Closing bug.