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

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Mon, May 06, 2019 at 04:15:27PM -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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Changes in v2: rename enum formatFamily to enum FormatFamily
> 
>  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..c18871e 100644
> --- a/src/qcam/format_converter.h
> +++ b/src/qcam/format_converter.h
> @@ -14,6 +14,12 @@ class QImage;
>  class FormatConverter
>  {
>  public:
> +	enum FormatFamily {
> +		MJPEG,
> +		RGB,
> +		YUV,
> +	};
> +

You only use this enum inside the class, you could move it to the
private section.

>  	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