On Tuesday 09 June 2009 17:56:01 Mauro Carvalho Chehab wrote:
> 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.
I'm not sure whether we can deprecate JPEGCOMP. It is used in too many
places. Perhaps we should create a set of JPEG controls that match what is
in v4l2_jpegcompression and convert all the drivers that use JPEGCOMP to
these new controls. Then the v4l core can map S/G_JPEGCOMP ioctls to a set
of control read/writes. I'm working on string control support, so that will
allow us to handle the APP_data and COM_data fields.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
--
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
