Em Tue, 9 Jun 2009 17:29:58 +0200
Laurent Pinchart <laurent.pinchart@xxxxxxxxx> escreveu:
>
> > 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 ;-)
It is more compact, and I used to write codes like above in the past. Yet, we
should stick on Kernel style at the kernelspace stuff.
About the readability, a code like:
ret = function;
if (ret < 0)
is just a little more readable than on a single line. However, if codes like
the above are accepted, it should be accepted things like:
if ((ret = functionX() * functionY() + 3) < 5 + 2 * functionZ())
that would be more complex to read than when broken into two separate statements:
ret = functionX() * functionY() + 3;
if (ret < 5 + 2 * functionZ())
Anyway, since Coding Style is global to the kernel as a hole, it is out of
topic to discuss about the rationale behind each rule, or proposing
improvements here. The proper forum for it is LKML.
> > > > 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.
Ok.
> 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.
I prefer the later. Adding a new ioctl support just to deprecate it on the next
kernel doesn't seem nice. Let's add directly the newer controls and add a patch
marking this as deprecated.
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
