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

[Daniel P . Berrangé]

On Tue, Mar 22, 2022 at 08:18:45AM +0000, Kshitij Suri wrote:
> Currently screendump only supports PPM format, which is un-compressed and not
> standard. Added a "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.suri@nutanix.com>
> ---
> diff to v1:
>   - Removed repeated alpha conversion operation.
>   - Modified logic to mirror png conversion in vnc-enc-tight.c file.
>   - Added a new CONFIG_PNG parameter for libpng support.
>   - Changed input format to enum instead of string.
>   - Improved error handling.
>  hmp-commands.hx    |  11 ++---
>  monitor/hmp-cmds.c |  20 ++++++++-
>  qapi/ui.json       |  24 +++++++++--
>  ui/console.c       | 101 +++++++++++++++++++++++++++++++++++++++++++--
>  4 files changed, 144 insertions(+), 12 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8476277aa9..19b7cab595 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -244,11 +244,12 @@ 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'",
> -        .cmd        = hmp_screendump,
> +        .args_type  = "filename:F,format:s?,device:s?,head:i?",
> +        .params     = "filename [format] [device [head]]",
> +        .help       = "save screen from head 'head' of display device 
> 'device'"
> +                      "in specified format 'format' as image 'filename'."
> +                      "Currently only 'png' and 'ppm' formats are 
> supported.",
> +         .cmd        = hmp_screendump,
>          .coroutine  = true,
>      },
>  
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 634968498b..bf3ba76bd3 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1720,9 +1720,27 @@ 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 *input_format  = qdict_get_try_str(qdict, "format");
>      Error *err = NULL;
> +    ImageFormat format;
>  
> -    qmp_screendump(filename, id != NULL, id, id != NULL, head, &err);
> +    int val = qapi_enum_parse(&ImageFormat_lookup, input_format,
> +                              IMAGE_FORMAT_PPM, &err);
> +    if (err) {
> +        goto end;
> +    }
> +
> +    switch (val) {
> +    case IMAGE_FORMAT_PNG:
> +        format = IMAGE_FORMAT_PNG;
> +        break;
> +    default:
> +        format = IMAGE_FORMAT_PPM;
> +    }

This switch looks pointless - the code is passing the default into
qapi_enum_parse already, this doesn't need to handle defaulting
again. This just needs

        format = qapi_enum_parse(&ImageFormat_lookup, input_format,
                                 IMAGE_FORMAT_PPM, &err);
        if (err) {
           goto end;
         }

> +
> +    qmp_screendump(filename, id != NULL, id, id != NULL, head,
> +                   input_format != NULL, format, &err);
> +end:
>      hmp_handle_error(mon, err);
>  }
>  

> +    for (y = 0; y < height; ++y) {
> +        qemu_pixman_linebuf_fill(linebuf, image, width, 0, y);
> +       png_write_row(png_ptr, buf);
> +    }

Tiny style bug, indent off-by-1


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|