Hi Daniel,
> Subject: Re: [PATCH v5 2/7] ui/spice: Add an option for users to provide a
> preferred codec
>
> 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é <berra...@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 <kra...@redhat.com>
> > > > > > Cc: Marc-André Lureau <marcandre.lur...@redhat.com>
> > > > > > Cc: Dmitry Osipenko <dmitry.osipe...@collabora.com>
> > > > > > Cc: Frediano Ziglio <fredd...@gmail.com>
> > > > > > Cc: Dongwon Kim <dongwon....@intel.com>
> > > > > > Cc: Michael Scherle <michael.sche...@rz.uni-freiburg.de>
> > > > > > Signed-off-by: Vivek Kasireddy <vivek.kasire...@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'
This patch series is intended to be used only with the "gstreamer" encoder
type. So, I guess Qemu can just accept video-codecs=<codec> where codec
could be any of mjpeg, vp8, vp9, h264 and we could then invoke the Spice API
spice_server_set_video_codecs() with the appropriately formatted codecs
string (i.e, gstreamer:<codec>).
Thanks,
Vivek
>
> 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 :|
