[FFmpeg-devel] [PATCH] avcodec: increase FF_INPUT_BUFFER_PADDING_SIZE to 32

[Andreas Cadhalpun]

On 21.07.2014 21:23, Michael Niedermayer wrote:
> On Mon, Jul 21, 2014 at 08:05:05PM +0200, Andreas Cadhalpun wrote:
>> On 21.07.2014 00:48, Michael Niedermayer wrote:
>>> On Sun, Jul 20, 2014 at 10:43:30PM +0200, Andreas Cadhalpun wrote:
>>>> On 12.06.2014 08:42, Christophe Gisquet wrote:
>>>>> Hi,
>>>>>
>>>>> 2014-06-11 21:29 GMT+02:00 Michael Niedermayer <michaelni at gmx.at>:
>>>>>> -#define FF_INPUT_BUFFER_PADDING_SIZE 16
>>>>>> +#define FF_INPUT_BUFFER_PADDING_SIZE 32
>>>>>
>>>>> So, following the discussion on how API users may be affected by this
>>>>> change, does that need an API bump?
>>>>
>>>> One effect of this change is that now mplayer2 fails to build,
>>>> because it uses this check:
>>>> #if MP_INPUT_BUFFER_PADDING_SIZE < FF_INPUT_BUFFER_PADDING_SIZE
>>>> #error MP_INPUT_BUFFER_PADDING_SIZE is too small!
>>>> #endif
>>>>
>>>> MP_INPUT_BUFFER_PADDING_SIZE is defined as 16. Increasing that to 32
>>>> allows building mplayer2 again.
>>>>
>>>> So I think an API bump wouldn't have been a bad idea.
>>>
>>> I think a soname bump would cause alot more work for everyone
>>
>> That's for sure. I didn't mean a soversion bump for this change
>> alone, but rather wait with this change until a soversion bump is
>> necessary due to a larger API change.
>> It's just my expectation that if the API (i.e. major soversion)
>> doesn't change, programs that built successfully with an older
>> version will continue to build fine.
>> This change breaks that expectation.
>
> yes though the failure is due to an app explicitly checking
> FF_INPUT_BUFFER_PADDING_SIZE to be within a specific range
> our API did not gurantee that it would be in that range.
> also FF_ is the prefix for internal stuff AV* for public, arguably
> it probably should have been AV_ but it isnt

I agree, it's rather unorthodox to error out if 
FF_INPUT_BUFFER_PADDING_SIZE is too large.

>>> but lets assume we did bump soname and changed it to 32 after that
>>>
>>> what with the old API/ABI ? it needs 32 too in the exact same corner
>>> cases that the new API/ABI needs it. But all apps that are build
>>> against the old AND which use these corner cases are then buggy.
>>> that doesnt sound better to me
>>
>> I don't know how severe these corner case bugs have been.
>> Would it have been a big problem to not fix them until the next
>> soversion bump?
>
> iam not sure i understand you here
> mplayer2 didnt pad by enough (assuming it can use that corner case)
> and ffmpeg didnt pad by enough either
>
> mplayer2 should be using 32 even if ffmpeg doesnt, you want to pad
> by enough (that is again assuming it uses a code path that needs it)
> so i really dont see who would be helped by leaving
> FF_INPUT_BUFFER_PADDING_SIZE at 16 (unless all code really needs just
> that)
>
> about severity of the corner cases, it can cause a crash.
> And i do not have a list of which functions need 32byte padding and
> which dont. One case is fate-vsynth3-rgb or more precissely the
> optimized colorspace convertion/scaling in it

That sounds severe enough to warrant an immediate fix.

I think the soversion should have been increased, when the first 
function needing 32 byte padding was added, together with increasing the 
FF_INPUT_BUFFER_PADDING_SIZE.

>> In any case mentioning such a change in APIChanges would be good.
>
> ok, done

Thanks.

>
> Thanks
>
> PS: i understand your point about this but i really think changing
> the FF_INPUT_BUFFER_PADDING_SIZE was better and less work for everyone
> than delaying it and leaving the bugs open.

OK, then it's fine.

Best regards,
Andreas