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

[Snir Sheriber]

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.


> 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