[FFmpeg-devel] [PATCH 4/8] lavu/opt: extend AVOptionRange by second value

[Michael Niedermayer]

On Sun, Mar 23, 2014 at 03:52:49PM +0100, Lukasz Marek wrote:
> On 08.03.2014 22:00, Nicolas George wrote:
> >Le tridi 13 ventôse, an CCXXII, Lukasz Marek a écrit :
> >>OK, I tried to make it different, we have following function:
> >>
> >>int av_opt_query_ranges(AVOptionRanges **, void *obj, const char
> >>*key, int flags);
> >>
> >>AVOptionRanges is **. We can return more than one struct without any
> >>changes to API.
> >>
> >>So querying option size will return 2 x AVOptionRanges, querying
> >>color may return 3/4 x AVOptionRanges.
> >
> >I believe this is a very good idea.
> >
> >>AVOptionRanges may be extended by one filed "option_name" for example
> >>
> >>typedef struct AVOptionRanges {
> >>     AVOptionRange **range;
> >>     int nb_ranges;
> >>     char *option_name
> >>} AVOptionRanges;
> >>
> >>So, when querying for example "window_size" it would return one
> >>struct with option_name set to "window_size.width" and struct with
> >>option_name set to "window_size.height".
> >>
> >>in general option_name param would return option name with suffix
> >>that would be defined per option type. all AVoptionRanges will
> >>require to return the same number of AVOptionRange structs.
> >
> >I do not think it is necessary: the name of the option is known by the
> >caller, and the order of the components is determined by the option type.
> >Parsing a string is not very convenient anyway.
> >
> >>av_opt_query_ranges so far returns >=0 for success, it can be
> >>redefined to return number of AVOptionRanges.
> >
> >That seems like an obvious change.
> 
> 
> I attached a patch that implements this.
> The problem I noticed later is how to free it.
> I added nb_components to AVOptionRanges, but this field is
> duplicated for each component which is probably not the best.
> If you have any other solution to solve freeing then give a hint.
> 
> I pushed whole working branch to my repo github.com/lukaszmluki/ffmpeg
> where implementation for opengl is also available and can be tested with
> tools/device_capabilities
> 
> 

>  opt.c |   33 ++++++++++++++++++++++++---------
>  opt.h |   13 +++++++++++--
>  2 files changed, 35 insertions(+), 11 deletions(-)
> e560f163c6f61b5819e9f382615877e0bdfe12fe  0001-lavu-opt-extend-AVOptionRange-by-extra-values.patch
> From 702508c0c97812ed72deaa133140291fe3637196 Mon Sep 17 00:00:00 2001
> From: Lukasz Marek <lukasz.m.luki at gmail.com>
> Date: Sat, 22 Feb 2014 23:32:57 +0100
> Subject: [PATCH] lavu/opt: extend AVOptionRange by extra values
> 
> TODO: micro bump
> 
> AVOptionRange is not flexible enough to store AV_OPT_TYPE_IMAGE_SIZE
> ranges. Current implementation can only store pixel count.
> This patch aims to keep backward compatibility and extend
> AVOptionRange with possibility to store width/height ranges.
> 
> Signed-off-by: Lukasz Marek <lukasz.m.luki at gmail.com>
> ---
>  libavutil/opt.c | 33 ++++++++++++++++++++++++---------
>  libavutil/opt.h | 13 +++++++++++--
>  2 files changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/libavutil/opt.c b/libavutil/opt.c
> index 652a2dd..77d20b9 100644
> --- a/libavutil/opt.c
> +++ b/libavutil/opt.c
> @@ -1511,6 +1511,7 @@ void *av_opt_ptr(const AVClass *class, void *obj, const char *name)
>  
>  int av_opt_query_ranges(AVOptionRanges **ranges_arg, void *obj, const char *key, int flags)
>  {
> +    int ret, i;
>      const AVClass *c = *(AVClass**)obj;
>      int (*callback)(AVOptionRanges **, void *obj, const char *key, int flags) = NULL;
>  
> @@ -1520,7 +1521,14 @@ int av_opt_query_ranges(AVOptionRanges **ranges_arg, void *obj, const char *key,
>      if (!callback)
>          callback = av_opt_query_ranges_default;
>  
> -    return callback(ranges_arg, obj, key, flags);
> +    ret = callback(ranges_arg, obj, key, flags);
> +    if (ret >= 0) {
> +        if (!(flags & AV_OPT_MULTI_COMPINENT_RANGE))
> +            ret = 1;

> +        for (i = 0; i < ret; i++)
> +            (*ranges_arg)[i].nb_components = ret;

doesnt this  depends on sizeof(AVOptionRanges) ?
sizeof(AVOptionRanges) is not and cannot be part of the public ABI
so iam not sure how this can be accessed by a user


[...]

> @@ -559,6 +560,12 @@ int av_opt_eval_q     (void *obj, const AVOption *o, const char *val, AVRational
>  #define AV_OPT_SEARCH_FAKE_OBJ   0x0002
>  
>  /**
> + *  Allows av_opt_query_ranges and av_opt_query_ranges_default to return more than
> + *  one instance of AVOptionRanges.
> + */
> +#define AV_OPT_MULTI_COMPINENT_RANGE 0x0004

what is a compInent ?
also how and where were the ranges defined for multi component options
before?
If there where not defined at all then a change is possibly not a
API/ABI break and this can be simplified away

also what do multiple component ranges mean exactly?
range1: 320,640
range2: 240,480

means 320x240 || 640x480
or
means 320x240 || 320x480 || 640x240 || 640x480
(this should be documented, and one of these looks insufficient)

also you can put any N-Dimensional array in a 1d array like:
ranges[component*nb_ranges + range] or
ranges[componen + range*nb_components]


also the patch needs a minor not micro bump as applications that
use the new API will not work with old lavu

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140329/3a00e5cb/attachment.asc>