[{"id":1560,"web_url":"https://patchwork.libcamera.org/comment/1560/","msgid":"<20190506203017.GC5021@pendragon.ideasonboard.com>","date":"2019-05-06T20:30:17","subject":"Re: [libcamera-devel] [PATCH v2 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 04:15:28PM -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 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 | 99 +++++++++++++++++++++++++++++++----\n> src/qcam/format_converter.h | 7 +++\n> 2 files changed, 96 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp\n> index 192767c..eab222f 100644\n> --- a/src/qcam/format_converter.cpp\n> +++ b/src/qcam/format_converter.cpp\n> @@ -72,6 +72,42 @@ int FormatConverter::configure(unsigned int format, unsigned int width,\n> \t\ty_pos_ = 0;\n> \t\tcb_pos_ = 1;\n> \t\tbreak;\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_MJPEG:\n> \t\tformatFamily_ = MJPEG;\n> \t\tbreak;\n> @@ -99,9 +135,62 @@ 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\nYou maye want to reorder the cases to match the order of the functions\nas well as the order of the enum.\n\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> +\tunsigned int x, y;\n\nNo need to define those variables at the top level scope, you can define\nthem inside the for statement (as in \"for (unsigne int x = 0; ...)\").\n\n> +\tint r, g, b;\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> +\tunsigned int base = width_ * height_;\n\nSmall improvement I believe, you could add\n\n\tconst unsigned char *src_c = src + width_ * height_;\n\nand remove the base variable.\n\n> +\n> +\tfor (y = 0; y < height_; y++) {\n> +\t\tunsigned char *src_y = (unsigned char *)src + y * width_;\n\nDon't blindly cast a const pointer to non-const, that's asking for\ntrouble (furthermore, C++-type casts should otherwise have been used,\nwith const_cast(src)). YOu can declare src_y as const\nunsigned char * and everything should be fine.\n\n> +\t\tunsigned char *src_cb = (unsigned char *)src + base + (y / vertSubSample_) * c_stride + cb_pos;\n\nWhile exceptions are allowed, we still try to cap the line length to 80\ncharacters when it doesn't hinder readability.\n\n> +\t\tunsigned char *src_cr = (unsigned char *)src + base + (y / vertSubSample_) * c_stride + cr_pos;\n> +\n> +\t\tfor (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 +213,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 c18871e..ad67081 100644\n> --- a/src/qcam/format_converter.h\n> +++ b/src/qcam/format_converter.h\n> @@ -16,6 +16,7 @@ class FormatConverter\n> public:\n> \tenum FormatFamily {\n> \t\tMJPEG,\n> +\t\tNV,\n> \t\tRGB,\n> \t\tYUV,\n> \t};\n> @@ -26,6 +27,7 @@ public:\n> \tvoid convert(const unsigned char *src, size_t size, QImage *dst);\n> \n> private:\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[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 266F360E63\n\tfor ;\n\tMon, 6 May 2019 22:30:32 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8A72B2E7;\n\tMon, 6 May 2019 22:30:31 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557174631;\n\tbh=h6kW3+9lN/V6OO7iDq8jWhrEQE5VAYMFpYkkBrW21CM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fX3YBJlwtOpsIkwaxBU4vD9TjdA94gxko2gUWqyrVX0E091/8rqlxI3ygq/gpUvfU\n\tsPz4dOtprtb8UWpEEJ7saLaaPeMCixiB9FtDIRuby2ZjECIohNlEWlMz6mAupX2UNo\n\tc4YCCdMh1J8rzzuE7Lt98JGPMh3UJX/599yU5xBE=","Date":"Mon, 6 May 2019 23:30:17 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190506203017.GC5021@pendragon.ideasonboard.com>","References":"<20190506201528.24445-1-paul.elder@ideasonboard.com>\n\t<20190506201528.24445-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190506201528.24445-2-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 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 20:30:32 -0000"}}]