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

[Markus Armbruster <armbru@redhat.com>]

Kshitij Suri <kshitij.suri@nutanix.com> writes:

> On 16/03/22 6:55 pm, Markus Armbruster wrote:
>> Kshitij Suri <kshitij.suri@nutanix.com> writes:
>>
>>> From: "kshitij.suri" <kshitij.suri@nutanix.com>
>>>
>>> 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=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=utjv19Ej9Fb0TB7_DX0o3faQ-OAm2ypPniPyqVSoj_w&m=UZZDywEeidD1LndEhKddMf_0v-jePIgMYErGImjYyvjYRJFdnv6LAHgRmZ0IpvIL&s=jc09kdvD1ULKCC9RgwWcsK6eweue3ZkyD8F9kCx5yUs&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 |  19 ++++++++-
>>>   qapi/ui.json       |  24 +++++++++--
>>>   ui/console.c       | 102 +++++++++++++++++++++++++++++++++++++++++++--
>>>   4 files changed, 144 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>>> index 70a9136ac2..5eda4eeb24 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'."
>>> +                      "Currently only 'png' and 'ppm' formats are supported.",
>>>           .cmd        = hmp_screendump,
>>>           .coroutine  = true,
>>>       },
>>>   
>>>   SRST
>>>   ``screendump`` *filename*
>>> -  Save screen into PPM image *filename*.
>>> +  Save screen as an image *filename*, with default format of PPM.
>>>   ERST
>>>   
>>>       {
>>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>>> index 8c384dc1b2..9a640146eb 100644
>>> --- a/monitor/hmp-cmds.c
>>> +++ b/monitor/hmp-cmds.c
>>> @@ -1677,9 +1677,26 @@ 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_str(qdict, "format");
>>>       Error *err = NULL;
>>> +    ImageFormat format;
>>>   
>>> -    qmp_screendump(filename, id != NULL, id, id != NULL, head, &err);
>>
>> The second id != NULL looks wrong.  Shouldn't it be head != NULL?

Can't: @head is not a pointer.

>> Not your patch's fault, of course.
>
> As per hmp-commands.hx input is of format [device [head]] so maybe
> works as a pair.

I looked: no, it works because it duplicates qmp_screendump()'s default.

qmp_screendump()'s behavior:

* If neither @device nor @head are present, default to console #0.

* If only @device is present, default to head 0 of the specified device.

* If both are present, default to the specified head of the specified
  device.

* If only @head is present, error out.

Ideally, we'd have hmp_screendump() pass its arguments unadulterated to
qmp_screendump(), so logic isn't duplicated.

Unfortunately, this turns out to be a bit troublesome.
qdict_get_try_int() *forces* us to supply a default value, and doesn't
tell us whether argument is present.  We'd have to use qdict_haskey()
for that:

    qmp_screendump(filename, id != NULL, id, qdict_haskey("head"), head,
                   &err);

The current code instead replaces case "only @device is present" by
"both are present".  Works, because the default it uses matches the one
in qmp_screendump().

I find this less obvious, but I'm not sure it's worth patching.

Note that case "only @head is present" isn't possible because the
arguments are positional.

>                  Is it alright if I investigate and send in another patch
> after this?

Let's not bother.