[PATCH] videodev2.h: add helper to validate colorspace

[PATCH] videodev2.h: add helper to validate colorspace

[Niklas Söderlund]

There is no way for drivers to validate a colorspace value, which could
be provided by user-space by VIDIOC_S_FMT for example. Add a helper to
validate that the colorspace value is part of enum v4l2_colorspace.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 include/uapi/linux/videodev2.h | 5 +++++
 1 file changed, 5 insertions(+)

Hi,

I hope this is the correct header to add this helper to. I think it's 
since if it's in uapi not only can v4l2 drivers use it but tools like 
v4l-compliance gets access to it and can be updated to use this instead 
of the hard-coded check of just < 0xff as it was last time I checked.

// Niklas

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 9827189651801e12..843afd7c5b000553 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -238,6 +238,11 @@ enum v4l2_colorspace {
 	V4L2_COLORSPACE_DCI_P3        = 12,
 };
 
+/* Determine if a colorspace is defined in enum v4l2_colorspace */
+#define V4L2_COLORSPACE_IS_VALID(colorspace)		\
+	(((colorspace) >= V4L2_COLORSPACE_DEFAULT) &&	\
+	 ((colorspace) <= V4L2_COLORSPACE_DCI_P3))
+
 /*
  * Determine how COLORSPACE_DEFAULT should map to a proper colorspace.
  * This depends on whether this is a SDTV image (use SMPTE 170M), an
-- 
2.16.1

Re: [PATCH] videodev2.h: add helper to validate colorspace

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wednesday, 14 February 2018 00:08:47 EET Niklas Söderlund wrote:
> There is no way for drivers to validate a colorspace value, which could
> be provided by user-space by VIDIOC_S_FMT for example. Add a helper to
> validate that the colorspace value is part of enum v4l2_colorspace.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  include/uapi/linux/videodev2.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> Hi,
> 
> I hope this is the correct header to add this helper to. I think it's
> since if it's in uapi not only can v4l2 drivers use it but tools like
> v4l-compliance gets access to it and can be updated to use this instead
> of the hard-coded check of just < 0xff as it was last time I checked.
> 
> // Niklas
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 9827189651801e12..843afd7c5b000553 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -238,6 +238,11 @@ enum v4l2_colorspace {
>  	V4L2_COLORSPACE_DCI_P3        = 12,
>  };
> 
> +/* Determine if a colorspace is defined in enum v4l2_colorspace */
> +#define V4L2_COLORSPACE_IS_VALID(colorspace)		\
> +	(((colorspace) >= V4L2_COLORSPACE_DEFAULT) &&	\
> +	 ((colorspace) <= V4L2_COLORSPACE_DCI_P3))
> +

This looks pretty good to me. I'd remove the parentheses around each test 
though.

One potential issue is that if this macro operates on an unsigned value (for 
instance an u32, which is the type used for the colorspace field in various 
structures) the compiler will generate a warning:

enum.c: In function ‘test_4’:                                                                                                                                                                             
enum.c:30:16: warning: comparison of unsigned expression >= 0 is always true 
[-Wtype-limits]                                                                                                              
  return V4L2_COLORSPACE_IS_VALID(colorspace);

Dropping the first check would fix that, but wouldn't catch invalid values 
when operating on a signed type, such as int or enum v4l2_colorspace.

>  /*
>   * Determine how COLORSPACE_DEFAULT should map to a proper colorspace.
>   * This depends on whether this is a SDTV image (use SMPTE 170M), an

-- 
Regards,

Laurent Pinchart

Re: [PATCH] videodev2.h: add helper to validate colorspace

[Sakari Ailus]

Hi Laurent and Niklas,

On Wed, Feb 14, 2018 at 12:23:05AM +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Wednesday, 14 February 2018 00:08:47 EET Niklas Söderlund wrote:
> > There is no way for drivers to validate a colorspace value, which could
> > be provided by user-space by VIDIOC_S_FMT for example. Add a helper to
> > validate that the colorspace value is part of enum v4l2_colorspace.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  include/uapi/linux/videodev2.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > Hi,
> > 
> > I hope this is the correct header to add this helper to. I think it's
> > since if it's in uapi not only can v4l2 drivers use it but tools like
> > v4l-compliance gets access to it and can be updated to use this instead
> > of the hard-coded check of just < 0xff as it was last time I checked.
> > 
> > // Niklas
> > 
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 9827189651801e12..843afd7c5b000553 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -238,6 +238,11 @@ enum v4l2_colorspace {
> >  	V4L2_COLORSPACE_DCI_P3        = 12,
> >  };
> > 
> > +/* Determine if a colorspace is defined in enum v4l2_colorspace */
> > +#define V4L2_COLORSPACE_IS_VALID(colorspace)		\
> > +	(((colorspace) >= V4L2_COLORSPACE_DEFAULT) &&	\
> > +	 ((colorspace) <= V4L2_COLORSPACE_DCI_P3))
> > +
> 
> This looks pretty good to me. I'd remove the parentheses around each test 
> though.

Agreed.

> 
> One potential issue is that if this macro operates on an unsigned value (for 
> instance an u32, which is the type used for the colorspace field in various 
> structures) the compiler will generate a warning:
> 
> enum.c: In function ‘test_4’:                                                                                                                                                                             
> enum.c:30:16: warning: comparison of unsigned expression >= 0 is always true 
> [-Wtype-limits]                                                                                                              
>   return V4L2_COLORSPACE_IS_VALID(colorspace);
> 
> Dropping the first check would fix that, but wouldn't catch invalid values 
> when operating on a signed type, such as int or enum v4l2_colorspace.

How about simply casting it to u32 first (and removing the first test)?

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi