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

[Michael Niedermayer]

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


> 
> >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


[...]

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

ok, done

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.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140721/a2a41d48/attachment.asc>