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

[Kshitij Suri]

On 11/03/22 5:50 pm, Markus Armbruster wrote:

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

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=SxmcA4FlCCy9O9eUaDUiSY37bauF6iJbDRVL--VUyTG5Vze_GFjmJuxgwAVYRjad&s=OIKnm9xXYjeat7TyIJ_-z9EvG2XYXMULNbHe0Bjzyjo&e=

Signed-off-by: Kshitij Suri <kshitij.s...@nutanix.com>
---

[...]

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);

+    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);
  }

diff --git a/qapi/ui.json b/qapi/ui.json

index 9354f4c467..6aa0dd7c1b 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -73,12 +73,27 @@
  ##
  { 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }

+##

+# @ImageFormat:
+#
+# Available list of supported types.

This is just a hair better than "Lorem ipsum" :)

Suggest: Supported image format types.

Will add in the updated patch

+#
+# @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.

Is "VGA screen" accurate?  Or does this work for other displays, too?

The patch didn't modify any display changes and VGA screen was

previously supported display type.

  #
-# @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,9 @@
  #        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. ppm is the set as the
+#          default format. (Since 7.0)

I figure you mean "is set as the default".  Suggest to replace the
sentence by "(default: ppm)".

Will add in the updated patch.

+#
  # Returns: Nothing on success
  #
  # Since: 0.14
@@ -99,7 +117,8 @@
  #
  ##
  { 'command': 'screendump',
-  'data': {'filename': 'str', '*device': 'str', '*head': 'int'},
+  'data': {'filename': 'str', '*device': 'str', '*head': 'int',
+           '*format': 'ImageFormat'},
    'coroutine': true }

##

[...]

Thank you for your review!

Regards,
Kshitij Suri