Hi Guennadi,
On Tuesday 30 August 2011 17:34:05 Guennadi Liakhovetski wrote:
> On Tue, 30 Aug 2011, Bastian Hecht wrote:
> > 2011/8/30 Guennadi Liakhovetski <g.liakhovetski@xxxxxx>:
> > > On Tue, 30 Aug 2011, Hans Verkuil wrote:
> > >> On Tuesday, August 30, 2011 16:24:55 Guennadi Liakhovetski wrote:
> > >> > Hi Hans
> > >
> > >> > On Tue, 30 Aug 2011, Hans Verkuil wrote:
> > > [snip]
> > >
> > >> > > The problem with S_FMT changing the crop rectangle (and I assume
> > >> > > we are not talking about small pixel tweaks to make the hardware
> > >> > > happy) is that the crop operation actually removes part of the
> > >> > > frame. That's not something you would expect S_FMT to do, ever.
> > >> > > Such an operation has to be explicitly requested by the user.
> > >> > >
> > >> > > It's also why properly written applications (e.g.
> > >> > > capture-example.c) has code like this to reset the crop rectangle
> > >> > > before starting streaming:
> > >> > >
> > >> > > if (0 == xioctl(fd, VIDIOC_CROPCAP, &cropcap)) {
> > >> > > crop.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > >> > > crop.c = cropcap.defrect; /* reset to default */
> > >> > >
> > >> > > if (-1 == xioctl(fd, VIDIOC_S_CROP, &crop)) {
> > >> > > switch (errno) {
> > >> > > case EINVAL:
> > >> > > /* Cropping not supported. */
> > >> > > break;
> > >> > > default:
> > >> > > /* Errors ignored. */
> > >> > > break;
> > >> > > }
> > >> > > }
> > >> > > }
> > >> > >
> > >> > > (Hmm, capture-example.c should also test for ENOTTY since we
> > >> > > changed the error code).
> > >> >
> > >> > I agree, that preserving input rectangle == output rectangle in
> > >> > reply to S_FMT is not nice, and should be avoided, wherever
> > >> > possible. Still, I prefer this to sticking with just one fixed
> > >> > output geometry, especially since (1) the spec doesn't prohibit
> > >> > this behaviour,
> > >>
> > >> Hmm, I think it should be prohibited. Few drivers actually implement
> > >> crop, and fewer applications use it. So I'm not surprised the spec
> > >> doesn't go into much detail.
> > >>
> > >> > (2) there are already
> > >> > precedents in the mainline.
> > >>
> > >> Which precedents? My guess is that any driver that does this was
> > >> either not (or poorly) reviewed, or everyone just missed it.
> > >
> > > My first two sensor drivers mt9m001 and mt9v022 do this, but, I
> > > suspect, I didn't invent it at that time, I think, I copied it from
> > > somewhere, cannot say for sure though anymore.
> > >
> > >> > Maybe, a bit of hardware background would help: the sensor is
> > >> > actually supposed to be able to both crop and scale, and we did try
> > >> > to implement scales other than 1:1, but the chip just refused to
> > >> > produce anything meaningful.
> > >>
> > >> I still don't see any reason why S_FMT would suddenly crop on such a
> > >> sensor. It's completely unexpected and the user does not get what he
> > >> expects.
> > >
> > > Good, let's make it simple for all (except Bastian) then: Bastian,
> > > sorry for having misguided you, please, switch to .s_crop().
> >
> > Sure, no problem. So s_fmt() shall be not available at all then,
> > right? Instead the cropping rectangle can be changed and the output
> > rectangle is adjusted accordingly.
>
> I would keep .s_fmt() and just return the current configuration from it.
That's what I would do as well. Having no .s_fmt() would be very confusing for
applications and/or bridges.
--
Regards,
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
