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
