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

Michael Niedermayer michaelni at gmx.at
Mon Mar 3 12:20:05 CET 2014


On Sun, Mar 02, 2014 at 09:19:57PM +0100, Lukasz Marek wrote:
> 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?

if its a field in a public struct resizing an array in the middle
of a struct is a problem as it is a change to the ABI


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

theres also the command line interface


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

this was a commen/question for the case of "with a non vector API"
if you have 2 components, its a vector



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

i think that would be very annoying to use from generic code

like
if(type == color){
    array[0].min = value_min;
    array[1].min = extra_value_min1;
    ...
}

IMO either there should be a AVOptionRange with an array in it
or 3 AVOptionRange


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

yes

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- 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/20140303/58719577/attachment.asc>


More information about the ffmpeg-devel mailing list