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

[Lukasz M]

On 26 February 2014 04:10, Michael Niedermayer <michaelni at gmx.at> wrote:

> On Tue, Feb 25, 2014 at 02:54:18AM +0100, Lukasz Marek wrote:
> > On 24.02.2014 12:52, wm4 wrote:
> > >On Sat, 22 Feb 2014 23:33:38 +0100
> > >Lukasz Marek <lukasz.m.luki at gmail.com> wrote:
> > >
> > >
> > >>  typedef struct AVOptionRange {
> > >>      const char *str;
> > >>-    double value_min, value_max;             ///< For string ranges
> this represents the min/max length, for dimensions this represents the
> min/max pixel count
> > >>-    double component_min, component_max;     ///< For string this
> represents the unicode range for chars, 0-127 limits to ASCII
> > >>-    int is_range;                            ///< if set to 1 the
> struct encodes a range, if set to 0 a single value
> > >>+    /**
> > >>+     * AV_OPT_TYPE_IMAGE_SIZE options use both elements to represent
> width and height ranges.
> > >>+     * Other options uses only first elements.
> > >>+     * For string ranges first elements represent the min/max length.
> > >>+     */
> > >>+    double value_min[2], value_max[2];
> > >>+    /**
> > >>+     * For string this represents the unicode range for chars, 0-127
> limits to ASCII
> > >>+     */
> > >>+    double component_min, component_max;
> > >>+    /**
> > >>+     * If set to 1 the struct encodes a range, if set to 0 a single
> value
> > >>+     */
> > >>+    int is_range;
> > >>  } AVOptionRange;
> > >
> > >I don't think you can do that without a major API bump. At least
> > >nothing suggests that this struct is not public.
> >
> > Yes, thanks I got suggested by Micheal's suggestion and didn't think
> > about it. I added new fields at the end of the struct.
> >
> > I will send updated patches 5-8 later. But I would be glad to not
> > postpone merging 1-4 more than needed.
> >
> > --
> > Best Regards,
> > Lukasz Marek
> >
> > I may be drunk, Miss, but in the morning I will be sober and you
> > will still be ugly. - Winston Churchill
>
> >  opt.h |    1 +
> >  1 file changed, 1 insertion(+)
> > 128063d585b6006822a70b3449f72440c6e018f0
>  0004-lavu-opt-extend-AVOptionRange-by-extra-values.patch
> > From 4a2b83198d1e79e99e8b5f40e84a9c16025bbab2 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 4/4] lavu/opt: extend AVOptionRange by extra values
> >
> > TODO: micro bump?
>
> should be minor but we could delay that if its considered unstable
> and still maybe might change
>
>
> >
> > 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.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/libavutil/opt.h b/libavutil/opt.h
> > index cd1b18e..5935e2f 100644
> > --- a/libavutil/opt.h
> > +++ b/libavutil/opt.h
> > @@ -316,6 +316,7 @@ typedef struct AVOptionRange {
> >      double value_min, value_max;             ///< For string ranges
> this represents the min/max length, for dimensions this represents the
> min/max pixel count
> >      double component_min, component_max;     ///< For string this
> represents the unicode range for chars, 0-127 limits to ASCII
> >      int is_range;                            ///< if set to 1 the
> struct encodes a range, if set to 0 a single value
> > +    double extra_value_min[2], extra_value_max[2]; ///< Dimensions use
> it to store min/max width and height.
>
> this naming contradicts the existing value and component (should
> be component i think)
>

Hmm, I treated it a bit different, but I think your proposition is correct.
So for example for size opt value would be number of pixels. components
would define ranges of width and height?


> also does any code use AVOptionRange ?
>

it is used only in opt.c, but av_opt_query_ranges is public so it
potentially may be used by users in their apps.


> and if a compatibility layer is added whats the plan for the future
> iam not sure if keeping both is a good idea and
> extra_value_min seems a suboptimal name if the old is eventually
> droped
>

I share your opinion. But technically this is api break. At major bump
extra_value_min can be removed and value_min/max can be array as in my
first patch.
On the other hand I believe in practice it would be better to change it
now. I doubt query_ranges is commonly used so far outside ffmpeg. When dev
cap api starts relying on it, users may start to use it more.