Re: [PATCH v5 2/7] ui/spice: Add an option for users to provide a preferred codec

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Jun 10, 2025 at 03:30:24PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Jun 6, 2025 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Fri, Jun 06, 2025 at 06:10:31AM +0000, Kasireddy, Vivek wrote:
> > > Hi Daniel,
> > >
> > > > Subject: Re: [PATCH v5 2/7] ui/spice: Add an option for users to
> > provide a
> > > > preferred codec
> > > >
> > > > On Wed, May 28, 2025 at 10:11:13PM -0700, Vivek Kasireddy wrote:
> > > > > Giving users an option to choose a particular codec will enable
> > > > > them to make an appropriate decision based on their hardware and
> > > > > use-case.
> > > > >
> > > > > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > > > > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > > > > Cc: Frediano Ziglio <freddy77@gmail.com>
> > > > > Cc: Dongwon Kim <dongwon.kim@intel.com>
> > > > > Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
> > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > > > ---
> > > > >  qemu-options.hx |  5 +++++
> > > > >  ui/spice-core.c | 12 ++++++++++++
> > > > >  2 files changed, 17 insertions(+)
> > > > >
> > > > > diff --git a/qemu-options.hx b/qemu-options.hx
> > > > > index 7eb8e02b4b..fcddb583c9 100644
> > > > > --- a/qemu-options.hx
> > > > > +++ b/qemu-options.hx
> > > > > @@ -2281,6 +2281,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
> > > > >      "
> >  [,streaming-video=[off|all|filter]][,disable-copy-paste=on|off]\n"
> > > > >      "
> >  [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
> > > > >      "       [,playback-compression=[on|off]][,seamless-
> > > > migration=[on|off]]\n"
> > > > > +    "       [,video-codecs=<encoder>:<codec>\n"
> > > > >      "       [,gl=[on|off]][,rendernode=<file>]\n"
> > > > >      "                enable spice\n"
> > > > >      "                at least one of {port, tls-port} is
> > mandatory\n",
> > > > > @@ -2369,6 +2370,10 @@ SRST
> > > > >      ``seamless-migration=[on|off]``
> > > > >          Enable/disable spice seamless migration. Default is off.
> > > > >
> > > > > +    ``video-codecs=<encoder>:<codec>``
> > > > > +        Provide the preferred codec the Spice server should use.
> > > > > +        Default would be spice:mjpeg.
> > > >
> > > > This looks like two distinct settings overloaded into one command
> > > > line parameter, which is a design anti-pattern.
> > > >
> > > > Why can't this be done as separate parameters
> > > The Spice server API used by Qemu (spice_server_set_video_codecs)
> > > to set the preferred codec requires the video-codecs string to be in
> > > encoder:codec format. AFAIK, there is no other option or API to set
> > > the encoder and codec values separately.
> >
> > QEMU can accept the separate parameters and format them into the string
> > format that the spice API requires so our public API is not impacted
> > by this spice design choice.
> >
> >
> Apparently you cannot mix and match freely, it has a rather fixed set of
> actually working values.
> 
> See here and related code:
> https://gitlab.freedesktop.org/spice/spice/-/blob/master/server/reds.cpp?ref_type=heads#L3468

That's just showing the built-in defaults - the parsing code is
not enforcing any constraints. The impl though cleary only allows
'mjpeg' with 'spice':

  https://gitlab.freedesktop.org/spice/spice/-/blob/master/server/mjpeg-encoder.c#L1371

> Tbh, I don't think the encoder matters much, and I don't know why it was
> decided to associate it with video codec names.

AFAICT the only way in which the encoder matters is to distinguish the
built-in "mjpeg" impl from the gstreamer "mjpeg" coder.

> Maybe the spice API should provide a simpler form: accept only codec names.
> 
> In the meantime, qemu should perhaps add the "working" encoder prefixes
> (spice: for mjpeg, gstreamer: for others)  itself and not expose any extra
> option to the user?

Ths question is whether we need to be able to request the gstreamer
'mjpeg' impl ?

If we do, and we also assume that 'spice' will never gain any more codec
impls as built-ins, we could do

  builtin, mjpeg, vp8, vp9, h264

where 'builtin' is the standard mjpeg encoder ?

Alternatively we could just go with

  mjpeg, vp8, vp9, h264

and in the unlikely event we need to be able to skip the built-in mjpeg,
we could add  a boolean 'prefer-gstreamer=on|off'

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