[FFmpeg-devel] [PATCH] avutil/utils: remove unnecessary things

wm4 nfxjfg at googlemail.com
Tue Mar 24 11:26:36 CET 2015


On Fri, 20 Mar 2015 18:33:37 +0100
Michael Niedermayer <michaelni at gmx.at> wrote:

> On Fri, Mar 20, 2015 at 05:41:43PM +0100, wm4 wrote:
> > The checks for the enums all were added in 2011 and 2012. They were
> > likely for checking that Libav changes do not change the FFmpeg ABI.
> > Given the amount of changes in the last 3 years, and the fact that
> > barely any other ABI specifics are tested, keeping these tests makes
> > not much sense anymore.
> 
> The tests check that ABI is not broken, for example if someone added
> a entry in the middle of a enum it would break ABI to our own
> previous release with same so-name.
> its better if the assert punches the person who puts that in the
> middle of a enum straight in the face instead of it being posted
> to the mailing list and someone needing to ask for it to be fixed
> and reposted with it moved to the end

Such invalid changes are easily caught in the patch reviews.

> Testing for this in a cleaner way is very welcome of course

Sure, but these barely test anything anyway and are not worth for the
required uglification. You can just move them into some test program if
you really want them.

> 
> > 
> > The assertion checking for "((size_t)-1) > 0" is useless. The condition
> 
> probably ok
> 
> 
> [...]
> 
> > The av_sat_dadd32() test was added in 2012 to check for broken ARM
> > binutils. Even if anyone argues that this is still relevant, it should
> > be done in configure instead. (And always should have.) The binutils
> > bug report (as cited in the original ffmpeg-devel thread discussing
> > this patch) was marked as fixed in 2009.
> 
> If this test can be done in configure and is reliable, iam in favor
> of doing it in configure as well
> 
> 
> > 
> > The llrint() was added in 2013 for NetBSD. I don't see what printing a
> > message changes. (Apparently nothing; from what I can see NetBSD didn't
> > touch its llrint implementation since 2004.) If it's critical, just make
> > configure refuse to run if NetBSD is detected, or print a compile-time
> > warning, or do not use llrint.
> 
> IMO its better to test for a problem than to test for the platform
> that has the problem.
> Other platforms do have issues with some *rint() functions too IIRC
> also most of the codecs wont be affected so hard failing in configure
> is not very nice to the user

But it's nicer to print an easily missed warning on program start after
configure, compilation, and installation are done?

If most of the codecs are affected, then it's just an argument for
removing the check entirely, too.

> 
> 
> [...]
> 
> > -    av_assert0(LIBAVUTIL_VERSION_MICRO >= 100);
> > -    av_assert0(HAVE_MMX2 == HAVE_MMXEXT);
> 
> These 2 lack any explanation of why they would be removed in the
> commit message
> 
> [...]

Nobody would do anything that these assertions trigger, plus it's
easily caught in patch review.


More information about the ffmpeg-devel mailing list