[Spice-devel] [spice v10 10/27] server: Make the RedDrawable refcount thread-safe

[Francois Gouget]

On Wed, 2 Mar 2016, Frediano Ziglio wrote:

> > 
> > Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> > ---
> > 
> > In theory this could be needed by the next patch.
> > 
> 
> Are you saying that now compression is done in another thread?

Nothing has changed with regards to threading. My understanding is that 
GStreamer always forks its own thread(s) to handle the pipeline.

The difference is that before the pipeline elements were not touching 
the RedDrawable refcount and now they do.

Furthermore, as explained in the next patch, I think that all the 
incrementing and decrementing of the RedDrawable refcount will happen 
during the encode_frame() call, i.e. there will be no race condition 
since the main thread will be blocked in the gst_app_sink_pull_sample() 
call.

However this relies on reasonable assumptions on how the pipeline 
elements behave, for instance that the videoconvert element will 
actually have to convert the frame and thus will release the input 
buffer as soon as that's done. But the goal of changing from the 
previous approach to this one is to get rid of these assumptions and 
thus ensure this works even if the buffer we push to the pipeline is 
freed some time after gst_app_sink_pull_sample() returns. If that 
happens then we could get a race condition. Hence this patch.


[...]
> >  void red_drawable_unref(RedDrawable *red_drawable)
> >  {
> > -    if (--red_drawable->refs) {
> > +    gint old_refs;
> > +    do {
> > +        old_refs = red_drawable->refs;
> > +    } while (!g_atomic_int_compare_and_exchange(&red_drawable->refs,
> > old_refs, old_refs - 1));
> 
> This code looks a bit odd. Usually there is a function to do it. 486
> introduced xadd instruction and GCC have atomic built-ins.
> Personally I would like to have a syntax that allows to use C11 instead of
> some library specific ones.

This is loosely modeled after the GObject refcount code, though that 
code is much more complex due to floating and weak references. I think I 
wanted to use g_atomic_int_dec_and_test() in another context and 
concluded I could not use it there, but here it seems like it would work 
just fine.

We could use the C11 atomic operations directly but GLib's gatomic.h 
header does that for us and makes it more portable.


-- 
Francois Gouget <fgouget at codeweavers.com>