Hi Mauro, On Monday 08 June 2009 03:52:12 Mauro Carvalho Chehab wrote: > Em Thu, 4 Jun 2009 14:39:52 +0200 > > Laurent Pinchart <laurent.pinchart@xxxxxxxxx> escreveu: > > Mauro, > > > > Please pull from http://linuxtv.org/hg/~pinchartl/uvcvideo/ > > > > for the following 3 changesets: > > > > uvcvideo: Add generic control blacklist. > > uvcvideo: Don't accept to change the format when buffers are allocated. > > uvcvideo: Implement VIDIOC_[GS]_JPEGCOMP > > I have a few comments about the code of the last patch: I'll have to drop the tree, clone v4l-dvb and re-apply patches. git ? :-) (don't take this too seriously, I would love to switch to git, but it doesn't bother me *that* much for now as I don't have to rework patches too often). > + jpeg->quality = video->streaming->ctrl.wCompQuality / 100; > > Wouldn't be better to round it to the closest value instead of just > truncating? Ok. I don't think it will really make a difference given how webcams handle JPEG compression quality, but better being correct anyway. > + memcpy(&probe, &video->streaming->ctrl, sizeof probe); > > Please use sizeof(probe) instead. Oops sorry. I'm used to the 'sizeof variable' syntax. Seems checkpatch doesn't catch this. I'll fix that. > ERROR: do not use assignment in if condition > #64: FILE: linux/drivers/media/video/uvc/uvc_v4l2.c:376: > + if ((ret = uvc_probe_video(video, &probe)) < 0) > > ERROR: do not use assignment in if condition > #80: FILE: linux/drivers/media/video/uvc/uvc_v4l2.c:872: > + if ((ret = uvc_acquire_privileges(handle)) < 0) > > total: 2 errors, 0 warnings, 81 lines checked > > Please, one statement per line. Is there any way I can convince you to accept the if ((ret = function(...) < 0) syntax on the basis that it's used throughout the rest of the driver ? :-) $ find . -type f -name \*.c -exec grep -l 'if (([a-zA-Z]\+ \?= \?[a-zA- Z_]\+([a-zA-Z0-9 ,*]*)) \?[<>=]\+ \?[a-zA-Z0-9]\+)' {} \; | wc 307 307 9204 I might have missed a few cases. Not that many occurrences. > Also, I have a few comments, from API POV. > > It doesn't seem that those ioctls are properly implemented. There are some > things there that sounded obfuscated for me, and it you aren't > implementing. Probably because it is not clear enough. The UVC standard doesn't specify a way to add/remove specific JPEG segments. As MJPEG has not COM, DRI and DHT segments, I've hardcoded flags to DQT. > Also, we used to have a similar set of ioctls for MPEG, that were removed, > in favor of the usage of extended controls, with a cleaner interface. > > That's said, instead of adding more support for this obfuscated API, maybe > we could deprecate those in favor of some controls that could make more > sense, and let vidio_ioctl2 provide backward compat for it by calling the > proper controls, just to preserve binary compatibility with legacy > applications. I have mixed feelings about this. On one hand I agree that VIDIOC_S_JPEGCOMP is under-specified and should be either properly document, or deprecate it in favor of a control. On the other hand, the uvcvideo driver assumes that controls can all be read and written while streaming is in progress. I suppose other drivers (probably non-MPEG ones) were written with the same assumption in mind. Using a control to set JPEG compression would mean that a proper infrastructure would need to be put in place in those drivers to handle controls that influence video streaming. I'd appreciate your opinion on that. Cheers, Laurent Pinchart -- 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
