[FFmpeg-devel] Threading issue with avcodec_decode_video2 ?

[Don Moir]

On Wed, Nov 14, 2012 at 1:22 AM, Don Moir <donmoir at comcast.net> wrote:
>> 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.

> The lock is only required for the static tables which are generated,
> everything else is in its own decoder context, and therefor thread
> safe.
> That means, when you can ensure that the tables are created during
> init, you do not need to lock anything during the decode call.

> - Hendrik

Thats the way it should be but it appears the initialization of the static tables is mixed in with other non static initialization. 
Extraction of the static table initialization may be a bit time consuming since those functions wind themselves into maybe 4 files 
or so but maybe the extraction would be easier than I am thinking.