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 <fgouget@xxxxxxxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel