[FFmpeg-devel] [PATCH] avcodec thread safety fix

[Andreas Öman]

Art Clarke wrote:
> On Tue, May 26, 2009 at 11:15 AM, Andreas ?man <andreas at lonelycoder.com> wrote:
>> Fair enough. Here is another variant.
> 
> Great idea by the way; I was just sitting down this morning to put
> together a similar patch.  A few comments:
> 
> - You don't actually provide a call to create the mutex or delete the
> mutex but you provide op codes for that.  My suggestion is to add a
> call to create the mutex when someone sets the callback (right AFTER
> you set the global callback pointer) and require them to ensure they
> set the callback in a single-thread. 

Well, my path does

if(cb(&codec_mutex, AV_LOCK_CREATE))
     return -1;

In the callback register function. I believe that should be enough?

 The other option is to create it
> in avcodec_register_all, but that would require people to have set the
> callback BEFORE calling avcodec_register_all which would violate the
> requirement that avcodec_register_all come first.
> 
> - In this case the mutex you're using lives for the entire time the
> libavcodec shared library is loaded or executable is running, so your
> delete lock call really should only come when the library unloads.
> Since there is no way to provide that hook safely that I know of, you
> should add docs comment letting people know that delete is not
> guaranteed to be called.  (In case you're wondering, yes there are
> some situations where unloading libavcodec would be desired, but
> that's a different topic).

Perhaps it could AV_LOCK_DESTROY all global mutexes if you invoke
ff_lockmgr_register with NULL. Or have av_lockmgr_unregister()?

> - Not for this patch, but ideas for later.  The locking avcodec_open
> and avcodec_close in libavcodec/utils.c is (AFAIK) protecting the
> specific AVCodec implementation so it can open and close without any
> other thread ON THE SAME CODEC accidentally initializing static
> AVCodec lookup tables and the like.  Therefore once you have the ideas
> of mutexes in your API (good suggestion Michael), the lock in utils.c
> should be on a member of AVCodec rather than a static (and therefore
> global across all codecs) variable which would slightly increase
> throughput.  The risk here is that I'm wrong and that lock is
> protecting more than the AVCodec's specific initialization.

This sounds reasonable to me. It would also make it possible to avoid
locking for codecs that don't need it at all.