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

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> On Fri, Nov 28, 2008 at 01:03:24AM +0000, M?ns Rullg?rd wrote:
>> 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.
>
> i think MAX/MIN should be public ...
> if you want to rename them, i dont mind though that would be a big
> diff ...

 132 files changed, 440 insertions(+), 440 deletions(-)

Would you like to see the diff?

>> > COPY3_IF_LT should IMHO be removed from libavutil, its too specific and
>> > obscure.
>> 
>> Tell me where you would like it.
>
> close to where its used
> a quick grep pointed just at motion_est*

I prefer to keep any asm in the per-arch subdirs.  It's much easier to
keep track of it that way.

>> > 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.
>
> yes, though IIRC it was correct for more than 16bit

The comment preceding the definition of ff_inverse says it goes up to
65536, so strictly speaking you're right if that comment is telling
the truth.

> which brings us to the question how far we should gurantee correctness
> some implementations may benefit from the 16bit limit ...
> iam just bringing this up as you are proposing to put the limit in the
> name, i guess DIV16_8 is fine though, at least better than FASTDIV

We know it's correct for 16 bits, so that's a start.

As I said, I'd like to move as much as possible of the asm into the
arch subdirs.  This would mean doing the same thing to FASTDIV and
others in common.h/internal.h as we've already done with bswap.h and
mathops.h.

I'm a bit concerned by the presence of asm in common.h, which is an
installed header where asm will of course be useless with no config.h.
The asm is of course good, and several of the function in common.h
could be made much faster with a little asm.  How would you like to
deal with this in a reasonably clean way?

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