On Sun, Aug 4, 2019 at 10:13 AM Snir Sheriber <ssheribe@xxxxxxxxxx> 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 <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?
>
>
> 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@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