[PATCH] v4l2-common: add s_selection helper function

[PATCH] v4l2-common: add s_selection helper function

[Hans Verkuil]

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>
---
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 5b80850..a2e5119 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,21 @@ void v4l_bound_align_image(u32 *w, unsigned int wmin, unsigned int wmax,
 }
 EXPORT_SYMBOL_GPL(v4l_bound_align_image);

+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..cfa9cbf 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -246,6 +246,17 @@ void v4l_bound_align_image(unsigned int *w, unsigned int wmin,
 			   unsigned int hmax, unsigned int halign,
 			   unsigned int salign);

+/**
+ * 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);
+
 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..858c8cb 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 r1 is inside r2
+ * @r1: rectangle.
+ * @r2: rectangle.
+ *
+ * Return true if r1 fits inside r2.
+ */
+static inline bool v4l2_rect_is_inside(const struct v4l2_rect *r1,
+				       const struct v4l2_rect *r2)
+{
+	return r1->left >= r2->left && r1->top >= r2->top &&
+	       r1->left + r1->width <= r2->left + r2->width &&
+	       r1->top + r1->height <= r2->top + r2->height;
+}
+
+/**
  * v4l2_rect_intersect() - calculate the intersection of two rects.
  * @r: intersection of @r1 and @r2.
  * @r1: rectangle.

Re: [PATCH] v4l2-common: add s_selection helper function

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Monday 01 Aug 2016 10:45:30 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.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> diff --git a/drivers/media/v4l2-core/v4l2-common.c
> b/drivers/media/v4l2-core/v4l2-common.c index 5b80850..a2e5119 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,21 @@ void v4l_bound_align_image(u32 *w, unsigned int wmin,
> unsigned int wmax, }
>  EXPORT_SYMBOL_GPL(v4l_bound_align_image);
> 
> +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;

I'd like to see how this function is used in drivers.

> +	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..cfa9cbf 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -246,6 +246,17 @@ void v4l_bound_align_image(unsigned int *w, unsigned
> int wmin, unsigned int hmax, unsigned int halign,
>  			   unsigned int salign);
> 
> +/**
> + * 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.
> + */

Part of the functions are documented in the header and part in the 
implementation. We need to pick one option, and I prefer the latter one (which 
is also more consistent with how functions are documented in most subsystems).

> +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..858c8cb 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 r1 is inside r2
> + * @r1: rectangle.
> + * @r2: rectangle.
> + *
> + * Return true if r1 fits inside r2.
> + */
> +static inline bool v4l2_rect_is_inside(const struct v4l2_rect *r1,
> +				       const struct v4l2_rect *r2)

How about calling the arguments inner and outer to make the purpose of each 
argument more explicit from the function prototype ?

Also, I would name the function v4l2_rect_is_contained(), or possibly 
v4l2_rect_contains() in which case the arguments should be switched. It should 
also be noted that C doesn't provide support for function overloading so we 
can't have

/* Rectangle contains rectangle */
bool v4l2_rect_contains(const struct v4l2_rect *outer,
			const struct v4l2_rect *inner);
/* Rectangle contains point */
bool v4l2_rect_contains(const struct v4l2_rect *rect,
			unsigned int x, unsigned int y);

Maybe we should thus name the function v4l2_rect_contains_rect() in prevision 
for a future v4l2_rect_contains_point() ?

> +{
> +	return r1->left >= r2->left && r1->top >= r2->top &&
> +	       r1->left + r1->width <= r2->left + r2->width &&
> +	       r1->top + r1->height <= r2->top + r2->height;

Isn't that's a big long for an inline function ?

> +}
> +
> +/**
>   * v4l2_rect_intersect() - calculate the intersection of two rects.
>   * @r: intersection of @r1 and @r2.
>   * @r1: rectangle.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] v4l2-common: add s_selection helper function

[Hans Verkuil]

On 08/01/2016 10:57 AM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Monday 01 Aug 2016 10:45:30 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.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>> diff --git a/drivers/media/v4l2-core/v4l2-common.c
>> b/drivers/media/v4l2-core/v4l2-common.c index 5b80850..a2e5119 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,21 @@ void v4l_bound_align_image(u32 *w, unsigned int wmin,
>> unsigned int wmax, }
>>  EXPORT_SYMBOL_GPL(v4l_bound_align_image);
>>
>> +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;
> 
> I'd like to see how this function is used in drivers.

See my comments here:

https://patchwork.linuxtv.org/patch/35794/

> 
>> +	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..cfa9cbf 100644
>> --- a/include/media/v4l2-common.h
>> +++ b/include/media/v4l2-common.h
>> @@ -246,6 +246,17 @@ void v4l_bound_align_image(unsigned int *w, unsigned
>> int wmin, unsigned int hmax, unsigned int halign,
>>  			   unsigned int salign);
>>
>> +/**
>> + * 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.
>> + */
> 
> Part of the functions are documented in the header and part in the 
> implementation. We need to pick one option, and I prefer the latter one (which 
> is also more consistent with how functions are documented in most subsystems).

I'll move it.

> 
>> +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..858c8cb 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 r1 is inside r2
>> + * @r1: rectangle.
>> + * @r2: rectangle.
>> + *
>> + * Return true if r1 fits inside r2.
>> + */
>> +static inline bool v4l2_rect_is_inside(const struct v4l2_rect *r1,
>> +				       const struct v4l2_rect *r2)
> 
> How about calling the arguments inner and outer to make the purpose of each 
> argument more explicit from the function prototype ?

Much better, thanks.

> Also, I would name the function v4l2_rect_is_contained(), or possibly 
> v4l2_rect_contains() in which case the arguments should be switched. It should 
> also be noted that C doesn't provide support for function overloading so we 
> can't have
> 
> /* Rectangle contains rectangle */
> bool v4l2_rect_contains(const struct v4l2_rect *outer,
> 			const struct v4l2_rect *inner);
> /* Rectangle contains point */
> bool v4l2_rect_contains(const struct v4l2_rect *rect,
> 			unsigned int x, unsigned int y);
> 
> Maybe we should thus name the function v4l2_rect_contains_rect() in prevision 
> for a future v4l2_rect_contains_point() ?

I prefer is_inside. The name implies that one rect is inside another
whereas 'contains' just suggests that some object is contained by a rect.
Also, the 'inside' terminology is already used in v4l2-rect.h.

> 
>> +{
>> +	return r1->left >= r2->left && r1->top >= r2->top &&
>> +	       r1->left + r1->width <= r2->left + r2->width &&
>> +	       r1->top + r1->height <= r2->top + r2->height;
> 
> Isn't that's a big long for an inline function ?

I don't think so.

> 
>> +}
>> +
>> +/**
>>   * v4l2_rect_intersect() - calculate the intersection of two rects.
>>   * @r: intersection of @r1 and @r2.
>>   * @r1: rectangle.
> 

Regards,

	Hans