[libcamera-devel,v2,2/2] qcam: format_converter: Add NV formats support
diff mbox series

Message ID 20190506201528.24445-2-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • [libcamera-devel,v2,1/2] qcam: format_converter: Add formatFamily enum
Related show

Commit Message

Paul Elder May 6, 2019, 8:15 p.m. UTC 
Add support for some NV formats:
- V4L2_PIX_FMT_NV12, V4L2_PIX_FMT_NV21
- V4L2_PIX_FMT_NV16, V4L2_PIX_FMT_NV61
- V4L2_PIX_FMT_NV24, V4L2_PIX_FMT_NV42

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
Changes in v2:
- reorder functions in format_converter.cpp to match format_converter.h
  (move yuv_to_rgb() to be before all specialized convert functions)
- renamed NV conversion parameters from xDownSample_ and yDownSample_ to
  horzSubSample_ and vertSubSample_, respectively
- unrolled the loop and simplify some of the calculations in convertNV()
  and achieved a 1.67 speedup compared to v1

 src/qcam/format_converter.cpp | 99 +++++++++++++++++++++++++++++++----
 src/qcam/format_converter.h   |  7 +++
 2 files changed, 96 insertions(+), 10 deletions(-)

Comments

Laurent Pinchart May 6, 2019, 8:30 p.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Mon, May 06, 2019 at 04:15:28PM -0400, Paul Elder wrote:
> Add support for some NV formats:
> - V4L2_PIX_FMT_NV12, V4L2_PIX_FMT_NV21
> - V4L2_PIX_FMT_NV16, V4L2_PIX_FMT_NV61
> - V4L2_PIX_FMT_NV24, V4L2_PIX_FMT_NV42
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> Changes in v2:
> - reorder functions in format_converter.cpp to match format_converter.h
>   (move yuv_to_rgb() to be before all specialized convert functions)
> - renamed NV conversion parameters from xDownSample_ and yDownSample_ to
>   horzSubSample_ and vertSubSample_, respectively
> - unrolled the loop and simplify some of the calculations in convertNV()
>   and achieved a 1.67 speedup compared to v1
> 
>  src/qcam/format_converter.cpp | 99 +++++++++++++++++++++++++++++++----
>  src/qcam/format_converter.h   |  7 +++
>  2 files changed, 96 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp
> index 192767c..eab222f 100644
> --- a/src/qcam/format_converter.cpp
> +++ b/src/qcam/format_converter.cpp
> @@ -72,6 +72,42 @@ int FormatConverter::configure(unsigned int format, unsigned int width,
>  		y_pos_ = 0;
>  		cb_pos_ = 1;
>  		break;
> +	case V4L2_PIX_FMT_NV12:
> +		formatFamily_ = NV;
> +		horzSubSample_ = 2;
> +		vertSubSample_ = 2;
> +		nvSwap_ = false;
> +		break;
> +	case V4L2_PIX_FMT_NV21:
> +		formatFamily_ = NV;
> +		horzSubSample_ = 2;
> +		vertSubSample_ = 2;
> +		nvSwap_ = true;
> +		break;
> +	case V4L2_PIX_FMT_NV16:
> +		formatFamily_ = NV;
> +		horzSubSample_ = 2;
> +		vertSubSample_ = 1;
> +		nvSwap_ = false;
> +		break;
> +	case V4L2_PIX_FMT_NV61:
> +		formatFamily_ = NV;
> +		horzSubSample_ = 2;
> +		vertSubSample_ = 1;
> +		nvSwap_ = true;
> +		break;
> +	case V4L2_PIX_FMT_NV24:
> +		formatFamily_ = NV;
> +		horzSubSample_ = 1;
> +		vertSubSample_ = 1;
> +		nvSwap_ = false;
> +		break;
> +	case V4L2_PIX_FMT_NV42:
> +		formatFamily_ = NV;
> +		horzSubSample_ = 1;
> +		vertSubSample_ = 1;
> +		nvSwap_ = true;
> +		break;
>  	case V4L2_PIX_FMT_MJPEG:
>  		formatFamily_ = MJPEG;
>  		break;
> @@ -99,9 +135,62 @@ void FormatConverter::convert(const unsigned char *src, size_t size,
>  	case RGB:
>  		convertRGB(src, dst->bits());
>  		break;
> +	case NV:
> +		convertNV(src, dst->bits());
> +		break;

You maye want to reorder the cases to match the order of the functions
as well as the order of the enum.

>  	};
>  }
>  
> +static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)
> +{
> +	int c = y - 16;
> +	int d = u - 128;
> +	int e = v - 128;
> +	*r = CLIP(( 298 * c           + 409 * e + 128) >> RGBSHIFT);
> +	*g = CLIP(( 298 * c - 100 * d - 208 * e + 128) >> RGBSHIFT);
> +	*b = CLIP(( 298 * c + 516 * d           + 128) >> RGBSHIFT);
> +}
> +
> +void FormatConverter::convertNV(const unsigned char *src, unsigned char *dst)
> +{
> +	unsigned int x, y;

No need to define those variables at the top level scope, you can define
them inside the for statement (as in "for (unsigne int x = 0; ...)").

> +	int r, g, b;
> +
> +	unsigned int c_stride = width_ * (2 / horzSubSample_);
> +	unsigned int c_inc = horzSubSample_ == 1 ? 2 : 0;
> +	unsigned int cb_pos = nvSwap_ ? 1 : 0;
> +	unsigned int cr_pos = nvSwap_ ? 0 : 1;
> +	unsigned int base = width_ * height_;

Small improvement I believe, you could add

	const unsigned char *src_c = src + width_ * height_;

and remove the base variable.

> +
> +	for (y = 0; y < height_; y++) {
> +		unsigned char *src_y = (unsigned char *)src + y * width_;

Don't blindly cast a const pointer to non-const, that's asking for
trouble (furthermore, C++-type casts should otherwise have been used,
with const_cast<unsigned char*>(src)). YOu can declare src_y as const
unsigned char * and everything should be fine.

> +		unsigned char *src_cb = (unsigned char *)src + base + (y / vertSubSample_) * c_stride + cb_pos;

While exceptions are allowed, we still try to cap the line length to 80
characters when it doesn't hinder readability.

> +		unsigned char *src_cr = (unsigned char *)src + base + (y / vertSubSample_) * c_stride + cr_pos;
> +
> +		for (x = 0; x < width_; x += 2) {
> +			yuv_to_rgb(*src_y, *src_cb, *src_cr, &r, &g, &b);
> +			dst[0] = b;
> +			dst[1] = g;
> +			dst[2] = r;
> +			dst[3] = 0xff;
> +			src_y++;
> +			src_cb += c_inc;
> +			src_cr += c_inc;
> +			dst += 4;
> +
> +			yuv_to_rgb(*src_y, *src_cb, *src_cr, &r, &g, &b);
> +			dst[0] = b;
> +			dst[1] = g;
> +			dst[2] = r;
> +			dst[3] = 0xff;
> +			src_y++;
> +			src_cb += 2;
> +			src_cr += 2;
> +			dst += 4;
> +		}
> +	}
> +}
> +
>  void FormatConverter::convertRGB(const unsigned char *src, unsigned char *dst)
>  {
>  	unsigned int x, y;
> @@ -124,16 +213,6 @@ void FormatConverter::convertRGB(const unsigned char *src, unsigned char *dst)
>  	}
>  }
>  
> -static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)
> -{
> -	int c = y - 16;
> -	int d = u - 128;
> -	int e = v - 128;
> -	*r = CLIP(( 298 * c           + 409 * e + 128) >> RGBSHIFT);
> -	*g = CLIP(( 298 * c - 100 * d - 208 * e + 128) >> RGBSHIFT);
> -	*b = CLIP(( 298 * c + 516 * d           + 128) >> RGBSHIFT);
> -}
> -
>  void FormatConverter::convertYUV(const unsigned char *src, unsigned char *dst)
>  {
>  	unsigned int src_x, src_y, dst_x, dst_y;
> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h
> index c18871e..ad67081 100644
> --- a/src/qcam/format_converter.h
> +++ b/src/qcam/format_converter.h
> @@ -16,6 +16,7 @@ class FormatConverter
>  public:
>  	enum FormatFamily {
>  		MJPEG,
> +		NV,
>  		RGB,
>  		YUV,
>  	};
> @@ -26,6 +27,7 @@ public:
>  	void convert(const unsigned char *src, size_t size, QImage *dst);
>  
>  private:
> +	void convertNV(const unsigned char *src, unsigned char *dst);
>  	void convertRGB(const unsigned char *src, unsigned char *dst);
>  	void convertYUV(const unsigned char *src, unsigned char *dst);
>  
> @@ -35,6 +37,11 @@ private:
>  
>  	enum FormatFamily formatFamily_;
>  
> +	/* NV parameters */
> +	unsigned int horzSubSample_;
> +	unsigned int vertSubSample_;
> +	bool nvSwap_;
> +
>  	/* RGB parameters */
>  	unsigned int bpp_;
>  	unsigned int r_pos_;

Patch
diff mbox series 

diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp
index 192767c..eab222f 100644
--- a/src/qcam/format_converter.cpp
+++ b/src/qcam/format_converter.cpp
@@ -72,6 +72,42 @@  int FormatConverter::configure(unsigned int format, unsigned int width,
 		y_pos_ = 0;
 		cb_pos_ = 1;
 		break;
+	case V4L2_PIX_FMT_NV12:
+		formatFamily_ = NV;
+		horzSubSample_ = 2;
+		vertSubSample_ = 2;
+		nvSwap_ = false;
+		break;
+	case V4L2_PIX_FMT_NV21:
+		formatFamily_ = NV;
+		horzSubSample_ = 2;
+		vertSubSample_ = 2;
+		nvSwap_ = true;
+		break;
+	case V4L2_PIX_FMT_NV16:
+		formatFamily_ = NV;
+		horzSubSample_ = 2;
+		vertSubSample_ = 1;
+		nvSwap_ = false;
+		break;
+	case V4L2_PIX_FMT_NV61:
+		formatFamily_ = NV;
+		horzSubSample_ = 2;
+		vertSubSample_ = 1;
+		nvSwap_ = true;
+		break;
+	case V4L2_PIX_FMT_NV24:
+		formatFamily_ = NV;
+		horzSubSample_ = 1;
+		vertSubSample_ = 1;
+		nvSwap_ = false;
+		break;
+	case V4L2_PIX_FMT_NV42:
+		formatFamily_ = NV;
+		horzSubSample_ = 1;
+		vertSubSample_ = 1;
+		nvSwap_ = true;
+		break;
 	case V4L2_PIX_FMT_MJPEG:
 		formatFamily_ = MJPEG;
 		break;
@@ -99,9 +135,62 @@  void FormatConverter::convert(const unsigned char *src, size_t size,
 	case RGB:
 		convertRGB(src, dst->bits());
 		break;
+	case NV:
+		convertNV(src, dst->bits());
+		break;
 	};
 }
 
+static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)
+{
+	int c = y - 16;
+	int d = u - 128;
+	int e = v - 128;
+	*r = CLIP(( 298 * c           + 409 * e + 128) >> RGBSHIFT);
+	*g = CLIP(( 298 * c - 100 * d - 208 * e + 128) >> RGBSHIFT);
+	*b = CLIP(( 298 * c + 516 * d           + 128) >> RGBSHIFT);
+}
+
+void FormatConverter::convertNV(const unsigned char *src, unsigned char *dst)
+{
+	unsigned int x, y;
+	int r, g, b;
+
+	unsigned int c_stride = width_ * (2 / horzSubSample_);
+	unsigned int c_inc = horzSubSample_ == 1 ? 2 : 0;
+	unsigned int cb_pos = nvSwap_ ? 1 : 0;
+	unsigned int cr_pos = nvSwap_ ? 0 : 1;
+	unsigned int base = width_ * height_;
+
+	for (y = 0; y < height_; y++) {
+		unsigned char *src_y = (unsigned char *)src + y * width_;
+		unsigned char *src_cb = (unsigned char *)src + base + (y / vertSubSample_) * c_stride + cb_pos;
+		unsigned char *src_cr = (unsigned char *)src + base + (y / vertSubSample_) * c_stride + cr_pos;
+
+		for (x = 0; x < width_; x += 2) {
+			yuv_to_rgb(*src_y, *src_cb, *src_cr, &r, &g, &b);
+			dst[0] = b;
+			dst[1] = g;
+			dst[2] = r;
+			dst[3] = 0xff;
+			src_y++;
+			src_cb += c_inc;
+			src_cr += c_inc;
+			dst += 4;
+
+			yuv_to_rgb(*src_y, *src_cb, *src_cr, &r, &g, &b);
+			dst[0] = b;
+			dst[1] = g;
+			dst[2] = r;
+			dst[3] = 0xff;
+			src_y++;
+			src_cb += 2;
+			src_cr += 2;
+			dst += 4;
+		}
+	}
+}
+
 void FormatConverter::convertRGB(const unsigned char *src, unsigned char *dst)
 {
 	unsigned int x, y;
@@ -124,16 +213,6 @@  void FormatConverter::convertRGB(const unsigned char *src, unsigned char *dst)
 	}
 }
 
