[RFC,spice-streaming-agent,1/4] gst-plugin: allow the instantiation of multiple GST encoder plugins

Submitted by Kevin Pouget on Aug. 6, 2019, 3:34 p.m.

Details

Message ID 20190806153453.20616-5-kpouget@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Kevin Pouget Aug. 6, 2019, 3:34 p.m.
With this patch, spice-streaming-agent can be launched with multiple
Gstreamer video codecs enabled:

> spice-streaming-agent -c gst.codec=vp8 -c gst.codec=vp9 ...

Signed-off-by: Kevin Pouget <kpouget@redhat.com>
---
 src/gst-plugin.cpp | 50 ++++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 19 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
index 6415ac0..5469647 100644
--- a/src/gst-plugin.cpp
+++ b/src/gst-plugin.cpp
@@ -102,7 +102,7 @@  class GstreamerPlugin final: public Plugin
 public:
     FrameCapture *CreateCapture() override;
     unsigned Rank() override;
-    void ParseOptions(const ConfigureOption *options);
+    void ParseOptions(const ConfigureOption *options, SpiceVideoCodecType codec);
     SpiceVideoCodecType VideoCodecType() const override {
         return settings.codec;
     }
@@ -431,8 +431,10 @@  unsigned GstreamerPlugin::Rank()
     return SoftwareMin;
 }
 
-void GstreamerPlugin::ParseOptions(const ConfigureOption *options)
+void GstreamerPlugin::ParseOptions(const ConfigureOption *options,  SpiceVideoCodecType codec)
 {
+    settings.codec = codec;
+
     for (; options->name; ++options) {
         const std::string name = options->name;
         const std::string value = options->value;
@@ -443,20 +445,6 @@  void GstreamerPlugin::ParseOptions(const ConfigureOption *options)
             } catch (const std::exception &e) {
                 throw std::runtime_error("Invalid value '" + value + "' for option 'framerate'.");
             }
-        } else if (name == "gst.codec") {
-            if (value == "h264") {
-                settings.codec = SPICE_VIDEO_CODEC_TYPE_H264;
-            } else if (value == "vp9") {
-                settings.codec = SPICE_VIDEO_CODEC_TYPE_VP9;
-            } else if (value == "vp8") {
-                settings.codec = SPICE_VIDEO_CODEC_TYPE_VP8;
-            } else if (value == "mjpeg") {
-                settings.codec = SPICE_VIDEO_CODEC_TYPE_MJPEG;
-            } else if (value == "h265") {
-                settings.codec = SPICE_VIDEO_CODEC_TYPE_H265;
-            } else {
-                throw std::runtime_error("Invalid value '" + value + "' for option 'gst.codec'.");
-            }
         } else if (name == "gst.encoder") {
             settings.encoder = value;
         } else if (name == "gst.prop") {
@@ -478,11 +466,35 @@  SPICE_STREAMING_AGENT_PLUGIN(agent)
 {
     gst_init(nullptr, nullptr);
 
-    auto plugin = std::make_shared<GstreamerPlugin>();
+    auto options = agent->Options();
+    for (; options->name; ++options) {
+        const std::string name = options->name;
+        const std::string value = options->value;
 
-    plugin->ParseOptions(agent->Options());
+        if (name == "gst.codec") {
+            SpiceVideoCodecType codec_type;
+            if (value == "mjpeg") {
+                codec_type = SPICE_VIDEO_CODEC_TYPE_MJPEG;
+            } else if (value == "h264") {
+                codec_type = SPICE_VIDEO_CODEC_TYPE_H264;
+            } else if (value == "h265") {
+                codec_type = SPICE_VIDEO_CODEC_TYPE_H265;
+            } else if (value == "vp8") {
+                codec_type = SPICE_VIDEO_CODEC_TYPE_VP8;
+            } else if (value == "vp9") {
+                codec_type = SPICE_VIDEO_CODEC_TYPE_VP9;
+            } else {
+                throw std::runtime_error("Invalid value '" + value + "' for option 'gst.codec'.");
+            }
+
+            auto plugin = std::make_shared<GstreamerPlugin>();
+            plugin->ParseOptions(agent->Options(), codec_type);
+            agent->Register(plugin);
+        }
+    }
 
-    agent->Register(plugin);
+    // no default value at the moment
+    // (used to be h264, see GstreamerEncoderSettings::codec)
 
     return true;
 }

Comments

Hi,



Tested, seems to work well! switching is smooth, if codec fails
it falls back to mjpeg (not very loudly), no default gstreamer
codec currently. As mentioned i find this feature useful :)

