[FFmpeg-devel] [PATCH 10/10] Document how av_cmp_q() deal when one of the values to be compared is 0/0.

[Stefano Sabatini]

On date Sunday 2010-10-03 23:37:19 +0200, Michael Niedermayer encoded:
> On Sun, Oct 03, 2010 at 11:07:22PM +0200, Stefano Sabatini wrote:
> > On date Saturday 2010-10-02 21:56:49 +0200, Michael Niedermayer encoded:
> > > On Fri, Oct 01, 2010 at 09:16:55AM +0200, Tomas H?rdin wrote:
> > > > On Thu, 2010-09-30 at 23:58 +0200, Michael Niedermayer wrote:
> > > > > On Thu, Sep 30, 2010 at 09:45:41PM +0200, Stefano Sabatini wrote:
> > > > > > ---
> > > > > >  libavutil/rational.h |    4 ++++
> > > > > >  1 files changed, 4 insertions(+), 0 deletions(-)
> > > > > > 
> > > > > > diff --git a/libavutil/rational.h b/libavutil/rational.h
> > > > > > index 2dd0c2c..bd2e711 100644
> > > > > > --- a/libavutil/rational.h
> > > > > > +++ b/libavutil/rational.h
> > > > > > @@ -41,6 +41,10 @@ typedef struct AVRational{
> > > > > >  
> > > > > >  /**
> > > > > >   * Compare two rationals.
> > > > > > + * If one of the compared values is 0/0, the result of the comparison
> > > > > > + * is always 0, so you should do an explicit check for that case
> > > > > > + * rather than use this function.
> > > > > 
> > > > > is there any return value that would in any case avoid an additional check?
> > > > 
> > > > Perhaps AVERROR_INVALIDDATA?
> > > 
> > > I think that if we do detect NAN then the most sensible value to return would
> > > be INT_MIN because INT_MIN == -INT_MIN thus in some sense it is the best
> > > non zero element that has both + and -
> > 
> > Check attached patch. I have a slightly preference for the first patch
> > I sent, as I like the idea of having a special value which results
> > equal to all other rationals (0/0), also I fear that people may do
> > something of the kind:
> > if (av_cmp_q(...) < 0)
> > 
> > in which case returning -INT_MIN with 0/0 will fail. On the other hand
> > I have no strong objection, so if Michael prefer this form I won't
> > object.
> > 
> > Regards.
> > -- 
> > FFmpeg = Freak Fabulous Mean Prodigious Eretic Gigant
> 
> >  rational.h |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 3a4323eb3fa490c69b78a1c662132d36ef75d66e  0004-Add-a-special-case-for-av_cmp_q-when-one-of-the-valu.patch
> > From 9de2a819d33b0a5f35315ac07a2df2b22c6e86fb Mon Sep 17 00:00:00 2001
> > From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> > Date: Thu, 30 Sep 2010 21:42:56 +0200
> > Subject: [PATCH 04/13] Add a special case for av_cmp_q() when one of the values to be
> >  compared is 0/0.
> > 
> > ---
> >  libavutil/rational.h |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/libavutil/rational.h b/libavutil/rational.h
> > index 21542a8..e9933f4 100644
> > --- a/libavutil/rational.h
> > +++ b/libavutil/rational.h
> > @@ -43,9 +43,12 @@ typedef struct AVRational{
> >   * Compare two rationals.
> >   * @param a first rational
> >   * @param b second rational
> > - * @return 0 if a==b, 1 if a>b and -1 if a<b
> > + * @return 0 if a==b, 1 if a>b, -1 if a<b, and INT_MIN if one of the
> > + * values is of the form 0/0
> >   */
> >  static inline int av_cmp_q(AVRational a, AVRational b){
> > +    if (a.num == 0 && a.den == 0 || b.num == 0 && b.den == 0)
> > +        return INT_MIN;
> >      const int64_t tmp= a.num * (int64_t)b.den - b.num * (int64_t)a.den;
> 
> you mix declaration and statement
> i dont care but gcc 2.95 will hate you and maybe some other obscure compilers

Yes I noticed after I sent the patch (and header missing).

Regards.
-- 
FFmpeg = Fantastic and Friendly Most Portable Elected Geisha