avoptions: Support getting flag values using av_get_int

Message ID 1305892787-56165-1-git-send-email-martin@martin.st
State Rejected
Headers show

Commit Message

Martin Storsjö May 20, 2011, 11:59 a.m.
---
 libavutil/opt.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Comments

Justin Ruggles May 22, 2011, 4:41 p.m. | #1
On 05/20/2011 07:59 AM, Martin Storsjö wrote:

> ---
>  libavutil/opt.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index 57e3248..9ff0a9a 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -262,7 +262,7 @@ static int av_get_number(void *obj, const char *name, const AVOption **o_out, do
>  {
>      const AVOption *o= av_find_opt(obj, name, NULL, 0, 0);
>      void *dst;
> -    if (!o || o->offset<=0)
> +    if (!o || (o->offset<=0 && o->type != FF_OPT_TYPE_CONST))
>          goto error;
>  
>      dst= ((uint8_t*)obj) + o->offset;
> @@ -278,6 +278,7 @@ static int av_get_number(void *obj, const char *name, const AVOption **o_out, do
>      case FF_OPT_TYPE_RATIONAL:  *intnum= ((AVRational*)dst)->num;
>                                  *den   = ((AVRational*)dst)->den;
>                                                          return 0;
> +    case FF_OPT_TYPE_CONST:     *intnum= o->default_val.dbl;return 0;
>      }
>  error:
>      *den=*intnum=0;


Does this fix something?  If not, does it make some user-level task easier?

-Justin
Martin Storsjö May 22, 2011, 5 p.m. | #2
On Sun, 22 May 2011, Justin Ruggles wrote:

> On 05/20/2011 07:59 AM, Martin Storsjö wrote:
> 
> > ---
> >  libavutil/opt.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/libavutil/opt.c b/libavutil/opt.c
> > index 57e3248..9ff0a9a 100644
> > --- a/libavutil/opt.c
> > +++ b/libavutil/opt.c
> > @@ -262,7 +262,7 @@ static int av_get_number(void *obj, const char *name, const AVOption **o_out, do
> >  {
> >      const AVOption *o= av_find_opt(obj, name, NULL, 0, 0);
> >      void *dst;
> > -    if (!o || o->offset<=0)
> > +    if (!o || (o->offset<=0 && o->type != FF_OPT_TYPE_CONST))
> >          goto error;
> >  
> >      dst= ((uint8_t*)obj) + o->offset;
> > @@ -278,6 +278,7 @@ static int av_get_number(void *obj, const char *name, const AVOption **o_out, do
> >      case FF_OPT_TYPE_RATIONAL:  *intnum= ((AVRational*)dst)->num;
> >                                  *den   = ((AVRational*)dst)->den;
> >                                                          return 0;
> > +    case FF_OPT_TYPE_CONST:     *intnum= o->default_val.dbl;return 0;
> >      }
> >  error:
> >      *den=*intnum=0;
> 
> 
> Does this fix something?  If not, does it make some user-level task easier?

It makes it possible to check whether a particular flag is set, without 
knowing the exact value of the flag (which after this patch could be 
queried with this function). You can set such flags with e.g. 
av_set_string3(ctx, "flags", "flag1+flag2") - but given ctx, you can't 
query for what constant "flag1" corresponds to, making it impossible to 
opaquely inspect whether that flag is set in the flags field.

// Martin
Justin Ruggles May 22, 2011, 5:39 p.m. | #3
On 05/22/2011 01:00 PM, Martin Storsjö wrote:

> On Sun, 22 May 2011, Justin Ruggles wrote:
> 
>> On 05/20/2011 07:59 AM, Martin Storsjö wrote:
>>
>>> ---
>>>  libavutil/opt.c |    3 ++-
>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>> index 57e3248..9ff0a9a 100644
>>> --- a/libavutil/opt.c
>>> +++ b/libavutil/opt.c
>>> @@ -262,7 +262,7 @@ static int av_get_number(void *obj, const char *name, const AVOption **o_out, do
>>>  {
>>>      const AVOption *o= av_find_opt(obj, name, NULL, 0, 0);
>>>      void *dst;
>>> -    if (!o || o->offset<=0)
>>> +    if (!o || (o->offset<=0 && o->type != FF_OPT_TYPE_CONST))
>>>          goto error;
>>>  
>>>      dst= ((uint8_t*)obj) + o->offset;
>>> @@ -278,6 +278,7 @@ static int av_get_number(void *obj, const char *name, const AVOption **o_out, do
>>>      case FF_OPT_TYPE_RATIONAL:  *intnum= ((AVRational*)dst)->num;
>>>                                  *den   = ((AVRational*)dst)->den;
>>>                                                          return 0;
>>> +    case FF_OPT_TYPE_CONST:     *intnum= o->default_val.dbl;return 0;
>>>      }
>>>  error:
>>>      *den=*intnum=0;
>>
>>
>> Does this fix something?  If not, does it make some user-level task easier?
> 
> It makes it possible to check whether a particular flag is set, without 
> knowing the exact value of the flag (which after this patch could be 
> queried with this function). You can set such flags with e.g. 
> av_set_string3(ctx, "flags", "flag1+flag2") - but given ctx, you can't 
> query for what constant "flag1" corresponds to, making it impossible to 
> opaquely inspect whether that flag is set in the flags field.


That makes sense.

One thing I'm worried about is duplicate names. The constant options
don't have to be unique. See "auto" in the libavcodec options for
example. The av_find_opt() call in av_get_number() doesn't specify the
unit parameter, which would distinguish between duplicate names.

-Justin
Martin Storsjö May 22, 2011, 6:04 p.m. | #4
On Sun, 22 May 2011, Justin Ruggles wrote:

> On 05/22/2011 01:00 PM, Martin Storsjö wrote:
> 
> > On Sun, 22 May 2011, Justin Ruggles wrote:
> > 
> >> On 05/20/2011 07:59 AM, Martin Storsjö wrote:
> >>
> >>> ---
> >>>  libavutil/opt.c |    3 ++-
> >>>  1 files changed, 2 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/libavutil/opt.c b/libavutil/opt.c
> >>> index 57e3248..9ff0a9a 100644
> >>> --- a/libavutil/opt.c
> >>> +++ b/libavutil/opt.c
> >>> @@ -262,7 +262,7 @@ static int av_get_number(void *obj, const char *name, const AVOption **o_out, do
> >>>  {
> >>>      const AVOption *o= av_find_opt(obj, name, NULL, 0, 0);
> >>>      void *dst;
> >>> -    if (!o || o->offset<=0)
> >>> +    if (!o || (o->offset<=0 && o->type != FF_OPT_TYPE_CONST))
> >>>          goto error;
> >>>  
> >>>      dst= ((uint8_t*)obj) + o->offset;
> >>> @@ -278,6 +278,7 @@ static int av_get_number(void *obj, const char *name, const AVOption **o_out, do
> >>>      case FF_OPT_TYPE_RATIONAL:  *intnum= ((AVRational*)dst)->num;
> >>>                                  *den   = ((AVRational*)dst)->den;
> >>>                                                          return 0;
> >>> +    case FF_OPT_TYPE_CONST:     *intnum= o->default_val.dbl;return 0;
> >>>      }
> >>>  error:
> >>>      *den=*intnum=0;
> >>
> >>
> >> Does this fix something?  If not, does it make some user-level task easier?
> > 
> > It makes it possible to check whether a particular flag is set, without 
> > knowing the exact value of the flag (which after this patch could be 
> > queried with this function). You can set such flags with e.g. 
> > av_set_string3(ctx, "flags", "flag1+flag2") - but given ctx, you can't 
> > query for what constant "flag1" corresponds to, making it impossible to 
> > opaquely inspect whether that flag is set in the flags field.
> 
> 
> That makes sense.
> 
> One thing I'm worried about is duplicate names. The constant options
> don't have to be unique. See "auto" in the libavcodec options for
> example. The av_find_opt() call in av_get_number() doesn't specify the
> unit parameter, which would distinguish between duplicate names.

Good point. If there's a risk for that case, I guess it's better to use 
av_find_opt straight away instead. (I didn't think of that option 
initially, but that would work without any modifications.)

In any case, do you think this patch is worth applying?

// Martin
Justin Ruggles May 22, 2011, 6:24 p.m. | #5
On 05/22/2011 02:04 PM, Martin Storsjö wrote:

> On Sun, 22 May 2011, Justin Ruggles wrote:
> 
>> On 05/22/2011 01:00 PM, Martin Storsjö wrote:
>>
>>> On Sun, 22 May 2011, Justin Ruggles wrote:
>>>
>>>> On 05/20/2011 07:59 AM, Martin Storsjö wrote:
>>>>
>>>>> ---
>>>>>  libavutil/opt.c |    3 ++-
>>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/libavutil/opt.c b/libavutil/opt.c
>>>>> index 57e3248..9ff0a9a 100644
>>>>> --- a/libavutil/opt.c
>>>>> +++ b/libavutil/opt.c
>>>>> @@ -262,7 +262,7 @@ static int av_get_number(void *obj, const char *name, const AVOption **o_out, do
>>>>>  {
>>>>>      const AVOption *o= av_find_opt(obj, name, NULL, 0, 0);
>>>>>      void *dst;
>>>>> -    if (!o || o->offset<=0)
>>>>> +    if (!o || (o->offset<=0 && o->type != FF_OPT_TYPE_CONST))
>>>>>          goto error;
>>>>>  
>>>>>      dst= ((uint8_t*)obj) + o->offset;
>>>>> @@ -278,6 +278,7 @@ static int av_get_number(void *obj, const char *name, const AVOption **o_out, do
>>>>>      case FF_OPT_TYPE_RATIONAL:  *intnum= ((AVRational*)dst)->num;
>>>>>                                  *den   = ((AVRational*)dst)->den;
>>>>>                                                          return 0;
>>>>> +    case FF_OPT_TYPE_CONST:     *intnum= o->default_val.dbl;return 0;
>>>>>      }
>>>>>  error:
>>>>>      *den=*intnum=0;
>>>>
>>>>
>>>> Does this fix something?  If not, does it make some user-level task easier?
>>>
>>> It makes it possible to check whether a particular flag is set, without 
>>> knowing the exact value of the flag (which after this patch could be 
>>> queried with this function). You can set such flags with e.g. 
>>> av_set_string3(ctx, "flags", "flag1+flag2") - but given ctx, you can't 
>>> query for what constant "flag1" corresponds to, making it impossible to 
>>> opaquely inspect whether that flag is set in the flags field.
>>
>>
>> That makes sense.
>>
>> One thing I'm worried about is duplicate names. The constant options
>> don't have to be unique. See "auto" in the libavcodec options for
>> example. The av_find_opt() call in av_get_number() doesn't specify the
>> unit parameter, which would distinguish between duplicate names.
> 
> Good point. If there's a risk for that case, I guess it's better to use 
> av_find_opt straight away instead. (I didn't think of that option 
> initially, but that would work without any modifications.)
> 
> In any case, do you think this patch is worth applying?


I think having this could be misleading, as it would not work as
expected in all cases.  The user would have to look at the option list
to know whether the constant name is unique to be sure what is being
returned is correct.  So I don't think the patch is worth applying
as-is.  If this functionality is really needed then we could make a new
public function (or set of functions) that takes both the parent option
name and the constant option name.  This would also be more
user-friendly than av_find_opt() with the unit param since the unit
string can be any unique string and doesn't necessarily match the parent
option name.

-Justin
Martin Storsjö May 22, 2011, 6:26 p.m. | #6
On Sun, 22 May 2011, Justin Ruggles wrote:

> I think having this could be misleading, as it would not work as
> expected in all cases.  The user would have to look at the option list
> to know whether the constant name is unique to be sure what is being
> returned is correct.  So I don't think the patch is worth applying
> as-is.  If this functionality is really needed then we could make a new
> public function (or set of functions) that takes both the parent option
> name and the constant option name.  This would also be more
> user-friendly than av_find_opt() with the unit param since the unit
> string can be any unique string and doesn't necessarily match the parent
> option name.

Indeed, makes sense. Patch dropped for now then.

// Martin

Patch

diff --git a/libavutil/opt.c b/libavutil/opt.c
index 57e3248..9ff0a9a 100644
--- a/libavutil/opt.c
+++ b/libavutil/opt.c
@@ -262,7 +262,7 @@  static int av_get_number(void *obj, const char *name, const AVOption **o_out, do
 {
     const AVOption *o= av_find_opt(obj, name, NULL, 0, 0);
     void *dst;
-    if (!o || o->offset<=0)
+    if (!o || (o->offset<=0 && o->type != FF_OPT_TYPE_CONST))
         goto error;
 
     dst= ((uint8_t*)obj) + o->offset;
@@ -278,6 +278,7 @@  static int av_get_number(void *obj, const char *name, const AVOption **o_out, do
     case FF_OPT_TYPE_RATIONAL:  *intnum= ((AVRational*)dst)->num;
                                 *den   = ((AVRational*)dst)->den;
                                                         return 0;
+    case FF_OPT_TYPE_CONST:     *intnum= o->default_val.dbl;return 0;
     }
 error:
     *den=*intnum=0;