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

[Frediano Ziglio]

> 
> From: test <test at localhost.localdomain>
> 
> This allows to set plugin key=value properties on run time.
> To add encoder plugin property use the following syntax:
> -c gst.prop="property=value" -c gst.prop="property2=value2"...
> Make sure syntax is accurate and that the property is supported by
> the chosen plugin, wrong properties may ignored silently.
> 
> Signed-off-by: Snir Sheriber <ssheribe at redhat.com>
> ---
> 
> Difference from v1:
> -Variables naming
> -Using vector of pairs
> -Add warnings for wrong input
> -Set propeties after log msg
> -Fix commit msg
> 
> *There is only basic input validation, assuming the user is
>  accurate with the properties he sets.
>  Currently the code checks property name exists for the encoder
>  object, i had a version that checks also the value matches the
>  property type but it seems to be over-coding for this purpose.
>  
> ---
>  src/gst-plugin.cpp | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
> index 4e802f1..3270cce 100644
> --- a/src/gst-plugin.cpp
> +++ b/src/gst-plugin.cpp
> @@ -35,6 +35,7 @@ struct GstreamerEncoderSettings
>      int fps = 25;
>      SpiceVideoCodecType codec = SPICE_VIDEO_CODEC_TYPE_H264;
>      std::string encoder;
> +    std::vector<std::pair<std::string, std::string>> prop_pairs;
>  };
>  
>  template <typename T>
> @@ -179,11 +180,18 @@ 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
> +    if (encoder) { // Set encoder properties
> +        for (int i = 0; i < settings.prop_pairs.size(); i++) {

I would use a simple

   for (const auto &prop: settings.prop_pairs) {

> +            const char *prop_name = settings.prop_pairs[i].first.c_str();

I would personally avoid the temp and use "prop.first.c_str()" below
or use an alias like

               const auto &name = prop.first;
               const auto &value = prop.second;

> +            if (!g_object_class_find_property(G_OBJECT_GET_CLASS(encoder),
> prop_name)) {
> +                gst_syslog(LOG_WARNING, "'%s' property was not found for
> this encoder", prop_name);
> +                continue;
> +            }
> +            const char *prop_val = settings.prop_pairs[i].second.c_str();
> +            gst_syslog(LOG_NOTICE, "Trying to set encoder property: '%s =
> %s'", prop_name, prop_val);
> +            /* Invalid properties will be ignored silently */
> +            gst_util_set_object_arg(G_OBJECT(encoder), prop_name, prop_val);
> +        }
>      }
>      gst_plugin_feature_list_free(filtered);
>      gst_plugin_feature_list_free(encoders);
> @@ -449,6 +457,16 @@ void GstreamerPlugin::ParseOptions(const ConfigureOption
> *options)
>              }
>          } else if (name == "gst.encoder") {
>              settings.encoder = value;
> +        } else if (name == "gst.prop") {
> +            std::string prop_name, prop_val;
> +            size_t pos = value.find('=');
> +            if (!pos || pos >= value.size() - 1) {

I would use a

   if (pos == std::string::npos || pos == 0) {

> +                gst_syslog(LOG_WARNING, "Property input is invalid ('%s'
> Ignored)", value.c_str());
> +                continue;
> +            }
> +            prop_name = value.substr(0, pos);
> +            prop_val = value.substr(pos + 1);

Why not use auto and avoid separated declaration? Potentially this will avoid
constructing empty string and then changing them.

> +            settings.prop_pairs.push_back(make_pair(prop_name, prop_val));
>          }
>      }
>  }

Frediano