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

[Michael Niedermayer]

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

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

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101003/20fc0891/attachment.pgp>