[FFmpeg-devel] Threading issue with avcodec_decode_video2 ?

Don Moir donmoir at comcast.net
Thu Nov 15 00:51:20 CET 2012


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

> You don't necessarily need to extract it. If you just run the first
> full init inside the init function, all static data is already
> initialized, and any subsequent calls to these functions would just
> create the context-specific data, and no longer touch the static data.

Correct. I was kind of leary about tracing thru that code and getting that all correct. I was looking at it from the stand point of 
least amount of extra coding and potential coding impact. 



More information about the ffmpeg-devel mailing list