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

[Kevin Pouget]

On Sun, Aug 11, 2019 at 4:41 PM Snir Sheriber <ssheribe at 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 at 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 at 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 at redhat.com>
Cc: Spice devel <spice-devel at 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 ...