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

Lukasz Marek lukasz.m.luki at gmail.com
Sun Mar 2 21:19:57 CET 2014

On 01.03.2014 16:51, Michael Niedermayer wrote:
> On Fri, Feb 28, 2014 at 02:59:41AM +0100, Lukasz Marek wrote:
>> On 26.02.2014 17:22, Michael Niedermayer wrote:
>>> On Wed, Feb 26, 2014 at 01:55:53PM +0100, Lukasz M wrote:
>>>> On 26 February 2014 12:41, Nicolas George <george at nsup.org> wrote:
>>>>> L'octidi 8 ventôse, an CCXXII, Lukasz M a écrit :
>>>>>> 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?
>>>>> Maybe I missed something, but I suspect you should completely ignore the
>>>>> component_min/max part.
>>>> I'm not sure how it is supposed to be extended.
>>>> It was a question to Michael, because it is not clear and both options make
>>>> sense - depending how you interpret them.
>>>> But my first guess was also to ignore component part
>>>>>> 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.
>>>>> Is there really a benefit in having a real array, i.e. being able to handle
>>>>> both values at once? If not, you could just add value2_min/max and be done
>>>>> with it. That has the advantage of not polluting the normal code with
>>>>> "[0]".
>>>> OK, I can change it this way.
>>>>> Also, is it worth the complexity? IIRC, you did not respond to the option
>>>>> of
>>>>> leaving width and height separate, and just set width to get the
>>>>> corresponding heights.
>>>> No, I haven't. Old version would work as you described (query width, set
>>>> width, query height),
>>>> Michael gave a hints about extending AVOptionRange struct and I think it is
>>>> better than previous solution.
>>>>  From client point of view it seems to have less complexity.
>>> yes, i think keeping the client complexity in mind is important as
>>> one developer had complained about the API
>>> the alternative to changing AVOptionRange would possibly be to add
>>> a function which can take 2 or more AVOptions and returns a list of
>>> their allowed values or value ranges
>>> This function would then querry the "One component" style API
>>> to create a list of allowed pairs/pair ranges
>>> That could then also be used to create widthxheight fps triplets
>>> if these depend on each other
>>> also i just realized that AVOptionRange probably should have a
>>> "multiple_of" field so that for example even width can be specified
>>> without having to specify each individual value in a list
>> I promise it is the last one. No more from me for this thread unless
>> you decide what you want :P
> I think a big question is if we want to directly support vectors or
> not.
> Both choices are possible and i dont realy know which is better nor
> do i have a real preferrance
> One question in case of supporting vectors is if a 2 element limit
> will ever be a problem, because its hard to raise later with this

I thought about colors too, but I doubt resizing an array/adding field 
is a real problem?

> also the question of how to handle other combinations matters
> for example widthxheight might depend on fps or pix_fmt
> It may be that the API we would need to conveniently create a
> list (for exampe for presentation to the user) of the supported
> combinations would end up rendering the 2 component vector support
> obsoleet

The idea was to return all possible vales that are not restricted by 
already set values. In example you mentioned unless you set fps or 
pix_fmt it will return all sizes that are remotely possible. When you 
set one of returned sizes then queried list of fps/pix_fmt will be 
limited for these which supports set size.
And vice versa. When you set pix_fmt first then querying size will 
return only these sizes which support set pix_fmt.
I doubt presenting all possible configs to user is a real use case.
I see the gui as list of dropdown lists and when user set value inside 
one of them, then other are reloaded unless already set.

> and for AV_OPT_TYPE_IMAGE_SIZE, with a non vector API how would one
> get the 2nd components range

query size, lets say there is only one valid

value_min/max - range of pixel count - backward compatibility
extra_value_min1/max1 - range of widths
extra_value_min2/max2 - range of heights

> and theres AV_OPT_TYPE_COLOR which is 3 component

for example:
value_min/max - R/Y
extra_value_min1/max1 - G/Cr
extra_value_min2/max2 - B/Cb

question is if alpha should be included. I can add one more.

>> +    double extra_value_min1, extra_value_max1; ///< Dimensions use it to store min width.
>> +    double extra_value_min2, extra_value_max2; ///< Dimensions use it to store max height.
> i think if they are together in the struct, they should be arrays

There are 2 different opinions then. I would prefer array too, but we 
need to keep value_min/max to not break API, and such case i prefer 
opposite. This is my opinion, waiting for comments.

>> +    double multiple_of;                        ///< Valid values have to be multiple of multiple_of. Ignore if multiple_of is 0.
> this can differ for each component of a vector
> yuv422 generally needs to have width to be a multiple of 2 but
> height is possibly odd

Agree, but will move it to separe patch as Nicolas suggested.

Best Regards,
Lukasz Marek

If you can't explain it simply, you don't understand it well enough. - 
Albert Einstein

More information about the ffmpeg-devel mailing list