[libcamera-devel] [PATCH 1/2] qcam: format_converter: Add formatFamily enum

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sat, May 04, 2019 at 04:33:53PM -0400, Paul Elder wrote:
> It is not sustainable to add a new flag for every new video format
> family (eg. YUYV, RGB, NV, MJPEG, etc), so add a formatFamily enum to
> indicate these in the FormatConverter.
> 
> Note that formats are grouped into families based on if they are able to
> share a set of parameters for conversion.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/qcam/format_converter.cpp | 27 ++++++++++++++++-----------
>  src/qcam/format_converter.h   | 11 ++++++++++-
>  2 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp
> index d9088c3..192767c 100644
> --- a/src/qcam/format_converter.cpp
> +++ b/src/qcam/format_converter.cpp
> @@ -32,48 +32,48 @@ int FormatConverter::configure(unsigned int format, unsigned int width,
>  {
>  	switch (format) {
>  	case V4L2_PIX_FMT_BGR24:
> -		yuv_ = false;
> +		formatFamily_ = RGB;
>  		r_pos_ = 2;
>  		g_pos_ = 1;
>  		b_pos_ = 0;
>  		bpp_ = 3;
>  		break;
>  	case V4L2_PIX_FMT_RGB24:
> -		yuv_ = false;
> +		formatFamily_ = RGB;
>  		r_pos_ = 0;
>  		g_pos_ = 1;
>  		b_pos_ = 2;
>  		bpp_ = 3;
>  		break;
>  	case V4L2_PIX_FMT_ARGB32:
> -		yuv_ = false;
> +		formatFamily_ = RGB;
>  		r_pos_ = 1;
>  		g_pos_ = 2;
>  		b_pos_ = 3;
>  		bpp_ = 4;
>  		break;
>  	case V4L2_PIX_FMT_VYUY:
> -		yuv_ = true;
> +		formatFamily_ = YUV;
>  		y_pos_ = 1;
>  		cb_pos_ = 2;
>  		break;
>  	case V4L2_PIX_FMT_YVYU:
> -		yuv_ = true;
> +		formatFamily_ = YUV;
>  		y_pos_ = 0;
>  		cb_pos_ = 3;
>  		break;
>  	case V4L2_PIX_FMT_UYVY:
> -		yuv_ = true;
> +		formatFamily_ = YUV;
>  		y_pos_ = 1;
>  		cb_pos_ = 0;
>  		break;
>  	case V4L2_PIX_FMT_YUYV:
> -		yuv_ = true;
> +		formatFamily_ = YUV;
>  		y_pos_ = 0;
>  		cb_pos_ = 1;
>  		break;
>  	case V4L2_PIX_FMT_MJPEG:
> -		yuv_ = false;
> +		formatFamily_ = MJPEG;
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -89,12 +89,17 @@ int FormatConverter::configure(unsigned int format, unsigned int width,
>  void FormatConverter::convert(const unsigned char *src, size_t size,
>  			      QImage *dst)
>  {
> -	if (format_ == V4L2_PIX_FMT_MJPEG)
> +	switch (formatFamily_) {
> +	case MJPEG:
>  		dst->loadFromData(src, size, "JPEG");
> -	else if (yuv_)
> +		break;
> +	case YUV:
>  		convertYUV(src, dst->bits());
> -	else
> +		break;
> +	case RGB:
>  		convertRGB(src, dst->bits());
> +		break;
> +	};
>  }
>  
>  void FormatConverter::convertRGB(const unsigned char *src, unsigned char *dst)
> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h
> index 76cd9f1..9820982 100644
> --- a/src/qcam/format_converter.h
> +++ b/src/qcam/format_converter.h
> @@ -14,6 +14,12 @@ class QImage;
>  class FormatConverter
>  {
>  public:
> +	enum formatFamily {

All type names should start with an uppercase letter, so this should be
FormatFamily (don't forget to update the one in the commit message). The
field name below is fine.

Apart from this,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		MJPEG,
> +		RGB,
> +		YUV,
> +	};
> +
>  	int configure(unsigned int format, unsigned int width,
>  		      unsigned int height);
>  
> @@ -27,12 +33,15 @@ private:
>  	unsigned int width_;
>  	unsigned int height_;
>  
> +	enum formatFamily formatFamily_;
> +
> +	/* RGB parameters */
>  	unsigned int bpp_;
>  	unsigned int r_pos_;
>  	unsigned int g_pos_;
>  	unsigned int b_pos_;
>  
> -	bool yuv_;
> +	/* YUV parameters */
>  	unsigned int y_pos_;
>  	unsigned int cb_pos_;
>  };

-- 
Regards,

Laurent Pinchart