[FFmpeg-devel] Threading issue with avcodec_decode_video2 ?

[Hendrik Leppkes]

On Mon, Oct 29, 2012 at 10:22 PM, Don Moir <donmoir at comcast.net> wrote:
>>> Don Moir <donmoir <at> comcast.net> writes:
>>>
>>>>> 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