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

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> 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;
>          }

Correct.  See my review of v1 for a detailed explanation.

>
>> +
>> +    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