* Re: [PATCH v3 3/4] ui/cocoa: Wraps CGImage creation in helper function
[not found] ` <20240706204345.99392-4-phil@philjordan.eu>
@ 2024-07-15 5:45 ` Akihiko Odaki
0 siblings, 0 replies; only message in thread
From: Akihiko Odaki @ 2024-07-15 5:45 UTC (permalink / raw)
To: Phil Dennis-Jordan, qemu-devel
Cc: peter.maydell, philmd, marcandre.lureau, lists
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);
> }
> }
>
^ permalink raw reply [flat|nested] only message in thread