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

[Frediano Ziglio]

> 
> 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.
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?)

>  };
>  
>  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