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

diff mbox

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(-)

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

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.

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