[FFmpeg-devel] [PATCH] Document av_gcd()

[Stefano Sabatini]

On date Sunday 2009-03-01 19:46:40 +0100, Reimar D?ffinger encoded:
> On Sun, Mar 01, 2009 at 07:34:31PM +0100, Stefano Sabatini wrote:
> > On date Saturday 2009-02-28 18:49:27 +0100, Reimar D?ffinger encoded:
> > > On Sat, Feb 28, 2009 at 06:23:37PM +0100, Michael Niedermayer wrote:
> > > > On Sat, Feb 28, 2009 at 04:53:08PM +0100, Stefano Sabatini wrote:
> > > > > Hi,
> > > > > as in subject, regards.
> > > > > -- 
> > > > > FFmpeg = Funny and Forgiving Multimedia Pitiful Elected Ghost
> > > > 
> > > > > Index: libavutil/mathematics.h
> > > > > ===================================================================
> > > > > --- libavutil/mathematics.h	(revision 17629)
> > > > > +++ libavutil/mathematics.h	(working copy)
> > > > > @@ -50,6 +50,9 @@
> > > > >      AV_ROUND_NEAR_INF = 5, ///< Round to nearest and halfway cases away from zero.
> > > > >  };
> > > > >  
> > > > > +/**
> > > > > + * Returns the greatest common divisor of a and b.
> > > > > + */
> > > > >  int64_t av_const av_gcd(int64_t a, int64_t b);
> > > > >  
> > > > >  /**
> > > > 
> > > > ok,also please add
> > > > "if either or both are <=0 then the behavior is undefined"
> > > 
> > > My r_frame_rate code in utils.c uses the = 0 case already, since I
> > > considered that the common mathematical definition...
> > 
> > Trying to find a compromise, I think documenting the case gcd(a, 0) = a
> > doesn't hurt.
> 
> The you'd have to flip the usage in utils.c though, it uses
> av_gcd(0, a) (I considered speed irrelevant there).

Yes, but it's not always a speed concern (av_gcd(0, N) requires two
recursive calls), according to ISO/IEC 9899:1999:

5 The result of the / operator is the quotient from the division of the first operand by the
  second; the result of the % operator is the remainder. In both operations, if the value of
  the second operand is zero, the behavior is undefined.

That means that currently the behavior of av_gcd(0, N) is undefined.

> Also just documenting that there are internal uses that use the =0 case
> in the .c file would be fine by me...

Uhm, by the =0 case do you mean av_gcd(0, 0) = 0?

This is undefined behaviour according to the current definition of
av_gcd(), so we have different options:

1) extend av_gcd and make it support explicitely that case.
   This is problematic since mathematically-wise there is not an
   univoque definition of the gcd operator in that case.
   Also it requires to support that case in every optimization.

2) deal with it in the lib/applications.

3) if we need that extended definition, maybe defining a new av_gcd2()
   or something like that,  but I'm not sure about where is the place
   in the code where you assumed that behaviour.

Regards.
-- 
FFmpeg = Faboulous and Fierce Mysterious Plastic Empowered Game
-------------- next part --------------
A non-text attachment was scrubbed...
Name: flip-undefined-gcd-use.patch
Type: text/x-diff
Size: 1636 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090301/171931b6/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: document-gcd.patch
Type: text/x-diff
Size: 534 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090301/171931b6/attachment-0001.patch>