Re: [PATCH] media: Add support for arbitrary resolution for the ov5642 camera driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tuesday, August 30, 2011 16:24:55 Guennadi Liakhovetski wrote:
> Hi Hans
> 
> On Tue, 30 Aug 2011, Hans Verkuil wrote:
> 
> > On Tuesday, August 30, 2011 15:13:25 Guennadi Liakhovetski wrote:
> > > (also replying to a similar comment by Sakari)
> > > 
> > > On Tue, 30 Aug 2011, Laurent Pinchart wrote:
> > > 
> > > > Hi Guennadi,
> > > > 
> > > > On Tuesday 30 August 2011 10:55:08 Guennadi Liakhovetski wrote:
> > > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote:
> > > > > > On Monday 29 August 2011 14:34:53 Guennadi Liakhovetski wrote:
> > > > > > > On Mon, 29 Aug 2011, Laurent Pinchart wrote:
> > > > > > > > On Monday 29 August 2011 14:18:50 Guennadi Liakhovetski wrote:
> > > > > > > > > On Sun, 28 Aug 2011, Laurent Pinchart wrote:
> > > > > > > > > 
> > > > > > > > > [snip]
> > > > > > > > > 
> > > > > > > > > > > @@ -774,17 +839,27 @@ static int ov5642_s_fmt(struct
> > > > > > > > > > > v4l2_subdev *sd,
> > > > > > > > > > > 
> > > > > > > > > > >  	ov5642_try_fmt(sd, mf);
> > > > > > > > > > > 
> > > > > > > > > > > +	priv->out_size.width		= mf->width;
> > > > > > > > > > > +	priv->out_size.height		= mf->height;
> > > > > > > > > > 
> > > > > > > > > > It looks like to me (but I may be wrong) that you achieve
> > > > > > > > > > different resolutions using cropping, not scaling. If 
that's
> > > > > > > > > > correct you should implement s_crop support and refuse 
> > changing
> > > > > > > > > > the resolution through s_fmt.
> > > > > > > > > 
> > > > > > > > > As the patch explains (I think) on several occasions, 
currently
> > > > > > > > > only the 1:1 scale is supported, and it was our deliberate 
> > choice
> > > > > > > > > to implement this using the scaling API
> > > > > > > > 
> > > > > > > > If you implement cropping, you should use the crop API, not 
the
> > > > > > > > scaling API
> > > > > > > > 
> > > > > > > > :-)
> > > > > > > 
> > > > > > > It's changing both - input and output sizes.
> > > > > > 
> > > > > > Sure, but it's still cropping.
> > > > > 
> > > > > Why? Isn't it a matter of the PoV?
> > > > 
> > > > No it isn't. Cropping is cropping, regardless of how you look at it.
> > > > 
> > > > > It changes the output window, i.e., implements S_FMT. And S_FMT is 
by 
> > far
> > > > > more important / widely used than S_CROP. Refusing to change the 
output
> > > > > window and always just returning the == crop size wouldn't be 
polite, 
> > IMHO.
> > > > 
> > > > If your sensor has no scaler the output size is equal to the crop 
> > rectangle. 
> > > > There's no way around that, and there's no reason to have the driver 
> > behave 
> > > > otherwise.
> > > > 
> > > > > Don't think many users would guess to use S_CROP.
> > > > 
> > > > Users who want to crop use S_CROP.
> > > > 
> > > > > Standard applications a la mplayer don't use S_CROP at all.
> > > > 
> > > > That's because they don't want to crop. mplayer expects the driver to 
> > perform 
> > > > scaling when it calls S_FMT, and users won't be happy if you crop 
instead.
> > > 
> > > So, here's my opinion, based on the V4L2 spec. I'm going to base on this 
> > > and pull this patch into my tree and let Mauro decide, unless he 
expresses 
> > > his negative opinion before that.
> > > 
> > > The spec defines S_FMT as an operation to set the output (in case of a 
> > > capture device) frame format. Which this driver clearly does. The output 
> > > format should be set, using scaling, however, if the driver or the 
> > > hardware are unable to preserve the exact same input rectangle to 
satisfy 
> > > the request, the driver is also allowed to change the cropping rectangle 
> > > _as much as necessary_ - S_FMT takes precedence. This has been discussed 
> > > before, and the conclusion was - of the two geometry calls (S_FMT and 
> > > S_CROP) the last call overrides previous setting of the opposite 
geometry.
> > > 
> > > It also defines S_CROP as an operation to set the cropping rectangle. 
The 
> > > driver is also allowed to change the output window, if it cannot be 
> > > preserved. Similarly, the last call wins.
> > > 
> > > Ideally in this situation I would implement both S_CROP and S_FMT and 
let 
> > > both change the opposite window as needed, which in this case means set 
it 
> > > equal to the one, being configured. Since most applications are 
primarily 
> > > interested in the S_FMT call to configure their user interface, I find 
it 
> > > a wrong approach to refuse S_FMT and always return the current cropping 
> > > rectangle. In such a case the application will possibly be stuck with 
some 
> > > default output rectangle, because it certainly will _not_ guess to use 
> > > S_CROP to configure it. Whereas if we implement S_FMT with a constant 
1:1 
> > > scale the application will get the required UI size. I agree, that 
> > > changing the view area, while changing the output window, is not exactly 
> > > what the user expects, but it's better than presenting all applications 
> > > with a fixed, possibly completely unsuitable, UI window.
> > 
> > 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.

> 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.

Regards,

	Hans
--
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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux