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

[Kevin Pouget]

On Wed, Jul 31, 2019 at 11:59 AM Christophe de Dinechin
<dinechin at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel