>
> On Wed, 2 Mar 2016, Frediano Ziglio wrote:
>
> > >
> > > Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
> > > ---
> > >
> > > 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel