[vlc-devel] [PATCH] lib: core: Use vlc_atomic_rc_t for refcounting

[Steve Lhomme]

On 2020-06-10 15:45, Hugo Beauzée-Luyssen wrote:
> On Wed, Jun 10, 2020, at 3:36 PM, Rémi Denis-Courmont wrote:
>> Is the lock still used for anything?
>>
>> Le 10 juin 2020 16:32:22 GMT+03:00, "Hugo Beauzée-Luyssen"
>> <hugo at beauzee.fr> a écrit :  lib/core.c            | 17
>> +++--------------
>>>   lib/libvlc_internal.h |  3 ++-
>>>   2 files changed, 5 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/lib/core.c b/lib/core.c
>>> index 56b985f597..8a998e3095 100644
>>> --- a/lib/core.c
>>> +++ b/lib/core.c
>>> @@ -60,7 +60,7 @@ libvlc_instance_t * libvlc_new( int argc, const char *const *argv )
>>>       }
>>>   
>>>       p_new->p_libvlc_int = p_libvlc_int;
>>> -    p_new->ref_count = 1;
>>> +    vlc_atomic_rc_init( &p_new->ref_count );
>>>       p_new->p_callback_list = NULL;
>>>       vlc_mutex_init(&p_new->instance_lock);
>>>       return p_new;
>>> @@ -74,24 +74,13 @@ error:
>>>   void libvlc_retain( libvlc_instance_t *p_instance )
>>>   {
>>>       assert( p_instance != NULL );
>>> -    assert( p_instance->ref_count < UINT_MAX );
>>>   
>>> -    vlc_mutex_lock( &p_instance->instance_lock );
>>> -    p_instance->ref_count++;
>>> -    vlc_mutex_unlock( &p_instance->instance_lock );
>>> +    vlc_atomic_rc_inc( &p_instance->ref_count );
>>>   }
>>>   
>>>   void libvlc_release( libvlc_instance_t *p_instance )
>>>   {
>>> -    vlc_mutex_t *lock = &p_instance->instance_lock;
>>> -    int refs;
>>> -
>>> -    vlc_mutex_lock( lock );
>>> -    assert( p_instance->ref_count > 0 );
>>> -    refs = --p_instance->ref_count;
>>> -    vlc_mutex_unlock( lock );
>>> -
>>> -    if( refs == 0 )
>>> +    if(vlc_atomic_rc_dec( &p_instance->ref_count ))
>>>       {
>>>           libvlc_Quit( p_instance->p_libvlc_int );
>>>           libvlc_InternalCleanup( p_instance->p_libvlc_int );
>>> diff --git a/lib/libvlc_internal.h b/lib/libvlc_internal.h
>>> index 5c1107b84f..6c8ff49a81 100644
>>> --- a/lib/libvlc_internal.h
>>> +++ b/lib/libvlc_internal.h
>>> @@ -33,6 +33,7 @@
>>>   #include <vlc/libvlc_picture.h>
>>>   #include <vlc/libvlc_media.h>
>>>   #include <vlc/libvlc_events.h>
>>> +#include <vlc_atomic.h>
>>>   
>>>   #include <vlc_common.h>
>>>   
>>> @@ -60,7 +61,7 @@ VLC_API void libvlc_SetExitHandler( libvlc_int_t *, void (*) (void *), void * );
>>>   struct libvlc_instance_t
>>>   {
>>>       libvlc_int_t *p_libvlc_int;
>>> -    unsigned      ref_count;
>>> +    vlc_atomic_rc_t ref_count;
>>>       vlc_mutex_t   instance_lock;
>>>       struct libvlc_callback_entry_list_t *p_callback_list;
>>>       struct
>>
>> -- 
>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser
>> ma brièveté.
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> 
> It's still used by libvlc_dialog_set_callbacks, but I can't see any other use, which feels like we're missing a lock when reading from the callback stored in the instance.

vlc_dialog_provider_set_callbacks() has an internal lock. Maybe it was 
protecting against libvlc_release() being called at the same time as 
another thread. Probably when resetting dialogs handlers at the end of a 
session.

Patch LGTM.