[RESEND,1/2] v4l2: modify the webcam video standard handling

Message ID 4A910C42.5000001@freemail.hu (mailing list archive)
State Superseded, archived
Headers

Commit Message

Németh Márton Aug. 23, 2009, 9:30 a.m. UTC
  From: Márton Németh <nm127@freemail.hu>

Change the handling of the case when vdev->tvnorms == 0.

Quote from V4L2 API specification, rev. 0.24 [1]:
> Special rules apply to USB cameras where the notion of video
> standards makes little sense. More generally any capture device,
> output devices accordingly, which is
>
> * incapable of capturing fields or frames at the nominal rate
>   of the video standard, or
> * where timestamps refer to the instant the field or frame was
>   received by the driver, not the capture time, or
> * where sequence numbers refer to the frames received by the
>   driver, not the captured frames.
>
> Here the driver shall set the std field of struct v4l2_input
> and struct v4l2_output to zero, the VIDIOC_G_STD, VIDIOC_S_STD,
> VIDIOC_QUERYSTD and VIDIOC_ENUMSTD ioctls shall return the
> EINVAL error code.

The changeset was tested together with v4l-test 0.19 [2] with
gspca_sunplus driver together with Trust 610 LCD POWERC@M ZOOM and
with gspca_pac7311 together with Labtec Webcam 2200.

References:
[1] V4L2 API specification, revision 0.24
    http://v4l2spec.bytesex.org/spec/x448.htm

[2] v4l-test: Test environment for Video For Linux Two API
    http://v4l-test.sourceforge.net/

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Comments

Mauro Carvalho Chehab Aug. 31, 2009, 2:41 a.m. UTC | #1
Hi Németh,

Em Sun, 23 Aug 2009 11:30:42 +0200
Németh Márton <nm127@freemail.hu> escreveu:

> From: Márton Németh <nm127@freemail.hu>
> 
> Change the handling of the case when vdev->tvnorms == 0.
> 

This patch (together with a few others related to tvnorms and camera drivers)
reopens an old discussion: should webcams report a tvnorm?

There's no easy answer for it since:

1) removing support for VIDIOC_G_STD/VIDIOC_S_STD causes regressions, since
some userspace apps stops working;

2) It is a common scenario to use cameras connected to some capture only devices
like several bttv boards used on surveillance systems. Those drivers report STD,
since they are used also on TV;

3) There are even some devices that allows cameras to be connected to one input and
TV on another input. This is another case were the driver will report a TV std;

4) Most webcam formats are based on ITU-T formats designed to be compatible
with TV (formats like CIF and like 640x480 - and their multiple/sub-multiples);

5) There are formats that weren't originated from TV on some digital webcams,
so, for those formats, it makes no sense to report an existing std.

Once people proposed to create an special format for those cases
(V4L2_STD_DIGITAL or something like that), but, after lots of discussions,
no changes were done at API nor at the drivers.

While we don't have an agreement on this, I don't think we should apply a patch
like this.



Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Németh Márton Aug. 31, 2009, 5:55 a.m. UTC | #2
Hi,

first of all thank you for your detailed answer.

Mauro Carvalho Chehab wrote:
> Hi Németh,
> 
> Em Sun, 23 Aug 2009 11:30:42 +0200
> Németh Márton <nm127@freemail.hu> escreveu:
> 
>> From: Márton Németh <nm127@freemail.hu>
>>
>> Change the handling of the case when vdev->tvnorms == 0.
>>
> 
> This patch (together with a few others related to tvnorms and camera drivers)
> reopens an old discussion: should webcams report a tvnorm?
> 
> There's no easy answer for it since:
> 
> 1) removing support for VIDIOC_G_STD/VIDIOC_S_STD causes regressions, since
> some userspace apps stops working;
> 
> 2) It is a common scenario to use cameras connected to some capture only devices
> like several bttv boards used on surveillance systems. Those drivers report STD,
> since they are used also on TV;
> 
> 3) There are even some devices that allows cameras to be connected to one input and
> TV on another input. This is another case were the driver will report a TV std;
> 
> 4) Most webcam formats are based on ITU-T formats designed to be compatible
> with TV (formats like CIF and like 640x480 - and their multiple/sub-multiples);
> 
> 5) There are formats that weren't originated from TV on some digital webcams,
> so, for those formats, it makes no sense to report an existing std.
> 
> Once people proposed to create an special format for those cases
> (V4L2_STD_DIGITAL or something like that), but, after lots of discussions,
> no changes were done at API nor at the drivers.
> 
> While we don't have an agreement on this, I don't think we should apply a patch
> like this.

I was reading the V4L2 specification and based my patch on the specification.

