Dave, please have a look at the HMP compatibility issue in
hmp-command.hx below.
Kshitij Suri <kshitij.s...@nutanix.com> writes:
> Currently screendump only supports PPM format, which is un-compressed and not
> standard.
If "standard" means "have to pay a standards organization $$$ to access
the spec", PPM is not standard. If it means "widely supported", it
certainly is. I'd drop "and not standard". Suggestion, not demand.
> 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.
Suggest to use imperative mood to describe the commit, and omit details
that aren't necessary here:
Add a "format" parameter to QMP and HMP screendump command
to support PNG image capture using libpng.
>
> Example usage:
> { "execute": "screendump", "arguments": { "filename": "/tmp/image",
> "format":"png" } }
Providing an example in the commit message is always nice, thanks!
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/718
>
> Signed-off-by: Kshitij Suri <kshitij.s...@nutanix.com>
>
> Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
> ---
> hmp-commands.hx | 11 ++---
> monitor/hmp-cmds.c | 12 +++++-
> qapi/ui.json | 24 +++++++++--
> ui/console.c | 101 +++++++++++++++++++++++++++++++++++++++++++--
> 4 files changed, 136 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?",
Incompatible change: meaning of "screendump ONE TWO" changes from
filename=ONE, device=TWO to filename=ONE, format=TWO.
As HMP is not a stable interface, incompatible change is permissible.
But is this one wise?
Could we add the new argument at the end instead?
.args_type = "filename:F,device:s?,head:i?,format:s?",
Could we do *without* an argument, and derive the format from the
filename extension? .png means format=png, anything else format=ppm.
Would be a bad idea for QMP. Okay for HMP?
> + .params = "filename [format] [device [head]]",
This tells us that parameter format can be omitted like so
screendump foo.ppm device-id
which isn't true. Better: "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..2442bfa989 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1720,9 +1720,19 @@ 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);
> + format = qapi_enum_parse(&ImageFormat_lookup, input_format,
> + IMAGE_FORMAT_PPM, &err);
> + if (err) {
> + goto end;
> + }
> +
> + qmp_screendump(filename, id != NULL, id, id != NULL, head,
> + input_format != NULL, format, &err);
> +end:
> hmp_handle_error(mon, err);
> }
>
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 664da9e462..24371fce05 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -157,12 +157,27 @@
> ##
> { 'command': 'expire_password', 'boxed': true, 'data':
> 'ExpirePasswordOptions' }
>
> +##
> +# @ImageFormat:
> +#
> +# Supported image format types.
> +#
> +# @png: PNG format
> +#
> +# @ppm: PPM format
> +#
> +# Since: 7.1
> +#
> +##
> +{ 'enum': 'ImageFormat',
> + 'data': ['ppm', 'png'] }
> +
> ##
> # @screendump:
> #
> -# Write a PPM of the VGA screen to a file.
> +# Capture the contents of a screen and write it to a file.
> #
> -# @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)
> @@ -171,6 +186,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.1)
Recommend
# @format: image format for screendump (default: ppm) (Since 7.1)
> +#
> # Returns: Nothing on success
> #
> # Since: 0.14
> @@ -183,7 +200,8 @@
> #
> ##
> { 'command': 'screendump',
> - 'data': {'filename': 'str', '*device': 'str', '*head': 'int'},
> + 'data': {'filename': 'str', '*device': 'str', '*head': 'int',
> + '*format': 'ImageFormat'},
> 'coroutine': true }
>
> ##
QAPI schema
Acked-by: Markus Armbruster <arm...@redhat.com>
[...]
