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

Submitted by Kevin Pouget on July 31, 2019, 8:36 a.m.

Details

Message ID 20190731083636.21493-3-kpouget@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Kevin Pouget July 31, 2019, 8:36 a.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 ...
---
 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 9858beb..6252e42 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;
     }
@@ -419,8 +419,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;
@@ -431,20 +433,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;
         }
@@ -459,11 +447,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

> 
> 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 ...
> ---
>  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 9858beb..6252e42 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;
>      }
> @@ -419,8 +419,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;
> @@ -431,20 +433,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;
>          }
> @@ -459,11 +447,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;
>  }

I didn't test it but it makes perfectly sense.
On the other hand I'm wondering what will happen to the other
parameters. They will be "broadcasted" to all plugins.
For "framerate" is fine but what will happen if you specify
also properties (gst.prop) and encoder (gst.encoder) ?

Frediano
On Wed, Jul 31, 2019 at 11:50 AM Frediano Ziglio <fziglio@redhat.com> 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 ...
> > ---
> >  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 9858beb..6252e42 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;
> >      }
> > @@ -419,8 +419,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;
> > @@ -431,20 +433,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;
> >          }
> > @@ -459,11 +447,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;
> >  }
>
> I didn't test it but it makes perfectly sense.
> On the other hand I'm wondering what will happen to the other
> parameters. They will be "broadcasted" to all plugins.
> For "framerate" is fine but what will happen if you specify
> also properties (gst.prop) and encoder (gst.encoder) ?

maybe the easiest is to say that "gst.XXX=YYY" applies to all the
codecs, and "gst.CODEC.XXX=YYY" is only for one codec, with
XXX=prop|encoder at the moment.
On Wed, Jul 31, 2019 at 11:59 AM Christophe de Dinechin
<dinechin@redhat.com> wrote:
>
> I don't really like the way option parsing is implemented, because I
> think it leads to somewhat counter-intuitive user behaviors.
>
> For example, if I say:
>
>     gst.codec=mjpeg gst.codec=h264 framerate=20
>
> I get two plugins with framerate=20, but if I have
>
>     gst.codec=mjpeg framerate=20 gst.codec=h264
>
> then the mjpeg plugin has framerate 20 and the h264 plugin has default
> framerate (25). If I specify
>
>     framerate=20 gst.codec=mjpeg gst.codec=h264
>
> then I get two plugins with default frame rate.
>
> Note that all these behaviors are especially problematic because they
> would be different from what you get with another plugin (where
> ParseOption will process all options).

I double checked, but it doesn't run like that, the order is not relevant,

>     gst.codec=mjpeg gst.codec=h264 framerate=20
>     gst.codec=mjpeg framerate=20 gst.codec=h264

these two lines will behave the same: all the instances receive the
full set of options

>> plugin->ParseOptions(agent->Options(), codec_type);

> Kevin Pouget writes:
>
> > 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 ...
> > ---
> >  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 9858beb..6252e42 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;
> >      }
> > @@ -419,8 +419,10 @@ unsigned GstreamerPlugin::Rank()
> >      return SoftwareMin;
> >  }
> >
> > -void GstreamerPlugin::ParseOptions(const ConfigureOption *options)
> > +void GstreamerPlugin::ParseOptions(const ConfigureOption *options,  SpiceVideoCodecType codec)
>
> Why pass a codec that is specified through the options?
>
> >  {
> > +    settings.codec = codec;
> > +
> >      for (; options->name; ++options) {
> >          const std::string name = options->name;
> >          const std::string value = options->value;
> > @@ -431,20 +433,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'.");
> > -            }
>
> Could you find a way to keep that option parsing within ParseOptions?
> Maybe pass agent instead, and register a new plugin when you see a new
> gst.codec option? You could probably also add a copy constructor, with
> something like:
>
>    auto plugin = std::make_shared<GstreamerPlugin>(*this);
>
> where you could pass the already-parsed settings from one plugin to the next.

it seems hard to be to have only one loop inside ParseOptions, and not
fall into the problem you described above
here I have a 'master' loop for instantiating the plugin objects, and
a 'config' loop for parsing the settings, I don't really see any
benefit in merging them,
and it would quite complicate the code

regards,

Kevin

> >          } else if (name == "gst.encoder") {
> >              settings.encoder = value;
> >          }
> > @@ -459,11 +447,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);
> > +        }
> > +    }
>
> Because of this second loop, you presently loop multiple times over the
> same options.
> >
> > -    agent->Register(plugin);
> > +    // no default value at the moment
> > +    // (used to be h264, see GstreamerEncoderSettings::codec)
> >
> >      return true;
> >  }
>
>
> --
> Cheers,
> Christophe de Dinechin (IRC c3d)
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Kevin Pouget writes:

> On Wed, Jul 31, 2019 at 11:59 AM Christophe de Dinechin
> <dinechin@redhat.com> wrote:
>>
>> I don't really like the way option parsing is implemented, because I
>> think it leads to somewhat counter-intuitive user behaviors.
>>
>> For example, if I say:
>>
>>     gst.codec=mjpeg gst.codec=h264 framerate=20
>>
>> I get two plugins with framerate=20, but if I have
>>
>>     gst.codec=mjpeg framerate=20 gst.codec=h264
>>
>> then the mjpeg plugin has framerate 20 and the h264 plugin has default
>> framerate (25). If I specify
>>
>>     framerate=20 gst.codec=mjpeg gst.codec=h264
>>
>> then I get two plugins with default frame rate.
>>
>> Note that all these behaviors are especially problematic because they
>> would be different from what you get with another plugin (where
>> ParseOption will process all options).
>
> I double checked, but it doesn't run like that, the order is not
> relevant,

Indeed, I mis-read the call to ParseOptions, I thought you were passing
options and not agent->Options().

>
>> > +
>> > +            auto plugin = std::make_shared<GstreamerPlugin>();
>> > +            plugin->ParseOptions(agent->Options(), codec_type);
>> > +            agent->Register(plugin);
>> > +        }
>> > +    }
>>
>> Because of this second loop, you presently loop multiple times over the
>> same options.

Well, I guess if we accept that for different plugins, it's OK to accept
it for multiple instances of the same plugin.

>> >
>> > -    agent->Register(plugin);
>> > +    // no default value at the moment
>> > +    // (used to be h264, see GstreamerEncoderSettings::codec)
>> >
>> >      return true;
>> >  }
>>
>>
>> --
>> Cheers,
>> Christophe de Dinechin (IRC c3d)
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel


--
Cheers,
Christophe de Dinechin (IRC c3d)