Maybe the specification is wrong at that point? (see Chapter 1.7 Video Standards
at http://v4l2spec.bytesex.org/spec/x448.htm , starting with paragraph 6:
"Special rules apply to USB cameras...")

Regards,

	Márton Németh

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Laurent Pinchart Aug. 31, 2009, 6:58 a.m. UTC | #3
Hi Mauro,

On Monday 31 August 2009 04:41:14 Mauro Carvalho Chehab wrote:
> Hi Németh,
>
> Em Sun, 23 Aug 2009 11:30:42 +0200
>
> Németh Márton <nm127@freemail.hu> escreveu:
> > From: Márton Németh <nm127@freemail.hu>
> >
> > Change the handling of the case when vdev->tvnorms == 0.
>
> This patch (together with a few others related to tvnorms and camera
> drivers) reopens an old discussion: should webcams report a tvnorm?
>
> There's no easy answer for it since:
>
> 1) removing support for VIDIOC_G_STD/VIDIOC_S_STD causes regressions, since
> some userspace apps stops working;

Then those applications don't work with the uvcvideo driver in the first 
place. This is getting less and less common :-)

> 2) It is a common scenario to use cameras connected to some capture only
> devices like several bttv boards used on surveillance systems. Those
> drivers report STD, since they are used also on TV;
>
> 3) There are even some devices that allows cameras to be connected to one
> input and TV on another input. This is another case were the driver will
> report a TV std;

TV standards are ill-named, they are actually analog standards. 
VIDIOC_[GS]_STD are perfectly valid for capture devices with analog inputs, 
even if they don't use a TV tuner.

> 4) Most webcam formats are based on ITU-T formats designed to be compatible
> with TV (formats like CIF and like 640x480 - and their
> multiple/sub-multiples);

Even HD formats still have roots in the analog TV world. It's a real mess. 
Nonetheless, even if the actual frame size is compatible with TV, there is 
simply no concept of PAL/NTSC for webcams.

> 5) There are formats that weren't originated from TV on some digital
> webcams, so, for those formats, it makes no sense to report an existing
> std.
>
> Once people proposed to create an special format for those cases
> (V4L2_STD_DIGITAL or something like that), but, after lots of discussions,
> no changes were done at API nor at the drivers.

TV standards only apply to analog video. Let's simply not use it for digital 
video. We don't expect drivers to implement VIDIOC_[GS]_JPEGCOMP with fake 
values when they don't support JPEG compression, so we should not expect them 
to implement VIDIOC_[GS]_STD when they don't support analog TV.

> While we don't have an agreement on this, I don't think we should apply a
> patch like this.
  
Hans Verkuil Aug. 31, 2009, 7:33 a.m. UTC | #4
> Hi Mauro,
>
> On Monday 31 August 2009 04:41:14 Mauro Carvalho Chehab wrote:
>> Hi Németh,
>>
>> Em Sun, 23 Aug 2009 11:30:42 +0200
>>
>> Németh Márton <nm127@freemail.hu> escreveu:
>> > From: Márton Németh <nm127@freemail.hu>
>> >
>> > Change the handling of the case when vdev->tvnorms == 0.
>>
>> This patch (together with a few others related to tvnorms and camera
>> drivers) reopens an old discussion: should webcams report a tvnorm?
>>
>> There's no easy answer for it since:
>>
>> 1) removing support for VIDIOC_G_STD/VIDIOC_S_STD causes regressions,
>> since
>> some userspace apps stops working;
>
> Then those applications don't work with the uvcvideo driver in the first
> place. This is getting less and less common :-)
>
>> 2) It is a common scenario to use cameras connected to some capture only
>> devices like several bttv boards used on surveillance systems. Those
>> drivers report STD, since they are used also on TV;
>>
>> 3) There are even some devices that allows cameras to be connected to
>> one
>> input and TV on another input. This is another case were the driver will
>> report a TV std;
>
> TV standards are ill-named, they are actually analog standards.
> VIDIOC_[GS]_STD are perfectly valid for capture devices with analog
> inputs,
> even if they don't use a TV tuner.
>
>> 4) Most webcam formats are based on ITU-T formats designed to be
>> compatible
>> with TV (formats like CIF and like 640x480 - and their
>> multiple/sub-multiples);
>
> Even HD formats still have roots in the analog TV world. It's a real mess.
> Nonetheless, even if the actual frame size is compatible with TV, there is
> simply no concept of PAL/NTSC for webcams.
>
>> 5) There are formats that weren't originated from TV on some digital
>> webcams, so, for those formats, it makes no sense to report an existing
>> std.
>>
>> Once people proposed to create an special format for those cases
>> (V4L2_STD_DIGITAL or something like that), but, after lots of
>> discussions,
>> no changes were done at API nor at the drivers.
>
> TV standards only apply to analog video. Let's simply not use it for
> digital
> video. We don't expect drivers to implement VIDIOC_[GS]_JPEGCOMP with fake
> values when they don't support JPEG compression, so we should not expect
> them
> to implement VIDIOC_[GS]_STD when they don't support analog TV.

