Bug 147391 - Remove dispatch_apply_f and instead use vImage more directly
Summary: Remove dispatch_apply_f and instead use vImage more directly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-28 17:17 PDT by Dean Jackson
Modified: 2015-07-28 18:47 PDT (History)
0 users

See Also:


Attachments
Patch (14.36 KB, patch)
2015-07-28 17:25 PDT, Dean Jackson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2015-07-28 17:17:56 PDT
Remove dispatch_apply_f and instead use vImage more directly
Comment 1 Dean Jackson 2015-07-28 17:25:39 PDT
Created attachment 257711 [details]
Patch
Comment 2 Dean Jackson 2015-07-28 17:27:50 PDT
<rdar://problem/21893047>
Comment 3 Simon Fraser (smfr) 2015-07-28 17:41:44 PDT
Comment on attachment 257711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257711&action=review

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:60
> +static void premultiplyBufferData(vImage_Buffer* src, vImage_Buffer* dest)

Could use const vImage_Buffer&, right?

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:65
> +    if (kvImageNoError != vImagePremultiplyData_RGBA8888(src, dest, kvImageDoNotTile))

Is there any reason to use kvImageDoNotTile, or should we just let vImage use threads if it wants to?

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:70
> +    vImagePermuteChannels_ARGB8888(dest, dest, map, kvImageDoNotTile);

Ditto.

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:73
> +static void unpremultiplyBufferData(vImage_Buffer* src, vImage_Buffer* dest)

const vImage_Buffer&

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:83
> +    vImagePermuteChannels_ARGB8888(dest, dest, map, kvImageDoNotTile);

kvImageNoFlags?

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:270
> +            // The unpremultiplying and channel-swapping will be done in-place ooooo.

oooo?

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:420
>              if (resolutionScale != 1) {
>                  vImage_AffineTransform scaleTransform = { resolutionScale, 0, 0, resolutionScale, 0, 0 }; // FIXME: Add subpixel translation.
>                  Pixel_8888 backgroundColor;
> -                vImageAffineWarp_ARGB8888(&src, &dst, 0, &scaleTransform, backgroundColor, kvImageEdgeExtend);
> +                vImageAffineWarp_ARGB8888(&src, &dest, 0, &scaleTransform, backgroundColor, kvImageEdgeExtend);
>                  // The premultiplying will be done in-place.
> -                src = dst;
> +                src = dest;
>              }
>  
> -            vImagePremultiplyData_RGBA8888(&src, &dst, kvImageNoFlags);
> +            vImagePremultiplyData_RGBA8888(&src, &dest, kvImageNoFlags);

Odd that this code isn't in its own function.
Comment 4 Dean Jackson 2015-07-28 18:02:47 PDT
(In reply to comment #3)
> Comment on attachment 257711 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257711&action=review
> 
> > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:60
> > +static void premultiplyBufferData(vImage_Buffer* src, vImage_Buffer* dest)
> 
> Could use const vImage_Buffer&, right?

Yes, fixed.

> 
> > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:65
> > +    if (kvImageNoError != vImagePremultiplyData_RGBA8888(src, dest, kvImageDoNotTile))
> 
> Is there any reason to use kvImageDoNotTile, or should we just let vImage
> use threads if it wants to?

Correct, we want kvImageNoFlags.

> 
> > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:420
> >              if (resolutionScale != 1) {
> >                  vImage_AffineTransform scaleTransform = { resolutionScale, 0, 0, resolutionScale, 0, 0 }; // FIXME: Add subpixel translation.
> >                  Pixel_8888 backgroundColor;
> > -                vImageAffineWarp_ARGB8888(&src, &dst, 0, &scaleTransform, backgroundColor, kvImageEdgeExtend);
> > +                vImageAffineWarp_ARGB8888(&src, &dest, 0, &scaleTransform, backgroundColor, kvImageEdgeExtend);
> >                  // The premultiplying will be done in-place.
> > -                src = dst;
> > +                src = dest;
> >              }
> >  
> > -            vImagePremultiplyData_RGBA8888(&src, &dst, kvImageNoFlags);
> > +            vImagePremultiplyData_RGBA8888(&src, &dest, kvImageNoFlags);
> 
> Odd that this code isn't in its own function.

Yeah, I made this:

static void affineWarpBufferData(const vImage_Buffer& src, const vImage_Buffer& dest, float scale)
{
    vImage_AffineTransform scaleTransform = { scale, 0, 0, scale, 0, 0 }; // FIXME: Add subpixel translation.
    Pixel_8888 backgroundColor;
    vImageAffineWarp_ARGB8888(&src, &dest, 0, &scaleTransform, backgroundColor, kvImageEdgeExtend);
}
Comment 5 Dean Jackson 2015-07-28 18:47:29 PDT
Committed r187534: <http://trac.webkit.org/changeset/187534>