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

[Francois Gouget]

On Mon, 2 May 2016, Christophe Fergeau wrote:
[...]
> > +    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);

Done. Here is the explanation for reference: gst_message_parse_error() 
cannot return an error (it returns void) but internally it calls 
gst_structure_id_get() which returns a boolean to signal 
success/failure. That information is lost and it would make sense for 
gst_structure_id_get() to not initialize the output parameters when it 
fails. Thus we must initialize the parameters ourselves so we can check 
if they got set. Yuck! That's not a very clean API.


> > +#ifdef HAVE_GSTREAMER_0_10
> > +    GstAppSinkCallbacks appsink_cbs = {NULL, NULL, &new_sample, NULL, {NULL}};
> > +#else
> 
> This bit can go to the last patch
[...]

I moved those two.


> > +#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.

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


-- 
Francois Gouget <fgouget at codeweavers.com>