-static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)
-{
-	int c = y - 16;
-	int d = u - 128;
-	int e = v - 128;
-	*r = CLIP(( 298 * c           + 409 * e + 128) >> RGBSHIFT);
-	*g = CLIP(( 298 * c - 100 * d - 208 * e + 128) >> RGBSHIFT);
-	*b = CLIP(( 298 * c + 516 * d           + 128) >> RGBSHIFT);
-}
-
 void FormatConverter::convertYUV(const unsigned char *src, unsigned char *dst)
 {
 	unsigned int src_x, src_y, dst_x, dst_y;
diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h
index c18871e..ad67081 100644
--- a/src/qcam/format_converter.h
+++ b/src/qcam/format_converter.h
@@ -16,6 +16,7 @@  class FormatConverter
 public:
 	enum FormatFamily {
 		MJPEG,
+		NV,
 		RGB,
 		YUV,
 	};
@@ -26,6 +27,7 @@  public:
 	void convert(const unsigned char *src, size_t size, QImage *dst);
 
 private:
+	void convertNV(const unsigned char *src, unsigned char *dst);
 	void convertRGB(const unsigned char *src, unsigned char *dst);
 	void convertYUV(const unsigned char *src, unsigned char *dst);
 
@@ -35,6 +37,11 @@  private:
 
 	enum FormatFamily formatFamily_;
 
+	/* NV parameters */
+	unsigned int horzSubSample_;
+	unsigned int vertSubSample_;
+	bool nvSwap_;
+
 	/* RGB parameters */
 	unsigned int bpp_;
 	unsigned int r_pos_;