Exactly. Work is underway to add an API for HDTV and similar digital video
formats. But we should just freeze the v4l2_std_id API and only use it for
the analog PAL/NTSC/SECAM type formats. This nicely corresponds with the
underlying standards as those have been frozen as well.

Regards,

        Hans

>
>> While we don't have an agreement on this, I don't think we should apply
>> a
>> patch like this.
>
> --
> Regards,
>
> Laurent Pinchart
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
  
Mauro Carvalho Chehab Aug. 31, 2009, 1:03 p.m. UTC | #5
Em Mon, 31 Aug 2009 08:58:24 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Monday 31 August 2009 04:41:14 Mauro Carvalho Chehab wrote:
> > Hi Németh,
> >
> > Em Sun, 23 Aug 2009 11:30:42 +0200
> >
> > Németh Márton <nm127@freemail.hu> escreveu:
> > > From: Márton Németh <nm127@freemail.hu>
> > >
> > > Change the handling of the case when vdev->tvnorms == 0.
> >
> > This patch (together with a few others related to tvnorms and camera
> > drivers) reopens an old discussion: should webcams report a tvnorm?
> >
> > There's no easy answer for it since:
> >
> > 1) removing support for VIDIOC_G_STD/VIDIOC_S_STD causes regressions, since
> > some userspace apps stops working;
> 
> Then those applications don't work with the uvcvideo driver in the first 
> place. This is getting less and less common :-)

Good to know. Yet, removing VIDIOC_[GS]_STD will break a behavior used on
webcams for a long time. One thing is to accept new webcam drivers without it.
This can be done (and were already done, when we accepted uvc). A separate
issue is to change the behavior of the userspace API on existing drivers.

> > 2) It is a common scenario to use cameras connected to some capture only
> > devices like several bttv boards used on surveillance systems. Those
> > drivers report STD, since they are used also on TV;
> >
> > 3) There are even some devices that allows cameras to be connected to one
> > input and TV on another input. This is another case were the driver will
> > report a TV std;
> 
> TV standards are ill-named, they are actually analog standards. 
> VIDIOC_[GS]_STD are perfectly valid for capture devices with analog inputs, 
> even if they don't use a TV tuner.

It is not that simple. In general, at the bridge chip, all inputs are digital.
The analog to digital conversion is done by a separate chip on most devices,
and there are some boards where you have, for example, digital sensors
connected to it.

> > 4) Most webcam formats are based on ITU-T formats designed to be compatible
> > with TV (formats like CIF and like 640x480 - and their
> > multiple/sub-multiples);
> 
> Even HD formats still have roots in the analog TV world. It's a real mess. 
> Nonetheless, even if the actual frame size is compatible with TV, there is 
> simply no concept of PAL/NTSC for webcams.

Ok, but still it is possible to use V4L2_STD_525_60 and V4L2_STD_625_50 when all
you need is to set the number of lines and the basis of the sampling rate.
There is also V4L2_STD_UNKNOWN standard used on some drivers.

> > 5) There are formats that weren't originated from TV on some digital
> > webcams, so, for those formats, it makes no sense to report an existing
> > std.
> >
> > Once people proposed to create an special format for those cases
> > (V4L2_STD_DIGITAL or something like that), but, after lots of discussions,
> > no changes were done at API nor at the drivers.
> 
> TV standards only apply to analog video. Let's simply not use it for digital 
> video. We don't expect drivers to implement VIDIOC_[GS]_JPEGCOMP with fake 
> values when they don't support JPEG compression, so we should not expect them 
> to implement VIDIOC_[GS]_STD when they don't support analog TV.

If you look at the tree, several drivers that returns compressed formats
implements those ioctls. This is even required on several cases, where you may
have an analog TV connected on it.

That's said, I understand your arguments that implementing [G/S]_STD is
senseless on almost all camera drivers, since it won't actually control
anything. So, IMO, a good compromise is to keep the existing implementation on
the legacy drivers [1], but think twice before implementing
it on a new driver.

[1] We could eventually drop it even for the existing drivers, provided that the
ioctl won't control anything at the device and that we properly document it in
Documentation/feature-removal-schedule.txt, giving enough time for the remaining
userspace apps and distros to change to the new behavior.

