On Sunday 29 March 2009 12:21:58 Trent Piepho wrote: > On Sun, 29 Mar 2009, Hans Verkuil wrote: > > On Sunday 29 March 2009 10:50:02 Patch from Trent Piepho wrote: > > > From: Trent Piepho <xyzzy@xxxxxxxxxxxxx> > > > v4l2-ioctl: Check format for S_PARM and G_PARM > > > > > > > > > Return EINVAL if VIDIOC_S/G_PARM is called for a buffer type that the > > > driver doesn't define a ->vidioc_try_fmt_XXX() method for. Several > > > other ioctls, like QUERYBUF, QBUF, and DQBUF, etc. do this too. It > > > saves each driver from having to check if the buffer type is one that > > > it supports. > > > > Hi Trent, > > > > I wonder whether this change is correct. Looking at the spec I see that > > g/s_parm only supports VIDEO_CAPTURE, VIDEO_OUTPUT and PRIVATE or up. > > > > So what should happen if the type is VIDEO_OVERLAY? I think the > > g/s_parm implementation in v4l2-ioctl.c should first exclude the > > unsupported types before calling check_fmt. > > This change doesn't actually enable g/s_parm for VIDEO_OVERLAY (or > VBI_CAPTURE). It's the later bttv and saa7146 changes that do that. I > considered this when I made those changes, as mentioned in those patch > descriptions, but decided it was better to allow it. > > In those drivers g_parm only returns the frame rate, which seems just as > valid for VIDEO_OVERLAY as it does for VIDEO_CAPTURE. Why should the > driver not be allowed to return the frame rate for video overlay? Why > should setting the overlay frame rate not be allowed? Those seems like > perfectly reasonable operations to me. Not to me. VIDEO_OVERLAY just defines where the overlay is. But the actual framerate is entirely dependent on the VIDEO_CAPTURE framerate. Just keep to the spec for now. If a new driver appears that needs it then we can always change it. > > The spec doesn't explicitly say that only VIDEO_CAPTURE, VIDEO_OUTPUT and > PRIVATE are supported. It says the "capture" field of the parm union is > used when type is VIDEO_CAPTURE, the "output" field is used for > VIDEO_OUTPUT, and "raw_data" is used for PRIVATE or higher. You're right > in that it doesn't say what you're supposed to use for VIDEO_OVERLAY, > VBI_CAPTURE or any other buffer types. But it doesn't say they're not > allowed either. IMHO, it's likely the spec authors' intent wasn't to not > allow g_parm with VIDEO_OVERLAY, but rather that they just didn't think > of that case. No, I agree with the spec in that I see no use case for it. Should there be one, then I'd like to see that in an actual driver implementation and in that case the spec should be adjusted as well. In addition, g/s_parm is only used in combination with webcams/sensors for which overlays and vbi are irrelevant. > > Thinking about it now, I think what makes the most sense is to use > "capture" for VIDEO_OVERLAY, VBI_CAPTURE, and SLICED_VBI_CAPTURE. And > use "output" for VBI_OUTPUT, SLICED_VBI_OUTPUT and VIDEO_OUTPUT_OVERLAY. > > > I also wonder whether check_fmt shouldn't check for the presence of the > > s_fmt callbacks instead of try_fmt since try_fmt is an optional ioctl. > > I noticed that too. saa7146 doesn't have a try_fmt call for vbi_capture > but is apparently supposed to support it. I sent a message about that > earlier. I saw that. So why not check for s_fmt instead of try_fmt? That would solve this potential problem. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
