[{"id":1559,"web_url":"https://patchwork.libcamera.org/comment/1559/","msgid":"<20190504230327.GC4922@pendragon.ideasonboard.com>","date":"2019-05-04T23:03:27","subject":"Re: [libcamera-devel] [PATCH 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 Sat, May 04, 2019 at 04:33:54PM -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> src/qcam/format_converter.cpp | 62 +++++++++++++++++++++++++++++++++++\n> src/qcam/format_converter.h | 7 ++++\n> 2 files changed, 69 insertions(+)\n> \n> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp\n> index 192767c..ca91c7e 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\txDownSample_ = 2;\n> +\t\tyDownSample_ = 2;\n\nThe usual term is sub-sampling, not down-sampling. It is also customary\nto talk about horizontal and vertical sub-sampling instead of x and y.\n\n> +\t\tnvSwap_ = false;\n> +\t\tbreak;\n> +\tcase V4L2_PIX_FMT_NV21:\n> +\t\tformatFamily_ = NV;\n> +\t\txDownSample_ = 2;\n> +\t\tyDownSample_ = 2;\n> +\t\tnvSwap_ = true;\n> +\t\tbreak;\n> +\tcase V4L2_PIX_FMT_NV16:\n> +\t\tformatFamily_ = NV;\n> +\t\txDownSample_ = 2;\n> +\t\tyDownSample_ = 1;\n> +\t\tnvSwap_ = false;\n> +\t\tbreak;\n> +\tcase V4L2_PIX_FMT_NV61:\n> +\t\tformatFamily_ = NV;\n> +\t\txDownSample_ = 2;\n> +\t\tyDownSample_ = 1;\n> +\t\tnvSwap_ = true;\n> +\t\tbreak;\n> +\tcase V4L2_PIX_FMT_NV24:\n> +\t\tformatFamily_ = NV;\n> +\t\txDownSample_ = 1;\n> +\t\tyDownSample_ = 1;\n> +\t\tnvSwap_ = false;\n> +\t\tbreak;\n> +\tcase V4L2_PIX_FMT_NV42:\n> +\t\tformatFamily_ = NV;\n> +\t\txDownSample_ = 1;\n> +\t\tyDownSample_ = 1;\n> +\t\tnvSwap_ = true;\n> +\t\tbreak;\n> \tcase V4L2_PIX_FMT_MJPEG:\n> \t\tformatFamily_ = MJPEG;\n> \t\tbreak;\n> @@ -99,6 +135,9 @@ 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> @@ -171,3 +210,26 @@ void FormatConverter::convertYUV(const unsigned char *src, unsigned char *dst)\n> \t\t}\n> \t}\n> }\n> +\n> +void FormatConverter::convertNV(const unsigned char *src, unsigned char *dst)\n\nWe try to define functions in the same order they are declared in the\nclass, at least within an access type (public, protected and private).\nThat is, for example, public and private functions can be interleaved in\nthe .cpp file, but public functions should appear in the same order as\nin the .h file, and private functions as well.\n\n> +{\n> +\tunsigned int i, j;\n> +\tint r, g, b, y, cr, cb, loc;\n> +\n> +\tfor (j = 0; j < height_; j++) {\n> +\t\tfor (i = 0; i < width_; i++) {\n\nIt would be nice to use x and y instead of i and j, but y is already\ntaken for luma. \n\n> +\t\t\ty = src[j * width_ + i];\n> +\t\t\tloc = height_ * width_ +\n> +\t\t\t (j / yDownSample_) * width_ * 2 / xDownSample_ +\n> +\t\t\t (i & -xDownSample_) * 2 / xDownSample_;\n> +\t\t\tcb = nvSwap_ ? src[loc + 1] : src[loc];\n> +\t\t\tcr = nvSwap_ ? src[loc] : src[loc + 1];\n> +\n> +\t\t\tyuv_to_rgb(y, cb, cr, &r, &g, &b);\n> +\t\t\tdst[j * width_ * 4 + i * 4 + 0] = b;\n> +\t\t\tdst[j * width_ * 4 + i * 4 + 1] = g;\n> +\t\t\tdst[j * width_ * 4 + i * 4 + 2] = r;\n> +\t\t\tdst[j * width_ * 4 + i * 4 + 3] = 0xff;\n\nYou could increase the dst pointer instead, as in\n\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\tdst += 4;\n\nI think this would be more efficient. Another improvement would be to\ncompute unroll the inner loop slightly by computing two dst pixels one after the\nother. This should allow you to optimise the body of the inner loop by\nreducing the calculation performed for each iteration. The following\ncode is completely untested but gives you an idea of what I mean.\n\n\tunsigned int c_stride = width_ * (2 / xDownSampling_);\n\tunsigned int c_increment = xDownSampling_ ? 1 : 0,\n\tint r, g, b;\n\n\tfor (unsigned int y = 0; y < height_; y++) {\n\t\tunsigned char *src_y = src + y * width_;\n\t\tunsigned char *src_cb = src + (y / yDownSample_) * c_stride,\n\t\tunsigned char *src_cr = src + (y / yDownSample_) * c_stride + 1;\n\n\t\tif (nvSwap_)\n\t\t\tstd::swap(src_cb, src_cr);\n\n\t\tfor (unsigned int x = 0; i 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_increment;\n\t\t\tsrc_cr += c_increment;\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++;\n\t\t\tsrc_cr++;\n\t\t\tdst += 4;\n\t\t}\n\t}\n\nWe could go one step further and compute four pixels in the inner loop,\ntwo horizontally and two vertically, possible saving a bit more\ncalculation, but then we would lose cache locality, so I think the\nresult would be slower.\n\n> +\t\t}\n> +\t}\n> +}\n> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h\n> index 9820982..48af0fc 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 xDownSample_;\n> +\tunsigned int yDownSample_;\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 8BB1460E54\n\tfor ;\n\tSun, 5 May 2019 01:03:41 +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 D395FD5;\n\tSun, 5 May 2019 01:03:40 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1557011021;\n\tbh=tkkvjB0LbWhTgjCRvnLRxknfSjoik/NGiuWxSMs48Zg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DQFls6Pgi5sMlSEzwUmtEZ4mXmsyDfrg2b5MSSX+FUTJLdxtggmvWZDxj5h0VsO0j\n\tnxEVAzsiEqG2+qbi5v2LWEwvBr/FBIuvQ6AnBN4J60Oj3yz5TYkgFac/zMvbNcHEDX\n\thlhBZwUhRCvqNqHSXubLxxYv16u7qfQUzPX/U7+Q=","Date":"Sun, 5 May 2019 02:03:27 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190504230327.GC4922@pendragon.ideasonboard.com>","References":"<20190504203354.20870-1-paul.elder@ideasonboard.com>\n\t<20190504203354.20870-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190504203354.20870-2-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 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":"Sat, 04 May 2019 23:03:41 -0000"}}]