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