Hi Mauro,
On Tuesday 09 June 2009 17:01:14 Mauro Carvalho Chehab wrote:
> Em Mon, 8 Jun 2009 16:15:57 +0200
>
> Laurent Pinchart <laurent.pinchart@xxxxxxxxx> escreveu:
> > On Monday 08 June 2009 03:52:12 Mauro Carvalho Chehab wrote:
> > > 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.
I'll fix the syntax.
> > 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.
Damn, I hoped you wouldn't notice that ;-)
I find the
if ((ret = function()) < 0)
more compact (which can't be debated) and as easy to read as the alternative,
if not easier (now this can be debated). As this point of view seems to be
minority I'll try to adjust my coding style accordingly. Of course I'll blame
you personally for any bug that it would introduce ;-)
> > > 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();
> }
This can only be done if the allocated buffers are big enough. As the device
reports the required buffer size the driver would have to query the device
before deciding if it allows to change the quality during streaming. Given the
usefulness of changing MJPEG compression quality during streaming, and given
how most webcams seem to ignore the quality anyway, I don't think it would be
worth it at the moment.
Should I submit a patch that implement VIDIOC_S_JPEGCOMP support in the UVC
driver and implement a JPEG compression quality control later, or would you
prefer the driver not to implement VIDIOC_S_JPEGCOMP at all ? As there are
existing applications using that ioctl a few users are pushing for
VIDIOC_S_JPEGCOMP support.
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