another comment below


On 8/6/19 6:34 PM, Kevin Pouget wrote:
> With this patch, spice-streaming-agent can be launched with multiple
> Gstreamer video codecs enabled:
>
>> spice-streaming-agent -c gst.codec=vp8 -c gst.codec=vp9 ...
> Signed-off-by: Kevin Pouget <kpouget@redhat.com>
> ---
>   src/gst-plugin.cpp | 50 ++++++++++++++++++++++++++++------------------
>   1 file changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
> index 6415ac0..5469647 100644
> --- a/src/gst-plugin.cpp
> +++ b/src/gst-plugin.cpp
> @@ -102,7 +102,7 @@ class GstreamerPlugin final: public Plugin
>   public:
>       FrameCapture *CreateCapture() override;
>       unsigned Rank() override;
> -    void ParseOptions(const ConfigureOption *options);
> +    void ParseOptions(const ConfigureOption *options, SpiceVideoCodecType codec);
>       SpiceVideoCodecType VideoCodecType() const override {
>           return settings.codec;
>       }
> @@ -431,8 +431,10 @@ unsigned GstreamerPlugin::Rank()
>       return SoftwareMin;
>   }
>   
> -void GstreamerPlugin::ParseOptions(const ConfigureOption *options)
> +void GstreamerPlugin::ParseOptions(const ConfigureOption *options,  SpiceVideoCodecType codec)
>   {
> +    settings.codec = codec;
> +
>       for (; options->name; ++options) {
>           const std::string name = options->name;
>           const std::string value = options->value;
> @@ -443,20 +445,6 @@ void GstreamerPlugin::ParseOptions(const ConfigureOption *options)
>               } catch (const std::exception &e) {
>                   throw std::runtime_error("Invalid value '" + value + "' for option 'framerate'.");
>               }
> -        } else if (name == "gst.codec") {
> -            if (value == "h264") {
> -                settings.codec = SPICE_VIDEO_CODEC_TYPE_H264;
> -            } else if (value == "vp9") {
> -                settings.codec = SPICE_VIDEO_CODEC_TYPE_VP9;
> -            } else if (value == "vp8") {
> -                settings.codec = SPICE_VIDEO_CODEC_TYPE_VP8;
> -            } else if (value == "mjpeg") {
> -                settings.codec = SPICE_VIDEO_CODEC_TYPE_MJPEG;
> -            } else if (value == "h265") {
> -                settings.codec = SPICE_VIDEO_CODEC_TYPE_H265;
> -            } else {
> -                throw std::runtime_error("Invalid value '" + value + "' for option 'gst.codec'.");
> -            }
>           } else if (name == "gst.encoder") {
>               settings.encoder = value;
>           } else if (name == "gst.prop") {
> @@ -478,11 +466,35 @@ SPICE_STREAMING_AGENT_PLUGIN(agent)
>   {
>       gst_init(nullptr, nullptr);
>   
> -    auto plugin = std::make_shared<GstreamerPlugin>();
> +    auto options = agent->Options();
> +    for (; options->name; ++options) {
> +        const std::string name = options->name;
> +        const std::string value = options->value;
>   
> -    plugin->ParseOptions(agent->Options());
> +        if (name == "gst.codec") {
> +            SpiceVideoCodecType codec_type;
> +            if (value == "mjpeg") {
> +                codec_type = SPICE_VIDEO_CODEC_TYPE_MJPEG;
> +            } else if (value == "h264") {
> +                codec_type = SPICE_VIDEO_CODEC_TYPE_H264;
> +            } else if (value == "h265") {
> +                codec_type = SPICE_VIDEO_CODEC_TYPE_H265;
> +            } else if (value == "vp8") {
> +                codec_type = SPICE_VIDEO_CODEC_TYPE_VP8;
> +            } else if (value == "vp9") {
> +                codec_type = SPICE_VIDEO_CODEC_TYPE_VP9;
> +            } else {
> +                throw std::runtime_error("Invalid value '" + value + "' for option 'gst.codec'.");
> +            }
> +
> +            auto plugin = std::make_shared<GstreamerPlugin>();
> +            plugin->ParseOptions(agent->Options(), codec_type);
> +            agent->Register(plugin);
> +        }
> +    }
>   
> -    agent->Register(plugin);
> +    // no default value at the moment
> +    // (used to be h264, see GstreamerEncoderSettings::codec)

When moving to multiple codecs registration we may want to do
something more solid, for example checking for gstreamer's codec
availability before registering plugins or even trying to initialize the
pipeline before registration.

This will probably require more efforts, I'll try invest some time on 
that too.

Snir.


>   
>       return true;
>   }
On Sun, Aug 11, 2019 at 4:41 PM Snir Sheriber <ssheribe@redhat.com> wrote:
>
> Hi,
>
>
>
> Tested, seems to work well! switching is smooth, if codec fails
> it falls back to mjpeg (not very loudly), no default gstreamer
> codec currently. As mentioned i find this feature useful :)
>
> another comment below
>
>
> On 8/6/19 6:34 PM, Kevin Pouget wrote:
> > With this patch, spice-streaming-agent can be launched with multiple
> > Gstreamer video codecs enabled:
> >
> >> spice-streaming-agent -c gst.codec=vp8 -c gst.codec=vp9 ...
> > Signed-off-by: Kevin Pouget <kpouget@redhat.com>
> > ---
> >   src/gst-plugin.cpp | 50 ++++++++++++++++++++++++++++------------------
> >   1 file changed, 31 insertions(+), 19 deletions(-)
> >
> > diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
> > index 6415ac0..5469647 100644
> > --- a/src/gst-plugin.cpp
> > +++ b/src/gst-plugin.cpp
> > @@ -102,7 +102,7 @@ class GstreamerPlugin final: public Plugin
> >   public:
> >       FrameCapture *CreateCapture() override;
> >       unsigned Rank() override;
> > -    void ParseOptions(const ConfigureOption *options);
> > +    void ParseOptions(const ConfigureOption *options, SpiceVideoCodecType codec);
> >       SpiceVideoCodecType VideoCodecType() const override {
> >           return settings.codec;
> >       }
> > @@ -431,8 +431,10 @@ unsigned GstreamerPlugin::Rank()
> >       return SoftwareMin;
> >   }
> >
> > -void GstreamerPlugin::ParseOptions(const ConfigureOption *options)
> > +void GstreamerPlugin::ParseOptions(const ConfigureOption *options,  SpiceVideoCodecType codec)
> >   {
> > +    settings.codec = codec;
> > +
> >       for (; options->name; ++options) {
> >           const std::string name = options->name;
> >           const std::string value = options->value;
> > @@ -443,20 +445,6 @@ void GstreamerPlugin::ParseOptions(const ConfigureOption *options)
> >               } catch (const std::exception &e) {
> >                   throw std::runtime_error("Invalid value '" + value + "' for option 'framerate'.");
> >               }
> > -        } else if (name == "gst.codec") {
> > -            if (value == "h264") {
> > -                settings.codec = SPICE_VIDEO_CODEC_TYPE_H264;
> > -            } else if (value == "vp9") {
> > -                settings.codec = SPICE_VIDEO_CODEC_TYPE_VP9;
> > -            } else if (value == "vp8") {
> > -                settings.codec = SPICE_VIDEO_CODEC_TYPE_VP8;
> > -            } else if (value == "mjpeg") {
> > -                settings.codec = SPICE_VIDEO_CODEC_TYPE_MJPEG;
> > -            } else if (value == "h265") {
> > -                settings.codec = SPICE_VIDEO_CODEC_TYPE_H265;
> > -            } else {
> > -                throw std::runtime_error("Invalid value '" + value + "' for option 'gst.codec'.");
> > -            }
> >           } else if (name == "gst.encoder") {
> >               settings.encoder = value;
> >           } else if (name == "gst.prop") {
> > @@ -478,11 +466,35 @@ SPICE_STREAMING_AGENT_PLUGIN(agent)
> >   {
> >       gst_init(nullptr, nullptr);
> >
> > -    auto plugin = std::make_shared<GstreamerPlugin>();
> > +    auto options = agent->Options();
> > +    for (; options->name; ++options) {
> > +        const std::string name = options->name;
> > +        const std::string value = options->value;
> >
> > -    plugin->ParseOptions(agent->Options());
> > +        if (name == "gst.codec") {
> > +            SpiceVideoCodecType codec_type;
> > +            if (value == "mjpeg") {
> > +                codec_type = SPICE_VIDEO_CODEC_TYPE_MJPEG;
> > +            } else if (value == "h264") {
> > +                codec_type = SPICE_VIDEO_CODEC_TYPE_H264;
> > +            } else if (value == "h265") {
> > +                codec_type = SPICE_VIDEO_CODEC_TYPE_H265;
> > +            } else if (value == "vp8") {
> > +                codec_type = SPICE_VIDEO_CODEC_TYPE_VP8;
> > +            } else if (value == "vp9") {
> > +                codec_type = SPICE_VIDEO_CODEC_TYPE_VP9;
> > +            } else {
> > +                throw std::runtime_error("Invalid value '" + value + "' for option 'gst.codec'.");
> > +            }
> > +
> > +            auto plugin = std::make_shared<GstreamerPlugin>();
> > +            plugin->ParseOptions(agent->Options(), codec_type);
> > +            agent->Register(plugin);
> > +        }
> > +    }
> >
> > -    agent->Register(plugin);
> > +    // no default value at the moment
> > +    // (used to be h264, see GstreamerEncoderSettings::codec)
>
> When moving to multiple codecs registration we may want to do
> something more solid, for example checking for gstreamer's codec
> availability before registering plugins or even trying to initialize the
> pipeline before registration.
>
> This will probably require more efforts, I'll try invest some time on
> that too.

Hello,

I come back on this patch now that I have a better view on what I want
to do with these multiple plugins

basically, I need 1/ to be able to uniquely identify an instance and
2/ set plugin parameters remotely.

For 1, currently I use "gst:<codec_name>", as the encoder is not
selected at the plugin instantiation time
For 2, I send a string from the server, "param1;param2;param3" which
equivalent to the CLI "-c param1 -c param2 -c param3"

so we need to settle for a pattern here, and I'll update the
SmartStreaming module to follow this rule

I put below what I suggested in early August, let me know if you agree

and BTW, I agree with a more solid plugin registration, that would
check that the plugin is able to run (and able to tell its encoder)
during the initialization


regards,

Kevin

---------- Forwarded message ---------
From: Kevin Pouget <kpouget@redhat.com>
Date: Thu, Aug 8, 2019 at 10:12 AM
Subject: Re: [Spice-devel] [RFC spice-streaming-agent 1/2] gst-plugin:
allow the instantiation of multiple GST encoder plugins
To: Snir Sheriber <ssheribe@redhat.com>
Cc: Spice devel <spice-devel@lists.freedesktop.org>

what would you think about something like this:

# use default encoder
gst.codec=vp8
gst.vp8.prop=opt=val

# use specific h264 encoder
gst.codec=h264=vaapih264enc
gst.vaapih264enc.prop=opt=val # pass encoder-specific property
gst.h264.prop=opt=val # pass gst codec-specific property

# load a second h264 encoder
gst.codec=h264=x264enc
gst.x264enc.prop=opt=val

for the backward compatibility, maybe we can enforce that no more than
1 codec must be set if "gst.encoder" is found, as it would make no
sense otherwise

gst.codec=h264
gst.encoder=x264enc
gst.codec=vp8 # cannot use x264enc ...