avoptions: Return NaN in av_get_double if the option wasn't found

Message ID 1306139920-26396-1-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö May 23, 2011, 8:38 a.m.
For fractions and doubles, returning 0.0/0.0 was intentional,
in order to return NaN - the division should be ok since
it's done in float. NaN is better than -1 for indicating a
value not found.

Keeping the return value check for av_get_int.

This partially reverts commit
8089b7fa8c5b5a48cc7101daa4be891d0ead5a5e,
"avoptions: Check the return value from av_get_number".
---
Sorry for the code churn - I realized that the original code
wasn't that bad for the cases other than av_get_int.

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

Comments

Justin Ruggles May 23, 2011, 2:44 p.m. | #1
On 05/23/2011 04:38 AM, Martin Storsjö wrote:

> For fractions and doubles, returning 0.0/0.0 was intentional,
> in order to return NaN - the division should be ok since
> it's done in float. NaN is better than -1 for indicating a
> value not found.
> 
> Keeping the return value check for av_get_int.

Somehow returning both an error code and setting the output values to a
separate error value seems wrong to me, especially if they're used
differently by different calling functions.  What seems simplest to me
would be:

1) do not modify num/den on error in av_get_number()
2) check for -1 in the calling functions
3) return approprate value for the calling function if -1 was returned
by av_get_number(), be it -1, 0/0, NaN or whatever.

-Justin
Stefano Sabatini May 23, 2011, 4:38 p.m. | #2
On date Monday 2011-05-23 10:44:02 -0400, Justin Ruggles encoded:
> On 05/23/2011 04:38 AM, Martin Storsjö wrote:
> 
> > For fractions and doubles, returning 0.0/0.0 was intentional,
> > in order to return NaN - the division should be ok since
> > it's done in float. NaN is better than -1 for indicating a
> > value not found.
> > 
> > Keeping the return value check for av_get_int.
> 
> Somehow returning both an error code and setting the output values to a
> separate error value seems wrong to me, especially if they're used
> differently by different calling functions.  What seems simplest to me
> would be:
> 
> 1) do not modify num/den on error in av_get_number()
> 2) check for -1 in the calling functions
> 3) return approprate value for the calling function if -1 was returned
> by av_get_number(), be it -1, 0/0, NaN or whatever.

Alternatively I was thinking about something of the kind:
int av_opt_get_double(void *ctx, double *res, const char *name, const AVOption **o_out);

which returns an error code, it may be AVERROR_OPTION_NOT_FOUND in
case no option was found, maybe overkill since we can get only one
error code most of the times, *and* o_out may be used to check if the
option was found.
Justin Ruggles May 23, 2011, 5:07 p.m. | #3
On 05/23/2011 12:38 PM, Stefano Sabatini wrote:

> On date Monday 2011-05-23 10:44:02 -0400, Justin Ruggles encoded:
>> On 05/23/2011 04:38 AM, Martin Storsjö wrote:
>>
>>> For fractions and doubles, returning 0.0/0.0 was intentional,
>>> in order to return NaN - the division should be ok since
>>> it's done in float. NaN is better than -1 for indicating a
>>> value not found.
>>>
>>> Keeping the return value check for av_get_int.
>>
>> Somehow returning both an error code and setting the output values to a
>> separate error value seems wrong to me, especially if they're used
>> differently by different calling functions.  What seems simplest to me
>> would be:
>>
>> 1) do not modify num/den on error in av_get_number()
>> 2) check for -1 in the calling functions
>> 3) return approprate value for the calling function if -1 was returned
>> by av_get_number(), be it -1, 0/0, NaN or whatever.
> 
> Alternatively I was thinking about something of the kind:
> int av_opt_get_double(void *ctx, double *res, const char *name, const AVOption **o_out);
> 
> which returns an error code, it may be AVERROR_OPTION_NOT_FOUND in
> case no option was found, maybe overkill since we can get only one
> error code most of the times, *and* o_out may be used to check if the
> option was found.


Yes, that would be good.  It would at least be clear that an error
occurred.  For av_get_int(), -1 is a valid number too...

-Justin

Patch

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