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

[Frediano Ziglio]

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

I was not suggesting to switch completely to C11 but be ready for it.
I agree for the time being gatomic.h is more portable. Just the way
you decrement the counter can be improved.

As documentation said

"For simple reference counting purposes you should use g_atomic_int_inc() and g_atomic_int_dec_and_test()"

or you could use g_atomic_int_add(&count, -1).

I would also add a comment in or before red_drawable_unref to say
that this function can be called by different threads (unlike most
of other functions).

Frediano