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

Submitted by Francois Gouget on Aug. 27, 2015, 7:01 p.m.

Details

Message ID alpine.DEB.2.20.1508272039420.22172@amboise
State New
Headers show

Not browsing as part of any series.

Commit Message

Francois Gouget Aug. 27, 2015, 7:01 p.m.
Otherwise it may get wrapped to a much lower value than intended.

Signed-off-by: Francois Gouget <fgouget@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(-)

Patch hide | download patch | download mbox

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);
+    }
+
     /* 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,

Comments

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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel