[vlc-devel] [PATCH 4/4] libvlc: media: use vlc_atomic_rc for refcount

[Alexandre Janniaux]

By doing a quick survey, I think we should have issues stemming
from the lack of atomic here since the use case you mention were
actually current case.

Preparser is indirectly using libvlc_media_release from a
different thread than the one that create the media.

The fact that it didn't break is probably that it led to silent
break (like leaks) or that the execution dependency order was
somehow forcing it to a correct state, though not explicitly.

I can add this to the commit message.

Regards,
--
Alexandre Janniaux
Videolabs

On Mon, Nov 23, 2020 at 10:34:35AM +0100, Steve Lhomme wrote:
> IMO it should be mentioned that it allows more use cases than before. I'm
> fine with the patch BTW.
>
> On 2020-11-23 9:58, Alexandre Janniaux wrote:
> > Hi,
> >
> > I've seen this but didn't find any issue that could link it
> > so didn't mention it (it's pretty obvious from the diff).
> >
> > I don't really see why media would not be thread-safe to
> > release whereas every other object would be.
> >
> > Regards,
> > --
> > Alexandre Janniaux
> > Videolabs
> >
> > On Mon, Nov 23, 2020 at 08:03:19AM +0100, Steve Lhomme wrote:
> > > This one was not protected by a lock. Is it possible to do the calls from
> > > different threads now ? Or was it broken ?
> > >
> > > On 2020-11-21 13:21, Alexandre Janniaux wrote:
> > > > ---
> > > >    lib/media.c          | 9 ++++-----
> > > >    lib/media_internal.h | 4 +++-
> > > >    2 files changed, 7 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/lib/media.c b/lib/media.c
> > > > index dd4faf0066..6f7911e0a2 100644
> > > > --- a/lib/media.c
> > > > +++ b/lib/media.c
> > > > @@ -37,6 +37,7 @@
> > > >    #include <vlc_meta.h>
> > > >    #include <vlc_url.h>
> > > >    #include <vlc_thumbnailer.h>
> > > > +#include <vlc_atomic.h>
> > > >    #include "../src/libvlc.h"
> > > > @@ -501,7 +502,7 @@ libvlc_media_t * libvlc_media_new_from_input_item(
> > > >        p_md->p_libvlc_instance = p_instance;
> > > >        p_md->p_input_item      = p_input_item;
> > > > -    p_md->i_refcount        = 1;
> > > > +    vlc_atomic_rc_init(&p_md->rc);
> > > >        vlc_cond_init(&p_md->parsed_cond);
> > > >        vlc_mutex_init(&p_md->parsed_lock);
> > > > @@ -643,9 +644,7 @@ void libvlc_media_release( libvlc_media_t *p_md )
> > > >        if (!p_md)
> > > >            return;
> > > > -    p_md->i_refcount--;
> > > > -
> > > > -    if( p_md->i_refcount > 0 )
> > > > +    if( !vlc_atomic_rc_dec(&p_md->rc) )
> > > >            return;
> > > >        uninstall_input_item_observer( p_md );
> > > > @@ -675,7 +674,7 @@ void libvlc_media_release( libvlc_media_t *p_md )
> > > >    void libvlc_media_retain( libvlc_media_t *p_md )
> > > >    {
> > > >        assert (p_md);
> > > > -    p_md->i_refcount++;
> > > > +    vlc_atomic_rc_inc( &p_md->rc );
> > > >    }
> > > >    // Duplicate a media descriptor object
> > > > diff --git a/lib/media_internal.h b/lib/media_internal.h
> > > > index e4d0d03fae..b8646abce2 100644
> > > > --- a/lib/media_internal.h
> > > > +++ b/lib/media_internal.h
> > > > @@ -35,10 +35,12 @@
> > > >    struct libvlc_media_t
> > > >    {
> > > >        libvlc_event_manager_t event_manager;
> > > > +
> > > >        input_item_t      *p_input_item;
> > > > -    int                i_refcount;
> > > >        libvlc_instance_t *p_libvlc_instance;
> > > >        libvlc_state_t     state;
> > > > +    vlc_atomic_rc_t    rc;
> > > > +
> > > >        VLC_FORWARD_DECLARE_OBJECT(libvlc_media_list_t*) p_subitems; /* A media descriptor can have Sub items. This is the only dependancy we really have on media_list */
> > > >        void *p_user_data;
> > > > --
> > > > 2.29.2
> > > >
> > > > _______________________________________________
> > > > vlc-devel mailing list
> > > > To unsubscribe or modify your subscription options:
> > > > https://mailman.videolan.org/listinfo/vlc-devel
> > > >
> > > _______________________________________________
> > > vlc-devel mailing list
> > > To unsubscribe or modify your subscription options:
> > > https://mailman.videolan.org/listinfo/vlc-devel
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> >
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel