[Spice-devel] [spice v10 16/27] server: Respect the GStreamer encoder's valid bit rate range

[Christophe Fergeau]

On Tue, Mar 01, 2016 at 04:55:01PM +0100, Francois Gouget wrote:
> Otherwise it may get wrapped to a much lower value than intended.

I don't understand this comment. If we go lower or higher than the range
of the bitrate property, I'd expect that it's going to be clamped to
either the minimum or the maximum, but a runtime warning will be issued.
This code is doing the same, but without the warning.
Are you seeing a different behaviour than this which gives a much lower
value than the minimum for example?

> 
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
>  server/gstreamer-encoder.c | 84 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 63 insertions(+), 21 deletions(-)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index 35afe65..9af78f9 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -20,6 +20,8 @@
>  #include <config.h>
>  #endif
>  
> +#include <inttypes.h>
> +
>  #include <gst/gst.h>
>  #include <gst/app/gstappsrc.h>
>  #include <gst/app/gstappsink.h>
> @@ -839,6 +841,65 @@ static gboolean create_pipeline(SpiceGstEncoder *encoder)
>      return TRUE;
>  }
>  
> +/* A helper for configure_pipeline() */
> +static void set_gstenc_bitrate(SpiceGstEncoder *encoder)
> +{
> +    GObjectClass *class = G_OBJECT_GET_CLASS(encoder->gstenc);
> +    GParamSpec *param = g_object_class_find_property(class, "bitrate");
> +    if (param == NULL) {
> +        param = g_object_class_find_property(class, "target-bitrate");
> +    }
> +    if (param) {
> +        uint64_t gst_bit_rate = encoder->video_bit_rate;
> +        if (strstr(g_param_spec_get_blurb(param), "kbit")) {
> +            gst_bit_rate = gst_bit_rate / 1024;
> +        }
> +        switch (param->value_type) {
> +        case G_TYPE_ULONG: {
> +            GParamSpecULong *range = G_PARAM_SPEC_ULONG(param);
> +            gst_bit_rate = MAX(range->minimum, MIN(range->maximum, gst_bit_rate));
> +            break;
> +        }
> +        case G_TYPE_LONG: {
> +            GParamSpecLong *range = G_PARAM_SPEC_LONG(param);
> +            gst_bit_rate = MAX(range->minimum, MIN(range->maximum, gst_bit_rate));
> +            break;
> +        }
> +        case G_TYPE_UINT: {
> +            GParamSpecUInt *range = G_PARAM_SPEC_UINT(param);
> +            gst_bit_rate = MAX(range->minimum, MIN(range->maximum, gst_bit_rate));
> +            break;
> +        }
> +        case G_TYPE_INT: {
> +            GParamSpecInt *range = G_PARAM_SPEC_INT(param);
> +            gst_bit_rate = MAX(range->minimum, MIN(range->maximum, gst_bit_rate));
> +            break;
> +        }
> +        case G_TYPE_UINT64: {
> +            GParamSpecUInt64 *range = G_PARAM_SPEC_UINT64(param);
> +            gst_bit_rate = MAX(range->minimum, MIN(range->maximum, gst_bit_rate));
> +            break;
> +        }
> +        case G_TYPE_INT64: {
> +            GParamSpecInt64 *range = G_PARAM_SPEC_INT64(param);
> +            gst_bit_rate = MAX(range->minimum, MIN(range->maximum, gst_bit_rate));
> +            break;
> +        }
> +        default:
> +            spice_debug("the %s property has an unsupported type %zu",
> +                        g_param_spec_get_name(param), param->value_type);
> +        }
> +        spice_debug("setting the GStreamer %s to %"PRIu64,
> +                    g_param_spec_get_name(param), gst_bit_rate);
> +        g_object_set(G_OBJECT(encoder->gstenc),
> +                     g_param_spec_get_name(param), gst_bit_rate,
> +                     NULL);

Hmm too bad we have to go through such complications :(

I'm not sure this is 100% correct though, you are passing a 64 bit gst_bit_rate
to g_object_set, I think a 32 bit integer is usually expected for
G_TYPE_INT/G_TYPE_UINT. This will probably be fine on little-endian
arches, but I'm not sure it's going to be ok on big-endian.
One way around it could be to use g_object_set_property() rather than
g_object_set(). Or it seems all these bitrate properties are int anyway,
no int64 ones, so just handle this case and make gst_bit_rate an uint
here (this issue, if it's one was preexisting in the earlier patches
though).

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160304/58ad4d81/attachment.sig>