[FFmpeg-devel] Threading issue with avcodec_decode_video2 ?

[Don Moir]

> On 13 Nov 2012, at 22:30, "Don Moir" <donmoir at comcast.net> wrote:
>>> On 13 Nov 2012, at 00:25, "Don Moir" <donmoir at comcast.net> wrote:
>>>
>>>> ----- Original Message ----- From: "Reimar Döffinger" <Reimar.Doeffinger at gmx.de>
>>>> To: "FFmpeg development discussions and patches" <ffmpeg-devel at ffmpeg.org>
>>>> Sent: Monday, November 12, 2012 4:44 PM
>>>> Subject: Re: [FFmpeg-devel] Threading issue with avcodec_decode_video2 ?
>>>>
>>>>
>>>>> On Mon, Oct 29, 2012 at 10:38:25PM +0100, Hendrik Leppkes wrote:
>>>>>> On Mon, Oct 29, 2012 at 10:22 PM, Don Moir <donmoir at comcast.net> wrote:
>>>>>> > 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.
>>>>>
>>>>> Could you explain how you came to that conclusion?
>>>>> At least for ff_msmpeg4_decode_init I am not so sure about that.
>>>>> But even if they need to be called there, I see nothing speaking against
>>>>> _also_ calling them in the init function, this results in the VLC tables
>>>>> being initialized there, thus they will no longer be initialized when
>>>>> calling those functions during decode, thus eliminating the race
>>>>> condition.
>>>>
>>>> Hendrik might have more info but in vc1_decode_frame you have this code:
>>>>
>>>>  if (s->context_initialized &&
>>>>      (s->width  != avctx->coded_width ||
>>>>       s->height != avctx->coded_height)) {
>>>>      ff_vc1_decode_end(avctx);
>>>>  }
>>>>
>>>> If  'ff_vc1_decode_end'  gets called then it's back to being unintialized and back to same problem as far as I can tell. You 
>>>> can call it in the init function but not sure that helps in the overall sense.
>>>
>>> There is no way to reset the static VLC tables, because it makes no sense to ever do that.
>>> So the part at issue should always remain initialised, which is the whole point of my suggestion.
>>
>> Moving the failing logic from decode to the init function does work for the particular file I have been testing with.
>>
>> You can get that file here:
>>
>> https://ffmpeg.org/trac/ffmpeg/ticket/1876
>>
>> The thing is if the width or height changes for some other file then ff_vc1_decode_end will be called. Then context_initialized 
>> will be 0 and ff_msmpeg4_decode_init will wind it's way thru several functions doing whatever. It's not just static info that is 
>> being initialized as far as I know. It does seem like quite a bit of overkill just because the width and height changed.
>
> Sure, it may be overkill and not the best design, but is there any real disadvantage to it?
> Performance of resolution changes is hardly critical I guess.

No real disadavantage and any fix with a redesign would appear to be much more work.

Probably a good thing would to be to go ahead and add 'ff_msmpeg4_decode_init' and 'ff_vc1_decode_init_alloc_tables'  to the init 
function while keeping them in the decode function except to also lock them down in decode. By adding them to the init function it 
will prevent a lockdown in the decode function in most cases.