|
From: | Kshitij Suri |
Subject: | Re: [PATCH v2 2/2] Added parameter to take screenshot with screendump as |
Date: | Wed, 16 Mar 2022 23:45:03 +0530 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 |
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? Not your patch's fault, of course.
As per hmp-commands.hx input is of format [device [head]] so maybe works as a pair. Is it alright if I investigate and send in another patch after this?
+ int val = qapi_enum_parse(&ImageFormat_lookup, input_format, -1, &err); + if (val < 0) { + goto end; + } + + switch (val) { + case IMAGE_FORMAT_PNG: + format = IMAGE_FORMAT_PNG; + break; + default: + format = IMAGE_FORMAT_PPM; + } + + qmp_screendump(filename, id != NULL, id, id != NULL, head, + input_format != NULL, format, &err); +end: hmp_handle_error(mon, err); }This isn't quite right. qapi_enum_parse() is awkward to use. If its second argument @input_format is null, it returns its third argument -1. Else, it @input_format is a valid enum string, it returns the enumeration value, i.e. either IMAGE_FORMAT_PPM or IMAGE_FORMAT_PNG. Else, it sets an error and returns its third argument -1. If @input_format is null, I suspect your code jumps to hmp_handle_error() with a null @err. Silently does nothing. It should default to IMAGE_FORMAT_PPM instead. I think you want something like this (completely untested): val = qapi_enum_parse(&ImageFormat_lookup, input_format, IMAGE_FORMAT_PPM, &err); if (err) { goto end; } qmp_screendump(filename, id != NULL, id, head != NULL, head, input_format != NULL, val, &err);
Oh okay. Will add it in the updated patch.
diff --git a/qapi/ui.json b/qapi/ui.jsonindex 9354f4c467..a6d994ba2c 100644 --- a/qapi/ui.json +++ b/qapi/ui.json @@ -73,12 +73,27 @@ ## { 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }+##+# @ImageFormat: +# +# Supported image format types. +# +# @png: PNG format +# +# @ppm: PPM format +# +# Since: 7.0 +# +## +{ 'enum': 'ImageFormat', + 'data': ['ppm', 'png'] } + ## # @screendump: # -# Write a PPM of the VGA screen to a file. +# Write a screenshot of the VGA screen to a file.Let's improve this to # Capture the contents of a screen and write it to a file.
Yes will send it in the updated patch.
# -# @filename: the path of a new PPM file to store the image +# @filename: the path of a new file to store the image # # @device: ID of the display device that should be dumped. If this parameter # is missing, the primary display will be used. (Since 2.12) @@ -87,6 +102,8 @@ # parameter is missing, head #0 will be used. Also note that the head # can only be specified in conjunction with the device ID. (Since 2.12) # +# @format: image format for screendump is specified. (default: ppm) (Since 7.0) +# # Returns: Nothing on success # # Since: 0.14 @@ -99,7 +116,8 @@ # ## { 'command': 'screendump', - 'data': {'filename': 'str', '*device': 'str', '*head': 'int'}, + 'data': {'filename': 'str', '*device': 'str', '*head': 'int', + '*format': 'ImageFormat'}, 'coroutine': true }##[...]@@ -327,7 +413,8 @@ static void graphic_hw_update_bh(void *con) /* Safety: coroutine-only, concurrent-coroutine safe, main thread only */ void coroutine_fn qmp_screendump(const char *filename, bool has_device, const char *device, - bool has_head, int64_t head, Error **errp) + bool has_head, int64_t head, bool has_format, + ImageFormat format, Error **errp)Please break the line like bool has_head, int64_t head, bool has_format, ImageFormat format, Error **errp)
This does look better. Will update it thank you!
{ g_autoptr(pixman_image_t) image = NULL; QemuConsole *con; @@ -383,8 +470,17 @@ qmp_screendump(const char *filename, bool has_device, const char *device, * yields and releases the BQL. It could produce corrupted dump, but * it should be otherwise safe. */ - if (!ppm_save(fd, image, errp)) { - qemu_unlink(filename); + + if (has_format && format == IMAGE_FORMAT_PNG) { + /* PNG format specified for screendump */ + if (!png_save(fd, image, errp)) { + qemu_unlink(filename); + } + } else { + /* PPM format specified/default for screendump */ + if (!ppm_save(fd, image, errp)) { + qemu_unlink(filename); + } } }
Thank you for your review! Regards, Kshitij Suri
[Prev in Thread] | Current Thread | [Next in Thread] |