[FFmpeg-devel] [PATCH] avcodec/utils: use a default lock manager that uses a spinlock

Reimar Döffinger Reimar.Doeffinger at gmx.de
Fri Oct 18 18:12:50 CEST 2013


On 18.10.2013, at 11:27, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Fri, Oct 18, 2013 at 08:41:36AM +0200, Reimar Döffinger wrote:
>> 
>> 
>> On 18.10.2013, at 01:47, Michael Niedermayer <michaelni at gmx.at> wrote:
>>>> 2. Use pthreads, and on Windows, one of the popular pthreads wrappers.
>>> 
>>> Last time this idea came up someone said it doesnt work because
>>> theres no gurantee that the pthread mutex you then use is compatible
>>> with the threads its trying to sychronize which may be something
>>> else than pthread based. IIRC
>> 
>> To be honest, neither of these approaches work properly.
>> Your atomics approach will completely break (as in cause hangs) in an environment using cooperative scheduling if the user yields in a FFmpeg callback.
> 
> i hoped that such platforms dont exist anymore

I believe many embedded an real-time systems still behave like that.

>> It also will burn the CPU for a whole time-slice in single-core environments if there is any congestion.
> 
> that sounds nasty but if theres congestion (which for the cases where
> it would be used i would not expect) you sure prefer burning cpu over
> it somehow mysteriously failing in hard to reproduce ways.

But as long as it is such rarely called functions, the question is if there's an advantage over using pthreads.

>> Note these also apply to the av_log changes.
> 
> the second patchset doesnt have the lock over the user callback but
> just the default, so the question would be if any function in there
> can cause a "cooperative schedulin environment" to yield

Hm, that might be too much for us to care about, but it's not unusual for IO to trigger a yield I believe.

>> Pthreads is not always available, and even if it is there is no guarantee it will be compatible with the threading system the application uses.
>> Or in short: we don't do it currently, because it simply doesn't work in general.
>> Whether we should support (either by default or with a simpler way that requires less code) a pthreads based application is a different question, but it's likely to break some use case, and probably needs to be considered and API change.
> 
> You miss a problem here
> the current code does not work, its not that my patches implement
> merely a convenient default ...
> 
> the lock manager registration itself is completely unsafe
> consider 2 libs or an application with plugins, or an application
> and a lib
> loading libavcodec & libavformat or in case of 2 libs libavcodec twice
> how could they set a lock manager ? each might require a different
> lock manager and even if they use the same the registration still
> is not thread safe as it would unregister the old at a random non
> synchronized point.
> And to make it worse the 2 libs/app using libavcodec could use
> different thread systems 
> 
> considering above iam not sure the spinlock (in environments where its
> safe) would be that bad of a solution.
> but iam happy to implement something else

Well, I just kind of thing that pthreads would be a better solution.
If compiled without threads, we'd automatically default to not using this (and thus admittedly not being thread safe, that's probably not fully ideal if an application cannot know that).
A pthread implementation could do a yield for systems requiring it.
Another option would possibly be to use a spinlock only for setting the lock manager? Still seems like it will be tricky to use though, and might just combine the worst of all options.


More information about the ffmpeg-devel mailing list