Bug 53134 – Use Accelerate framework's vImage vectorized (un)premultiplyImageData functions for ImageBufferCG.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Matthew Delaney]

Use Accelerate framework's vImage vectorized (un)premultiplyImageData functions for ImageBufferCG.cpp

[Matthew Delaney]

Created attachment 80138 [details]
WIP patch1

This is a WIP patch. Leaving it here for storage at the moment.

[Mark Rowe (bdash)]

WebCore already links against Accelerate so the soft linking isn’t necessary.

[Matthew Delaney]

Created attachment 86723 [details]
Patch

[Build Bot]

Attachment 86723 [details] did not build on win:
Build output: http://queues.webkit.org/results/8237044

[Matthew Delaney]

Created attachment 86807 [details]
Patch

[Sam Weinig]

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.

[Matthew Delaney]

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

[Sam Weinig]

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

[Maciej Stachowiak]

It should be USE(), not HAVE(), if anything.

[Maciej Stachowiak]

In any case, let's get this reviewed and landed soon!

[Matthew Delaney]

Created attachment 87015 [details]
Patch

[Simon Fraser (smfr)]

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?

[Matthew Delaney]

Created attachment 87448 [details]
Patch

[Matthew Delaney]

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

[James Robinson]

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.

[Build Bot]

Attachment 87448 [details] did not build on win:
Build output: http://queues.webkit.org/results/8297190

[Simon Fraser (smfr)]

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

[Matthew Delaney]

Created attachment 87558 [details]
Patch

[Simon Fraser (smfr)]

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.

[Matthew Delaney]

Created attachment 87580 [details]
Patch

[Simon Fraser (smfr)]

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.

[Matthew Delaney]

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

[Matthew Delaney]

Created attachment 87625 [details]
Patch

[Simon Fraser (smfr)]

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.

[Matthew Delaney]

Committed r82516: <http://trac.webkit.org/changeset/82516>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82516 might have broken Windows 7 Release (Tests)