[FFmpeg-devel] Race conditions in libavcodec/pthread.c

Loren Merritt lorenm at u.washington.edu
Fri Aug 26 15:17:39 CEST 2011


On Fri, 26 Aug 2011, Reimar Döffinger wrote:
> On 26 Aug 2011, at 00:55, Aaron Colwell wrote:
>> On Thu, Aug 25, 2011 at 3:15 PM, Reimar Döffinger wrote:
>>> On Thu, Aug 25, 2011 at 02:45:25PM -0700, Aaron Colwell wrote:
>>>
>>>> park_frame_worker_threads() has an instance. p->state shouldn't be
>>>> checked outside of the lock.
>>>> frame_worker_thread() has an instance at the top of the while loop.
>>>> submit_packet() has an instance after the if(prev_thread) line.
>>>> ff_thread_decode_frame() has an instance at the top of the do loop.
>>>
>>> Ah. I don't think state is supposed to/intended to be protected by a
>>> lock, but the lock/unlock is only there because you can't use
>>> pthread_cond_wait without it.
>>> I guess changing that would make it more correct, but it would be a
>>> change in semantics and it looks to me like you'd have to add a whole
>>> lot more lock/unlock around places where it is used.
>>
>> The thing is that state is modified by the main thread (submit_packet()) &
>> worker threads (frame_worker_thread()). It needs lock protection to avoid
>> race conditions. I agree it's a non-trivial change to fix this. That's why I
>> wanted to ping the list first. I figured it was worth a discussion before
>> diving in. :)
>
> See Uoti's comment, except possibly for architectures with strange caching models that should be the right answer.

In particular, a double-checked condition variable can fail if your
architecture allows two stores to different addresses by one processor to
arrive at another processor out of order, or if the condition is not an
atomic read. (No comment on how common that is among real cpus.)

--Loren Merritt


More information about the ffmpeg-devel mailing list