libv4l2 misbehavior after calling S_STD or S_DV_PRESET

libv4l2 misbehavior after calling S_STD or S_DV_PRESET

[Hans Verkuil]

Hi Hans!

I've been looking into a problem with libv4l2 that occurs when you change TV 
standard or video preset using VIDIOC_S_STD or VIDIOC_S_DV_PRESET. These calls 
will change the format implicitly (e.g. if the current format is set for PAL 
at 720x576 and you select NTSC, then the format will be reset to 720x480).

However, libv4l2 isn't taking this into account and will keep using the cached 
dest_fmt value. It is easy to reproduce this using qv4l2.

The same problem is likely to occur with S_CROP (haven't tested that yet, 
though): calling S_CROP can also change the format.

To be precise: S_STD and S_DV_PRESET can change both the crop rectangle and 
the format, and S_CROP can change the format.

I've been trying to find a quick solution for this in libv4l2.c but without any 
luck.

Can you look at this? Or do you have ideas how this should be done?

Regards,

	Hans

Re: libv4l2 misbehavior after calling S_STD or S_DV_PRESET

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

Hi,

Hmm, nasty...

On 10/06/2011 01:13 PM, Hans Verkuil wrote:
> Hi Hans!
>
> I've been looking into a problem with libv4l2 that occurs when you change TV
> standard or video preset using VIDIOC_S_STD or VIDIOC_S_DV_PRESET. These calls
> will change the format implicitly (e.g. if the current format is set for PAL
> at 720x576 and you select NTSC, then the format will be reset to 720x480).
>
> However, libv4l2 isn't taking this into account and will keep using the cached
> dest_fmt value. It is easy to reproduce this using qv4l2.
>
> The same problem is likely to occur with S_CROP (haven't tested that yet,
> though): calling S_CROP can also change the format.
>
> To be precise: S_STD and S_DV_PRESET can change both the crop rectangle and
> the format, and S_CROP can change the format.

First of all it would be good to actually document this behavior of
VIDIOC_S_STD or VIDIOC_S_DV_PRESET, the current docs don't mention this at all:
http://linuxtv.org/downloads/v4l-dvb-apis/standard.html

I've attached 2 patches which should make libv4l2 deal with this correctly.
I assume you've a reproducer for this and I would appreciate it if you could test
if these patches actually fix the issue you are seeing.

Regards,

Hans

