On Sun, Aug 4, 2019 at 10:13 AM Snir Sheriber <ssher...@redhat.com> wrote: > > HI, > > On 8/1/19 6:27 PM, Kevin Pouget wrote: > > Hello Snir, > > > > On Thu, Aug 1, 2019 at 5:02 PM Snir Sheriber <ssher...@redhat.com> wrote: > >> Hi, > >> > >> > >> On 8/1/19 3:43 PM, Kevin Pouget wrote: > >>> On Wed, Jul 31, 2019 at 12:09 PM Kevin Pouget <kpou...@redhat.com> wrote: > >>>> On Wed, Jul 31, 2019 at 11:50 AM Frediano Ziglio <fzig...@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. > >>> 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? > > > For the switching codec use case it is indeed doesn't matter > It's just make more sense to me to associate the properties in > such manner since the available properties are usually associated > with the encoder-plugin and not with the codec type itself > > Another point is that may also be used for future purposes. > For example to register h264 encoder in two different > configurations and switch between them, or to register hw > encoder and sw encoder as a fallback with same codec...
Hello Snir, 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 ... > > > >> 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@lists.freedesktop.org > >>> https://lists.freedesktop.org/mailman/listinfo/spice-devel > >> _______________________________________________ > >> Spice-devel mailing list > >> Spice-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel