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

[Kshitij Suri]

On 22/03/22 3:17 pm, Daniel P. Berrangé wrote:

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://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_issues_718&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=utjv19Ej9Fb0TB7_DX0o3faQ-OAm2ypPniPyqVSoj_w&m=n4gY8td5J_mIXLjj-t4OvqdRNDLAKuLn003VbLjWtqVaIGciyzeqQ8Fij45WWQ9m&s=gw9lEcMePStOP8Kcb4RSP_znNQSVAEUtBC63b1g1x5Q&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 |  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;
          }

Apologies, missed that. Will update in the following patch.

+
+    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

Thank you for your review!

Regards,
Kshitij Suri