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

[Michael Niedermayer]

On Thu, Oct 17, 2013 at 01:15:27AM +0200, wm4 wrote:
> 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.

posted a patch that adds it only around the default callback


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

yes, but better than corrupted output

also i think if any significant time is spend in av_log() then theres
something wrong, especially if its called from multiple threads at
the same time.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131017/b207d48d/attachment.asc>