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@xxxxxxxxx>
> ---
> 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
--
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
