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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Snir,

On Thu, Aug 1, 2019 at 5:02 PM Snir Sheriber <ssheribe@xxxxxxxxxx> wrote:
>
> Hi,
>
>
> On 8/1/19 3:43 PM, Kevin Pouget wrote:
> > On Wed, Jul 31, 2019 at 12:09 PM Kevin Pouget <kpouget@xxxxxxxxxx> wrote:
> >> On Wed, Jul 31, 2019 at 11:50 AM Frediano Ziglio <fziglio@xxxxxxxxxx> 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.
> > Hello,
> >
> > I did the modifications below to allow passing codec-specific gst parameters:
>
>
> Basically Gstreamer properties are more tied to the encoder than
> the codec type e.g. vaapih264enc plugin has a property named "low-delay-b"
> and x264enc hasn't.
>
> Now I'm thinking maybe i should have allow to set properties only when the
> encoder is specified :P, anyway i think we do need to parse it in order
> somehow
> i.e. vp8 + its encoder + its props... , another vp8 + its props ... ,

I am confused with this: how should the 2nd vp8 encoder get into execution?
at the moment the server requests "vp8" playback, so a second gst.vp8
plugin cannot be selected

or you have something else in mind?

> h264 + its encoder...
> I did no try it yet, I'll test it and have a closer look on everything,
> i like this feature!
>
> Thanks,
> Snir.
>
>
> >
> >> gst.codec=CODEC
> >> gst.prop | gst.CODEC.prop
> >> gst.encoder | gst.CODEC.prop
> >> framerate | gst.CODEC.framerate
> > regards,
> >
> > Kevin
> >
> > ---
> > diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
> > index 5469647..4194485 100644
> > --- a/src/gst-plugin.cpp
> > +++ b/src/gst-plugin.cpp
> > @@ -102,7 +102,8 @@ class GstreamerPlugin final: public Plugin
> >   public:
> >       FrameCapture *CreateCapture() override;
> >       unsigned Rank() override;
> > -    void ParseOptions(const ConfigureOption *options,
> > SpiceVideoCodecType codec);
> > +    void ParseOptions(const ConfigureOption *options,
> > SpiceVideoCodecType codec,
> > +                      const std::string codec_name);
> >       SpiceVideoCodecType VideoCodecType() const override {
> >           return settings.codec;
> >       }
> > @@ -431,7 +432,8 @@ unsigned GstreamerPlugin::Rank()
> >       return SoftwareMin;
> >   }
> >
> > -void GstreamerPlugin::ParseOptions(const ConfigureOption *options,
> > SpiceVideoCodecType codec)
> > +void GstreamerPlugin::ParseOptions(const ConfigureOption *options,
> > +                                   SpiceVideoCodecType codec, const
> > std::string codec_name)
> >   {
> >       settings.codec = codec;
> >
> > @@ -439,15 +441,17 @@ void GstreamerPlugin::ParseOptions(const
> > ConfigureOption *options,  SpiceVideoCo
> >           const std::string name = options->name;
> >           const std::string value = options->value;
> >
> > -        if (name == "framerate") {
> > +        if (name == "framerate" || name == "gst." + codec_name +
> > ".framerate") {
> >               try {
> >                   settings.fps = std::stoi(value);
> > +
> >               } catch (const std::exception &e) {
> > -                throw std::runtime_error("Invalid value '" + value +
> > "' for option 'framerate'.");
> > +                throw std::runtime_error("Invalid value '" + value + '" "
> > +                                         "for option '" + name + "'.");
> >               }
> > -        } else if (name == "gst.encoder") {
> > +        } else if (name == "gst.encoder" || name == "gst." +
> > codec_name +".encoder") {
> >               settings.encoder = value;
> > -        } else if (name == "gst.prop") {
> > +        } else if (name == "gst.prop" || name == "gst." + codec_name
> > + ".prop") {
> >               size_t pos = value.find('=');
> >               if (pos == 0 || pos >= value.size() - 1) {
> >                   gst_syslog(LOG_WARNING, "Property input is invalid
> > ('%s' Ignored)", value.c_str());
> > @@ -488,7 +492,7 @@ SPICE_STREAMING_AGENT_PLUGIN(agent)
> >               }
> >
> >               auto plugin = std::make_shared<GstreamerPlugin>();
> > -            plugin->ParseOptions(agent->Options(), codec_type);
> > +            plugin->ParseOptions(agent->Options(), codec_type, value);
> >               agent->Register(plugin);
> >           }
> >       }
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]