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

[Christophe Fergeau]

On Tue, May 03, 2016 at 04:33:47PM +0200, Francois Gouget wrote:
> On Mon, 2 May 2016, Christophe Fergeau wrote:
> > > +#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?
> 
> I tried various alternatives but they did not work. encode_frame() gets 
> called from the main loop and then calls gst_app_sink_pull_sample(). But 
> if an error occurs the buffer gets dropped and we never return from 
> gst_app_sink_pull_sample(), never get back to the main loop, and thus 
> our gst_bus_add_watch() callback will not get called. Deadlock.

Ah ok, please add this to the commit log, as I was not sure why we
needed the 'new sample' callback as well.
So yeah, before the code was calling the pull_sample() method, which
blocks and will only return once it has received a buffer. This is not
going to work if an error occurs and the buffer is never received.
What the new code is doing is that the main thread blocks on a GCond,
and watches for new buffers and for errors. If a new buffer has been
received, then we can call pull_sample() knowing it won't block for
erros. If an error occurs, we log it. Then in both cases, we signal the
condition to unlock the 'main' thread.
(this would be useful to have in the commit log too I guess).

And GStreamer GstBus/GstAppSink API tends to be async, while we need
encode_frame() to be sync, so there is indeed several useful APIs that
we cannot use without additional complexity. I'm wondering if the
simpler patch attached to this message could work too.

> 
> Also I'm not convinced we really have a glib main loop in Xspice. If we 
> did I would expect APIs like g_idle_source_new() to work. See: 
> https://lists.freedesktop.org/archives/spice-devel/2016-March/027502.html

Yes, I addressed this in
https://lists.freedesktop.org/archives/spice-devel/2016-March/027550.html
« One gotcha though is that queueing an idle this way is currently not
working, you have to workaround it with g_timeout_source_new(1).  The
reason for that is worker_source_check() which always returns TRUE.
This means worker_source_dispatch() is going to run at every mainloop
iteration, preventing idles from running as there is non-idle work to be
done. We'll have to fix that too... :) »
This is still not fixed as far as I know.

Christophe
-------------- next part --------------
From f6416279ba1460dcc79ebc495bedc0cbc5d2c313 Mon Sep 17 00:00:00 2001
From: Francois Gouget <fgouget at codeweavers.com>
Date: Tue, 12 Apr 2016 20:35:19 +0200
Subject: [spice-server] server: Handle and recover from GStreamer encoding
 errors

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 | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 04a74cb..310b185 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -214,6 +214,28 @@ 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 = NULL;
+        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 which may be stuck calling gst_app_sink_pull_sample() */
+        gst_element_set_state(encoder->pipeline, GST_STATE_NULL);
+    }
+    return GST_BUS_PASS;
+}
+
 static int physical_core_count = 0;
 static int get_physical_core_count(void)
 {
@@ -293,6 +315,15 @@ 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"));
 
+    /* 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);
+#else
+    gst_bus_set_sync_handler(bus, handle_pipeline_message, encoder, NULL);
+#endif
+    gst_object_unref(bus);
+
     if (encoder->base.codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG) {
         /* See https://bugzilla.gnome.org/show_bug.cgi?id=753257 */
         spice_debug("removing the pipeline clock");
-- 
2.7.4

-------------- 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/20160503/eba950f9/attachment-0001.sig>