public inbox for linux-media@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCHv2] v4l2-common: add s_selection helper function
@ 2016-08-01 10:33 Hans Verkuil
  2016-08-04 14:03 ` Sakari Ailus
  0 siblings, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2016-08-01 10:33 UTC (permalink / raw)
  To: linux-media@vger.kernel.org, Tiffany Lin; +Cc: Laurent Pinchart, Sakari Ailus

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCHv2] v4l2-common: add s_selection helper function
  2016-08-01 10:33 [PATCHv2] v4l2-common: add s_selection helper function Hans Verkuil
@ 2016-08-04 14:03 ` Sakari Ailus
  2016-08-04 14:11   ` Hans Verkuil
  0 siblings, 1 reply; 10+ messages in thread
From: Sakari Ailus @ 2016-08-04 14:03 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media@vger.kernel.org, Tiffany Lin, Laurent Pinchart

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.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2] v4l2-common: add s_selection helper function
  2016-08-04 14:03 ` Sakari Ailus
@ 2016-08-04 14:11   ` Hans Verkuil
  2016-08-04 14:17     ` Sakari Ailus
  0 siblings, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2016-08-04 14:11 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media@vger.kernel.org, Tiffany Lin, Laurent Pinchart



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2] v4l2-common: add s_selection helper function
  2016-08-04 14:11   ` Hans Verkuil
@ 2016-08-04 14:17     ` Sakari Ailus
  2016-08-04 14:27       ` Hans Verkuil
  0 siblings, 1 reply; 10+ messages in thread
From: Sakari Ailus @ 2016-08-04 14:17 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media@vger.kernel.org, Tiffany Lin, Laurent Pinchart

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?

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2] v4l2-common: add s_selection helper function
  2016-08-04 14:17     ` Sakari Ailus
@ 2016-08-04 14:27       ` Hans Verkuil
  2016-08-04 14:38         ` Sakari Ailus
  0 siblings, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2016-08-04 14:27 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media@vger.kernel.org, Tiffany Lin, Laurent Pinchart



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2] v4l2-common: add s_selection helper function
  2016-08-04 14:27       ` Hans Verkuil
@ 2016-08-04 14:38         ` Sakari Ailus
  2016-08-04 14:47           ` Hans Verkuil
  0 siblings, 1 reply; 10+ messages in thread
From: Sakari Ailus @ 2016-08-04 14:38 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media@vger.kernel.org, Tiffany Lin, Laurent Pinchart

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.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2] v4l2-common: add s_selection helper function
  2016-08-04 14:38         ` Sakari Ailus
@ 2016-08-04 14:47           ` Hans Verkuil
  2016-08-04 14:59             ` Sakari Ailus
  0 siblings, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2016-08-04 14:47 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media@vger.kernel.org, Tiffany Lin, Laurent Pinchart



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2] v4l2-common: add s_selection helper function
  2016-08-04 14:47           ` Hans Verkuil
@ 2016-08-04 14:59             ` Sakari Ailus
  2016-08-05 13:57               ` Hans Verkuil
  0 siblings, 1 reply; 10+ messages in thread
From: Sakari Ailus @ 2016-08-04 14:59 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media@vger.kernel.org, Tiffany Lin, Laurent Pinchart

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@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2] v4l2-common: add s_selection helper function
  2016-08-04 14:59             ` Sakari Ailus
@ 2016-08-05 13:57               ` Hans Verkuil
  2016-08-09 10:38                 ` Tiffany Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2016-08-05 13:57 UTC (permalink / raw)
  To: Tiffany Lin; +Cc: Sakari Ailus, linux-media@vger.kernel.org, Laurent Pinchart

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2] v4l2-common: add s_selection helper function
  2016-08-05 13:57               ` Hans Verkuil
@ 2016-08-09 10:38                 ` Tiffany Lin
  0 siblings, 0 replies; 10+ messages in thread
From: Tiffany Lin @ 2016-08-09 10:38 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Sakari Ailus, linux-media@vger.kernel.org, Laurent Pinchart

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-08-09 10:38 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-01 10:33 [PATCHv2] v4l2-common: add s_selection helper function Hans Verkuil
2016-08-04 14:03 ` Sakari Ailus
2016-08-04 14:11   ` Hans Verkuil
2016-08-04 14:17     ` Sakari Ailus
2016-08-04 14:27       ` Hans Verkuil
2016-08-04 14:38         ` Sakari Ailus
2016-08-04 14:47           ` Hans Verkuil
2016-08-04 14:59             ` Sakari Ailus
2016-08-05 13:57               ` Hans Verkuil
2016-08-09 10:38                 ` Tiffany Lin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox