avoptions: Return explicitly NAN or {0, 0} if the option isn't found

Message ID 1306331690-8473-1-git-send-email-martin@martin.st
State Committed
Commit 80068da3a0bafc7e24ce6b1f91cefec7d793b4d2
Headers show

Commit Message

Martin Storsjö May 25, 2011, 1:54 p.m.
This actually matches what av_get_double did earlier, the
0.0/0.0 division was intentional, for producing NAN.

Still keeping the check for the return value from
av_get_number, for clarity.
---
Implemented part of the suggestion now - the setting
of num/den to 0 could be removed from the error case
later.

 libavutil/opt.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Comments

Justin Ruggles May 25, 2011, 6:01 p.m. | #1
On 05/25/2011 09:54 AM, Martin Storsjö wrote:

> This actually matches what av_get_double did earlier, the
> 0.0/0.0 division was intentional, for producing NAN.
> 
> Still keeping the check for the return value from
> av_get_number, for clarity.
> ---
> Implemented part of the suggestion now - the setting
> of num/den to 0 could be removed from the error case
> later.
> 
>  libavutil/opt.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index 9e06b01..74c39fe 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -291,7 +291,7 @@ double av_get_double(void *obj, const char *name, const AVOption **o_out)
>      int den=1;
>  
>      if (av_get_number(obj, name, o_out, &num, &den, &intnum) < 0)
> -        return -1;
> +        return NAN;
>      return num*intnum/den;
>  }
>  
> @@ -302,7 +302,7 @@ AVRational av_get_q(void *obj, const char *name, const AVOption **o_out)
>      int den=1;
>  
>      if (av_get_number(obj, name, o_out, &num, &den, &intnum) < 0)
> -        return (AVRational){-1, 0};
> +        return (AVRational){0, 0};
>      if (num == 1.0 && (int)intnum == intnum)
>          return (AVRational){intnum, den};
>      else


patch ok. I do like Stefano's idea of having an output parameter so the
error code can be passed on, but this patch is at least better that the
current code.

Thanks,
Justin
Martin Storsjö May 25, 2011, 7:04 p.m. | #2
On Wed, 25 May 2011, Justin Ruggles wrote:

> On 05/25/2011 09:54 AM, Martin Storsjö wrote:
> 
> > This actually matches what av_get_double did earlier, the
> > 0.0/0.0 division was intentional, for producing NAN.
> > 
> > Still keeping the check for the return value from
> > av_get_number, for clarity.
> > ---
> > Implemented part of the suggestion now - the setting
> > of num/den to 0 could be removed from the error case
> > later.
> > 
> >  libavutil/opt.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavutil/opt.c b/libavutil/opt.c
> > index 9e06b01..74c39fe 100644
> > --- a/libavutil/opt.c
> > +++ b/libavutil/opt.c
> > @@ -291,7 +291,7 @@ double av_get_double(void *obj, const char *name, const AVOption **o_out)
> >      int den=1;
> >  
> >      if (av_get_number(obj, name, o_out, &num, &den, &intnum) < 0)
> > -        return -1;
> > +        return NAN;
> >      return num*intnum/den;
> >  }
> >  
> > @@ -302,7 +302,7 @@ AVRational av_get_q(void *obj, const char *name, const AVOption **o_out)
> >      int den=1;
> >  
> >      if (av_get_number(obj, name, o_out, &num, &den, &intnum) < 0)
> > -        return (AVRational){-1, 0};
> > +        return (AVRational){0, 0};
> >      if (num == 1.0 && (int)intnum == intnum)
> >          return (AVRational){intnum, den};
> >      else
> 
> 
> patch ok. I do like Stefano's idea of having an output parameter so the
> error code can be passed on, but this patch is at least better that the
> current code.

Pushed. Changing that would break public API, so that's a slightly bigger 
change...

// Martin
Stefano Sabatini May 26, 2011, 9:19 a.m. | #3
On date Wednesday 2011-05-25 22:04:45 +0300, Martin Storsjö encoded:
> On Wed, 25 May 2011, Justin Ruggles wrote:
[...]
> > patch ok. I do like Stefano's idea of having an output parameter so the
> > error code can be passed on, but this patch is at least better that the
> > current code.
> 
> Pushed. Changing that would break public API, so that's a slightly bigger 
> change...

No that would employ the usual replace+deprecate mechanism with no API
breaks, I plan to send a patchset to ffmpeg-devel, I'll cross-post it
here.
Martin Storsjö May 26, 2011, 9:31 a.m. | #4
On Thu, 26 May 2011, Stefano Sabatini wrote:

> On date Wednesday 2011-05-25 22:04:45 +0300, Martin Storsjö encoded:
> > On Wed, 25 May 2011, Justin Ruggles wrote:
> [...]
> > > patch ok. I do like Stefano's idea of having an output parameter so the
> > > error code can be passed on, but this patch is at least better that the
> > > current code.
> > 
> > Pushed. Changing that would break public API, so that's a slightly bigger 
> > change...
> 
> No that would employ the usual replace+deprecate mechanism with no API
> breaks, I plan to send a patchset to ffmpeg-devel, I'll cross-post it
> here.

Yes, of course, if we add new functions, it's doable...

// Martin

Patch

diff --git a/libavutil/opt.c b/libavutil/opt.c
index 9e06b01..74c39fe 100644
--- a/libavutil/opt.c
+++ b/libavutil/opt.c
@@ -291,7 +291,7 @@  double av_get_double(void *obj, const char *name, const AVOption **o_out)
     int den=1;
 
     if (av_get_number(obj, name, o_out, &num, &den, &intnum) < 0)
-        return -1;
+        return NAN;
     return num*intnum/den;
 }
 
@@ -302,7 +302,7 @@  AVRational av_get_q(void *obj, const char *name, const AVOption **o_out)
     int den=1;
 
     if (av_get_number(obj, name, o_out, &num, &den, &intnum) < 0)
-        return (AVRational){-1, 0};
+        return (AVRational){0, 0};
     if (num == 1.0 && (int)intnum == intnum)
         return (AVRational){intnum, den};
     else