On Thu, Aug 04, 2016 at 04:47:46PM +0200, Hans Verkuil wrote: > > > On 08/04/2016 04:38 PM, Sakari Ailus wrote: > > Hi Hans, > > > > On Thu, Aug 04, 2016 at 04:27:27PM +0200, Hans Verkuil wrote: > >> > >> > >> On 08/04/2016 04:17 PM, Sakari Ailus wrote: > >>> On Thu, Aug 04, 2016 at 04:11:55PM +0200, Hans Verkuil wrote: > >>>> > >>>> > >>>> On 08/04/2016 04:03 PM, Sakari Ailus wrote: > >>>>> Hi Hans, > >>>>> > >>>>> On Mon, Aug 01, 2016 at 12:33:39PM +0200, Hans Verkuil wrote: > >>>>>> Checking the selection constraint flags is often forgotten by drivers, especially > >>>>>> if the selection code just clamps the rectangle to the minimum and maximum allowed > >>>>>> rectangles. > >>>>>> > >>>>>> This patch adds a simple helper function that checks the adjusted rectangle against > >>>>>> the constraint flags and either returns -ERANGE if it doesn't fit, or fills in the > >>>>>> new rectangle and returns 0. > >>>>>> > >>>>>> It also adds a small helper function to v4l2-rect.h to check if one rectangle fits > >>>>>> inside another. > >>>>> > >>>>> I could have misunderstood the purpose of the patch but... these flags are > >>>>> used by drivers in guidance in adjusting the rectangle in case there are > >>>>> hardware limitations, to make it larger or smaller than requested if the > >>>>> request can't be fulfillsed as such. The intent is *not* to return an error > >>>>> back to the user. In this respect it works quite like e.g. S_FMT does in > >>>>> cases an exact requested format can't be supported. > >>>>> > >>>>> <URL:https://www.linuxtv.org/downloads/v4l-dvb-apis/apb.html#v4l2-selection-flags> > >>>>> > >>>>> What can be done is rather driver specific. > >>>>> > >>>> > >>>> That's not what the spec says: > >>>> > >>>> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/vidioc-g-selection.html > >>>> > >>>> ERANGE > >>>> It is not possible to adjust struct v4l2_rect r rectangle to satisfy all constraints given in the flags argument. > >>>> > >>>> It's rather unambiguous, I think. > >>>> > >>>> If you don't want an error, then just leave 'flags' to 0. That makes sense. > >>> > >>> Does it? I can't imagine a use case for that. > >> > >> That's just the standard behavior: "I'd like this selection rectangle, but adjust > >> however you like it to something that works." > > > > That's not how this patch works though: it returns an error instead. > > No. If flags == 0, then it returns 0. Did you misread the patch? That's correct. But if you specify flags, instead of adjusting the rectangle the function in the patch returns an error. The purpose of adjusting the rectangle to a particular direction (either up or down) is that often in a pipeline scaling is only available either way. For cropping this is obvious. So if you need an image the size of which is no less than a given one, then you specify what you want and V4L2_SEL_FL_GE. The flags are there for convenience. What I don't quite understand is in which case would an error be preferred instead. > > > > >> > >>> The common section still defines these flags differently, and that's the > >>> behaviour on V4L2 sub-device interface. Do we have a driver that implements > >>> support for these flags as you described? > >>> > >> > >> A quick check: fimc-capture, gsc-m2m, am437, vivid, fimc-lite, bdisp. > >> > >> Note that VIDIOC_SUBDEV_S_SELECTION doesn't specify an ERANGE error, but I don't know > >> if that is intentional or an oversight. At least smiapp-core.c doesn't return an error. > > > > Please read the description of the flags in common documentation. The smiapp > > driver implements them as described in the common and V4L2 sub-device > > documentation: > > > > <URL:https://www.linuxtv.org/downloads/v4l-dvb-apis/subdev.html#v4l2-subdev-selections> > > <URL:https://www.linuxtv.org/downloads/v4l-dvb-apis/apb.html#v4l2-selection-flags> > > > > I.e. they affect rounding in the case where an exact match can't be found, > > hardware limitations taken into account. The V4L2 behaviour can be > > implemented using the common / sub-device flag definitions but not the other > > way around, so we don't necessary have a problem here. It's just that > > returning an error in such a case doesn't really make much sense. > > > > I think we need to clarify the spec first, since it is clearly ambiguous in some areas. Not ambiguous but internally conflicting. > Do you have some time tomorrow to discuss this on irc? Tomorrow works fine. -- Sakari Ailus e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx -- 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
