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

[Lukasz Marek]

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
> API

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