v4l regression: V4L2_CID_[VH]CENTER disappeared without notice.

v4l regression: V4L2_CID_[VH]CENTER disappeared without notice.

[David Woodhouse]

These were removed in commit 26d507fcfef7f7d0cd2eec874a87169cc121c835:
V4L/DVB (7166): [v4l] Add new user class controls and deprecate others

> -#define V4L2_CID_HCENTER               (V4L2_CID_BASE+22)
> -#define V4L2_CID_VCENTER               (V4L2_CID_BASE+23)
> -#define V4L2_CID_LASTP1                        (V4L2_CID_BASE+24) /*
> last CID + 1 */
> +
> +/* Deprecated, use V4L2_CID_PAN_RESET and V4L2_CID_TILT_RESET */
> +#define V4L2_CID_HCENTER_DEPRECATED    (V4L2_CID_BASE+22)
> +#define V4L2_CID_VCENTER_DEPRECATED    (V4L2_CID_BASE+23)

But there was no warning in Documentation/feature-removal-schedule.txt
and I'm receiving reports that it's breaking userspace apps (the
gstreamer-v4l2 plugin breaks in Fedora rawhide). You can't just pull
things from the published userspace API like that.

Please can we revert the addition of _DEPRECATED to these ioctl
definitions. Perhaps we can add a runtime warning if they actually get
used? Or a compile-time warning if we can manage that?

Signed-off-by: David Woodhouse <dwmw2@infradead.org>

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index c141118..4a535ea 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -865,9 +865,9 @@ struct v4l2_querymenu
 #define V4L2_CID_HFLIP			(V4L2_CID_BASE+20)
 #define V4L2_CID_VFLIP			(V4L2_CID_BASE+21)
 
-/* Deprecated, use V4L2_CID_PAN_RESET and V4L2_CID_TILT_RESET */
-#define V4L2_CID_HCENTER_DEPRECATED	(V4L2_CID_BASE+22)
-#define V4L2_CID_VCENTER_DEPRECATED	(V4L2_CID_BASE+23)
+/* Deprecated; use V4L2_CID_PAN_RESET and V4L2_CID_TILT_RESET */
+#define V4L2_CID_HCENTER		(V4L2_CID_BASE+22)
+#define V4L2_CID_VCENTER		(V4L2_CID_BASE+23)
 
 #define V4L2_CID_POWER_LINE_FREQUENCY	(V4L2_CID_BASE+24)
 enum v4l2_power_line_frequency {


-- 
dwmw2

Re: v4l regression: V4L2_CID_[VH]CENTER disappeared without notice.

[Alan Cox]

> Please can we revert the addition of _DEPRECATED to these ioctl
> definitions. Perhaps we can add a runtime warning if they actually get
> used? Or a compile-time warning if we can manage that?

Can you clarify the problem here ? Is this compile breakage as it appears
(See 'do not include kernel headers in user space apps') or runtime ?

Alan

Re: v4l regression: V4L2_CID_[VH]CENTER disappeared without notice.

[Brandon Philips]

On 16:56 Mon 02 Jun 2008, Alan Cox wrote:
> > Please can we revert the addition of _DEPRECATED to these ioctl
> > definitions. Perhaps we can add a runtime warning if they actually get
> > used? Or a compile-time warning if we can manage that?
> 
> Can you clarify the problem here ? Is this compile breakage as it appears
> (See 'do not include kernel headers in user space apps') or runtime ?

It is compile breakage and was also discussed on
fedora-devel-list@redhat.com with Mauro and myself CC'd.  The control
number will be reserved forever to avoid binary breakage.

However, as Mauro noted in that conversation: "Those controls haven't
been used by any Kernel drivers for a long time (I suspect that they
were never used)."  Since no driver had ever implemented the control I
figured user space application wouldn't be using it either.

If David wants to revert the _DEPRECATED tags that is fine; we can add a
feature removal for a year from now if it gets merged.

Thanks,

	Brandon

Re: v4l regression: V4L2_CID_[VH]CENTER disappeared without notice.

[Mauro Carvalho Chehab]

On Mon, 2 Jun 2008 11:10:35 -0700
Brandon Philips <bphilips@suse.de> wrote:

> On 16:56 Mon 02 Jun 2008, Alan Cox wrote:
> > > Please can we revert the addition of _DEPRECATED to these ioctl
> > > definitions. Perhaps we can add a runtime warning if they actually get
> > > used? Or a compile-time warning if we can manage that?

A runtime warning won't probably work, since the control doesn't appear at the
ioctls that enumerates the supported controls, since no kernel driver uses
those controls.

> > Can you clarify the problem here ? Is this compile breakage as it appears
> > (See 'do not include kernel headers in user space apps') or runtime ?

The breakage appears only if you compile an userspace driver that uses those
defined symbols. No in-kernel driver uses such controls. I'm not sure if there
is any closed source or out-of-tree driver using they.

> It is compile breakage and was also discussed on
> fedora-devel-list@redhat.com with Mauro and myself CC'd.  The control
> number will be reserved forever to avoid binary breakage.
> 
> However, as Mauro noted in that conversation: "Those controls haven't
> been used by any Kernel drivers for a long time (I suspect that they
> were never used)."  Since no driver had ever implemented the control I
> figured user space application wouldn't be using it either.
> 
> If David wants to revert the _DEPRECATED tags that is fine; we can add a
> feature removal for a year from now if it gets merged.

I'm also ok on reverting the _DEPRECATED tags, but I would schedule its removal
at Documentation/feature-removal-schedule.txt to a shorter time. It doesn't
make much sense on keeping those unused controls for more than one kernel
version. So, I think we may schedule such removal for October/2008.

I'll apply your patch on my tree and send Linus on my next pull request.

Cheers,
Mauro

Re: v4l regression: V4L2_CID_[VH]CENTER disappeared without notice.

[David Woodhouse]

On Mon, 2008-06-02 at 16:56 +0100, Alan Cox wrote:
> (See 'do not include kernel headers in user space apps')

We don't just hide behind that statement any more.

If you _mean_ it that <linux/videodev2.h> is not intended for userspace
usage, then submit a patch to remove it from the Kbuild file so that it
no longer gets exported.

Don't forget to document how people are _supposed_ to use the v4l api
from userspace, though.

-- 
dwmw2

Re: v4l regression: V4L2_CID_[VH]CENTER disappeared without notice.

[Alan Cox]

> Don't forget to document how people are _supposed_ to use the v4l api
> from userspace, though.

Correct but those methods are not part of the V4L2 API and have not been
for some considerable time. Removing it from the Kbuild file would simply
break even more stuff. 

Re: v4l regression: V4L2_CID_[VH]CENTER disappeared without notice.

[David Woodhouse]

On Mon, 2008-06-02 at 20:19 +0100, Alan Cox wrote:
> > Don't forget to document how people are _supposed_ to use the v4l api
> > from userspace, though.
> 
> Correct but those methods are not part of the V4L2 API and have not been
> for some considerable time. 

It seems that message hadn't got through. Any suggestions as to how we
could make sure it does in future?

The usual answers are (in order of preference):
 1) don't remove userspace APIs
 2) don't remove userspace APIs
 3) Documentation/feature-removal-schedule.txt
 4) don't remove userspace APIs

