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

[Daniel P . Berrangé]

On Fri, Feb 25, 2022 at 11:26:20AM +0530, Kshitij Suri wrote:
> 
> On 24/02/22 9:48 pm, Daniel P. Berrangé wrote:
> > On Thu, Feb 24, 2022 at 11:59:08AM +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=Hu1QTP-aSF7FeXYQcdODcxg2wW1sBEpxaD4jWHYF5kxD2Z6ihhXkxRFOkovubo-f&s=YwpDKYWnYlYBM7aE1jNrISGXxP9nKm5f9Kfotxm5rZ4&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.
> > > 
> > >   hmp-commands.hx    | 11 +++---
> > >   meson.build        | 13 +++++--
> > >   meson_options.txt  |  2 +
> > >   monitor/hmp-cmds.c |  8 +++-
> > >   qapi/ui.json       | 24 ++++++++++--
> > >   ui/console.c       | 97 ++++++++++++++++++++++++++++++++++++++++++++--
> > >   6 files changed, 139 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > > index 70a9136ac2..e43c9954b5 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*.
> > >   ERST
> > >       {
> > > diff --git a/meson.build b/meson.build
> > > index 8df40bfac4..fd550c01ec 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -1112,13 +1112,16 @@ if gtkx11.found()
> > >     x11 = dependency('x11', method: 'pkg-config', required: 
> > > gtkx11.found(),
> > >                      kwargs: static_kwargs)
> > >   endif
> > > -vnc = not_found
> > >   png = not_found
> > > +png = dependency('libpng', required: get_option('png'),
> > > +                   method: 'pkg-config', kwargs: static_kwargs)
> > > +vnc = not_found
> > > +vnc_png = not_found
> > >   jpeg = not_found
> > >   sasl = not_found
> > >   if get_option('vnc').allowed() and have_system
> > >     vnc = declare_dependency() # dummy dependency
> > > -  png = dependency('libpng', required: get_option('vnc_png'),
> > > +  vnc_png = dependency('libpng', required: get_option('vnc_png'),
> > >                      method: 'pkg-config', kwargs: static_kwargs)
> > >     jpeg = dependency('libjpeg', required: get_option('vnc_jpeg'),
> > >                       method: 'pkg-config', kwargs: static_kwargs)
> > > @@ -1537,9 +1540,10 @@ config_host_data.set('CONFIG_TPM', have_tpm)
> > >   config_host_data.set('CONFIG_USB_LIBUSB', libusb.found())
> > >   config_host_data.set('CONFIG_VDE', vde.found())
> > >   config_host_data.set('CONFIG_VHOST_USER_BLK_SERVER', 
> > > have_vhost_user_blk_server)
> > > +config_host_data.set('CONFIG_PNG', png.found())
> > >   config_host_data.set('CONFIG_VNC', vnc.found())
> > >   config_host_data.set('CONFIG_VNC_JPEG', jpeg.found())
> > > -config_host_data.set('CONFIG_VNC_PNG', png.found())
> > > +config_host_data.set('CONFIG_VNC_PNG', vnc_png.found())
> > 
> > I think we should be removing  CONFIG_VNC_PNG entirely - the VNC
> > code should just use  CONFIG_PNG.
> > 
> > I'd suggest splitting ths into two patches.  The first patch
> > updates meson.build to look for libpng unconditionally and
> > rename to CONFIG_PNG.  The second patch introduces the PNG
> > support for screendump.
> Yes can remove entirely with a combination of CONFIG_VNC and CONFIG_PNG as
> follows
> 
> #ifdef CONFIG_VNC_PNG -> #if defined(CONFIG_VNC) && defined(CONFIG_PNG)
> 
> But won't removing the vnc_png option cause problems in the build scripts of
> users with the current
> version? Instead, can we use the new png meson-option to denote PNG format
> for VNC as well while maintaining backward compatibility? The change would
> look like

Configure arguments / meson options are not a stable interface
from QEMU. We can change them at any time.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|