[Spice-devel] [PATCH spice-streaming-agent] gst-plugin: receive encoder properties as command parameters

[Frediano Ziglio]

> Hi,
> 
> 
> On 7/16/19 2:35 PM, Frediano Ziglio wrote:
> >> This allows to set plugin key=value properties on run time.
> >> To add encoder plugin property use the following syntax:
> >> -gst.prop="key=value"
> >> Make sure syntax is accurate and that the property is supported by
> >> the chosen plugin, wrong/invalid properties will be ignored silently.
> >> Specific encoder available properties can be viewed by:
> >> gst-inspect-1.0 <PLUGIN-NAME>
> >> ---
> >> * This patch useful for encoders tuning and testing (later we can
> >> introduce
> >>    fixed encoders setups), hence not checking for validity of input.
> >> * I dropped sstream in previous patch but i found it useful here and added
> >> it
> >>    again, alternative suggestions are welcome
> >>
> > Nothing wrong against its usage, but see below
> >
> >> ---
> >>   src/gst-plugin.cpp | 21 +++++++++++++++++----
> >>   1 file changed, 17 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
> >> index 4e802f1..0d8773d 100644
> >> --- a/src/gst-plugin.cpp
> >> +++ b/src/gst-plugin.cpp
> >> @@ -6,6 +6,7 @@
> >>   
> >>   #include <config.h>
> >>   #include <cstring>
> >> +#include <sstream>
> >>   #include <exception>
> >>   #include <stdexcept>
> >>   #include <memory>
> >> @@ -35,6 +36,7 @@ struct GstreamerEncoderSettings
> >>       int fps = 25;
> >>       SpiceVideoCodecType codec = SPICE_VIDEO_CODEC_TYPE_H264;
> >>       std::string encoder;
> >> +    std::vector<std::string> prop_strings;
> > Why not a std::map?
> > It looks a bit unnatural, it's not a vector, even elements are property
> > names,
> > odd elements are values.
> 
> 
> These are {key,value} pairs, which are also passed as pairs to gstreamer,
> accessing them is by iterating over all pairs.
> Since extraction of values through their key is not required i'd suggest
> to use
> vector of pairs instead.
> 

It's not hard to code:

        map<string, string> m;
        m["foo"] = "foo value";
        for (const auto i : m) {
                cout << i.first << i.second << endl;
        }

> 
> > std::map will avoid duplication of properties with same name (which look
> > good)
> > but you would lose the order (which could be an issue... or not?)
> 
> 
> Yes, losing the order is not an issue.
> 
> 
> 
> I'll send a new version, fixing the other stuff as well.
> 
> Thanks!
> 
> 
> >
> >>   };
> >>   
> >>   template <typename T>
> >> @@ -180,10 +182,15 @@ GstElement
> >> *GstreamerFrameCapture::get_encoder_plugin(const GstreamerEncoderSett
> >>   
> >>       encoder = factory ? gst_element_factory_create(factory, "encoder") :
> >>       nullptr;
> >>       if (encoder) { // Invalid properties will be ignored silently
> >> -        /* x264enc properties */
> >> -        gst_util_set_object_arg(G_OBJECT(encoder), "tune",
> >> "zerolatency");//
> >> stillimage, fastdecode, zerolatency
> >> -        gst_util_set_object_arg(G_OBJECT(encoder), "bframes", "0");
> >> -        gst_util_set_object_arg(G_OBJECT(encoder), "speed-preset",
> >> "1");//
> >> 1-ultrafast, 6-med, 9-veryslow
> >> +        const char *key;
> >> +        const char *val;
> >> +        for (int i = 1; i < settings.prop_strings.size(); i += 2) {
> >> +            key = settings.prop_strings[i-1].c_str();
> >> +            val = settings.prop_strings[i].c_str();
> > I would personally declare and initialize, more C++ style
> >
> >> +            gst_util_set_object_arg(G_OBJECT(encoder), key, val);
> >> +            gst_syslog(LOG_NOTICE, "Trying to set encoder property: '%s =
> >> %s'", key, val);
> > "Trying" looks like you will doing, but after IMHO should be "Tried to".
> >
> >> +        }
> >> +
> >>       }
> >>       gst_plugin_feature_list_free(filtered);
> >>       gst_plugin_feature_list_free(encoders);
> >> @@ -449,6 +456,12 @@ void GstreamerPlugin::ParseOptions(const
> >> ConfigureOption
> >> *options)
> >>               }
> >>           } else if (name == "gst.encoder") {
> >>               settings.encoder = value;
> >> +        } else if (name == "gst.prop") {
> >> +            std::stringstream ss(value);
> >> +            std::string item;
> >> +            while (std::getline(ss, item, '=')) {
> > This has some problems:
> > - if value contains multiple "=" this will be split and part of values will
> >    become property names;
> > - if value doesn't contain any "=" the entire value will be considered as
> >    the property name of following arguments.
> > In Python that would probably be a string.split('=', 1).
> > Maybe a combination of string.find and string.substr would avoid these
> > issues.
> > I could also potentially pass something like (shell syntax)
> >
> >
> > $ program ... -c "gst.prop=name=value
> > next line=2"
> >
> >
> > (property value with "=" and new line embedded).
> >
> >> +               settings.prop_strings.push_back(item);
> >> +            }
> >>           }
> >>       }
> >>   }
> > Frediano
>