[FFmpeg-devel] Threading issue with avcodec_decode_video2 ?

[Don Moir]

>>>>>> Forgot to mention that abort seemed to be called as one
>>>>>> of the spurious crashes I was seeing before lock was in
>>>>>> place.
>>
>>
>>>>> As you know, we consider crashes important, so if you
>>>>> have a sample that crashes current FFmpeg (no matter if
>>>>> with abort() or differently), please either open a ticket
>>>>> on trac or send a mail to ffmpeg-user.
>>>
>>>
>>>> All crashes related to the issue in this email had to do with unsafe
>>>> thread issue as detailed in email which can create spurious and seemingly
>>>> unrelated crashes. It's not going to be reproducible with ffmpeg command
>>>> line since its a multi-threaded issue.
>>>
>>>
>>>> Would be great if this could be addressed right away and will do what I
>>>> can. We probably just need confirmation from Michael or Hendrik or whoever
>>>> that it's ok to move the mentioned failing code into the init procedure.
>>>> Also, whatever else this might effect and whatever else might be doing the
>>>> same thing.
>>
>>
>>> patch for this is welcome
>>
>>
>> Yeah would love to supply a patch, but just not patch savvy and seems I
>> never have to time to get that way.
>>
>> I looked around for what code calls ff_msmpeg4_decode_init and
>> ff_vc1_decode_init_alloc_tables.
>>
>> Within avcodec both are called in only 2 places.
>>
>> In mss2.c they are called in wmv9_init and as we know in vc1_dec.c they are
>> called in vc1_decode_frame.
>>
>> In wmv2dec.c only ff_msmpeg4_decode_init is called in its wmv2_decode_init
>> function. ff_vc1_decode_init_alloc_tables is not called from code in
>> wmv2dec.c directly.
>>
>> vc1dec.c is the odd man out here and doesn't call these functions in its
>> vc1_decode_init function and there are threading issues because of it.
>>
>> In vc1dec.c there is a little more to it but not much.
>>
>>
>>    if (!s->context_initialized) {
>>        if (ff_msmpeg4_decode_init(avctx) < 0 ||
>> ff_vc1_decode_init_alloc_tables(v) < 0)
>>            return -1;
>>        s->low_delay = !avctx->has_b_frames || v->res_sprite;
>>        if (v->profile == PROFILE_ADVANCED) {
>>            s->h_edge_pos = avctx->coded_width;
>>            s->v_edge_pos = avctx->coded_height;
>>        }
>>    }
>>
>> In mss2.c and wm2dec.c they just call init functions or function and thats
>> it.
>>
>> In vc1dec.c it calls the init functions and does a little more. The code
>> under return -1; appears to be ok and seems to be happy in either
>> vc1_decode_init or vc1_decode_frame, but "if (ff_msmpeg4_decode_init(avctx)
>> < 0 || ff_vc1_decode_init_alloc_tables(v) < 0)" only seems to be happy in
>> vc1_decode_init or when locked down.
>>
>> Do you have a multi-threaded test case for this kind of thing ?
>
> I looked over those two functions, and sadly its required to call them
> there, because its possible that width/height of the video are not
> known during the init function, or may even change in case of vc1image
> decoding.
> Now, the static structures should only be initialized once, and guard
> themself against re-init, so i see two solutions: Either lock this
> block, or call them once in the init function to create the static
> data, and keep calling them in there.
>
> I'm partial to the locking.
>
> - Hendrik

Thank you Hendrik. Given what you said, putting a lock around the init functions in vc1_decode_frame does for sure have the least 
amount of potential impact. That was my first pass on it but found it necessary to add avpriv_lock_avcodec and avpriv_unlock_avcodec 
since these don't currently exist. Could be other ways as well. Should be a rare event.