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

[Michael Niedermayer]

On Fri, Nov 28, 2008 at 02:50:31AM +0000, M?ns Rullg?rd wrote:
> 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?

no, just commit it if you think its ok


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

per arch subdirs in libavcodec still are closer than libavutil


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

its exact upto 1<<24 at least


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

i dont know, but i agree that its problematic as it is

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081128/c9a40e3a/attachment.pgp>