Re: [PATCH] Added parameter to take screenshot with screendump as PNG

[Kshitij Suri <kshitij.suri@nutanix.com>]

On 23/02/22 4:30 pm, Dr. David Alan Gilbert wrote:
> * Kshitij Suri (kshitij.suri@nutanix.com) wrote:
>> Currently screendump only supports PPM format, which is un-compressed and not
>> standard. Added an "format" parameter to qemu monitor screendump capabilites
>> to support PNG image capture using libpng. The param was added in QAPI schema
>> of screendump present in ui.json along with png_save() function which converts
>> pixman_image to PNG. HMP command equivalent was also modified to support the
>> feature.
>>
>> Example usage:
>> { "execute": "screendump", "arguments": { "filename": "/tmp/image", "format":"png" } }
>>
>> Resolves: https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_issues_718&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=utjv19Ej9Fb0TB7_DX0o3faQ-OAm2ypPniPyqVSoj_w&m=YAoUy9TYXWb8lktv5hZwRWjZEBcoHKxfdrHMd8KkTnQAESANHEcW96C1ukSngzJ2&s=xJP6q9IE0req5UkOFLablyJWWREdmwS1NX4Yse0pYxE&e=
>>
>> Signed-off-by: Kshitij Suri <kshitij.suri@nutanix.com>
>> ---
>>   hmp-commands.hx    |  11 ++--
>>   monitor/hmp-cmds.c |   4 +-
>>   qapi/ui.json       |   7 ++-
>>   ui/console.c       | 153 ++++++++++++++++++++++++++++++++++++++++++++-
>>   4 files changed, 165 insertions(+), 10 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 70a9136ac2..2163337f35 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -244,17 +244,18 @@ ERST
>>   
>>       {
>>           .name       = "screendump",
>> -        .args_type  = "filename:F,device:s?,head:i?",
>> -        .params     = "filename [device [head]]",
>> -        .help       = "save screen from head 'head' of display device 'device' "
>> -                      "into PPM image 'filename'",
>> +        .args_type  = "filename:F,device:s?,head:i?,format:f?",
>> +        .params     = "filename [device [head]] [format]",
>> +        .help       = "save screen from head 'head' of display device 'device'"
>> +                      "in specified format 'format' as image 'filename'."
>> +                      "Default format for screendump is PPM.",
>>           .cmd        = hmp_screendump,
>>           .coroutine  = true,
>>       },
>>   
>>   SRST
>>   ``screendump`` *filename*
>> -  Save screen into PPM image *filename*.
>> +  Save screen as image *filename*.
>>   ERST
>>   
>>       {
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index 2669156b28..3fb1394561 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -1665,9 +1665,11 @@ hmp_screendump(Monitor *mon, const QDict *qdict)
>>       const char *filename = qdict_get_str(qdict, "filename");
>>       const char *id = qdict_get_try_str(qdict, "device");
>>       int64_t head = qdict_get_try_int(qdict, "head", 0);
>> +    const char *format  = qdict_get_str(qdict, "format");
>>       Error *err = NULL;
>>   
>> -    qmp_screendump(filename, id != NULL, id, id != NULL, head, &err);
>> +    qmp_screendump(filename, id != NULL, id, id != NULL, head,
>> +                   format != NULL, format, &err);
>>       hmp_handle_error(mon, err);
>>   }
> I think that's OK from the HMP front, see some questions below.
>
>> diff --git a/qapi/ui.json b/qapi/ui.json
>> index 9354f4c467..9fdb56b60b 100644
>> --- a/qapi/ui.json
>> +++ b/qapi/ui.json
>> @@ -76,7 +76,7 @@
>>   ##
>>   # @screendump:
>>   #
>> -# Write a PPM of the VGA screen to a file.
>> +# Write a screenshot of the VGA screen to a file.
>>   #
>>   # @filename: the path of a new PPM file to store the image
>>   #
>> @@ -87,6 +87,9 @@
>>   #        parameter is missing, head #0 will be used. Also note that the head
>>   #        can only be specified in conjunction with the device ID. (Since 2.12)
>>   #
>> +# @format: image format for screendump is specified. Currently only PNG and
>> +#             PPM are supported.
>> +#
>>   # Returns: Nothing on success
>>   #
>>   # Since: 0.14
>> @@ -99,7 +102,7 @@
>>   #
>>   ##
>>   { 'command': 'screendump',
>> -  'data': {'filename': 'str', '*device': 'str', '*head': 'int'},
>> +  'data': {'filename': 'str', '*device': 'str', '*head': 'int', '*format': 'str'},
>>     'coroutine': true }
>>   
>>   ##
>> diff --git a/ui/console.c b/ui/console.c
>> index 40eebb6d2c..7813b195ac 100644
>> --- a/ui/console.c
>> +++ b/ui/console.c
>> @@ -37,6 +37,9 @@
>>   #include "exec/memory.h"
>>   #include "io/channel-file.h"
>>   #include "qom/object.h"
>> +#ifdef CONFIG_VNC_PNG
>> +#include "png.h"
>> +#endif
>>   
>>   #define DEFAULT_BACKSCROLL 512
>>   #define CONSOLE_CURSOR_PERIOD 500
>> @@ -289,6 +292,137 @@ void graphic_hw_invalidate(QemuConsole *con)
>>       }
>>   }
>>   
>> +#ifdef CONFIG_VNC_PNG
>> +/**
>> + * a8r8g8b8_to_rgba: Convert a8r8g8b8 to rgba format
>> + *
>> + * @dst: Destination pointer.
>> + * @src: Source pointer.
>> + * @n_pixels: Size of image.
>> + */
>> +static void a8r8g8b8_to_rgba(uint32_t *dst, uint32_t *src, int n_pixels)
>> +{
>> +    uint8_t *dst8 = (uint8_t *)dst;
>> +    int i;
>> +
>> +    for (i = 0; i < n_pixels; ++i) {
>> +        uint32_t p = src[i];
>> +        uint8_t a, r, g, b;
>> +
>> +        a = (p & 0xff000000) >> 24;
>> +        r = (p & 0x00ff0000) >> 16;
>> +        g = (p & 0x0000ff00) >> 8;
>> +        b = (p & 0x000000ff) >> 0;
>> +
>> +        if (a != 0) {
>> +            #define DIVIDE(c, a) \
>> +            do { \
>> +                int t = ((c) * 255) / a; \
>> +                (c) = t < 0 ? 0 : t > 255 ? 255 : t; \
> I can't see how 't' can be less than 0 here?
>
>> +            } while (0)
>> +
>> +            DIVIDE(r, a);
>> +            DIVIDE(g, a);
>> +            DIVIDE(b, a);
>> +            #undef DIVIDE
>> +        }
>> +
>> +        *dst8++ = r;
>> +        *dst8++ = g;
>> +        *dst8++ = b;
>> +        *dst8++ = a;
>> +    }
>> +}
> Now, I'm not too much of a graphics person, but I'm a bit confused; this
> seems to be applying the alpha scaling to the r/g/b/ data - but then
> still including the alpha value in the output; doesn't that mean that
> the thing displaying the result will also apply the alpha scaling again?
Yes, missed that. Modified it in the updated patch.
>> +/**
>> + * png_save: Take a screenshot as PNG
>> + *
>> + * Saves screendump as a PNG file
>> + *
>> + * Returns true for success or false for error.
>> + * Inspired from png test utils from https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_aseprite_pixman&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=utjv19Ej9Fb0TB7_DX0o3faQ-OAm2ypPniPyqVSoj_w&m=YAoUy9TYXWb8lktv5hZwRWjZEBcoHKxfdrHMd8KkTnQAESANHEcW96C1ukSngzJ2&s=WhMqvptTX23UfMHwjTaBDTYU4xqtPO0dvK7B8Tc1nCw&e=
>> + *
>> + * @fd: File descriptor for PNG file.
>> + * @image: Image data in pixman format.
>> + * @errp: Pointer to an error.
>> + */
>> +static bool png_save(int fd, pixman_image_t *image, Error **errp)
>> +{
>> +    int width = pixman_image_get_width(image);
>> +    int height = pixman_image_get_height(image);
>> +    int stride = width * 4;
>> +    g_autofree uint32_t *src_data = g_malloc(height * stride);
>> +    g_autofree uint32_t *dest_data = g_malloc(height * stride);
>> +    g_autoptr(pixman_image_t) src_copy;
>> +    g_autoptr(pixman_image_t) dest_copy;
>> +    g_autofree png_struct *write_struct;
>> +    g_autofree png_info *info_struct;
>> +    g_autofree png_bytep *row_pointers = g_malloc(height * sizeof(png_bytep));
>> +    FILE *f = fdopen(fd, "wb");
>> +    int y;
>> +    if (!f) {
>> +        error_setg(errp, "Failed to create file from file descriptor");
>> +        return false;
>> +    }
>> +
>> +    src_copy = pixman_image_create_bits(PIXMAN_a8r8g8b8, width, height,
>> +                                        src_data, stride);
>> +
>> +    pixman_image_composite32(PIXMAN_OP_SRC, image, NULL, src_copy, 0, 0, 0, 0,
>> +                             0, 0, width, height);
> Nothing in the rest of qemu uses pixman_image_composite*32* - all the
> rest uses the plain version; what's the difference?
>
>> +    memcpy(dest_data, src_data, sizeof(*src_data));
> Why if you're just about to nuke dest_data with the converted version?
>
>> +    a8r8g8b8_to_rgba(dest_data, src_data, height * width);
>> +
>> +    for (y = 0; y < height; ++y) {
>> +        row_pointers[y] = (png_bytep)(src_data + y * width);
>> +    }
>> +
>> +    write_struct = png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL,
>> +                                                 NULL, NULL);
>> +    if (!write_struct) {
>> +        error_setg(errp, "PNG creation failed. Unable to write struct");
>> +        return false;
>> +    }
>> +
>> +    info_struct = png_create_info_struct(write_struct);
>> +
>> +    if (!info_struct) {
>> +        error_setg(errp, "PNG creation failed. Unable to write info");
>> +        return false;
>> +    }
>> +
>> +    png_init_io(write_struct, f);
>> +
>> +    png_set_IHDR(write_struct, info_struct, width, height, 8,
>> +                 PNG_COLOR_TYPE_RGB_ALPHA, PNG_INTERLACE_NONE,
>> +                 PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE);
>> +
>> +    png_write_info(write_struct, info_struct);
>> +
>> +    png_write_image(write_struct, row_pointers);
> Had you considered using png_write_row and avoiding having to
> hold/allocate the destination and row_pointers at all?
> (In actual fact, can you steal the code in ui/vnc-enc-tight.c
> send_png_rect ? )
>
> Dave
yes thank you for the reference! Updated patch according to vnc-enc-tight.c.
>> +    png_write_end(write_struct, NULL);
>> +
>> +    if (fclose(f) != 0) {
>> +        error_setg(errp, "PNG creation failed. Unable to close file");
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +#else /* no png support */
>> +
>> +static bool png_save(int fd, pixman_image_t *image, Error **errp)
>> +{
>> +    error_setg(errp, "Enable VNC PNG support for png screendump");
>> +    return false;
>> +}
>> +
>> +#endif /* CONFIG_VNC_PNG */
>> +
>>   static bool ppm_save(int fd, pixman_image_t *image, Error **errp)
>>   {
>>       int width = pixman_image_get_width(image);
>> @@ -327,7 +461,8 @@ static void graphic_hw_update_bh(void *con)
>>   /* Safety: coroutine-only, concurrent-coroutine safe, main thread only */
>>   void coroutine_fn
>>   qmp_screendump(const char *filename, bool has_device, const char *device,
>> -               bool has_head, int64_t head, Error **errp)
>> +               bool has_head, int64_t head, bool has_format,
>> +               const char *format, Error **errp)
>>   {
>>       g_autoptr(pixman_image_t) image = NULL;
>>       QemuConsole *con;
>> @@ -383,8 +518,22 @@ qmp_screendump(const char *filename, bool has_device, const char *device,
>>        * yields and releases the BQL. It could produce corrupted dump, but
>>        * it should be otherwise safe.
>>        */
>> -    if (!ppm_save(fd, image, errp)) {
>> +
>> +    if (has_format && strcmp(format, "png") == 0) {
>> +        /* PNG format specified for screendump */
>> +        if (!png_save(fd, image, errp)) {
>> +            qemu_unlink(filename);
>> +        }
>> +    } else if (!has_format || (has_format && strcmp(format, "ppm") == 0)) {
>> +        /* PPM format specified/default for screendump */
>> +        if (!ppm_save(fd, image, errp)) {
>> +            qemu_unlink(filename);
>> +        }
>> +    } else {
>> +        /* Invalid specified for screendump */
>> +        error_setg(errp, "Invalid format provided for screendump.");
>>           qemu_unlink(filename);
>> +        return;
>>       }
>>   }
>>   
>> -- 
>> 2.22.3
>>
Thank you for your prompt review!

Regards,
Kshitij