Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units
Hi Sakari,
On Fri, Sep 28, 2018 at 11:00 PM Hans Verkuil wrote:
>
> On 09/25/2018 12:14 PM, Sakari Ailus wrote:
> > Add support for exponential bases, prefixes as well as units for V4L2
> > controls. This makes it possible to convey information on the relation
> > between the control value and the hardware feature being controlled.
> >
Sorry for being late to the party.
Thanks for the series. I think it has a potential to be very useful.
Please see my comments below.
> > Signed-off-by: Sakari Ailus
> > ---
> > include/uapi/linux/videodev2.h | 32 +++-
> > 1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index ae083978988f1..23b02f2db85a1 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1652,6 +1652,32 @@ struct v4l2_queryctrl {
> > __u32reserved[2];
> > };
> >
> > +/* V4L2 control exponential bases */
> > +#define V4L2_CTRL_BASE_UNDEFINED 0
> > +#define V4L2_CTRL_BASE_LINEAR1
>
> I'm not really sure you need BASE_LINEAR. That is effectively the same
> as UNDEFINED since what else can you do? It's also weird to have this
> as 'base' if the EXPONENTIAL flag is set.
>
> I don't see why you need the EXPONENTIAL flag at all: if this is non-0,
> then you know the exponential base.
Or vice versa, we could remove UNDEFINED and LINEAR altogether and
have the EXPONENTIAL flag actually signify the presence of a valid
base? Besides that, "linear exponential base" just doesn't sound right
or am I missing some basic maths? ;)
Then we could actually have a LOGARITHMIC flag and it could reuse the
same bases enum.
>
> > +#define V4L2_CTRL_BASE_2 2
> > +#define V4L2_CTRL_BASE_1010
> > +
> > +/* V4L2 control unit prefixes */
> > +#define V4L2_CTRL_PREFIX_NANO-9
> > +#define V4L2_CTRL_PREFIX_MICRO -6
> > +#define V4L2_CTRL_PREFIX_MILLI -3
> > +#define V4L2_CTRL_PREFIX_1 0
>
> I would prefer PREFIX_NONE, since there is no prefix in this case.
>
> I assume this prefix is only valid if the unit is not UNDEFINED and not
> NONE?
>
> Is 'base' also dependent on a valid unit? (it doesn't appear to be)
>
> > +#define V4L2_CTRL_PREFIX_KILO3
> > +#define V4L2_CTRL_PREFIX_MEGA6
> > +#define V4L2_CTRL_PREFIX_GIGA9
> > +
> > +/* V4L2 control units */
> > +#define V4L2_CTRL_UNIT_UNDEFINED 0
> > +#define V4L2_CTRL_UNIT_NONE 1
Hmm, what's the meaning of NONE? How does it differ from UNDEFINED?
> > +#define V4L2_CTRL_UNIT_SECOND2
> > +#define V4L2_CTRL_UNIT_AMPERE3
> > +#define V4L2_CTRL_UNIT_LINE 4
> > +#define V4L2_CTRL_UNIT_PIXEL 5
> > +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC6
> > +#define V4L2_CTRL_UNIT_HZ7
> > +
> > +
> > /* Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls
> > */
> > struct v4l2_query_ext_ctrl {
> > __u32id;
> > @@ -1666,7 +1692,10 @@ struct v4l2_query_ext_ctrl {
> > __u32elems;
> > __u32nr_of_dims;
> > __u32dims[V4L2_CTRL_MAX_DIMS];
> > - __u32reserved[32];
> > + __u8 base;
> > + __s8 prefix;
Should we make those bigger just in case, or leave some reserved
fields around so we can make them bigger when we need it?
Best regards,
Tomasz
Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units
Hi! > Add support for exponential bases, prefixes as well as units for V4L2 > controls. This makes it possible to convey information on the relation > between the control value and the hardware feature being controlled. > > Signed-off-by: Sakari Ailus Sounds good. > +/* V4L2 control exponential bases */ > +#define V4L2_CTRL_BASE_UNDEFINED 0 > +#define V4L2_CTRL_BASE_LINEAR1 > +#define V4L2_CTRL_BASE_2 2 > +#define V4L2_CTRL_BASE_1010 Do we also want to support logarithmic and 1/x? For example focus is usually in diopters and thats 1/meter... > +/* V4L2 control units */ > +#define V4L2_CTRL_UNIT_UNDEFINED 0 > +#define V4L2_CTRL_UNIT_NONE 1 > +#define V4L2_CTRL_UNIT_SECOND2 > +#define V4L2_CTRL_UNIT_AMPERE3 > +#define V4L2_CTRL_UNIT_LINE 4 > +#define V4L2_CTRL_UNIT_PIXEL 5 > +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC6 > +#define V4L2_CTRL_UNIT_HZ7 And Hz is 1/second. Should we also have some support for ISO-sensitivity and f/ aperture numbers? Thanks, Pavel
Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units
On 10/01/2018 11:27 AM, Helmut Grohne wrote: > On Fri, Sep 28, 2018 at 04:00:17PM +0200, Hans Verkuil wrote: >> On 09/25/2018 12:14 PM, Sakari Ailus wrote: >>> +/* V4L2 control unit prefixes */ >>> +#define V4L2_CTRL_PREFIX_NANO -9 >>> +#define V4L2_CTRL_PREFIX_MICRO -6 >>> +#define V4L2_CTRL_PREFIX_MILLI -3 >>> +#define V4L2_CTRL_PREFIX_1 0 >> >> I would prefer PREFIX_NONE, since there is no prefix in this case. >> >> I assume this prefix is only valid if the unit is not UNDEFINED and not >> NONE? > > Why should it? The prefix is concerned with rescaling a value prior to > presenting it to a user. Even a unitless quantity or a value of > undefined unit can be reasonably scaled. Displaying a unit and scaling > look like orthogonal concepts to me. What's the point? If I have a unit-less control with values 1-1000, then what would a prefix 'milli' tell me as a user? Why would 0.001-1 be better compared to 1-1000? Without a unit it is just an integer range and scaling is meaningless. > >> Is 'base' also dependent on a valid unit? (it doesn't appear to be) > > I'd argue it should not depend on a valid unit like the prefix. I think I agree with that, although I am dubious about the value of the base field as I commented on elsewhere. Regards, Hans
Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units
On Fri, Sep 28, 2018 at 04:00:17PM +0200, Hans Verkuil wrote: > On 09/25/2018 12:14 PM, Sakari Ailus wrote: > > +/* V4L2 control unit prefixes */ > > +#define V4L2_CTRL_PREFIX_NANO -9 > > +#define V4L2_CTRL_PREFIX_MICRO -6 > > +#define V4L2_CTRL_PREFIX_MILLI -3 > > +#define V4L2_CTRL_PREFIX_1 0 > > I would prefer PREFIX_NONE, since there is no prefix in this case. > > I assume this prefix is only valid if the unit is not UNDEFINED and not > NONE? Why should it? The prefix is concerned with rescaling a value prior to presenting it to a user. Even a unitless quantity or a value of undefined unit can be reasonably scaled. Displaying a unit and scaling look like orthogonal concepts to me. > Is 'base' also dependent on a valid unit? (it doesn't appear to be) I'd argue it should not depend on a valid unit like the prefix. Helmut
Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units
On 09/25/2018 12:14 PM, Sakari Ailus wrote:
> Add support for exponential bases, prefixes as well as units for V4L2
> controls. This makes it possible to convey information on the relation
> between the control value and the hardware feature being controlled.
>
> Signed-off-by: Sakari Ailus
> ---
> include/uapi/linux/videodev2.h | 32 +++-
> 1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index ae083978988f1..23b02f2db85a1 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1652,6 +1652,32 @@ struct v4l2_queryctrl {
> __u32reserved[2];
> };
>
> +/* V4L2 control exponential bases */
> +#define V4L2_CTRL_BASE_UNDEFINED 0
> +#define V4L2_CTRL_BASE_LINEAR1
I'm not really sure you need BASE_LINEAR. That is effectively the same
as UNDEFINED since what else can you do? It's also weird to have this
as 'base' if the EXPONENTIAL flag is set.
I don't see why you need the EXPONENTIAL flag at all: if this is non-0,
then you know the exponential base.
> +#define V4L2_CTRL_BASE_2 2
> +#define V4L2_CTRL_BASE_1010
> +
> +/* V4L2 control unit prefixes */
> +#define V4L2_CTRL_PREFIX_NANO-9
> +#define V4L2_CTRL_PREFIX_MICRO -6
> +#define V4L2_CTRL_PREFIX_MILLI -3
> +#define V4L2_CTRL_PREFIX_1 0
I would prefer PREFIX_NONE, since there is no prefix in this case.
I assume this prefix is only valid if the unit is not UNDEFINED and not
NONE?
Is 'base' also dependent on a valid unit? (it doesn't appear to be)
> +#define V4L2_CTRL_PREFIX_KILO3
> +#define V4L2_CTRL_PREFIX_MEGA6
> +#define V4L2_CTRL_PREFIX_GIGA9
> +
> +/* V4L2 control units */
> +#define V4L2_CTRL_UNIT_UNDEFINED 0
> +#define V4L2_CTRL_UNIT_NONE 1
> +#define V4L2_CTRL_UNIT_SECOND2
> +#define V4L2_CTRL_UNIT_AMPERE3
> +#define V4L2_CTRL_UNIT_LINE 4
> +#define V4L2_CTRL_UNIT_PIXEL 5
> +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC6
> +#define V4L2_CTRL_UNIT_HZ7
> +
> +
> /* Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */
> struct v4l2_query_ext_ctrl {
> __u32id;
> @@ -1666,7 +1692,10 @@ struct v4l2_query_ext_ctrl {
> __u32elems;
> __u32nr_of_dims;
> __u32dims[V4L2_CTRL_MAX_DIMS];
> - __u32reserved[32];
> + __u8 base;
> + __s8 prefix;
> + __u16unit;
> + __u32reserved[31];
> };
>
> /* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */
> @@ -1692,6 +1721,7 @@ struct v4l2_querymenu {
> #define V4L2_CTRL_FLAG_HAS_PAYLOAD 0x0100
> #define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE 0x0200
> #define V4L2_CTRL_FLAG_MODIFY_LAYOUT 0x0400
> +#define V4L2_CTRL_FLAG_EXPONENTIAL 0x0800
>
> /* Query flags, to be ORed with the control ID */
> #define V4L2_CTRL_FLAG_NEXT_CTRL 0x8000
>
Regards,
Hans