It would be good if we could combine #3 with some form of
__deprecated... can we make that work for ioctls in userspace?

-- 
dwmw2

Re: v4l regression: V4L2_CID_[VH]CENTER disappeared without notice.

[Alan Cox]

On Mon, 02 Jun 2008 20:59:38 +0100
David Woodhouse <dwmw2@infradead.org> wrote:

> On Mon, 2008-06-02 at 20:19 +0100, Alan Cox wrote:
> > > Don't forget to document how people are _supposed_ to use the v4l api
> > > from userspace, though.
> > 
> > Correct but those methods are not part of the V4L2 API and have not been
> > for some considerable time. 
> 
> It seems that message hadn't got through. Any suggestions as to how we
> could make sure it does in future?

Removing it appears to be the only effective message. No real news there.

Alan

Re: v4l regression: V4L2_CID_[VH]CENTER disappeared without notice.

[Mauro Carvalho Chehab]

On Mon, 02 Jun 2008 20:59:38 +0100
David Woodhouse <dwmw2@infradead.org> wrote:

> On Mon, 2008-06-02 at 20:19 +0100, Alan Cox wrote:
> > > Don't forget to document how people are _supposed_ to use the v4l api
> > > from userspace, though.
> > 
> > Correct but those methods are not part of the V4L2 API and have not been
> > for some considerable time. 
> 
> It seems that message hadn't got through. Any suggestions as to how we
> could make sure it does in future?
> 
> The usual answers are (in order of preference):
>  1) don't remove userspace APIs
>  2) don't remove userspace APIs
>  3) Documentation/feature-removal-schedule.txt
>  4) don't remove userspace APIs
> 
> It would be good if we could combine #3 with some form of
> __deprecated... can we make that work for ioctls in userspace?

David, 

V4L2_CID_[VH]CENTER are not ioctls.

They are just magic id's, to uniquely identify a parameter that needs to be
controlled by userspace. There are magic numbers for volume, hue, contrast, etc.

The V4L API has two ioctl's that lists what magic numbers exist at a given driver
and helps the userspace app to build an input entry for that parameter. 

On a very few cases, the userspace app might need to use the symbol aliases.
That's why those symbols are at videodev2.h. For example, the volume ID is
somewhat interesting for an userspace app to know, since it can associate the IR
volume UP/Down keys to control the board volume.

On most cases, userspace will just call VIDIOC_QUERYCTRL ioctl, passing an index,
starting on 0, until it receives an -EINVAL. If the ioctl returns 0, the
userspace will have the magic number, a string with the control name,
its minimum/maximum value, its type (integer/boolean), and its default value
and step, and will dynamically construct a table of controls.

In the case of V4L2_CID_[VH]CENTER those magic numbers were intended to control
X and Y positions, but were never used, in fact.

So, it was a complete surprise to me that an userspace API wants to do a
special treatment to an id that weren't used (since no kernel driver will
enumerate  V4L2_CID_[VH]CENTER).

So, I don't think that a __deprecated macro for ioctls should deal with those
stuff.

Cheers,
Mauro