Re: [PATCH v3 3/4] ui/cocoa: Wraps CGImage creation in helper function

Re: [PATCH v3 3/4] ui/cocoa: Wraps CGImage creation in helper function

[Akihiko Odaki]

On 2024/07/07 5:43, Phil Dennis-Jordan wrote:
> This reduces the incidental complexity of the screen update draw and
> cursor conversion functions and minimally reduces overall code size.
> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>   ui/cocoa.m | 85 +++++++++++++++++++++++++++---------------------------
>   1 file changed, 43 insertions(+), 42 deletions(-)
> 
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 309d1320d7..36abb679d0 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -292,6 +292,39 @@ static void handleAnyDeviceErrors(Error * err)
>       }
>   }
>   
> +static CGImageRef create_cg_image(void *data, unsigned width, unsigned height,
> +                                  size_t stride, unsigned bpp, unsigned bpc,
> +                                  bool use_alpha, CGColorSpaceRef colorspace)

The choice of parameters looks arbitrary. The number of bytes per pixel 
(4ul) and CGBitmapInfo are hardcoded while the other parameters are not. 
Having a function that simply hardcodes some parameters of another 
function while passing through the others makes little sense; such a 
function will be no use once some parameters change.

However I do see some benefit in extracting the pair of 
CGDataProviderCreateWithData() and CGDataProviderRelease() into a 
function. A better design of this function would be to pass through 
parameters of CGImageCreate() while deriving the parameters of 
CGDataProviderCreateWithData() from them.

I also suggest moving this function into QemuCocoaView. QemuCocoaView 
has its colorspace defined so its method can assume that defined colorspace.

Regards,
Akihiko Odaki

> +{
> +    CGImageRef image;
> +    CGDataProviderRef provider;
> +    CGBitmapInfo bitmap_info = kCGBitmapByteOrder32Little
> +        | (use_alpha ? kCGImageAlphaFirst : kCGImageAlphaNoneSkipFirst);
> +
> +    provider = CGDataProviderCreateWithData(
> +        NULL,
> +        data,
> +        4ul * width * height,
> +        NULL
> +    );
> +    image = CGImageCreate(
> +        width, //width
> +        height, //height
> +        bpc, //bitsPerComponent
> +        bpp, //bitsPerPixel
> +        width * 4ul, //bytesPerRow
> +        colorspace, //colorspace
> +        bitmap_info, //bitmapInfo
> +        provider, //provider
> +        NULL, //decode
> +        0, //interpolate
> +        kCGRenderingIntentDefault //intent
> +    );
> +
> +    CGDataProviderRelease(provider);
> +    return image;
> +}
> +
>   /*
>    ------------------------------------------------------
>       QemuCocoaView
> @@ -464,7 +497,6 @@ - (void)setMouseX:(int)x y:(int)y on:(bool)on
>   
>   - (void)setCursor:(QEMUCursor *)given_cursor
>   {
> -    CGDataProviderRef provider;
>       CGImageRef image;
>       CGRect bounds = CGRectZero;
>   
> @@ -480,28 +512,12 @@ - (void)setCursor:(QEMUCursor *)given_cursor
>       bounds.size.width = cursor->width;
>       bounds.size.height = cursor->height;
>   
> -    provider = CGDataProviderCreateWithData(
> -        NULL,
> -        cursor->data,
> -        cursor->width * cursor->height * 4,
> -        NULL
> -    );
> +    image = create_cg_image(
> +        cursor->data, cursor->width, cursor->height,
> +        cursor->width * 4 /* bytes per row */,
> +        32 /* bits/pixel */, 8 /* bits/component */, true /* alpha */,
> +        colorspace);
>   
> -    image = CGImageCreate(
> -        cursor->width, //width
> -        cursor->height, //height
> -        8, //bitsPerComponent
> -        32, //bitsPerPixel
> -        cursor->width * 4, //bytesPerRow
> -        colorspace, //colorspace
> -        kCGBitmapByteOrder32Little | kCGImageAlphaFirst, //bitmapInfo
> -        provider, //provider
> -        NULL, //decode
> -        0, //interpolate
> -        kCGRenderingIntentDefault //intent
> -    );
> -
> -    CGDataProviderRelease(provider);
>       [CATransaction begin];
>       [CATransaction setDisableActions:YES];
>       [cursorLayer setBounds:bounds];
> @@ -533,25 +549,11 @@ - (void) drawRect:(NSRect) rect
>           int bitsPerPixel = PIXMAN_FORMAT_BPP(format);
>           int bitsPerComponent = PIXMAN_FORMAT_R(format);
>           int stride = pixman_image_get_stride(pixman_image);
> -        CGDataProviderRef dataProviderRef = CGDataProviderCreateWithData(
> -            NULL,
> -            pixman_image_get_data(pixman_image),
> -            stride * h,
> -            NULL
> -        );
> -        CGImageRef imageRef = CGImageCreate(
> -            w, //width
> -            h, //height
> -            bitsPerComponent, //bitsPerComponent
> -            bitsPerPixel, //bitsPerPixel
> -            stride, //bytesPerRow
> -            colorspace, //colorspace
> -            kCGBitmapByteOrder32Little | kCGImageAlphaNoneSkipFirst, //bitmapInfo
> -            dataProviderRef, //provider
> -            NULL, //decode
> -            0, //interpolate
> -            kCGRenderingIntentDefault //intent
> -        );
> +
> +        CGImageRef imageRef = create_cg_image(
> +            pixman_image_get_data(pixman_image), w, h, stride,
> +            bitsPerPixel, bitsPerComponent, false /* no alpha */, colorspace);
> +
>           // selective drawing code (draws only dirty rectangles) (OS X >= 10.4)
>           const NSRect *rectList;
>           NSInteger rectCount;
> @@ -571,7 +573,6 @@ - (void) drawRect:(NSRect) rect
>               CGImageRelease (clipImageRef);
>           }
>           CGImageRelease (imageRef);
> -        CGDataProviderRelease(dataProviderRef);
>       }
>   }
>