[FFmpeg-devel] Threading issue with avcodec_decode_video2 ?

[Don Moir]

>>> On Sat, Oct 27, 2012 at 9:40 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>>>> On Sat, Oct 27, 2012 at 12:59:37AM -0400, Don Moir wrote:
>>>>> >I load up multiple instances of the same file. These are used for
>>>>> >playback, thumbnails, and other things. Each is in a separate
>>>>> >thread with their own context, variables, etc.
>>>>> >
>>>>> >We noticed a crash when decoding WMV3 files. It's was a rare event so hard to track down.
>>>>> >
>>>>> >Recently I put some code in to help find the problem. It seems
>>>>> >that avcodec_decode_video2 is not thread safe at least for WMV3
>>>>> >decoding.
>>>>> >
>>>>> >I have disable-pthreads set. I use av_lockmgr_register and see it
>>>>> >allows a lock for avformat and avcodec. The lock for avcodec never
>>>>> >gets called.
>>>>> >
>>>>> >If I put a lock around avcodec_decode_video2, the problem goes
>>>>> >away. After several crashes that led nowhere, I isolated it to
>>>>> >avcodec_decode_video2.
>>>>> >
>>>>> >Shouldn't related decoders be setting the lock via the lock manager if needed ?
>>>>> >
>>>>> >I don't want to have to lock the entire avcodec_decode_video2 call
>>>>> >and lock should be special cased for each decoder and so I am left
>>>>> >wondering.
>>>>> >
>>>>>
>>>>> Looks like the call to ff_msmpeg4_decode_init is causing the problem.
>>>>>
>>>>> About midway down in vc1_decode_frame you will see:
>>>>>
>>>>> if (ff_msmpeg4_decode_init(avctx) < 0 || ff_vc1_decode_init_alloc_tables(v) < 0)
>>>>>
>>>>> I added avpriv_lock_avcodec and avpriv_unlock_avcodec and wrapped
>>>>> ff_msmpeg4_decode_init(avctx) with a lock and problem went away. So
>>>>> there may be more to it but can't get it to crash anymore.
>>>>> ff_vc1_decode_init_alloc_tables(v) appears to be ok.
>>>>>
>>>>> The scary part is what about the other decoders. I have not noticed
>>>>> a problem outside of this though. There could also be a way to fix
>>>>> it without using a lock but didn't look further.
>>>>
>>>> thanks for pointing me to these functions, they indeed had some
>>>> thread saftey bugs.
>>>> should be fixed, does it work without the lock now ?
>>>>
>>>
>>> Couldn't ff_msmpeg4_decode_init be called from the decoder init
>>> function, which is already inside a lock, so the static portions get
>>> initialized there?
>>> On a quick look, it doesn't seem to depend on any important values
>>> into avctx which would only be available later.
>>>
>>> - Hendrik
>>
>> I am still looking into issues. Doesn't look like Michael's patch will be enough but should know more soon.
>>
>> Depending on which way the wind is blowing :) I can see different types of bad behavior.
>>
>> I was wondering why my app would sometimes blow up in a hard exception and abort even in debugger. So out of curiosity, I 
>> searched for abort in the ffmpeg libraries. In windows, abort() will cause the app to exit with some stupid error message that 
>> means nothing to anyone.
>>
>> In ffmpeg proper, abort() is called in bitstream.c, mpegvideo.c, vp8.c in avcodec, and sctp.c in avformat.
>>
>> I have not yet confirmed that abort is actually called from ffmpeg but I suspect it is. I will see about confirming it but not 
>> just yet. It may be coming into play in  vc1_decode_frame and getting in the way of finding real problems. In vc1_decode_frame, 
>> it calls 'ff_find_unused_picture'. 'ff_find_unused_picture' calls 'find_unused_picture' and it calls abort() if it fails. 
>> 'ff_find_unused_picture' and 'find_unused_picture' are in mpegvideo.c
>>
>> Question right now so I am not chasing ghosts, is why abort is ever called in ffmpeg. It's ok if it's just a command line app, 
>> but not at all ok to put down an app that is probably doing a lot more.
>>
>> So again I have not yet confirmed abort is being called but it should not be there in the first place.
>>
>> I am pushing the test case hard with up to 100 files. (aslo tried 500 but mostly use less than 100). These numbers are overkill I 
>> know but helps to define the problem. I think the abort can also happen with like 5 files so could be another thread issue in 
>> vc1_decode_frame. When it goes thru clean, it all works as expected even with 500 files bangin the hell out of it.
>
> Here's the deal:
>
> If I lock down the entire statement: (also accounting for goto err if failed)
>
> if (ff_msmpeg4_decode_init(avctx) < 0 || ff_vc1_decode_init_alloc_tables(v) < 0)
>
> it works perfect. Any attempt to piecemeal the lock in either in either function results in eventual failure.
>
> As Hendrik pointed out, the lock is already in place in the init call, so I moved this code from vc1_decode_frame:
>
>    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;
>        }
>    }
>
> into vc1_decode_init. Probably can remove test "if (!s->context_initialized)" but I didn't bother with it. This also appears to 
> work perfect.
>
> Also, if you look at the code ff_h263_decode_init in h263dec.c toward the bottom you will see a strange indentation. This is 
> called from ff_msmpeg4_decode_init.
>
> look at statement toward bottom of ff_h263_decode_init:
>
> if (avctx->codec->id != AV_CODEC_ID_H263 && avctx->codec->id != AV_CODEC_ID_H263P && avctx->codec->id != AV_CODEC_ID_MPEG4)
>
> below this you will see strange indentation of ff_h263_decode_init_vlc(s); and you can't tell if a brace was intended for the 
> above statement or not.
>
> Last but not least is get rid of the abort() calls in ffmpeg libraries. This is just a cop out and has no place in any of the 
> ffmpeg libraries. No one I know ever calls abort from a GUI app and app should recover gracefully.

Forgot to mention that abort seemed to be called as one of the spurious crashes I was seeing before lock was in place. So this is 
not part of the particular problem but it's still a problem in that it is in the ffmpeg libraries and should be removed. abort() 
appears to be in ffmpeg as a legacy thing back from when maybe just command line and single video.