[Spice-devel] [PATCH v5 15/20] server: Respect the GStreamer encoder's valid bit rate range.

[Christophe Fergeau]

On Thu, Aug 27, 2015 at 09:01:38PM +0200, Francois Gouget wrote:
> Otherwise it may get wrapped to a much lower value than intended.
> 
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
> 
> For instance x264enc has a 100Mbps limit so 102Mbps would give a 2Mbps 
> bit rate.
> 
>  server/gstreamer_encoder.c | 64 +++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 58 insertions(+), 6 deletions(-)
> 
> diff --git a/server/gstreamer_encoder.c b/server/gstreamer_encoder.c
> index de496c1..5f6d09c 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>
> @@ -814,12 +816,65 @@ static gboolean construct_pipeline(GstEncoder *encoder, const SpiceBitmap *bitma
>      encoder->gstenc = gst_bin_get_by_name(GST_BIN(encoder->pipeline), "encoder");
>      encoder->appsink = GST_APP_SINK(gst_bin_get_by_name(GST_BIN(encoder->pipeline), "sink"));
>  
> +    /* Set the bit rate. */
> +    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 %"PRIu64,
> +                        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);
> +    } else {
> +        spice_printerr("Could not find the bit rate property for %s", gstenc_name);
> +    }
> +

If you want to do something generic, this belongs in a separate helper
function imo, but wouldn't it be easier to keep the per-element bitrate
setting as the proper unit is already known there?

>      /* Configure the encoders for a zero-frame latency, and real-time speed */
>      switch (encoder->base.codec_type) {
>      case SPICE_VIDEO_CODEC_TYPE_MJPEG:
> -        g_object_set(G_OBJECT(encoder->gstenc),
> -                     "bitrate", encoder->video_bit_rate,
> -                     NULL);
> +        /* Nothing to tweak */
>          break;
>      case SPICE_VIDEO_CODEC_TYPE_VP8: {
>          /* See http://www.webmproject.org/docs/encoder-parameters/ */
> @@ -833,13 +888,11 @@ static gboolean construct_pipeline(GstEncoder *encoder, const SpiceBitmap *bitma
>                       "min-quantizer", 10, /* seems virtually lossless */
>  #ifdef HAVE_GSTREAMER_0_10
>                       "mode", 1, /* CBR */
> -                     "bitrate", encoder->video_bit_rate,
>                       "max-latency", 0, /* zero-frame latency */
>                       "error-resilient", TRUE, /* for client frame drops */
>                       "speed", 7, /* ultrafast */
>  #else
>                       "end-usage", 1, /* CBR */
> -                     "target-bitrate", encoder->video_bit_rate,
>                       "lag-in-frames", 0, /* zero-frame latency */
>                       "error-resilient", 1, /* for client frame drops */
>                       "deadline", 1000000 / get_source_fps(encoder) / 2, /* usec */
> @@ -850,7 +903,6 @@ static gboolean construct_pipeline(GstEncoder *encoder, const SpiceBitmap *bitma
>          }
>      case SPICE_VIDEO_CODEC_TYPE_H264:
>          g_object_set(G_OBJECT(encoder->gstenc),
> -                     "bitrate", encoder->video_bit_rate / 1024,
>                       "qp-min", 15, /* virtually lossless */
>                       "byte-stream", TRUE,
>                       "aud", FALSE,
> -- 
> 2.5.0
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150922/84ccc9dc/attachment.sig>