[FFmpeg-devel] [PATCH] avutil/log: make av_log/av_vlog() thread safe

[wm4]

On Thu, 17 Oct 2013 01:04:41 +0200
Michael Niedermayer <michaelni at gmx.at> wrote:

> On Thu, Oct 17, 2013 at 12:04:11AM +0200, wm4 wrote:
> > On Wed, 16 Oct 2013 23:43:28 +0200
> > Michael Niedermayer <michaelni at gmx.at> wrote:
> > 
> > > This uses a spinlock
> > > 
> > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > > ---
> > >  libavutil/log.c |    6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/libavutil/log.c b/libavutil/log.c
> > > index 53be3ea..b871c8f 100644
> > > --- a/libavutil/log.c
> > > +++ b/libavutil/log.c
> > > @@ -34,6 +34,7 @@
> > >  #endif
> > >  #include <stdarg.h>
> > >  #include <stdlib.h>
> > > +#include "atomic.h"
> > >  #include "avutil.h"
> > >  #include "bprint.h"
> > >  #include "common.h"
> > > @@ -268,8 +269,13 @@ void av_log(void* avcl, int level, const char *fmt, ...)
> > >  
> > >  void av_vlog(void* avcl, int level, const char *fmt, va_list vl)
> > >  {
> > > +    static void * volatile state;
> > > +    while (avpriv_atomic_ptr_cas(&state, NULL, (void*)fmt))
> > > +        ;
> > >      if(av_log_callback)
> > >          av_log_callback(avcl, level, fmt, vl);
> > > +    if (!fmt || avpriv_atomic_ptr_cas(&state, (void*)fmt, NULL) != (void*)fmt)
> > > +        abort(); //cant use av_assert*() here due to that using av_log
> > >  }
> > >  
> > >  int av_log_get_level(void)
> > 
> > What problem is that trying to solve?
> 
> av_log() gets called from multiple decoders or threads within a
> decoder. The results are sometimes intermixed log messages, above
> patch fixes that. Yes ive confirmed the bug as well as that this
> fixes it.
> 
> 
> > There could be a race condition
> > between checking and using av_log_callback,
> 
> uhm, right, fixed, that was independant of this patch though
> 
> 
> > but av_log_callback itself
> > is user-provided. So, as far as I can see it, it doesn't sound like a
> > good idea to call a user-provided callback under a spinlock. Instead,
> > you should document that the user-provided callback is expected to be
> > thread-safe.
> 
> If thats preferred, i can move the lock to our default callback
> but all applications need
> a lock or thread safe callback if they provide a callback then, even
> ones that have a single thread and process only. This doesnt sound
> very nice for apps that use no locking primitives anywhere else

This won't solve things for single threaded applications, because the
spinlock will serialize the logging calls, but not whatever other
application state is touched by its logging callbacks.

Also, a spinlock around slow I/O sounds like a bad idea.