Re: [RFC PATCH 06/11] videodev2.h: add new v4l2_ext_control flags field
Hi Hans,
On Mon, Nov 17, 2014 at 09:57:24AM +0100, Hans Verkuil wrote:
> On 11/15/2014 06:44 PM, Sakari Ailus wrote:
> > Hi,
> >
> > On Sat, Nov 15, 2014 at 04:18:59PM +0200, Sakari Ailus wrote:
> > ...
> >>> union {
> >>> __s32 value;
> >>> __s64 value64;
> >>> @@ -1294,6 +1294,10 @@ struct v4l2_ext_control {
> >>> };
> >>> } __attribute__ ((packed));
> >>>
> >>> +/* v4l2_ext_control flags */
> >>> +#define V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE 0x0001
> >>> +#define V4L2_EXT_CTRL_FL_IGN_STORE 0x0002
> >>
> >> Do we need both? Aren't these mutually exclusive, and you must have either
> >> to be meaningful in the context of a store?
> >
> > Ah. Now I think I understand what do these mean. Please ignore my previous
> > comment.
> >
> > I might call them differently. What would you think of
>
> I was never happy with the naming :-)
:-)
> > V4L2_EXT_CTRL_FL_STORE_IGNORE and V4L2_EXT_CTRL_FL_STORE_ONCE?
>
> I will give this some more thought.
>
> > V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE is quite long IMO. Up to you.
> >
> > I wonder if we need EXT in V4L2_EXT_CTRL_FL. It's logical but also
> > redundant since the old control interface won't have flags either.
>
> True.
I think I'm inclined to keep EXT there. These values aren't used in that
many places in typical programs.
> > I'd assume that for cameras the vast majority of users will always want to
> > just apply the values once. How are the use cases in video decoding?
>
> I am wondering whether 'apply once' shouldn't be the default and whether I
> really need to implement the 'apply always' (Hey, not bad names either!)
> functionality for this initial version.
After thinking more about it, I'm still leaning towards making the values
stick to a store by default. Forgetting the values after use is something on
top of the basic behaviour. Just my 5 euro cents. Pawel, others?
It could be nice to be able to forget an entire store. An application might
fill it, but only later figure out it will never be needed.
Do you think this could be a button control? :-) No need for this now,
though, we could see when someone needs that.
> I only used the 'apply always' functionality for a somewhat contrived test
> example where I changed the cropping rectangle (this is with the selection
> controls from patch 10/11) for each buffer so that while streaming I would
> get a continuous zoom-in/zoom-out effect. While nice for testing, it isn't
> really practical in reality.
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 06/11] videodev2.h: add new v4l2_ext_control flags field
On 11/15/2014 06:44 PM, Sakari Ailus wrote:
> Hi,
>
> On Sat, Nov 15, 2014 at 04:18:59PM +0200, Sakari Ailus wrote:
> ...
>>> union {
>>> __s32 value;
>>> __s64 value64;
>>> @@ -1294,6 +1294,10 @@ struct v4l2_ext_control {
>>> };
>>> } __attribute__ ((packed));
>>>
>>> +/* v4l2_ext_control flags */
>>> +#define V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE 0x0001
>>> +#define V4L2_EXT_CTRL_FL_IGN_STORE 0x0002
>>
>> Do we need both? Aren't these mutually exclusive, and you must have either
>> to be meaningful in the context of a store?
>
> Ah. Now I think I understand what do these mean. Please ignore my previous
> comment.
>
> I might call them differently. What would you think of
I was never happy with the naming :-)
> V4L2_EXT_CTRL_FL_STORE_IGNORE and V4L2_EXT_CTRL_FL_STORE_ONCE?
I will give this some more thought.
> V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE is quite long IMO. Up to you.
>
> I wonder if we need EXT in V4L2_EXT_CTRL_FL. It's logical but also
> redundant since the old control interface won't have flags either.
True.
> I'd assume that for cameras the vast majority of users will always want to
> just apply the values once. How are the use cases in video decoding?
I am wondering whether 'apply once' shouldn't be the default and whether I
really need to implement the 'apply always' (Hey, not bad names either!)
functionality for this initial version.
I only used the 'apply always' functionality for a somewhat contrived test
example where I changed the cropping rectangle (this is with the selection
controls from patch 10/11) for each buffer so that while streaming I would
get a continuous zoom-in/zoom-out effect. While nice for testing, it isn't
really practical in reality.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 06/11] videodev2.h: add new v4l2_ext_control flags field
On 11/15/2014 03:18 PM, Sakari Ailus wrote:
> Hi Hans,
>
> On Sun, Sep 21, 2014 at 04:48:24PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil
>>
>> Replace reserved2 by a flags field. This is used to tell whether
>> setting a new store value is applied only once or every time that
>> v4l2_ctrl_apply_store() is called for that store.
>>
>> Signed-off-by: Hans Verkuil
>> ---
>> include/uapi/linux/videodev2.h | 6 +-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 2ca44ed..fa84070 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -1282,7 +1282,7 @@ struct v4l2_control {
>> struct v4l2_ext_control {
>> __u32 id;
>> __u32 size;
>> -__u32 reserved2[1];
>> +__u32 flags;
>
> 16 bits, please.
Good idea.
> The pad number (for sub-devices) would need to be added
> here as well,
Why? We never needed that for subdevs in the past. Not that I am against
reserving space for it, I'm just wondering if you have something specific
in mind.
> and that's 16 bits. A flag might be needed to tell it's valid,
> too.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 06/11] videodev2.h: add new v4l2_ext_control flags field
Hi,
On Sat, Nov 15, 2014 at 04:18:59PM +0200, Sakari Ailus wrote:
...
> > union {
> > __s32 value;
> > __s64 value64;
> > @@ -1294,6 +1294,10 @@ struct v4l2_ext_control {
> > };
> > } __attribute__ ((packed));
> >
> > +/* v4l2_ext_control flags */
> > +#define V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE 0x0001
> > +#define V4L2_EXT_CTRL_FL_IGN_STORE 0x0002
>
> Do we need both? Aren't these mutually exclusive, and you must have either
> to be meaningful in the context of a store?
Ah. Now I think I understand what do these mean. Please ignore my previous
comment.
I might call them differently. What would you think of
V4L2_EXT_CTRL_FL_STORE_IGNORE and V4L2_EXT_CTRL_FL_STORE_ONCE?
V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE is quite long IMO. Up to you.
I wonder if we need EXT in V4L2_EXT_CTRL_FL. It's logical but also
redundant since the old control interface won't have flags either.
I'd assume that for cameras the vast majority of users will always want to
just apply the values once. How are the use cases in video decoding?
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 06/11] videodev2.h: add new v4l2_ext_control flags field
Hi Hans,
On Sun, Sep 21, 2014 at 04:48:24PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil
>
> Replace reserved2 by a flags field. This is used to tell whether
> setting a new store value is applied only once or every time that
> v4l2_ctrl_apply_store() is called for that store.
>
> Signed-off-by: Hans Verkuil
> ---
> include/uapi/linux/videodev2.h | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 2ca44ed..fa84070 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1282,7 +1282,7 @@ struct v4l2_control {
> struct v4l2_ext_control {
> __u32 id;
> __u32 size;
> - __u32 reserved2[1];
> + __u32 flags;
16 bits, please. The pad number (for sub-devices) would need to be added
here as well, and that's 16 bits. A flag might be needed to tell it's valid,
too.
> union {
> __s32 value;
> __s64 value64;
> @@ -1294,6 +1294,10 @@ struct v4l2_ext_control {
> };
> } __attribute__ ((packed));
>
> +/* v4l2_ext_control flags */
> +#define V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE 0x0001
> +#define V4L2_EXT_CTRL_FL_IGN_STORE 0x0002
Do we need both? Aren't these mutually exclusive, and you must have either
to be meaningful in the context of a store?
> +
> struct v4l2_ext_controls {
> union {
> __u32 ctrl_class;
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 06/11] videodev2.h: add new v4l2_ext_control flags field
From: Hans Verkuil
Replace reserved2 by a flags field. This is used to tell whether
setting a new store value is applied only once or every time that
v4l2_ctrl_apply_store() is called for that store.
Signed-off-by: Hans Verkuil
---
include/uapi/linux/videodev2.h | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 2ca44ed..fa84070 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1282,7 +1282,7 @@ struct v4l2_control {
struct v4l2_ext_control {
__u32 id;
__u32 size;
- __u32 reserved2[1];
+ __u32 flags;
union {
__s32 value;
__s64 value64;
@@ -1294,6 +1294,10 @@ struct v4l2_ext_control {
};
} __attribute__ ((packed));
+/* v4l2_ext_control flags */
+#define V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE 0x0001
+#define V4L2_EXT_CTRL_FL_IGN_STORE 0x0002
+
struct v4l2_ext_controls {
union {
__u32 ctrl_class;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
