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.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.
> > >
> > > 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 :|
