* Re: libv4l2 misbehavior after calling S_STD or S_DV_PRESET
2011-10-06 11:13 libv4l2 misbehavior after calling S_STD or S_DV_PRESET Hans Verkuil
@ 2011-10-07 7:57 ` Hans de Goede
2011-10-07 9:06 ` Hans Verkuil
0 siblings, 1 reply; 4+ messages in thread
From: Hans de Goede @ 2011-10-07 7:57 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media
[-- 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
^ permalink raw reply related [flat|nested] 4+ messages in thread