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

[Christophe de Dinechin]

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).


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.

>          } 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)