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

[Kshitij Suri]

On 22/03/22 3:17 pm, Daniel P. Berrangé wrote:
> 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://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_issues_718&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=utjv19Ej9Fb0TB7_DX0o3faQ-OAm2ypPniPyqVSoj_w&m=n4gY8td5J_mIXLjj-t4OvqdRNDLAKuLn003VbLjWtqVaIGciyzeqQ8Fij45WWQ9m&s=gw9lEcMePStOP8Kcb4RSP_znNQSVAEUtBC63b1g1x5Q&e=
> >
> > 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;
>           }
Apologies, missed that. Will update in the following patch.
> > +
> > +    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
Thank you for your review!

Regards,
Kshitij Suri