RE: [PATCH v5 2/7] ui/spice: Add an option for users to provide a preferred codec
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é
> > wrote:
> >
> > > On Fri, Jun 06, 2025 at 06:10:31AM +, 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
> > > > > > Cc: Marc-André Lureau
> > > > > > Cc: Dmitry Osipenko
> > > > > > Cc: Frediano Ziglio
> > > > > > Cc: Dongwon Kim
> > > > > > Cc: Michael Scherle
> > > > > > Signed-off-by: Vivek Kasireddy
> > > > > > ---
> > > > > > 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=:\n"
> > > > > > " [,gl=[on|off]][,rendernode=]\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=:``
> > > > > > +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
Re: [PATCH v5 2/7] ui/spice: Add an option for users to provide a preferred codec
On 10.06.25 14:20, Daniel P. Berrangé wrote: 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é wrote: 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 With 'spice' you mean only that the builtin will not get any more codecs right? because I want to do an MR for spice gstreamer to add more codecs, like av1. Also It might be useful to set more parameters in future, like chroma sub-sampling or which GStreamer plugin is wanted like: msdk, va, vaapi Another point is that it can actually be a list of codecs like: gstreamer:vp8;gstreamer:h264;spice:mjpeg Which is maybe not clear from the current documentation, which might rather be: [,video-codecs=:[;...]] This is useful the specify the allowed codecs and also set the priority. 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 Greetings, Michael
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é
> wrote:
>
> > On Fri, Jun 06, 2025 at 06:10:31AM +, 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
> > > > > Cc: Marc-André Lureau
> > > > > Cc: Dmitry Osipenko
> > > > > Cc: Frediano Ziglio
> > > > > Cc: Dongwon Kim
> > > > > Cc: Michael Scherle
> > > > > Signed-off-by: Vivek Kasireddy
> > > > > ---
> > > > > 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=:\n"
> > > > > " [,gl=[on|off]][,rendernode=]\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=:``
> > > > > +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 :|
Re: [PATCH v5 2/7] ui/spice: Add an option for users to provide a preferred codec
Hi
On Fri, Jun 6, 2025 at 11:16 AM Daniel P. Berrangé
wrote:
> On Fri, Jun 06, 2025 at 06:10:31AM +, 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
> > > > Cc: Marc-André Lureau
> > > > Cc: Dmitry Osipenko
> > > > Cc: Frediano Ziglio
> > > > Cc: Dongwon Kim
> > > > Cc: Michael Scherle
> > > > Signed-off-by: Vivek Kasireddy
> > > > ---
> > > > 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=:\n"
> > > > " [,gl=[on|off]][,rendernode=]\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=:``
> > > > +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
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.
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?
Re: [PATCH v5 2/7] ui/spice: Add an option for users to provide a preferred codec
On Fri, Jun 06, 2025 at 06:10:31AM +, 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
> > > Cc: Marc-André Lureau
> > > Cc: Dmitry Osipenko
> > > Cc: Frediano Ziglio
> > > Cc: Dongwon Kim
> > > Cc: Michael Scherle
> > > Signed-off-by: Vivek Kasireddy
> > > ---
> > > 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=:\n"
> > > " [,gl=[on|off]][,rendernode=]\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=:``
> > > +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.
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 :|
RE: [PATCH v5 2/7] ui/spice: Add an option for users to provide a preferred codec
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
> > Cc: Marc-André Lureau
> > Cc: Dmitry Osipenko
> > Cc: Frediano Ziglio
> > Cc: Dongwon Kim
> > Cc: Michael Scherle
> > Signed-off-by: Vivek Kasireddy
> > ---
> > 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=:\n"
> > " [,gl=[on|off]][,rendernode=]\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=:``
> > +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.
Thanks,
Vivek
>
>video-encoder=
>video-codec=
>
>
> 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
> :|
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
> Cc: Marc-André Lureau
> Cc: Dmitry Osipenko
> Cc: Frediano Ziglio
> Cc: Dongwon Kim
> Cc: Michael Scherle
> Signed-off-by: Vivek Kasireddy
> ---
> 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=:\n"
> " [,gl=[on|off]][,rendernode=]\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=:``
> +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
video-encoder=
video-codec=
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 :|