[-- Attachment #2: 0001-libv4l2-Move-s_fmt-handling-code-into-a-helper-funct.patch --]
[-- Type: text/plain, Size: 7264 bytes --]

>From a5abaaa08602b540c88ae4776f557a3b0c34b24d Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Fri, 7 Oct 2011 09:18:39 +0200
Subject: [PATCH 1/2] libv4l2: Move s_fmt handling code into a helper function

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 lib/libv4l2/libv4l2.c |  195 +++++++++++++++++++++++++------------------------
 1 files changed, 98 insertions(+), 97 deletions(-)

diff --git a/lib/libv4l2/libv4l2.c b/lib/libv4l2/libv4l2.c
index 977179a..f7ec85d 100644
--- a/lib/libv4l2/libv4l2.c
+++ b/lib/libv4l2/libv4l2.c
@@ -881,6 +881,101 @@ static void v4l2_set_src_and_dest_format(int index,
 	devices[index].dest_fmt = *dest_fmt;
 }
 
+static int v4l2_s_fmt(int index, struct v4l2_format *dest_fmt)
+{
+	struct v4l2_format src_fmt;
+	struct v4l2_pix_format req_pix_fmt;
+	int result;
+
+	/* Don't be lazy on uvc cams, as this triggers a bug in the uvcvideo
+	   driver in kernel <= 2.6.28 (with certain cams) */
+	if (!(devices[index].flags & V4L2_IS_UVC) &&
+	    v4l2_pix_fmt_compat(&devices[index].dest_fmt, dest_fmt)) {
+		*dest_fmt = devices[index].dest_fmt;
+		return 0;
+	}
+
+	if (v4l2_log_file) {
+		int pixfmt = dest_fmt->fmt.pix.pixelformat;
+
+		fprintf(v4l2_log_file, "VIDIOC_S_FMT app requesting: %c%c%c%c\n",
+				pixfmt & 0xff,
+				(pixfmt >> 8) & 0xff,
+				(pixfmt >> 16) & 0xff,
+				pixfmt >> 24);
+	}
+
+	result = v4lconvert_try_format(devices[index].convert,
+				       dest_fmt, &src_fmt);
+	if (result) {
+		int saved_err = errno;
+		V4L2_LOG("S_FMT error trying format: %s\n", strerror(errno));
+		errno = saved_err;
+		return result;
+	}
+
+	if (src_fmt.fmt.pix.pixelformat != dest_fmt->fmt.pix.pixelformat &&
+			v4l2_log_file) {
+		int pixfmt = src_fmt.fmt.pix.pixelformat;
+
+		fprintf(v4l2_log_file,
+			"VIDIOC_S_FMT converting from: %c%c%c%c\n",
+			pixfmt & 0xff, (pixfmt >> 8) & 0xff,
+			(pixfmt >> 16) & 0xff, pixfmt >> 24);
+	}
+
+	/* Maybe after try format has adjusted width/height etc, to whats
+	   available nothing has changed (on the cam side) ? */
+	if (!(devices[index].flags & V4L2_IS_UVC) &&
+	    v4l2_pix_fmt_compat(&devices[index].src_fmt, &src_fmt)) {
+		v4l2_set_src_and_dest_format(index, &devices[index].src_fmt,
+				dest_fmt);
+		return 0;
+	}
+
+	result = v4l2_check_buffer_change_ok(index);
+	if (result)
+		return result;
+
+	req_pix_fmt = src_fmt.fmt.pix;
+	result = devices[index].dev_ops->ioctl(devices[index].dev_ops_priv,
+					       devices[index].fd,
+					       VIDIOC_S_FMT, &src_fmt);
+	if (result) {
+		int saved_err = errno;
+		V4L2_LOG_ERR("setting pixformat: %s\n", strerror(errno));
+		/* Report to the app dest_fmt has not changed */
+		*dest_fmt = devices[index].dest_fmt;
+		errno = saved_err;
+		return result;
+	}
+
+	/* See if we've gotten what try_fmt promised us
+	   (this check should never fail) */
+	if (src_fmt.fmt.pix.width != req_pix_fmt.width ||
+	    src_fmt.fmt.pix.height != req_pix_fmt.height ||
+	    src_fmt.fmt.pix.pixelformat != req_pix_fmt.pixelformat) {
+		V4L2_LOG_ERR("set_fmt gave us a different result then try_fmt!\n");
+		/* Not what we expected / wanted, disable conversion */
+		*dest_fmt = src_fmt;
+	}
+
+	v4l2_set_src_and_dest_format(index, &src_fmt, dest_fmt);
+
+	if (devices[index].flags & V4L2_SUPPORTS_TIMEPERFRAME) {
+		struct v4l2_streamparm parm = {
+			.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+		};
+		if (devices[index].dev_ops->ioctl(devices[index].dev_ops_priv,
+						  devices[index].fd,
+						  VIDIOC_G_PARM, &parm))
+			return 0;
+		v4l2_update_fps(index, &parm);
+	}
+
+	return 0;
+}
+
 int v4l2_ioctl(int fd, unsigned long int request, ...)
 {
 	void *arg;
@@ -991,11 +1086,8 @@ no_capture_request:
 			struct v4l2_format fmt = devices[index].dest_fmt;
 
 			V4L2_LOG("Setting pixelformat to RGB24 (supported_dst_fmt_only)");
-			devices[index].flags |= V4L2_STREAM_TOUCHED;
 			fmt.fmt.pix.pixelformat = V4L2_PIX_FMT_RGB24;
-			pthread_mutex_unlock(&devices[index].stream_lock);
-			v4l2_ioctl(fd, VIDIOC_S_FMT, &fmt);
-			pthread_mutex_lock(&devices[index].stream_lock);
+			v4l2_s_fmt(index, &fmt);
 			V4L2_LOG("Done setting pixelformat (supported_dst_fmt_only)");
 		}
 		devices[index].flags |= V4L2_STREAM_TOUCHED;
@@ -1046,100 +1138,9 @@ no_capture_request:
 					       arg, NULL);
 		break;
 
-	case VIDIOC_S_FMT: {
-		struct v4l2_format src_fmt, *dest_fmt = arg;
-		struct v4l2_pix_format req_pix_fmt;
-
-		/* Don't be lazy on uvc cams, as this triggers a bug in the uvcvideo
-		   driver in kernel <= 2.6.28 (with certain cams) */
-		if (!(devices[index].flags & V4L2_IS_UVC) &&
-		    v4l2_pix_fmt_compat(&devices[index].dest_fmt, dest_fmt)) {
-			*dest_fmt = devices[index].dest_fmt;
-			result = 0;
-			break;
-		}
-
-		if (v4l2_log_file) {
-			int pixfmt = dest_fmt->fmt.pix.pixelformat;
-
-			fprintf(v4l2_log_file, "VIDIOC_S_FMT app requesting: %c%c%c%c\n",
-					pixfmt & 0xff,
-					(pixfmt >> 8) & 0xff,
-					(pixfmt >> 16) & 0xff,
-					pixfmt >> 24);
-		}
-
-		result = v4lconvert_try_format(devices[index].convert,
-					       dest_fmt, &src_fmt);
-		if (result) {
-			saved_err = errno;
-			V4L2_LOG("S_FMT error trying format: %s\n", strerror(errno));
-			errno = saved_err;
-			break;
-		}
-
-		if (src_fmt.fmt.pix.pixelformat != dest_fmt->fmt.pix.pixelformat &&
-				v4l2_log_file) {
-			int pixfmt = src_fmt.fmt.pix.pixelformat;
-
-			fprintf(v4l2_log_file, "VIDIOC_S_FMT converting from: %c%c%c%c\n",
-					pixfmt & 0xff,
-					(pixfmt >> 8) & 0xff,
-					(pixfmt >> 16) & 0xff,
-					pixfmt >> 24);
-		}
-
-		/* Maybe after try format has adjusted width/height etc, to whats
-		   available nothing has changed (on the cam side) ? */
-		if (!(devices[index].flags & V4L2_IS_UVC) &&
-				v4l2_pix_fmt_compat(&devices[index].src_fmt, &src_fmt)) {
-			v4l2_set_src_and_dest_format(index, &devices[index].src_fmt,
-					dest_fmt);
-			result = 0;
-			break;
-		}
-
-		result = v4l2_check_buffer_change_ok(index);
-		if (result)
-			break;
-
-		req_pix_fmt = src_fmt.fmt.pix;
-		result = devices[index].dev_ops->ioctl(
-				devices[index].dev_ops_priv,
-				fd, VIDIOC_S_FMT, &src_fmt);
-		if (result) {
-			saved_err = errno;
-			V4L2_LOG_ERR("setting pixformat: %s\n", strerror(errno));
-			/* Report to the app dest_fmt has not changed */
-			*dest_fmt = devices[index].dest_fmt;
-			errno = saved_err;
-			break;
-		}
-		/* See if we've gotten what try_fmt promised us
-		   (this check should never fail) */
-		if (src_fmt.fmt.pix.width != req_pix_fmt.width ||
-				src_fmt.fmt.pix.height != req_pix_fmt.height ||
-				src_fmt.fmt.pix.pixelformat != req_pix_fmt.pixelformat) {
-			V4L2_LOG_ERR("set_fmt gave us a different result then try_fmt!\n");
-			/* Not what we expected / wanted, disable conversion */
-			*dest_fmt = src_fmt;
-		}
-
-		v4l2_set_src_and_dest_format(index, &src_fmt, dest_fmt);
-
-		if (devices[index].flags & V4L2_SUPPORTS_TIMEPERFRAME) {
-			struct v4l2_streamparm parm = {
-				.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
-			};
-			if (devices[index].dev_ops->ioctl(
-					devices[index].dev_ops_priv,
-					fd, VIDIOC_G_PARM, &parm))
-				break;
-			v4l2_update_fps(index, &parm);
-		}
-
+	case VIDIOC_S_FMT:
+		result = v4l2_s_fmt(index, arg);
 		break;
-	}
 
 	case VIDIOC_G_FMT: {
 		struct v4l2_format *fmt = arg;
-- 
1.7.6.4


[-- Attachment #3: 0002-libv4l-handle-VIDIOC_S_STD-or-VIDIOC_S_DV_PRESET-cha.patch --]
[-- Type: text/plain, Size: 3212 bytes --]

>From ce898a8e33b361424b554879faff4bd619aaf979 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Fri, 7 Oct 2011 09:54:24 +0200
Subject: [PATCH 2/2] libv4l: handle VIDIOC_S_STD or VIDIOC_S_DV_PRESET
 changing the format

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 lib/libv4l2/libv4l2-priv.h |    1 +
 lib/libv4l2/libv4l2.c      |   50 ++++++++++++++++++++++++++++++++++++++++++++
 lib/libv4l2/log.c          |    2 +-
 3 files changed, 52 insertions(+), 1 deletions(-)

diff --git a/lib/libv4l2/libv4l2-priv.h b/lib/libv4l2/libv4l2-priv.h
index 2e99200..730f193 100644
--- a/lib/libv4l2/libv4l2-priv.h
+++ b/lib/libv4l2/libv4l2-priv.h
@@ -100,6 +100,7 @@ void v4l2_plugin_cleanup(void *plugin_lib, void *plugin_priv,
 			 const struct libv4l2_dev_ops *dev_ops);
 
 /* From log.c */
+extern const char *v4l2_ioctls[];
 void v4l2_log_ioctl(unsigned long int request, void *arg, int result);
 
 #endif
diff --git a/lib/libv4l2/libv4l2.c b/lib/libv4l2/libv4l2.c
index f7ec85d..d205674 100644
--- a/lib/libv4l2/libv4l2.c
+++ b/lib/libv4l2/libv4l2.c
@@ -1059,6 +1059,11 @@ int v4l2_ioctl(int fd, unsigned long int request, ...)
 				stream_needs_locking = 1;
 		}
 		break;
+	case VIDIOC_S_STD:
+	case VIDIOC_S_DV_PRESET:
+		is_capture_request = 1;
+		stream_needs_locking = 1;
+		break;		
 	}
 
 	if (!is_capture_request) {
@@ -1150,6 +1155,51 @@ no_capture_request:
 		break;
 	}
 
+	case VIDIOC_S_STD:
+	case VIDIOC_S_DV_PRESET: {
+		struct v4l2_format src_fmt, try_fmt;
+
+		result = devices[index].dev_ops->ioctl(
+				devices[index].dev_ops_priv,
+				fd, request, arg);
+		if (result)
+			break;
+
+		/* These ioctls may have changed the device's fmt */
+		result = devices[index].dev_ops->ioctl(
+				devices[index].dev_ops_priv,
+				fd, VIDIOC_G_FMT, &src_fmt);
+		if (result) {
+			V4L2_LOG_ERR("getting pixformat after %s: %s\n",
+				     v4l2_ioctls[_IOC_NR(request)],
+				     strerror(errno));
+			result = 0; /* The original command did succeed */
+			break;
+		}
+
+		if (v4l2_pix_fmt_compat(&devices[index].src_fmt, &src_fmt)) {
+			v4l2_set_src_and_dest_format(index, &src_fmt,
+						     &devices[index].dest_fmt);
+			break;
+		}
+
+		/* The fmt has been changed, try to restore the last set
+		   destination pixelformat, using the new dimenstions */
+		try_fmt = src_fmt;
+		try_fmt.fmt.pix.pixelformat =
+			devices[index].dest_fmt.fmt.pix.pixelformat;
+		result = v4l2_s_fmt(index, &try_fmt);
+		if (result) {
+			V4L2_LOG_WARN("Restoring destination pixelformat after %s failed\n",
+				      v4l2_ioctls[_IOC_NR(request)]);
+			devices[index].src_fmt = src_fmt;
+			devices[index].dest_fmt = src_fmt;
+			result = 0; /* The original command did succeed */
+		}
+
+		break;
+	}
+
 	case VIDIOC_REQBUFS: {
 		struct v4l2_requestbuffers *req = arg;
 
diff --git a/lib/libv4l2/log.c b/lib/libv4l2/log.c
index 37d2e1f..f7ce06f 100644
--- a/lib/libv4l2/log.c
+++ b/lib/libv4l2/log.c
@@ -31,7 +31,7 @@
 
 FILE *v4l2_log_file = NULL;
 
-static const char *v4l2_ioctls[] = {
+const char *v4l2_ioctls[] = {
 	/* start v4l2 ioctls */
 	[_IOC_NR(VIDIOC_QUERYCAP)]         = "VIDIOC_QUERYCAP",
 	[_IOC_NR(VIDIOC_RESERVED)]         = "VIDIOC_RESERVED",
-- 
1.7.6.4

Re: libv4l2 misbehavior after calling S_STD or S_DV_PRESET

[Hans Verkuil]

On Friday 07 October 2011 09:57:42 Hans de Goede wrote:
> Hi,
> 
> Hmm, nasty...
> 
> On 10/06/2011 01:13 PM, Hans Verkuil wrote:
> > Hi Hans!
> > 
> > I've been looking into a problem with libv4l2 that occurs when you change
> > TV standard or video preset using VIDIOC_S_STD or VIDIOC_S_DV_PRESET.
> > These calls will change the format implicitly (e.g. if the current
> > format is set for PAL at 720x576 and you select NTSC, then the format
> > will be reset to 720x480).
> > 
> > However, libv4l2 isn't taking this into account and will keep using the
> > cached dest_fmt value. It is easy to reproduce this using qv4l2.
> > 
> > The same problem is likely to occur with S_CROP (haven't tested that yet,
> > though): calling S_CROP can also change the format.
> > 
> > To be precise: S_STD and S_DV_PRESET can change both the crop rectangle
> > and the format, and S_CROP can change the format.
> 
> First of all it would be good to actually document this behavior of
> VIDIOC_S_STD or VIDIOC_S_DV_PRESET, the current docs don't mention this at
> all: http://linuxtv.org/downloads/v4l-dvb-apis/standard.html

Odd, I'd have sworn that it was in the docs.

The full list of ioctls that can change both the crop settings and the format 
is:

VIDIOC_S_STD
VIDIOC_S_DV_PRESET
VIDIOC_S_DV_TIMINGS
VIDIOC_S_INPUT (can implicitly change standard/preset)
VIDIOC_S_OUTPUT (ditto)
VIDIOC_S_CROP

Note that I suspect that there are quite a few drivers that do not handle this
correctly. After all, for normal SDTV capture cards you almost never change
the TV standard once it is set up at the start so I doubt if this has been
tested much. For DV_PRESET it is much more common to switch from e.g.
720p to 1080p. That is how I found this issue.

> I've attached 2 patches which should make libv4l2 deal with this correctly.
> I assume you've a reproducer for this and I would appreciate it if you
> could test if these patches actually fix the issue you are seeing.

Almost working. The second patch forgot to set src_fmt.type, so I got an error 
back. After initializing it to BUF_TYPE_VIDEO_CAPTURE it worked fine.

Regards,

	Hans

Re: libv4l2 misbehavior after calling S_STD or S_DV_PRESET

[Hans de Goede]

Hi,

On 10/07/2011 11:06 AM, Hans Verkuil wrote:
> On Friday 07 October 2011 09:57:42 Hans de Goede wrote:
>> Hi,
>>
>> Hmm, nasty...
>>
>> On 10/06/2011 01:13 PM, Hans Verkuil wrote:
>>> Hi Hans!
>>>
>>> I've been looking into a problem with libv4l2 that occurs when you change
>>> TV standard or video preset using VIDIOC_S_STD or VIDIOC_S_DV_PRESET.
>>> These calls will change the format implicitly (e.g. if the current
>>> format is set for PAL at 720x576 and you select NTSC, then the format
>>> will be reset to 720x480).
>>>
>>> However, libv4l2 isn't taking this into account and will keep using the
>>> cached dest_fmt value. It is easy to reproduce this using qv4l2.
>>>
>>> The same problem is likely to occur with S_CROP (haven't tested that yet,
>>> though): calling S_CROP can also change the format.
>>>
>>> To be precise: S_STD and S_DV_PRESET can change both the crop rectangle
>>> and the format, and S_CROP can change the format.
>>
>> First of all it would be good to actually document this behavior of
>> VIDIOC_S_STD or VIDIOC_S_DV_PRESET, the current docs don't mention this at
>> all: http://linuxtv.org/downloads/v4l-dvb-apis/standard.html
>
> Odd, I'd have sworn that it was in the docs.
>
> The full list of ioctls that can change both the crop settings and the format
> is:
>
> VIDIOC_S_STD
> VIDIOC_S_DV_PRESET

The patch already handles these 2 :)

> VIDIOC_S_DV_TIMINGS
> VIDIOC_S_INPUT (can implicitly change standard/preset)

I'll add these 2.

> VIDIOC_S_OUTPUT (ditto)

libv4l2 only cares about capture, all the rest it just passes
through completely unmodified.

> VIDIOC_S_CROP

Hmm, can this also change the fmt? That would be an unexpected
side effect. But if it does I can handle it the same way, right?

> Note that I suspect that there are quite a few drivers that do not handle this
> correctly. After all, for normal SDTV capture cards you almost never change
> the TV standard once it is set up at the start so I doubt if this has been
> tested much. For DV_PRESET it is much more common to switch from e.g.
> 720p to 1080p. That is how I found this issue.
>
>> I've attached 2 patches which should make libv4l2 deal with this correctly.
>> I assume you've a reproducer for this and I would appreciate it if you
>> could test if these patches actually fix the issue you are seeing.
>
> Almost working. The second patch forgot to set src_fmt.type, so I got an error
> back. After initializing it to BUF_TYPE_VIDEO_CAPTURE it worked fine.

Thanks for testing, and duh wrt my G_FMT mistake. If you can answer my question
about S_CROP then I'll go add the other ioctls which need similar handling to
my 2nd patch and push both patches.

Regards,

Hans