[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/2] Added parameter to take screenshot with screendump as
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v2 2/2] Added parameter to take screenshot with screendump as PNG |
Date: |
Tue, 22 Mar 2022 11:15:16 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
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
[PATCH v2 1/2] Replacing CONFIG_VNC_PNG with CONFIG_PNG, Kshitij Suri, 2022/03/22
Re: [PATCH v2 1/2] Replacing CONFIG_VNC_PNG with CONFIG_PNG, Daniel P . Berrangé, 2022/03/22
[PATCH v2 1/2] Replacing CONFIG_VNC_PNG with CONFIG_PNG, Kshitij Suri, 2022/03/22
Re: [PATCH v2 1/2] Replacing CONFIG_VNC_PNG with CONFIG_PNG, Kshitij Suri, 2022/03/28
[PATCH v2 1/2] Replacing CONFIG_VNC_PNG with CONFIG_PNG, Kshitij Suri, 2022/03/28