[Spice-devel] [spice v13 10/29] server: Handle and recover from GStreamer encoding errors

[Christophe Fergeau]

On Tue, Apr 19, 2016 at 09:49:39AM +0200, Francois Gouget wrote:
> This avoids getting stuck if the codec is buggy or fails to encode a
> frame for whatever reason (e.g. odd frame size).
> 
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
>  server/gstreamer-encoder.c | 105 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 90 insertions(+), 15 deletions(-)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index 04a74cb..f9b3579 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -76,6 +76,11 @@ typedef struct SpiceGstEncoder {
>  #   define SPICE_GST_VIDEO_PIPELINE_CAPS     0x4
>      uint32_t set_pipeline;
>  
> +    /* Output buffer */
> +    GMutex outbuf_mutex;
> +    GCond outbuf_cond;
> +    VideoBuffer *outbuf;
> +
>      /* The bit rate target for the outgoing network stream. (bits per second) */
>      uint64_t bit_rate;
>  
> @@ -214,6 +219,56 @@ static void set_appsrc_caps(SpiceGstEncoder *encoder)
>      gst_app_src_set_caps(encoder->appsrc, encoder->src_caps);
>  }
>  
> +static GstBusSyncReply handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer video_encoder)
> +{
> +    SpiceGstEncoder *encoder = video_encoder;
> +
> +    if (GST_MESSAGE_TYPE(msg) == GST_MESSAGE_ERROR) {
> +        GError *err;

'err' must be set to NULL here.

> +        gchar *debug_info;
> +        gst_message_parse_error(msg, &err, &debug_info);
> +        spice_warning("GStreamer error from element %s: %s",
> +                      GST_OBJECT_NAME(msg->src), err->message);
> +        if (debug_info) {
> +            spice_debug("debug details: %s", debug_info);
> +            g_free(debug_info);
> +        }
> +        g_clear_error(&err);
> +
> +        /* Unblock the main thread */
> +        g_mutex_lock(&encoder->outbuf_mutex);
> +        encoder->outbuf = (VideoBuffer*)create_gst_video_buffer();
> +        g_cond_signal(&encoder->outbuf_cond);
> +        g_mutex_unlock(&encoder->outbuf_mutex);
> +    }
> +    return GST_BUS_PASS;
> +}
> +
> +static GstFlowReturn new_sample(GstAppSink *gstappsink, gpointer video_encoder)
> +{
> +    SpiceGstEncoder *encoder = (SpiceGstEncoder*)video_encoder;
> +    SpiceGstVideoBuffer *outbuf = create_gst_video_buffer();
> +
> +    GstSample *sample = gst_app_sink_pull_sample(encoder->appsink);
> +    if (sample) {
> +        outbuf->gst_buffer = gst_sample_get_buffer(sample);
> +        gst_buffer_ref(outbuf->gst_buffer);
> +        gst_sample_unref(sample);
> +        if (gst_buffer_map(outbuf->gst_buffer, &outbuf->map, GST_MAP_READ)) {
> +            outbuf->base.data = outbuf->map.data;
> +            outbuf->base.size = gst_buffer_get_size(outbuf->gst_buffer);
> +        }
> +    }
> +
> +    /* Notify the main thread that the output buffer is ready */
> +    g_mutex_lock(&encoder->outbuf_mutex);
> +    encoder->outbuf = (VideoBuffer*)outbuf;
> +    g_cond_signal(&encoder->outbuf_cond);
> +    g_mutex_unlock(&encoder->outbuf_mutex);
> +
> +    return GST_FLOW_OK;
> +}
> +
>  static int physical_core_count = 0;
>  static int get_physical_core_count(void)
>  {
> @@ -293,6 +348,22 @@ static gboolean create_pipeline(SpiceGstEncoder *encoder)
>      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"));
>  
> +#ifdef HAVE_GSTREAMER_0_10
> +    GstAppSinkCallbacks appsink_cbs = {NULL, NULL, &new_sample, NULL, {NULL}};
> +#else

This bit can go to the last patch

> +    GstAppSinkCallbacks appsink_cbs = {NULL, NULL, &new_sample, {NULL}};
> +#endif
> +    gst_app_sink_set_callbacks(encoder->appsink, &appsink_cbs, encoder, NULL);
> +
> +    /* Hook into the bus so we can handle errors */
> +    GstBus *bus = gst_element_get_bus(encoder->pipeline);
> +#ifdef HAVE_GSTREAMER_0_10
> +    gst_bus_set_sync_handler(bus, handle_pipeline_message, encoder);

Can go in the last patch as well.

> +#else
> +    gst_bus_set_sync_handler(bus, handle_pipeline_message, encoder, NULL);

Using _sync_handler which runs in a different thread seems quite
complicated. Have you tried using gst_bus_add_watch() (we have a
mainloop in the display thread where this code runs if I'm not
mistaken), or even gst_bus_pop_filtered() if all that you need is to
catch encoding errors?

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/20160502/bbfb3c5c/attachment.sig>