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

Andreas Öman andreas
Fri May 29 21:04:33 CEST 2009


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.




More information about the ffmpeg-devel mailing list