Re: [spice v10 08/27] server: Add VP8 support to the GStreamer video encoder

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
> ---
> 
> Changed the VP8 encoder parameters based on the realtime profile to
> improve performance. The patch does not use the realtime profile
> directly because profiles don't seem to be supported from 'gst-launch'
> pipeline strings yet.
> 
> Furthermore it turns out that it is important to get the number of
> physical cores right, and that's something the encoder does not know how
> to do itself.
> 
> 
>  configure.ac               |  4 ++
>  server/gstreamer-encoder.c | 98
>  ++++++++++++++++++++++++++++++++++++++++++----
>  server/reds.c              |  2 +-
>  3 files changed, 95 insertions(+), 9 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index d9aa073..9865240 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -130,6 +130,10 @@ AC_SUBST([SPICE_PROTOCOL_MIN_VER])
>  PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= 2.22])
>  AS_VAR_APPEND([SPICE_REQUIRES], [" glib-2.0 >= 2.22"])
>  
> +AC_CHECK_LIB(glib-2.0, g_get_num_processors,
> +             AC_DEFINE([HAVE_G_GET_NUMPROCESSORS], 1, [Defined if we have
> g_get_num_processors()]),,
> +             $GLIB2_LIBS)
> +
>  PKG_CHECK_MODULES([GOBJECT2], [gobject-2.0 >= 2.22])
>  AS_VAR_APPEND([SPICE_REQUIRES], [" gobject-2.0 >= 2.22"])
>  
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index aae47c7..3dfbdfb 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -188,14 +188,70 @@ static void set_appsrc_caps(SpiceGstEncoder *encoder)
>      g_object_set(G_OBJECT(encoder->appsrc), "caps", encoder->src_caps,
>      NULL);
>  }
>  
> +static int physical_core_count = 0;
> +static int get_physical_core_count(void)
> +{
> +    if (!physical_core_count) {
> +#ifdef HAVE_G_GET_NUMPROCESSORS
> +        physical_core_count = g_get_num_processors();
> +#endif

you could use sysconf if g_get_num_processors is not detected.

> +        if (system("egrep -l '^flags\\b.*: .*\\bht\\b' /proc/cpuinfo
> >/dev/null 2>&1") == 0) {
> +            /* Hyperthreading is enabled so divide by two to get the number
> +             * of physical cores.
> +             */
> +            physical_core_count = physical_core_count / 2;
> +        }

Or as you are using /proc/cpuinfo detect from it.

I'm not sure we should consider compatibility with other processors (like ARM).

> +        if (physical_core_count == 0)
> +            physical_core_count = 1;
> +    }
> +    return physical_core_count;
> +}
> +
>  /* A helper for spice_gst_encoder_encode_frame() */
>  static gboolean create_pipeline(SpiceGstEncoder *encoder)
>  {
> +    gchar *gstenc;
> +    switch (encoder->base.codec_type)
> +    {
> +    case SPICE_VIDEO_CODEC_TYPE_MJPEG:
> +        /* Set max-threads to ensure zero-frame latency */
> +        gstenc = g_strdup("avenc_mjpeg max-threads=1");
> +        break;
> +    case SPICE_VIDEO_CODEC_TYPE_VP8: {
> +        /* See http://www.webmproject.org/docs/encoder-parameters/
> +         * - Set end-usage to get a constant bitrate to help with streaming.
> +         * - resize-allowed allows trading resolution for low bitrates while
> +         *   min-quantizer ensures the bitrate does not get needlessly high.
> +         * - error-resilient minimises artifacts in case the client drops a
> +         *   frame.
> +         * - Set lag-in-frames, deadline and cpu-used to match
> +         *   "Profile Realtime". lag-in-frames ensures zero-frame latency,
> +         *   deadline turns on realtime behavior, and cpu-used targets a 75%
> +         *   CPU usage.
> +         * - deadline is supposed to be set in microseconds but in practice
> +         *   it behaves like a boolean.
> +         * - At least up to GStreamer 1.6.2, vp8enc cannot be trusted to
> pick
> +         *   the optimal number of threads. Also exceeding the number of
> +         *   physical core really degrades image quality.
> +         * - token-partitions parallelizes more operations.
> +         */
> +        int threads = get_physical_core_count();
> +        int parts = threads < 2 ? 0 : threads < 4 ? 1 : threads < 8 ? 2 : 3;
> +        gstenc = g_strdup_printf("vp8enc end-usage=cbr min-quantizer=10
> resize-allowed=true error-resilient=true lag-in-frames=0 deadline=1
> cpu-used=4 threads=%d token-partitions=%d", threads, parts);
> +        break;
> +        }
> +    default:
> +        /* gstreamer_encoder_new() should have rejected this codec type */
> +        spice_warning("unsupported codec type %d",
> encoder->base.codec_type);
> +        return FALSE;
> +    }
> +
>      GError *err = NULL;
> -    /* Set max-threads to ensure zero-frame latency */
> -    const gchar *desc = "appsrc is-live=true format=time do-timestamp=true
> name=src ! videoconvert ! avenc_mjpeg max-threads=1 name=encoder ! appsink
> name=sink";
> +    gchar *desc = g_strdup_printf("appsrc is-live=true format=time
> do-timestamp=true name=src ! videoconvert ! %s name=encoder ! appsink
> name=sink", gstenc);
>      spice_debug("GStreamer pipeline: %s", desc);
>      encoder->pipeline = gst_parse_launch_full(desc, NULL,
>      GST_PARSE_FLAG_FATAL_ERRORS, &err);
> +    g_free(gstenc);
> +    g_free(desc);
>      if (!encoder->pipeline || err) {
>          spice_warning("GStreamer error: %s", err->message);
>          g_clear_error(&err);
> @@ -221,11 +277,27 @@ static gboolean configure_pipeline(SpiceGstEncoder
> *encoder,
>  
>      /* Configure the encoder bitrate */
>      adjust_bit_rate(encoder);
> -    g_object_set(G_OBJECT(encoder->gstenc), "bitrate", encoder->bit_rate,
> NULL);
> -
> -    /* See https://bugzilla.gnome.org/show_bug.cgi?id=753257 */
> -    spice_debug("removing the pipeline clock");
> -    gst_pipeline_use_clock(GST_PIPELINE(encoder->pipeline), NULL);
> +    switch (encoder->base.codec_type)
> +    {
> +    case SPICE_VIDEO_CODEC_TYPE_MJPEG:
> +        g_object_set(G_OBJECT(encoder->gstenc),
> +                     "bitrate", encoder->bit_rate,
> +                     NULL);
> +        /* See https://bugzilla.gnome.org/show_bug.cgi?id=753257 */
> +        spice_debug("removing the pipeline clock");
> +        gst_pipeline_use_clock(GST_PIPELINE(encoder->pipeline), NULL);
> +        break;
> +    case SPICE_VIDEO_CODEC_TYPE_VP8:
> +        g_object_set(G_OBJECT(encoder->gstenc),
> +                     "target-bitrate", encoder->bit_rate,
> +                     NULL);
> +        break;
> +    default:
> +        /* gstreamer_encoder_new() should have rejected this codec type */
> +        spice_warning("unsupported codec type %d",
> encoder->base.codec_type);
> +        free_pipeline(encoder);
> +        return FALSE;
> +    }
>  
>      /* Set the source caps */
>      set_appsrc_caps(encoder);
> @@ -247,6 +319,15 @@ static void reconfigure_pipeline(SpiceGstEncoder
> *encoder)
>      if (!is_pipeline_configured(encoder)) {
>          return;
>      }
> +    if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_VP8) {
> +        /* vp8enc fails to account for caps changes that modify the frame
> +         * size and complains about the buffer size.
> +         * So recreate the pipeline from scratch.
> +         */
> +        free_pipeline(encoder);
> +        return;
> +    }
> +
>      if (gst_element_set_state(encoder->pipeline, GST_STATE_PAUSED) ==
>      GST_STATE_CHANGE_FAILURE) {
>          spice_debug("GStreamer error: could not pause the pipeline,
>          rebuilding it instead");
>          free_pipeline(encoder);
> @@ -501,7 +582,8 @@ VideoEncoder *gstreamer_encoder_new(SpiceVideoCodecType
> codec_type,
>                                      uint64_t starting_bit_rate,
>                                      VideoEncoderRateControlCbs *cbs)
>  {
> -    spice_return_val_if_fail(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG,
> NULL);
> +    spice_return_val_if_fail(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG ||
> +                             codec_type == SPICE_VIDEO_CODEC_TYPE_VP8,
> NULL);
>  
>      GError *err = NULL;
>      if (!gst_init_check(NULL, NULL, &err)) {
> diff --git a/server/reds.c b/server/reds.c
> index e97038b..de98509 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3427,7 +3427,7 @@ err:
>  static const char default_renderer[] = "sw";
>  
>  #define RED_MAX_VIDEO_CODECS 8
> -static const char default_video_codecs[] = "spice:mjpeg;gstreamer:mjpeg";
> +static const char default_video_codecs[] =
> "spice:mjpeg;gstreamer:mjpeg;gstreamer:vp8";
>  
>  /* new interface */
>  SPICE_GNUC_VISIBLE SpiceServer *spice_server_new(void)

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]