[FFmpeg-devel] [PATCH 1/2] pthreads: Fix bug introduced with thread_safe_callbacks

[Ronald S. Bultje]

Hi,

On Mon, Feb 28, 2011 at 5:46 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> On Mon, Feb 28, 2011 at 4:34 PM, Alexander Strange
> <astrange at ithinksw.com> wrote:
>> On Feb 17, 2011, at 3:43 PM, Ronald S. Bultje wrote:
>>> On Thu, Feb 17, 2011 at 4:10 AM, Alexander Strange
>>> <astrange at ithinksw.com> wrote:
>>>> For intra codecs, ff_thread_finish_setup() is called before decoding starts
>>>> automatically. However, get_buffer can only be used before it's called, so
>>>> adding this requirement broke frame threading for them. Fixed by moving the
>>>> call until after get_buffer is finished.
>>>> ---
>>>> ?libavcodec/pthread.c | ? ?8 ++++++--
>>>> ?1 files changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/libavcodec/pthread.c b/libavcodec/pthread.c
>>>> index 0e033d3..096b371 100644
>>>> --- a/libavcodec/pthread.c
>>>> +++ b/libavcodec/pthread.c
>>>> @@ -292,7 +292,8 @@ static attribute_align_arg void *frame_worker_thread(void *arg)
>>>>
>>>> ? ? ? ? if (fctx->die) break;
>>>>
>>>> - ? ? ? ?if (!codec->update_thread_context) ff_thread_finish_setup(avctx);
>>>> + ? ? ? ?if (!codec->update_thread_context && !avctx->thread_safe_callbacks)
>>>> + ? ? ? ? ? ?ff_thread_finish_setup(avctx);
>>>>
>>>> ? ? ? ? pthread_mutex_lock(&p->mutex);
>>>> ? ? ? ? avcodec_get_frame_defaults(&p->frame);
>>>> @@ -779,7 +780,7 @@ int ff_thread_get_buffer(AVCodecContext *avctx, AVFrame *f)
>>>> ? ? ? ? return avctx->get_buffer(avctx, f);
>>>> ? ? }
>>>>
>>>> - ? ?if (p->state != STATE_SETTING_UP) {
>>>> + ? ?if (p->state != STATE_SETTING_UP && avctx->codec->update_thread_context) {
>>>> ? ? ? ? av_log(avctx, AV_LOG_ERROR, "get_buffer() cannot be called after ff_thread_finish_setup()\n");
>>>> ? ? ? ? return -1;
>>>> ? ? }
>>>> @@ -810,6 +811,9 @@ int ff_thread_get_buffer(AVCodecContext *avctx, AVFrame *f)
>>>> ? ? ? ? err = p->result;
>>>>
>>>> ? ? ? ? pthread_mutex_unlock(&p->progress_mutex);
>>>> +
>>>> + ? ? ? ?if (!avctx->codec->update_thread_context)
>>>> + ? ? ? ? ? ?ff_thread_finish_setup(avctx);
>>>> ? ? }
>>>>
>>>> ? ? pthread_mutex_unlock(&p->parent->buffer_mutex);
>>>> --
>>>> 1.7.4.1
>>>
>>> Does this mean in some situations ff_thread_finish_setup will be called twice?
>>
>> No, but there was a deadlock bug in the first line...
>>
>> Also made the error condition in the second chunk more complex.
>> (p->state != STATE_SETTING_UP && !avctx->thread_safe_callbacks) will never happen in current code, but it's the kind of bug that's easy to introduce with an unrelated change.
>>
>> Reattached both patches here.
>
> Current version looks OK to me, I'll leave it for a few hours for
> others to comment and then apply.

Both pushed, great work! In your next commit msgs for multithreading
support, can you again post the speedup achieved by using >1 thread
compared to the 1-thread case? That's nice for PR-purposes.

Thanks!
Ronald