Kshitij Suri <kshitij.s...@nutanix.com> writes:
> On 16/03/22 6:55 pm, Markus Armbruster wrote:
>> Kshitij Suri <kshitij.s...@nutanix.com> writes:
>>
>>> From: "kshitij.suri" <kshitij.s...@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.s...@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.
