[FFmpeg-devel] Threading issue with avcodec_decode_video2 ?

Don Moir donmoir at comcast.net
Sun Oct 28 20:34:07 CET 2012

>> 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 

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.

More information about the ffmpeg-devel mailing list