>
> 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
No, this is the problem. Every RedWorker has a different glib context, if
you pass through the core main_context should be NULL pointing to default
one which is not used.
> - 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.
You could use red_qxl_get_server but as said before you would get
the wrong context.
I think here what's missing is a way to get a RedWorker from a QXLInstance.
This could be added to red-qxl.[ch].
>
> 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.
>
There is similar code to deal with Glz (see display_channel_free_glz_drawables_to_free
call in red-worker.c).
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel