Em Mon, 8 Jun 2009 16:15:57 +0200 Laurent Pinchart <laurent.pinchart@xxxxxxxxx> escreveu: > 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). With hg, there aren't any alternatives, AFAIK, since "hg push -f" won't delete an object that were removed locally with hg strip (or changed via hg mq). > > > + 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. Ok. > > > + 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. Checkpatch doesn't catch all problems (and, sometimes, a new version "forgets" to report some errors that previous versions used to report). > > > 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) I prefer if you fix it, although I won't nack your pull just due to that. > > 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. Try to count the proper coding style over the kernel source code. I bet they you'll find much more cases of: ret = function(); if (ret < 0) /* and other similar tests, like ret == NULL, ret, !ret, etc */ foo; Since uvc is part of the kernel, in fact, it is uvcvideo that is adding more exceptions to the Kernel style. > > 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. I'm in favor of deprecate VIDIOC_S_JPEGCOMP. The issue you've described of changing the format while streaming exists, being it a control, or an ioctl. So, it deserves a separate discussion. - The MPEG drivers block trials of changing the MPEG format that can't be done while streaming. On the other hand, user preference controls, like bright, contrast, (auto)gain, exposure, etc can be done anytime. As most drivers just exposes such controls, they don't need a logic to block a control while streaming. The case of JPEG/MJPEG is similar to MPEG: there are some changes that may not be possible while streaming. So, such changes should, in thesis, be blocked. On the other hand, from users perspective, it seems interesting for they to start an application and, for example, change the JPEG quality while streaming, to see what better fits his needs. Since JPEG is just a set of independent jpeg images, in thesis, it is possible to change the encoding while streaming, without breaking for userspace, assuming that the allocated buffers are large enough to support such changes. So, IMO, we should try to allow such changes where possible, even by doing something like this, at the loop that fills the video buffer: if (streaming && frame_is_complete && quality_changed) { stop_streaming(); change_quality(); start_streaming(); } Cheers, Mauro -- 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
