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
From f6416279ba1460dcc79ebc495bedc0cbc5d2c313 Mon Sep 17 00:00:00 2001
From: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx>
---
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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel