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
>From a5abaaa08602b540c88ae4776f557a3b0c34b24d Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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
>From ce898a8e33b361424b554879faff4bd619aaf979 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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