> > While we don't have an agreement on this, I don't think we should apply a
> > patch like this.



Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Mauro Carvalho Chehab Aug. 31, 2009, 1:08 p.m. UTC | #6
Em Mon, 31 Aug 2009 09:33:23 +0200
"Hans Verkuil" <hverkuil@xs4all.nl> escreveu:

> > TV standards only apply to analog video. Let's simply not use it for
> > digital
> > video. We don't expect drivers to implement VIDIOC_[GS]_JPEGCOMP with fake
> > values when they don't support JPEG compression, so we should not expect
> > them
> > to implement VIDIOC_[GS]_STD when they don't support analog TV.
> 
> Exactly. Work is underway to add an API for HDTV and similar digital video
> formats. But we should just freeze the v4l2_std_id API and only use it for
> the analog PAL/NTSC/SECAM type formats. This nicely corresponds with the
> underlying standards as those have been frozen as well.

Could you please point the thread where this API is being discussed

Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Hans Verkuil Aug. 31, 2009, 2:44 p.m. UTC | #7
> Em Mon, 31 Aug 2009 09:33:23 +0200
> "Hans Verkuil" <hverkuil@xs4all.nl> escreveu:
>
>> > TV standards only apply to analog video. Let's simply not use it for
>> > digital
>> > video. We don't expect drivers to implement VIDIOC_[GS]_JPEGCOMP with
>> fake
>> > values when they don't support JPEG compression, so we should not
>> expect
>> > them
>> > to implement VIDIOC_[GS]_STD when they don't support analog TV.
>>
>> Exactly. Work is underway to add an API for HDTV and similar digital
>> video
>> formats. But we should just freeze the v4l2_std_id API and only use it
>> for
>> the analog PAL/NTSC/SECAM type formats. This nicely corresponds with the
>> underlying standards as those have been frozen as well.
>
> Could you please point the thread where this API is being discussed

Sure, here is the thread:

http://osdir.com/ml/linux-media/2009-08/msg00452.html

It's currently just brainstorming and not an official RFC.

Regards,

         Hans
  

Patch

diff -upr linux-2.6.31-rc7.orig/drivers/media/video/v4l2-dev.c linux-2.6.31-rc7/drivers/media/video/v4l2-dev.c
--- linux-2.6.31-rc7.orig/drivers/media/video/v4l2-dev.c	2009-08-23 07:36:09.000000000 +0200
+++ linux-2.6.31-rc7/drivers/media/video/v4l2-dev.c	2009-08-23 10:47:03.000000000 +0200
@@ -396,6 +396,11 @@  int video_register_device_index(struct v
 	if (!vdev->release)
 		return -EINVAL;

+	/* if no video standards are supported then no need to get and set
+	   them: they will never be called */
+	WARN_ON(!vdev->tvnorms && vdev->ioctl_ops->vidioc_g_std);
+	WARN_ON(!vdev->tvnorms && vdev->ioctl_ops->vidioc_s_std);
+
 	/* Part 1: check device type */
 	switch (type) {
 	case VFL_TYPE_GRABBER:
diff -upr linux-2.6.31-rc7.orig/drivers/media/video/v4l2-ioctl.c linux-2.6.31-rc7/drivers/media/video/v4l2-ioctl.c
--- linux-2.6.31-rc7.orig/drivers/media/video/v4l2-ioctl.c	2009-08-23 07:36:09.000000000 +0200
+++ linux-2.6.31-rc7/drivers/media/video/v4l2-ioctl.c	2009-08-23 10:50:08.000000000 +0200
@@ -1077,14 +1077,17 @@  static long __video_do_ioctl(struct file
 	{
 		v4l2_std_id *id = arg;

-		ret = 0;
-		/* Calls the specific handler */
-		if (ops->vidioc_g_std)
-			ret = ops->vidioc_g_std(file, fh, id);
-		else if (vfd->current_norm)
-			*id = vfd->current_norm;
-		else
-			ret = -EINVAL;
+		/* Check if any standard is supported */
+		if (vfd->tvnorms) {
+			ret = 0;
+			/* Calls the specific handler */
+			if (ops->vidioc_g_std)
+				ret = ops->vidioc_g_std(file, fh, id);
+			else if (vfd->current_norm)
+				*id = vfd->current_norm;
+			else
+				ret = -EINVAL;
+		}

 		if (!ret)
 			dbgarg(cmd, "std=0x%08Lx\n", (long long unsigned)*id);
@@ -1097,7 +1100,7 @@  static long __video_do_ioctl(struct file
 		dbgarg(cmd, "std=%08Lx\n", (long long unsigned)*id);

 		norm = (*id) & vfd->tvnorms;
-		if (vfd->tvnorms && !norm)	/* Check if std is supported */
+		if (!norm)	/* Check if std is supported */
 			break;

 		/* Calls the specific handler */