* Kshitij Suri (kshitij.s...@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://gitlab.com/qemu-project/qemu/-/issues/718
>
> Signed-off-by: Kshitij Suri <kshitij.s...@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?
> +/**
> + * 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://github.com/aseprite/pixman
> + *
> + * @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
> + 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
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
