Use Accelerate framework's vImage vectorized (un)premultiplyImageData functions for ImageBufferCG.cpp
Created attachment 80138 [details] WIP patch1 This is a WIP patch. Leaving it here for storage at the moment.
WebCore already links against Accelerate so the soft linking isn’t necessary.
Created attachment 86723 [details] Patch
Attachment 86723 [details] did not build on win: Build output: http://queues.webkit.org/results/8237044
Created attachment 86807 [details] Patch
Comment on attachment 86807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86807&action=review > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:45 > +#if OS(DARWIN) I think this would be better as a HAVE(ACCELERATE) check. Which would need to be added to Platform.h > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:314 > +#if OS(DARWIN) Again, the HAVE() check would be cleaner.
(In reply to comment #6) > (From update of attachment 86807 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=86807&action=review > > > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:45 > > +#if OS(DARWIN) Looking through the project for preexisting code that uses the Accelerate framework I've found them all to be using #if OS(DARWIN). Apparently Accelerate was introduced in Panther and was extended in Tiger. Given that, should I still make a HAVE for Accelerate?
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 86807 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=86807&action=review > > > > > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:45 > > > +#if OS(DARWIN) > Looking through the project for preexisting code that uses the Accelerate framework I've found them all to be using #if OS(DARWIN). Apparently Accelerate was introduced in Panther and was extended in Tiger. Given that, should I still make a HAVE for Accelerate? Yes, I think so.
It should be USE(), not HAVE(), if anything.
In any case, let's get this reviewed and landed soon!
Created attachment 87015 [details] Patch
Comment on attachment 87015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87015&action=review > Source/JavaScriptCore/ChangeLog:5 > + Use Accelerate framework's vImage vectorized (un)premultiplyImageData functions for ImageBufferCG.cpp The changelog is not HTML; entities won't work here. > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:328 > + vImage_Flags flags = kvImageNoFlags; > + vImageUnpremultiplyData_RGBA8888(&src, &dst, flags); You could pass kvImageNoFlags directly. > LayoutTests/ChangeLog:5 > + Use Accelerate framework's vImage vectorized (un)premultiplyImageData functions for ImageBufferCG.cpp Remove the entity. > LayoutTests/ChangeLog:15 > + now different. Note: the spec does not define how they *should* be rounded, so having them there is basically just But is our rounding compatible with other browsers?
Created attachment 87448 [details] Patch
> Remove the entity. > Removed. > > LayoutTests/ChangeLog:15 > > + now different. Note: the spec does not define how they *should* be rounded, so having them there is basically just > > But is our rounding compatible with other browsers? I checked a handful of browsers and platform combos and they all appear to be different from each other - go figure. Though, it's at most the matter of a difference of 1 for a given subpixel value after calling getImageData on a pixel. This "error" if you will cannot accumulate. It's just the initial rounding error due to storing in a lower precision than that inputted. I added in a runtime check for the OS in order to avoid using the buggy vImage calls on older snow leopards. Also, I removed the "rgba getImageData" pixel spew that two of the tests had. Basically, the only utility that had for us was to be able to track if our initial rounding behavior (the one mentioned above) ever changed. Since there's no easy way for us to track the different "expected" values of this between 10.6.6 and 10.6.7 in the LayoutTests then I think it's more hassle than it's worth. A suggestion was to change the test to instead check that those values are within +/- 1 of the original, but this case of ensuring that we don't have a drifting error is covered by many other tests.
Comment on attachment 87448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87448&action=review > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:325 > +#if USE(ACCELERATE) > + SInt32 majorVersion = 0; > + SInt32 minorVersion = 0; > + SInt32 bugFixVersion = 0; > + Gestalt(gestaltSystemVersionMajor, &majorVersion); > + Gestalt(gestaltSystemVersionMinor, &minorVersion); > + Gestalt(gestaltSystemVersionBugFix, &bugFixVersion); > + useVImage |= (majorVersion > 10); > + useVImage |= (majorVersion == 10 && minorVersion > 6); > + useVImage |= (majorVersion == 10 && minorVersion == 6 && bugFixVersion > 6); > +#endif nit: since this codepath might be hot and the value of useVImage will never change during execution, could you put this check in a static helper that caches the result? I don't know how expensive querying the version is but I assume it's not free. If this was moved into a helper then you could avoid having to type it out again in putImageData() http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/mac/FontMac.mm#L58 shows an example of caching like this.
Attachment 87448 [details] did not build on win: Build output: http://queues.webkit.org/results/8297190
Comment on attachment 87448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87448&action=review > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:324 > + SInt32 majorVersion = 0; > + SInt32 minorVersion = 0; > + SInt32 bugFixVersion = 0; > + Gestalt(gestaltSystemVersionMajor, &majorVersion); > + Gestalt(gestaltSystemVersionMinor, &minorVersion); > + Gestalt(gestaltSystemVersionBugFix, &bugFixVersion); > + useVImage |= (majorVersion > 10); > + useVImage |= (majorVersion == 10 && minorVersion > 6); > + useVImage |= (majorVersion == 10 && minorVersion == 6 && bugFixVersion > 6); You should check the error returned from Gestalt(). You can also do SInt32 version; if (Gestalt(gestaltSystemVersion, &version) == noErr && version > 0x1066)...
Created attachment 87558 [details] Patch
Comment on attachment 87558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87558&action=review > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:116 > + static bool isCached = false; > + static bool result = false; > + > + if (!isCached) { > +#if USE(ACCELERATE) > + SInt32 version; > + result = (Gestalt(gestaltSystemVersion, &version) == noErr && version > 0x1066); > +#endif > + isCached = true; > + } > + return result; You can do this on one line in C++: static haveVImageFix = Gestalt(gestaltSystemVersion, &version) == noErr && version > 0x1066; hasFixedVImage() is not a very descriptive function name.
Created attachment 87580 [details] Patch
Comment on attachment 87580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87580&action=review > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:483 > + for (int y = 0; y < height; ++y) { > + for (int x = 0; x < width; x++) { > + int basex = x * 4; > + unsigned char alpha = srcRows[basex + 3]; > + if (alpha != 255) { > + destRows[basex] = (srcRows[basex] * alpha + 254) / 255; > + destRows[basex + 1] = (srcRows[basex + 1] * alpha + 254) / 255; > + destRows[basex + 2] = (srcRows[basex + 2] * alpha + 254) / 255; > + destRows[basex + 3] = alpha; > + } > + } Isn't this loop missing: srcRows += srcBytesPerRow; destRows += destBytesPerRow; which indicates that you didn't test it.
> Isn't this loop missing: > > srcRows += srcBytesPerRow; > destRows += destBytesPerRow; > Oh wow. Yep. > which indicates that you didn't test it. Yea, I didn't test all the old configurations. I double check some of them now though.
Created attachment 87625 [details] Patch
Comment on attachment 87625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87625&action=review > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:311 > + printf("Getting image data with height or width equal to 0, early return \n"); Remove this before committing. > LayoutTests/fast/canvas/getPutImageDataPairTest.html:26 > + r = b = g = Math.round(255*Math.random()); > + var a = Math.random(); Tests that use randomness are annoying to debug. I don't really see the value in using a random color; just hardcode something.
Committed r82516: <http://trac.webkit.org/changeset/82516>
http://trac.webkit.org/changeset/82516 might have broken Windows 7 Release (Tests)