On Wed, Jul 31, 2019 at 11:59 AM Christophe de Dinechin
<dinechin@xxxxxxxxxx> wrote:
>
> 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).
I double checked, but it doesn't run like that, the order is not relevant,
> gst.codec=mjpeg gst.codec=h264 framerate=20
> gst.codec=mjpeg framerate=20 gst.codec=h264
these two lines will behave the same: all the instances receive the
full set of options
>> plugin->ParseOptions(agent->Options(), codec_type);
> 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.
it seems hard to be to have only one loop inside ParseOptions, and not
fall into the problem you described above
here I have a 'master' loop for instantiating the plugin objects, and
a 'config' loop for parsing the settings, I don't really see any
benefit in merging them,
and it would quite complicate the code
regards,
Kevin
> > } 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)
> _______________________________________________
> 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