| Message ID | c6379bf1-4fdf-7deb-4312-86d26d0ee106@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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?
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." > 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. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > > > 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.
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? > >> >>> 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. Do you have some time tomorrow to discuss this on irc? Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Hi Tiffany, We just had a long discussion about whether or not -ERANGE should be returned if the constraint flags could not be satisfied, and the end result was that the driver should not return an error in that case, but just select a rectangle that works with the hardware and is closest to the requested rectangle. See the irc log for the discussion: https://linuxtv.org/irc/irclogger_log/v4l?date=2016-08-05,Fri This will simplify your code, and I'll drop this patch for a v4l2-common helper function, since that is no longer relevant. I will try to find time to fix the documentation (since that's wrong) and any drivers that do return ERANGE. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Fri, 2016-08-05 at 15:57 +0200, Hans Verkuil wrote: > Hi Tiffany, > > We just had a long discussion about whether or not -ERANGE should be returned > if the constraint flags could not be satisfied, and the end result was that > the driver should not return an error in that case, but just select a rectangle > that works with the hardware and is closest to the requested rectangle. > > See the irc log for the discussion: > > https://linuxtv.org/irc/irclogger_log/v4l?date=2016-08-05,Fri > > This will simplify your code, and I'll drop this patch for a v4l2-common helper > function, since that is no longer relevant. > > I will try to find time to fix the documentation (since that's wrong) and any > drivers that do return ERANGE. Got it. I will refine and send a new patch. best regards, Tiffany > Regards, > > Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index 5b80850..f7c34f6 100644 --- a/drivers/media/v4l2-core/v4l2-common.c +++ b/drivers/media/v4l2-core/v4l2-common.c @@ -61,6 +61,7 @@ #include <media/v4l2-common.h> #include <media/v4l2-device.h> #include <media/v4l2-ctrls.h> +#include <media/v4l2-rect.h> #include <linux/videodev2.h> @@ -371,6 +372,30 @@ void v4l_bound_align_image(u32 *w, unsigned int wmin, unsigned int wmax, } EXPORT_SYMBOL_GPL(v4l_bound_align_image); +/** + * v4l2_s_selection - Helper to check adjusted rectangle against constraint flags + * + * @s: pointer to &struct v4l2_selection containing the original rectangle + * @r: pointer to &struct v4l2_rect containing the adjusted rectangle. + * + * Returns -ERANGE if the adjusted rectangle doesn't fit the constraints + * or 0 if it is fine. On success it sets @s->r to @r. + */ +int v4l2_s_selection(struct v4l2_selection *s, const struct v4l2_rect *r) +{ + /* The original rect must lay inside the adjusted one */ + if ((s->flags & V4L2_SEL_FLAG_GE) && + !v4l2_rect_is_inside(&s->r, r)) + return -ERANGE; + /* The adjusted rect must lay inside the original one */ + if ((s->flags & V4L2_SEL_FLAG_LE) && + !v4l2_rect_is_inside(r, &s->r)) + return -ERANGE; + s->r = *r; + return 0; +} +EXPORT_SYMBOL_GPL(v4l2_s_selection); + const struct v4l2_frmsize_discrete *v4l2_find_nearest_format( const struct v4l2_discrete_probe *probe, s32 width, s32 height) diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h index 350cbf9..226e0cf 100644 --- a/include/media/v4l2-common.h +++ b/include/media/v4l2-common.h @@ -246,6 +246,8 @@ void v4l_bound_align_image(unsigned int *w, unsigned int wmin, unsigned int hmax, unsigned int halign, unsigned int salign); +int v4l2_s_selection(struct v4l2_selection *s, const struct v4l2_rect *r); + struct v4l2_discrete_probe { const struct v4l2_frmsize_discrete *sizes; int num_sizes; diff --git a/include/media/v4l2-rect.h b/include/media/v4l2-rect.h index d2125f0..6d9de07 100644 --- a/include/media/v4l2-rect.h +++ b/include/media/v4l2-rect.h @@ -95,6 +95,21 @@ static inline bool v4l2_rect_same_size(const struct v4l2_rect *r1, } /** + * v4l2_rect_is_inside() - return true if inner is inside outer + * @inner: rectangle. + * @outer: rectangle. + * + * Return true if @inner fits inside @outer. + */ +static inline bool v4l2_rect_is_inside(const struct v4l2_rect *inner, + const struct v4l2_rect *outer) +{ + return inner->left >= outer->left && inner->top >= outer->top && + inner->left + inner->width <= outer->left + outer->width && + inner->top + inner->height <= outer->top + outer->height; +} + +/** * v4l2_rect_intersect() - calculate the intersection of two rects. * @r: intersection of @r1 and @r2. * @r1: rectangle.
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. Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> --- v2: - renamed r1/r2 to inner/outer - moved documentation to source --- -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html