usbvideo: usbvideo.c should check palette in VIDIOCSPICT (resend again)

From: Chris MacGregor <[email protected]>

This change makes VIDIOCSPICT return EINVAL if the requested palette (format) doesn't match anything in paletteBits; this is essentially the same check as is already made in VIDIOCMCAPTURE. The patch is against 2.6.24.2.

The problem I had was that vlc (www.videolan.org) was failing to read video from my Logitech Quickcam Messenger. I ultimately tracked it to the fact that vlc was testing what formats the driver+hardware could handle by trying to select them with VIDIOCSPICT, and was assuming that if it didn't get an error then the requested format was kosher. However, usbvideo.c was not even looking at that field, and so anything would succeed, but then VIDIOCMCAPTURE would bomb out silently (EINVAL, but no log message) if the right paletteBits bit wasn't set.

The risk, of course, is that this patch will break existing v4l clients that accidentally pass non-zero garbage in the video_picture.palette field when calling VIDIOCSPICT, but which pass a valid format to VIDIOCMCAPTURE. However, what vlc does seems reasonable, and in fact I don't see an alternative to it (though I'm no v4l expert), so I think we're better off with the change than without. I found plaintive, unanswered cries for help on forums from folks wanting to know why vlc wasn't working with their webcams.

One could make the argument that a similar check should be made on the depth field, but vlc doesn't mess with it and I'm not feeling that ambitious today.

---

Hi. I was going to post this in lkml, but after looking in MAINTAINERS this list seems more appropriate - if I'm wrong about that, please tell me (directly, as I don't generally subscribe to either list).

I sent this message previously on February 16, 2008 (11:27 pm PST) but it doesn't seem to have shown up on the list (based on looking through the archives). Ditto yesterday, so this time I'm sending it via a different route in case the problem is an overzealous spam filter someplace. Apologies to anyone who has already seen it.

Please CC me on replies as I'm not necessarily subscribed here.

Chris MacGregor
my first name @ cybermato.com
www.cybermato.com

@@ -1299,6 +1299,9 @@ static int usbvideo_v4l_do_ioctl(struct 

		case VIDIOCSPICT:
		{
			struct video_picture *pic = arg;

+ if (pic->palette
+ && ((1L << pic->palette) & uvd->paletteBits) == 0)
+ return -EINVAL;
/*
* Use temporary 'video_picture' structure to preserve our
* own settings (such as color depth, palette) that we

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

This concept of setting the format with VIDIOCMCAPTURE isn't something that's defined in the V4L1 specs, but some software like vlc tries it because it worked (not anymore) with the bttv driver.

I'm almost certain that the v4l1 compat module, and therefore all the drivers that use it for v4l1 support, already works this way; returning EINVAL for invalid formats (those that fail the v4l2 try format ioctl) in VIDIOCSPICT.

You should probably NOT add this. In fact, bttv had this check and I removed it, as it broke vlc! vlc, and most software, just leaves the depth field 0 or uninitialized, as it seems like it was intended as an output from the driver, not an input. Though, as usual, the v4l1 spec is vague on this issue. The format already defines the depth, so specifying it a second times seems pointless. It is slightly useful as an output from the driver, so that software that the data is only going to pass through can know the size without needing to know about any particular formats.

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list