[{"id":1562,"web_url":"https://patchwork.libcamera.org/comment/1562/","msgid":"<20190506235835.GE5021@pendragon.ideasonboard.com>","date":"2019-05-06T23:58:35","subject":"Re: [libcamera-devel] [PATCH v3 2/2] qcam: format_converter: Add NV\n\tformats support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Mon, May 06, 2019 at 06:34:30PM -0400, Paul Elder wrote:\n> Add support for some NV formats:\n> - V4L2_PIX_FMT_NV12, V4L2_PIX_FMT_NV21\n> - V4L2_PIX_FMT_NV16, V4L2_PIX_FMT_NV61\n> - V4L2_PIX_FMT_NV24, V4L2_PIX_FMT_NV42\n> \n> Signed-off-by: Paul Elder \n> ---\n> Changes in v3:\n> - reorder the switch cases to match the order of the functions and enum\n> - fix const -> non-const pointer cast in convertNV()\n> - other minor syle changes\n> \n> Changes in v2:\n> - reorder functions in format_converter.cpp to match format_converter.h\n> (move yuv_to_rgb() to be before all specialized convert functions)\n> - renamed NV conversion parameters from xDownSample_ and yDownSample_ to\n> horzSubSample_ and vertSubSample_, respectively\n> - unrolled the loop and simplify some of the calculations in convertNV()\n> and achieved a 1.67 speedup compared to v1\n> \n> src/qcam/format_converter.cpp | 100 ++++++++++++++++++++++++++++++----\n> src/qcam/format_converter.h | 7 +++\n> 2 files changed, 97 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp\n> index 192767c..d90f933 100644\n> --- a/src/qcam/format_converter.cpp\n> +++ b/src/qcam/format_converter.cpp\n> @@ -31,6 +31,42 @@ int FormatConverter::configure(unsigned int format, unsigned int width,\n> \t\t\t unsigned int height)\n> {\n> \tswitch (format) {\n> +\tcase V4L2_PIX_FMT_NV12:\n> +\t\tformatFamily_ = NV;\n> +\t\thorzSubSample_ = 2;\n> +\t\tvertSubSample_ = 2;\n> +\t\tnvSwap_ = false;\n> +\t\tbreak;\n> +\tcase V4L2_PIX_FMT_NV21:\n> +\t\tformatFamily_ = NV;\n> +\t\thorzSubSample_ = 2;\n> +\t\tvertSubSample_ = 2;\n> +\t\tnvSwap_ = true;\n> +\t\tbreak;\n> +\tcase V4L2_PIX_FMT_NV16:\n> +\t\tformatFamily_ = NV;\n> +\t\thorzSubSample_ = 2;\n> +\t\tvertSubSample_ = 1;\n> +\t\tnvSwap_ = false;\n> +\t\tbreak;\n> +\tcase V4L2_PIX_FMT_NV61:\n> +\t\tformatFamily_ = NV;\n> +\t\thorzSubSample_ = 2;\n> +\t\tvertSubSample_ = 1;\n> +\t\tnvSwap_ = true;\n> +\t\tbreak;\n> +\tcase V4L2_PIX_FMT_NV24:\n> +\t\tformatFamily_ = NV;\n> +\t\thorzSubSample_ = 1;\n> +\t\tvertSubSample_ = 1;\n> +\t\tnvSwap_ = false;\n> +\t\tbreak;\n> +\tcase V4L2_PIX_FMT_NV42:\n> +\t\tformatFamily_ = NV;\n> +\t\thorzSubSample_ = 1;\n> +\t\tvertSubSample_ = 1;\n> +\t\tnvSwap_ = true;\n> +\t\tbreak;\n> \tcase V4L2_PIX_FMT_BGR24:\n> \t\tformatFamily_ = RGB;\n> \t\tr_pos_ = 2;\n> @@ -99,9 +135,63 @@ void FormatConverter::convert(const unsigned char *src, size_t size,\n> \tcase RGB:\n> \t\tconvertRGB(src, dst->bits());\n> \t\tbreak;\n> +\tcase NV:\n> +\t\tconvertNV(src, dst->bits());\n> +\t\tbreak;\n> \t};\n> }\n> \n> +static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)\n> +{\n> +\tint c = y - 16;\n> +\tint d = u - 128;\n> +\tint e = v - 128;\n> +\t*r = CLIP(( 298 * c + 409 * e + 128) >> RGBSHIFT);\n> +\t*g = CLIP(( 298 * c - 100 * d - 208 * e + 128) >> RGBSHIFT);\n> +\t*b = CLIP(( 298 * c + 516 * d + 128) >> RGBSHIFT);\n> +}\n> +\n> +void FormatConverter::convertNV(const unsigned char *src, unsigned char *dst)\n> +{\n> +\tint r, g, b;\n> +\n\nNo need for a blank line here (and I'd also move this line as the last\nof the variable declarations). Apart from that,\n\nReviewed-by: Laurent Pinchart \n\nand feel free to push to revised patch.\n\n> +\tunsigned int c_stride = width_ * (2 / horzSubSample_);\n> +\tunsigned int c_inc = horzSubSample_ == 1 ? 2 : 0;\n> +\tunsigned int cb_pos = nvSwap_ ? 1 : 0;\n> +\tunsigned int cr_pos = nvSwap_ ? 0 : 1;\n> +\tconst unsigned char *src_c = src + width_ * height_;\n> +\n> +\tfor (unsigned int y = 0; y < height_; y++) {\n> +\t\tconst unsigned char *src_y = src + y * width_;\n> +\t\tconst unsigned char *src_cb = src_c + (y / vertSubSample_) *\n> +\t\t\t\t\t c_stride + cb_pos;\n> +\t\tconst unsigned char *src_cr = src_c + (y / vertSubSample_) *\n> +\t\t\t\t\t c_stride + cr_pos;\n> +\n> +\t\tfor (unsigned int x = 0; x < width_; x += 2) {\n> +\t\t\tyuv_to_rgb(*src_y, *src_cb, *src_cr, &r, &g, &b);\n> +\t\t\tdst[0] = b;\n> +\t\t\tdst[1] = g;\n> +\t\t\tdst[2] = r;\n> +\t\t\tdst[3] = 0xff;\n> +\t\t\tsrc_y++;\n> +\t\t\tsrc_cb += c_inc;\n> +\t\t\tsrc_cr += c_inc;\n> +\t\t\tdst += 4;\n> +\n> +\t\t\tyuv_to_rgb(*src_y, *src_cb, *src_cr, &r, &g, &b);\n> +\t\t\tdst[0] = b;\n> +\t\t\tdst[1] = g;\n> +\t\t\tdst[2] = r;\n> +\t\t\tdst[3] = 0xff;\n> +\t\t\tsrc_y++;\n> +\t\t\tsrc_cb += 2;\n> +\t\t\tsrc_cr += 2;\n> +\t\t\tdst += 4;\n> +\t\t}\n> +\t}\n> +}\n> +\n> void FormatConverter::convertRGB(const unsigned char *src, unsigned char *dst)\n> {\n> \tunsigned int x, y;\n> @@ -124,16 +214,6 @@ void FormatConverter::convertRGB(const unsigned char *src, unsigned char *dst)\n> \t}\n> }\n> \n> -static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)\n> -{\n> -\tint c = y - 16;\n> -\tint d = u - 128;\n> -\tint e = v - 128;\n> -\t*r = CLIP(( 298 * c + 409 * e + 128) >> RGBSHIFT);\n> -\t*g = CLIP(( 298 * c - 100 * d - 208 * e + 128) >> RGBSHIFT);\n> -\t*b = CLIP(( 298 * c + 516 * d + 128) >> RGBSHIFT);\n> -}\n> -\n> void FormatConverter::convertYUV(const unsigned char *src, unsigned char *dst)\n> {\n> \tunsigned int src_x, src_y, dst_x, dst_y;\n> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h\n> index bca44aa..391e6a4 100644\n> --- a/src/qcam/format_converter.h\n> +++ b/src/qcam/format_converter.h\n> @@ -22,10 +22,12 @@ public:\n> private:\n> \tenum FormatFamily {\n> \t\tMJPEG,\n> +\t\tNV,\n> \t\tRGB,\n> \t\tYUV,\n> \t};\n> \n> +\tvoid convertNV(const unsigned char *src, unsigned char *dst);\n> \tvoid convertRGB(const unsigned char *src, unsigned char *dst);\n> \tvoid convertYUV(const unsigned char *src, unsigned char *dst);\n> \n> @@ -35,6 +37,11 @@ private:\n> \n> \tenum FormatFamily formatFamily_;\n> \n> +\t/* NV parameters */\n> +\tunsigned int horzSubSample_;\n> +\tunsigned int vertSubSample_;\n> +\tbool nvSwap_;\n> +\n> \t/* RGB parameters */\n> \tunsigned int bpp_;\n> \tunsigned int r_pos_;","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94AEC60E51\n\tfor ;\n\tTue, 7 May 2019 01:58:50 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D97CC2E7;\n\tTue, 7 May 2019 01:58:49 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557187130;\n\tbh=b8oiFwsG7M8kpTBhdPo+iZZdG9h3r/o5bGeiQGqvTlA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wcXvaw78YvqnvINROTSPsDecyvpag4WQV9hoJAxfMnpXtUTFlpifWaNxF9UgMwmF8\n\twwkwg2U3+rJGY2GR/w8yjE3NkvR/sAEGsxVb/M91z1xrf0jxphwQre1mTiiMsQCFjE\n\tP9ISM4Ic9gXwf8MRDJuGYGTMioEhdZBdy0Z1WJqM=","Date":"Tue, 7 May 2019 02:58:35 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190506235835.GE5021@pendragon.ideasonboard.com>","References":"<20190506223430.3203-1-paul.elder@ideasonboard.com>\n\t<20190506223430.3203-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190506223430.3203-2-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] qcam: format_converter: Add NV\n\tformats support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 06 May 2019 23:58:50 -0000"}}]