On Thu, 3 Mar 2016, Christophe Fergeau wrote: [...] > Making the refcounting thread safe is one thing, but then the code > freeing the drawable needs to be thread-safe too. red_put_drawable might > be fine (only looked briefly), but release_resource which is a vfunc > provided by QEMU seems much less obvious. I think the drawable refcount > is changed in the display channel thread, so I would try to queue an > idle on its main_context, and do the unref from there. Hopefully it will > not be too messy... (and I'll mention again > g_main_context_push_thread_default which could be useful there ;) or > some thread-local storage variable). > > By "queueing an idle", I mean something like > GSource *source = g_idle_source_new(); > g_source_set_callback(..); > g_source_attach(source, worker->core.main_context);
I have tried this approach but I get a feeling it cannot work and is fundamentally flawed :-( There are two sticking points, the dependency on the glib main loop and getting access to main_context. I have not found a clean way to get the main_context information from the RedDrawable: - From the RedDrawable I can get a QXLState pointer with red_drawable->qxl->st. - And given a RedsState pointer I can get the main_context with reds_get_core_interface(reds)->main_context - But while QXLState does have a reds field, it's defined in red-qxl.c so it's not accessible from red-worker.c and I have not found a function that would return this information. There is a loophole though: reds.h defines a reds global variable so I can just use reds_get_core_interface(reds)->main_context! That really does not feel right though. Anyway, doing that I got: if (g_atomic_int_dec_and_test(&red_drawable->refs)) { GSource *source = g_idle_source_new(); g_source_set_callback(source, red_drawable_unref_cb, red_drawable, NULL); g_source_attach(source, reds_get_core_interface(reds)->main_context); } But this resulted in red_drawable_unref_cb never being called. The same is true if using g_timeout_source_new() instead wit various timeout values. I'll get back to this. (there is also the issue of freeing the source which is uglyly fixable) I'm also concerned about this use of reds_get_core_interface(). As far as I can tell it's only used to call red_cannel_create() / red_channel_create_parser(). And access to main_context seems to be restricted to initializing RedWorker objects and implementing the timer functions in event-loop.c. That got me looking at the event-loop.c code and it essentially does all the source manipulations we want, except as timers. That makes me think that bypassing the reds_core_timer_*() functions is wrong. So I implemented the unref through the reds_core_timer_*() functions with a 1ms delay (0ms did not work). The callback got called and was freeing RedDrawables... for a short time. Then I got a crash in TimerSet()! TimerSet() is an Xorg function on top of which the reds_core_timer_*() functions are implemented in Xspice. - It turns out TimerSet() & co are not thread-safe, hence the crash. So reds_core_timer_*() cannot be used to solve thread-safety issues. - I feel that this confirms the glib approach is wrong as it bypasses the Xorg implementation. - I now wonder if there is really a glib main loop in the Xspice case. If not that would explain why the callback was not called with g_idle_source_new(). A completely different approach to solving this would be to handle it all in the GStreamer encoder code: - Only ref the RedDrawable once but wrap it in a struct with its own refcount. It's that struct which will track the actual refcounting by the GstMemory objects. - When that struct's refcount drops to zero, put the RedDrawable into a GAsyncQueue. - encode_frame() is always called from a safe thread, so unref all the RedDrawables in the async queue whenever we enter / leave encode_frame() or destroy the GStreamer encoder. The drawback is that we could get up to a 1 frame delay before a RedDrawable is freed. -- Francois Gouget <fgou...@codeweavers.com> _______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel