[FFmpeg-devel] Threading issue with avcodec_decode_video2 ?

[Reimar Döffinger]

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.