[FFmpeg-devel] [PATCH] Move mathops.h to libavutil.

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> On Thu, Nov 27, 2008 at 11:10:10PM +0000, M?ns Rullg?rd wrote:
>> Michael Niedermayer <michaelni at gmx.at> writes:
>> 
>> > On Thu, Nov 27, 2008 at 01:18:05PM -0000, M?ns Rullg?rd wrote:
>> >> 
>> >> Mans Rullgard wrote:
>> >> > ---
>> >> >  libavcodec/lsp.c                               |    2 +-
>> >> >  libavcodec/mpegaudiodec.c                      |    2 +-
>> >> >  {libavcodec/armv4l => libavutil/arm}/mathops.h |    6 +++---
>> >> >  {libavcodec => libavutil}/bfin/mathops.h       |    6 +++---
>> >> >  {libavcodec => libavutil}/mathops.h            |   12 ++++++------
>> >> >  {libavcodec => libavutil}/ppc/mathops.h        |    6 +++---
>> >> >  {libavcodec/i386 => libavutil/x86}/mathops.h   |    6 +++---
>> >> >  7 files changed, 20 insertions(+), 20 deletions(-)
>> >> >  rename {libavcodec/armv4l => libavutil/arm}/mathops.h (96%)
>> >> >  rename {libavcodec => libavutil}/bfin/mathops.h (95%)
>> >> >  rename {libavcodec => libavutil}/mathops.h (91%)
>> >> >  rename {libavcodec => libavutil}/ppc/mathops.h (92%)
>> >> >  rename {libavcodec/i386 => libavutil/x86}/mathops.h (93%)
>> >> 
>> >> ping
>> >
>> > why should mathops be moved to libavutil?
>> 
>> libavutil already has a number of similar macros/functions, scattered
>> throughout common.h and internal.h:
>> 
>> FASTDIV()
>> FFABS()
>> FFSIGN()
>> FFMAX()
>> COPY3_IF_LT()
>> MASK_ABS()
>> av_log2()
>> av_log2_16bit()
>> ff_sqrt()
>> mid_pred()
>> av_clip()
>> av_clip_uint8()
>> av_clip_int16()
>> av_clipf()
>> 
>> Most, if not all, of these are only used in lavc.
>
> i know that at least mplayer uses several of them
>
>> 
>> IMO, it would make sense to have all elementary maths operations
>> defined the same place.
>
> it would make sense.
> But considering the theoretic extreem, there are many math operations that
> some might consider elementary that probably dont belong in libav* like
> for example primality testing or factorization of large numbers.

Can we please have a serious discussion?  I am obviously only talking
about things that would be used somewhere in FFmpeg.

>> What criteria do you use deciding where to place a function of this
>> type?
>
> IMO Libavutils goal is to stay small and only contain things that are
> really usefull for a wide range of applications or tasks.
> some of the functions like clip or ffmax allow code to be written in a
> more compact and readable way.

The ff* ones are, by virtue of their name, not meant to be used
outside FFmpeg.  If you think this should change, please say so.

> COPY3_IF_LT should IMHO be removed from libavutil, its too specific and
> obscure.

Tell me where you would like it.

> FASTDIV is a nice and fast way to perform divission when the operands are
> within a limited range. A compiler couldnt just replace a a/b by it ...

Which brings back the question of ff_inverse, which is defined in
lavc.  Surely it is wrong for a macro in lavu to depend on lavc.
BTW, I think a name like DIV16_8 would be more descriptive of what it
actually is.

> for MULL/... it really is a question of people wanting it in libavutil.
> If people do want to use it iam not at all against putting it in libavutil
> But if noone wants it, it feels like bloat even if its just a header ...

I suggested it because it would put all the simple maths stuff in one
place.  My idea was to move at least some of other macros above to
this header as well.

> Besides it really is gccs job to compile an elementary multiplication 
> (possibly with a shift) to a reasonable sequence of instrctions. And i
> hope these things have been reported to the gcc devels. It would be
> silly if we where maintaining these workarounds while the gcc developers
> where unaware of the issue.

What about the 16-bit ones?  They are used in places where a compiler
cannot easily, if at all, determine that the input operands are
restricted to 16 significant bits.  I certainly do not expect a
compiler to recognise the code as an MPEG audio decoder and apply
facts known only from the MPEG specification.

-- 
M?ns Rullg?rd
mans at mansr.com