[{"id":12462,"web_url":"https://patchwork.libcamera.org/comment/12462/","msgid":"<20200912010331.GI6808@pendragon.ideasonboard.com>","date":"2020-09-12T01:03:31","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/2] qcam: format_converter:\n\tadd 10 and 12 bit packed raw Bayer formats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Andrey,\n\nThank you for the patch.\n\nOn Tue, Sep 08, 2020 at 06:07:39PM +0300, Andrey Konovalov wrote:\n> No interpolation is used to get more speed by the price of lower image\n> quality. In qcam format_converter is used for viewfinder only, and in\n> this case lower lag is more important than the image quality.\n> \n> Signed-off-by: Andrey Konovalov \n> ---\n> src/qcam/format_converter.cpp | 138 ++++++++++++++++++++++++++++++++++\n> src/qcam/format_converter.h | 21 ++++++\n> 2 files changed, 159 insertions(+)\n> \n> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp\n> index 3481330..f370907 100644\n> --- a/src/qcam/format_converter.cpp\n> +++ b/src/qcam/format_converter.cpp\n> @@ -10,6 +10,7 @@\n> #include \n> \n> #include \n> +#include \n> \n> #include \n> \n> @@ -132,6 +133,62 @@ int FormatConverter::configure(const libcamera::PixelFormat &format,\n> \t\tparams_.yuv.cb_pos = 1;\n> \t\tbreak;\n> \n> +\tcase libcamera::formats::SRGGB10_CSI2P:\n> +\t\tformatFamily_ = RAW_CSI2P;\n> +\t\tparams_.rawp.bpp_numer = 5;\t/* 1.25 bytes per pixel */\n> +\t\tparams_.rawp.bpp_denom = 4;\n> +\t\tparams_.rawp.r_pos = 0;\n> +\t\tbreak;\n> +\n\nThere are no blank lines between formats of the same group for other\ngroups, I'd follow the same pattern here.\n\n> +\tcase libcamera::formats::SGRBG10_CSI2P:\n> +\t\tformatFamily_ = RAW_CSI2P;\n> +\t\tparams_.rawp.bpp_numer = 5;\n> +\t\tparams_.rawp.bpp_denom = 4;\n> +\t\tparams_.rawp.r_pos = 1;\n> +\t\tbreak;\n> +\n> +\tcase libcamera::formats::SGBRG10_CSI2P:\n> +\t\tformatFamily_ = RAW_CSI2P;\n> +\t\tparams_.rawp.bpp_numer = 5;\n> +\t\tparams_.rawp.bpp_denom = 4;\n> +\t\tparams_.rawp.r_pos = 2;\n> +\t\tbreak;\n> +\n> +\tcase libcamera::formats::SBGGR10_CSI2P:\n> +\t\tformatFamily_ = RAW_CSI2P;\n> +\t\tparams_.rawp.bpp_numer = 5;\n> +\t\tparams_.rawp.bpp_denom = 4;\n> +\t\tparams_.rawp.r_pos = 3;\n> +\t\tbreak;\n> +\n> +\tcase libcamera::formats::SRGGB12_CSI2P:\n> +\t\tformatFamily_ = RAW_CSI2P;\n> +\t\tparams_.rawp.bpp_numer = 3;\t/* 1.5 bytes per pixel */\n> +\t\tparams_.rawp.bpp_denom = 2;\n> +\t\tparams_.rawp.r_pos = 0;\n> +\t\tbreak;\n> +\n> +\tcase libcamera::formats::SGRBG12_CSI2P:\n> +\t\tformatFamily_ = RAW_CSI2P;\n> +\t\tparams_.rawp.bpp_numer = 3;\n> +\t\tparams_.rawp.bpp_denom = 2;\n> +\t\tparams_.rawp.r_pos = 1;\n> +\t\tbreak;\n> +\n> +\tcase libcamera::formats::SGBRG12_CSI2P:\n> +\t\tformatFamily_ = RAW_CSI2P;\n> +\t\tparams_.rawp.bpp_numer = 3;\n> +\t\tparams_.rawp.bpp_denom = 2;\n> +\t\tparams_.rawp.r_pos = 2;\n> +\t\tbreak;\n> +\n> +\tcase libcamera::formats::SBGGR12_CSI2P:\n> +\t\tformatFamily_ = RAW_CSI2P;\n> +\t\tparams_.rawp.bpp_numer = 3;\n> +\t\tparams_.rawp.bpp_denom = 2;\n> +\t\tparams_.rawp.r_pos = 3;\n> +\t\tbreak;\n> +\n> \tcase libcamera::formats::MJPEG:\n> \t\tformatFamily_ = MJPEG;\n> \t\tbreak;\n> @@ -144,6 +201,45 @@ int FormatConverter::configure(const libcamera::PixelFormat &format,\n> \twidth_ = size.width();\n> \theight_ = size.height();\n> \n> +\tif (formatFamily_ == RAW_CSI2P) {\n> +\t\t/*\n> +\t\t * For RAW_CSI2P the assumption is that height_ and width_\n> +\t\t * are even numbers.\n> +\t\t */\n> +\t\tif ( width_ % 2 != 0 || height_ % 2 != 0 ) {\n\nNo space after ( and before ). You can also drop the != 0.\n\n> +\t\t\tqWarning() << \"Image width or height isn't even number\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tparams_.rawp.srcLineLength = width_ * params_.rawp.bpp_numer /\n> +\t\t\t\t\t params_.rawp.bpp_denom;\n> +\n> +\t\t/*\n> +\t\t * Calculate [g1,g2,b]_pos based on r_pos.\n> +\t\t * On entrance, r_pos is the position of red pixel in the\n\ns/entrace/entry/ (Kieran can correct me if this isn't right)\ns/red pixel/the red pixel/\n\n> +\t\t * 2-by-2 pixel Bayer pattern, and is from 0 to 3 range:\n\n\"is in the 0 to 3 range\" or \"is in the [0, 3] range\"\n\n> +\t\t * +---+---+\n> +\t\t * | 0 | 1 |\n> +\t\t * +---+---+\n> +\t\t * | 2 | 3 |\n> +\t\t * +---+---+\n> +\t\t * At exit, [r,g1,g2,b]_pos are offsetts of the color\n\ns/offsetts/the offsets/\n\n> +\t\t * values in the source buffer.\n> +\t\t */\n> +\t\tif (params_.rawp.r_pos > 1) {\n> +\t\t\tparams_.rawp.r_pos = params_.rawp.r_pos - 2 +\n> +\t\t\t\t\t params_.rawp.srcLineLength;\n> +\t\t\tparams_.rawp.b_pos = 3 - params_.rawp.r_pos;\n> +\t\t} else {\n> +\t\t\tparams_.rawp.b_pos = 1 - params_.rawp.r_pos +\n> +\t\t\t\t\t params_.rawp.srcLineLength;\n> +\t\t}\n> +\t\tparams_.rawp.g1_pos = (params_.rawp.r_pos == 0 ||\n> +\t\t\t\t params_.rawp.b_pos == 0) ? 1 : 0;\n> +\t\tparams_.rawp.g2_pos = 1 - params_.rawp.g1_pos +\n> +\t\t\t\t params_.rawp.srcLineLength;\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -163,9 +259,51 @@ void FormatConverter::convert(const unsigned char *src, size_t size,\n> \tcase NV:\n> \t\tconvertNV(src, dst->bits());\n> \t\tbreak;\n> +\tcase RAW_CSI2P:\n> +\t\tconvertRawCSI2P(src, dst->bits());\n> +\t\tbreak;\n> \t};\n> }\n> \n> +void FormatConverter::convertRawCSI2P(const unsigned char *src,\n> +\t\t\t\t unsigned char *dst)\n> +{\n> +\tunsigned int g;\n> +\tstatic unsigned char dst_buf[8] = { 0, 0, 0, 0xff };\n> +\tunsigned int dstLineLength = width_ * 4;\n> +\n> +\tfor (unsigned int y = 0; y < height_; y += 2) {\n> +\t\tfor (unsigned x = 0; x < width_; x += params_.rawp.bpp_denom) {\n> +\t\t\tfor (unsigned int i = 0; i < params_.rawp.bpp_denom ;\n\ns/ ;/;/\n\n> +\t\t\t i += 2) {\n\nI wouldn't wrap this to the next line, it looks weird.\n\n> +\t\t\t\t/*\n> +\t\t\t\t * Process the current 2x2 group.\n> +\t\t\t\t * Use the average of the two green pixels as\n> +\t\t\t\t * the green value for all the pixels in the\n> +\t\t\t\t * group.\n> +\t\t\t\t */\n> +\t\t\t\tdst_buf[0] = src[params_.rawp.b_pos];\n> +\t\t\t\tg = src[params_.rawp.g1_pos];\n> +\t\t\t\tg = (g + src[params_.rawp.g2_pos]) >> 1;\n> +\t\t\t\tdst_buf[1] = (unsigned char)g;\n> +\t\t\t\tdst_buf[2] = src[params_.rawp.r_pos];\n> +\t\t\t\tsrc += 2;\n> +\n> +\t\t\t\tmemcpy(dst_buf + 4, dst_buf, 4);\n> +\t\t\t\tmemcpy(dst, dst_buf, 8);\n> +\t\t\t\tmemcpy(dst + dstLineLength, dst_buf, 8);\n\nThe memcpy() calls look really awkward. I suppose this is what you've\nmentioned as a performance improvement in the cover letter ? I'm\nconcerned someone will forget about it in the future and refactor this\ncode. We need at least a comment that explains what's going on.\n\nI wonder if you won't get better performances from splitting this\nfunction in two, and unrolling the inner loop for the 10bpp case.\n\n> +\t\t\t\tdst += 8;\n> +\t\t\t}\n> +\t\t\tsrc += params_.rawp.bpp_numer - params_.rawp.bpp_denom;\n> +\t\t}\n\nCan you add a blank line here ?\n\n> +\t\t/* odd lines are copies of the even lines they follow: */\n\ns/odd/Odd/\ns/follow:/follow/.\n\n> +\t\tmemcpy(dst, dst-dstLineLength, dstLineLength);\n\nBlank line here too.\n\n> +\t\t/* move to the next even line: */\n\ns/move/Move/\ns/line:/line./\n\n> +\t\tsrc += params_.rawp.srcLineLength;\n> +\t\tdst += dstLineLength;\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> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h\n> index c53fa28..dc20e0d 100644\n> --- a/src/qcam/format_converter.h\n> +++ b/src/qcam/format_converter.h\n> @@ -26,11 +26,13 @@ private:\n> \tenum FormatFamily {\n> \t\tMJPEG,\n> \t\tNV,\n> +\t\tRAW_CSI2P,\n\nI'd name this RawCSI2Packed.\n\n> \t\tRGB,\n> \t\tYUV,\n> \t};\n> \n> \tvoid convertNV(const unsigned char *src, unsigned char *dst);\n> +\tvoid convertRawCSI2P(const unsigned char *src, unsigned char *dst);\n\nAnd s/CSI2P/CSI2Packed/\n\n> \tvoid convertRGB(const unsigned char *src, unsigned char *dst);\n> \tvoid convertYUV(const unsigned char *src, unsigned char *dst);\n> \n> @@ -48,6 +50,25 @@ private:\n> \t\t\tbool nvSwap;\n> \t\t} nv;\n> \n> +\t\t/* RAW Bayer CSI2P parameters */\n> +\t\tstruct raw_csi2p_params {\n> +\t\t\t/*\n> +\t\t\t * Bytes per pixel is a fractional number, and is\n> +\t\t\t * represented by integer numerator and denominator.\n> +\t\t\t */\n> +\t\t\tunsigned int bpp_numer;\n> +\t\t\tunsigned int bpp_denom;\n> +\t\t\tunsigned int r_pos;\n> +\t\t\t/*\n> +\t\t\t * The fields below are used to hold the values computed\n> +\t\t\t * in configure() based on r_pos and width_.\n> +\t\t\t */\n> +\t\t\tunsigned int g1_pos;\n> +\t\t\tunsigned int g2_pos;\n> +\t\t\tunsigned int b_pos;\n> +\t\t\tunsigned int srcLineLength;\n> +\t\t} rawp;\n\nThese parameters are applicable to all Bayer formats, not only the\npacked ones. And they're specific to Bayer formats, they're not\napplicable to other color filter array patterns (or other non-CFA raw\nformats). I would thus rename the structure from raw_csi2p_params to\nbayer_params and the member from rawp to bayer.\n\n> +\n> \t\t/* RGB parameters */\n> \t\tstruct rgb_params {\n> \t\t\tunsigned int bpp;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 73A03C3B5B\n\tfor ;\n\tSat, 12 Sep 2020 01:04:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 044D562C8C;\n\tSat, 12 Sep 2020 03:04:01 +0200 (CEST)","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 8BCB362C43\n\tfor ;\n\tSat, 12 Sep 2020 03:03:59 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 03F3F539;\n\tSat, 12 Sep 2020 03:03:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"E4Nt5A5k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599872639;\n\tbh=Qg310QgwrL4/6JErMv5nOB3psESzspGH2DzpBXm5W+w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E4Nt5A5kVTWT7W4mqX3XIi+J3QwSi+hYSecLNAxiu4z3jJaGhdNEUKEooxB7zB5j+\n\t7aeQVhJIZWemNZZfeH2zOB99pIZzDKuPYjURZtZ8McIf3rho7gNVAawHjuqmGkXblI\n\tezmVy2Xo5VurLjQ9sLUwdfo4cK+Mg2n48rk7Vmq4=","Date":"Sat, 12 Sep 2020 04:03:31 +0300","From":"Laurent Pinchart ","To":"Andrey Konovalov ","Message-ID":"<20200912010331.GI6808@pendragon.ideasonboard.com>","References":"<20200908150739.1552-1-andrey.konovalov@linaro.org>\n\t<20200908150739.1552-3-andrey.konovalov@linaro.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200908150739.1552-3-andrey.konovalov@linaro.org>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/2] qcam: format_converter:\n\tadd 10 and 12 bit packed raw Bayer formats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12476,"web_url":"https://patchwork.libcamera.org/comment/12476/","msgid":"","date":"2020-09-12T20:07:50","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/2] qcam: format_converter:\n\tadd 10 and 12 bit packed raw Bayer formats","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 12/09/2020 02:03, Laurent Pinchart wrote:\n> Hi Andrey,\n> \n> Thank you for the patch.\n> \n> On Tue, Sep 08, 2020 at 06:07:39PM +0300, Andrey Konovalov wrote:\n>> No interpolation is used to get more speed by the price of lower image\n>> quality. In qcam format_converter is used for viewfinder only, and in\n>> this case lower lag is more important than the image quality.\n>>\n>> Signed-off-by: Andrey Konovalov \n>> ---\n>> src/qcam/format_converter.cpp | 138 ++++++++++++++++++++++++++++++++++\n>> src/qcam/format_converter.h | 21 ++++++\n>> 2 files changed, 159 insertions(+)\n>>\n>> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp\n>> index 3481330..f370907 100644\n>> --- a/src/qcam/format_converter.cpp\n>> +++ b/src/qcam/format_converter.cpp\n>> @@ -10,6 +10,7 @@\n>> #include \n>> \n>> #include \n>> +#include \n>> \n>> #include \n>> \n>> @@ -132,6 +133,62 @@ int FormatConverter::configure(const libcamera::PixelFormat &format,\n>> \t\tparams_.yuv.cb_pos = 1;\n>> \t\tbreak;\n>> \n>> +\tcase libcamera::formats::SRGGB10_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 5;\t/* 1.25 bytes per pixel */\n>> +\t\tparams_.rawp.bpp_denom = 4;\n>> +\t\tparams_.rawp.r_pos = 0;\n>> +\t\tbreak;\n>> +\n> \n> There are no blank lines between formats of the same group for other\n> groups, I'd follow the same pattern here.\n> \n>> +\tcase libcamera::formats::SGRBG10_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 5;\n>> +\t\tparams_.rawp.bpp_denom = 4;\n>> +\t\tparams_.rawp.r_pos = 1;\n>> +\t\tbreak;\n>> +\n>> +\tcase libcamera::formats::SGBRG10_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 5;\n>> +\t\tparams_.rawp.bpp_denom = 4;\n>> +\t\tparams_.rawp.r_pos = 2;\n>> +\t\tbreak;\n>> +\n>> +\tcase libcamera::formats::SBGGR10_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 5;\n>> +\t\tparams_.rawp.bpp_denom = 4;\n>> +\t\tparams_.rawp.r_pos = 3;\n>> +\t\tbreak;\n>> +\n>> +\tcase libcamera::formats::SRGGB12_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 3;\t/* 1.5 bytes per pixel */\n>> +\t\tparams_.rawp.bpp_denom = 2;\n>> +\t\tparams_.rawp.r_pos = 0;\n>> +\t\tbreak;\n>> +\n>> +\tcase libcamera::formats::SGRBG12_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 3;\n>> +\t\tparams_.rawp.bpp_denom = 2;\n>> +\t\tparams_.rawp.r_pos = 1;\n>> +\t\tbreak;\n>> +\n>> +\tcase libcamera::formats::SGBRG12_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 3;\n>> +\t\tparams_.rawp.bpp_denom = 2;\n>> +\t\tparams_.rawp.r_pos = 2;\n>> +\t\tbreak;\n>> +\n>> +\tcase libcamera::formats::SBGGR12_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 3;\n>> +\t\tparams_.rawp.bpp_denom = 2;\n>> +\t\tparams_.rawp.r_pos = 3;\n>> +\t\tbreak;\n>> +\n>> \tcase libcamera::formats::MJPEG:\n>> \t\tformatFamily_ = MJPEG;\n>> \t\tbreak;\n>> @@ -144,6 +201,45 @@ int FormatConverter::configure(const libcamera::PixelFormat &format,\n>> \twidth_ = size.width();\n>> \theight_ = size.height();\n>> \n>> +\tif (formatFamily_ == RAW_CSI2P) {\n>> +\t\t/*\n>> +\t\t * For RAW_CSI2P the assumption is that height_ and width_\n>> +\t\t * are even numbers.\n>> +\t\t */\n>> +\t\tif ( width_ % 2 != 0 || height_ % 2 != 0 ) {\n> \n> No space after ( and before ). You can also drop the != 0.\n> \n>> +\t\t\tqWarning() << \"Image width or height isn't even number\";\n>> +\t\t\treturn -EINVAL;\n>> +\t\t}\n>> +\n>> +\t\tparams_.rawp.srcLineLength = width_ * params_.rawp.bpp_numer /\n>> +\t\t\t\t\t params_.rawp.bpp_denom;\n>> +\n>> +\t\t/*\n>> +\t\t * Calculate [g1,g2,b]_pos based on r_pos.\n>> +\t\t * On entrance, r_pos is the position of red pixel in the\n> \n> s/entrace/entry/ (Kieran can correct me if this isn't right)\n\n\"Upon entry, \"\n\n> s/red pixel/the red pixel/\n> \n>> +\t\t * 2-by-2 pixel Bayer pattern, and is from 0 to 3 range:\n> \n> \"is in the 0 to 3 range\" or \"is in the [0, 3] range\"\n> \n>> +\t\t * +---+---+\n>> +\t\t * | 0 | 1 |\n>> +\t\t * +---+---+\n>> +\t\t * | 2 | 3 |\n>> +\t\t * +---+---+\n>> +\t\t * At exit, [r,g1,g2,b]_pos are offsetts of the color\n> \n> s/offsetts/the offsets/\n> \n>> +\t\t * values in the source buffer.\n>> +\t\t */\n>> +\t\tif (params_.rawp.r_pos > 1) {\n>> +\t\t\tparams_.rawp.r_pos = params_.rawp.r_pos - 2 +\n>> +\t\t\t\t\t params_.rawp.srcLineLength;\n>> +\t\t\tparams_.rawp.b_pos = 3 - params_.rawp.r_pos;\n>> +\t\t} else {\n>> +\t\t\tparams_.rawp.b_pos = 1 - params_.rawp.r_pos +\n>> +\t\t\t\t\t params_.rawp.srcLineLength;\n>> +\t\t}\n>> +\t\tparams_.rawp.g1_pos = (params_.rawp.r_pos == 0 ||\n>> +\t\t\t\t params_.rawp.b_pos == 0) ? 1 : 0;\n>> +\t\tparams_.rawp.g2_pos = 1 - params_.rawp.g1_pos +\n>> +\t\t\t\t params_.rawp.srcLineLength;\n>> +\t}\n>> +\n>> \treturn 0;\n>> }\n>> \n>> @@ -163,9 +259,51 @@ void FormatConverter::convert(const unsigned char *src, size_t size,\n>> \tcase NV:\n>> \t\tconvertNV(src, dst->bits());\n>> \t\tbreak;\n>> +\tcase RAW_CSI2P:\n>> +\t\tconvertRawCSI2P(src, dst->bits());\n>> +\t\tbreak;\n>> \t};\n>> }\n>> \n>> +void FormatConverter::convertRawCSI2P(const unsigned char *src,\n>> +\t\t\t\t unsigned char *dst)\n>> +{\n>> +\tunsigned int g;\n>> +\tstatic unsigned char dst_buf[8] = { 0, 0, 0, 0xff };\n>> +\tunsigned int dstLineLength = width_ * 4;\n>> +\n>> +\tfor (unsigned int y = 0; y < height_; y += 2) {\n>> +\t\tfor (unsigned x = 0; x < width_; x += params_.rawp.bpp_denom) {\n>> +\t\t\tfor (unsigned int i = 0; i < params_.rawp.bpp_denom ;\n> \n> s/ ;/;/\n> \n>> +\t\t\t i += 2) {\n> \n> I wouldn't wrap this to the next line, it looks weird.\n> \n>> +\t\t\t\t/*\n>> +\t\t\t\t * Process the current 2x2 group.\n>> +\t\t\t\t * Use the average of the two green pixels as\n>> +\t\t\t\t * the green value for all the pixels in the\n>> +\t\t\t\t * group.\n>> +\t\t\t\t */\n>> +\t\t\t\tdst_buf[0] = src[params_.rawp.b_pos];\n>> +\t\t\t\tg = src[params_.rawp.g1_pos];\n>> +\t\t\t\tg = (g + src[params_.rawp.g2_pos]) >> 1;\n>> +\t\t\t\tdst_buf[1] = (unsigned char)g;\n>> +\t\t\t\tdst_buf[2] = src[params_.rawp.r_pos];\n>> +\t\t\t\tsrc += 2;\n>> +\n>> +\t\t\t\tmemcpy(dst_buf + 4, dst_buf, 4);\n>> +\t\t\t\tmemcpy(dst, dst_buf, 8);\n>> +\t\t\t\tmemcpy(dst + dstLineLength, dst_buf, 8);\n> \n> The memcpy() calls look really awkward. I suppose this is what you've\n> mentioned as a performance improvement in the cover letter ? I'm\n> concerned someone will forget about it in the future and refactor this\n> code. We need at least a comment that explains what's going on.\n> \n> I wonder if you won't get better performances from splitting this\n> function in two, and unrolling the inner loop for the 10bpp case.\n> \n>> +\t\t\t\tdst += 8;\n>> +\t\t\t}\n>> +\t\t\tsrc += params_.rawp.bpp_numer - params_.rawp.bpp_denom;\n>> +\t\t}\n> \n> Can you add a blank line here ?\n> \n>> +\t\t/* odd lines are copies of the even lines they follow: */\n> \n> s/odd/Odd/\n> s/follow:/follow/.\n> \n>> +\t\tmemcpy(dst, dst-dstLineLength, dstLineLength);\n> \n> Blank line here too.\n> \n>> +\t\t/* move to the next even line: */\n> \n> s/move/Move/\n> s/line:/line./\n> \n>> +\t\tsrc += params_.rawp.srcLineLength;\n>> +\t\tdst += dstLineLength;\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>> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h\n>> index c53fa28..dc20e0d 100644\n>> --- a/src/qcam/format_converter.h\n>> +++ b/src/qcam/format_converter.h\n>> @@ -26,11 +26,13 @@ private:\n>> \tenum FormatFamily {\n>> \t\tMJPEG,\n>> \t\tNV,\n>> +\t\tRAW_CSI2P,\n> \n> I'd name this RawCSI2Packed.\n> \n>> \t\tRGB,\n>> \t\tYUV,\n>> \t};\n>> \n>> \tvoid convertNV(const unsigned char *src, unsigned char *dst);\n>> +\tvoid convertRawCSI2P(const unsigned char *src, unsigned char *dst);\n> \n> And s/CSI2P/CSI2Packed/\n> \n>> \tvoid convertRGB(const unsigned char *src, unsigned char *dst);\n>> \tvoid convertYUV(const unsigned char *src, unsigned char *dst);\n>> \n>> @@ -48,6 +50,25 @@ private:\n>> \t\t\tbool nvSwap;\n>> \t\t} nv;\n>> \n>> +\t\t/* RAW Bayer CSI2P parameters */\n>> +\t\tstruct raw_csi2p_params {\n>> +\t\t\t/*\n>> +\t\t\t * Bytes per pixel is a fractional number, and is\n>> +\t\t\t * represented by integer numerator and denominator.\n>> +\t\t\t */\n>> +\t\t\tunsigned int bpp_numer;\n>> +\t\t\tunsigned int bpp_denom;\n>> +\t\t\tunsigned int r_pos;\n>> +\t\t\t/*\n>> +\t\t\t * The fields below are used to hold the values computed\n>> +\t\t\t * in configure() based on r_pos and width_.\n>> +\t\t\t */\n>> +\t\t\tunsigned int g1_pos;\n>> +\t\t\tunsigned int g2_pos;\n>> +\t\t\tunsigned int b_pos;\n>> +\t\t\tunsigned int srcLineLength;\n>> +\t\t} rawp;\n> \n> These parameters are applicable to all Bayer formats, not only the\n> packed ones. And they're specific to Bayer formats, they're not\n> applicable to other color filter array patterns (or other non-CFA raw\n> formats). I would thus rename the structure from raw_csi2p_params to\n> bayer_params and the member from rawp to bayer.\n> \n>> +\n>> \t\t/* RGB parameters */\n>> \t\tstruct rgb_params {\n>> \t\t\tunsigned int bpp;\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E390FBF01C\n\tfor ;\n\tSat, 12 Sep 2020 20:07:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6208862D27;\n\tSat, 12 Sep 2020 22:07:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 743356036D\n\tfor ;\n\tSat, 12 Sep 2020 22:07:54 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D0E03276;\n\tSat, 12 Sep 2020 22:07:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"i1c45UK2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599941274;\n\tbh=HTA1d78WCyVCa5H3EhtRv/XgOONzF0m+My+uRuTrYac=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=i1c45UK2UiNHFhNZNUTTAo2d9O3edftjq7cIb6MsJTQAEnlxQZfaLcF8Cd+/a8TM1\n\tS+zbMyuhaAqjOrM57iylBJRSLq6wTqf6DhbHSy2abEX8twSzRVVwfXyDCg1cttoa7S\n\tGPWNcDWzK+z8BXScSjsh1HksgrhKjTy5EPHcSL2A=","To":"Laurent Pinchart ,\n\tAndrey Konovalov ","References":"<20200908150739.1552-1-andrey.konovalov@linaro.org>\n\t<20200908150739.1552-3-andrey.konovalov@linaro.org>\n\t<20200912010331.GI6808@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Sat, 12 Sep 2020 21:07:50 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200912010331.GI6808@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/2] qcam: format_converter:\n\tadd 10 and 12 bit packed raw Bayer formats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12646,"web_url":"https://patchwork.libcamera.org/comment/12646/","msgid":"","date":"2020-09-22T20:36:50","subject":"Re: [libcamera-devel] [RFC PATCH v2 2/2] qcam: format_converter:\n\tadd 10 and 12 bit packed raw Bayer formats","submitter":{"id":25,"url":"https://patchwork.libcamera.org/api/people/25/","name":"Andrey Konovalov","email":"andrey.konovalov@linaro.org"},"content":"Hi Laurent,\n\nOn 12.09.2020 04:03, Laurent Pinchart wrote:\n> Hi Andrey,\n> \n> Thank you for the patch.\n> \n> On Tue, Sep 08, 2020 at 06:07:39PM +0300, Andrey Konovalov wrote:\n>> No interpolation is used to get more speed by the price of lower image\n>> quality. In qcam format_converter is used for viewfinder only, and in\n>> this case lower lag is more important than the image quality.\n>>\n>> Signed-off-by: Andrey Konovalov \n>> ---\n>> src/qcam/format_converter.cpp | 138 ++++++++++++++++++++++++++++++++++\n>> src/qcam/format_converter.h | 21 ++++++\n>> 2 files changed, 159 insertions(+)\n>>\n>> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp\n>> index 3481330..f370907 100644\n>> --- a/src/qcam/format_converter.cpp\n>> +++ b/src/qcam/format_converter.cpp\n>> @@ -10,6 +10,7 @@\n>> #include \n>> \n>> #include \n>> +#include \n>> \n>> #include \n>> \n>> @@ -132,6 +133,62 @@ int FormatConverter::configure(const libcamera::PixelFormat &format,\n>> \t\tparams_.yuv.cb_pos = 1;\n>> \t\tbreak;\n>> \n>> +\tcase libcamera::formats::SRGGB10_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 5;\t/* 1.25 bytes per pixel */\n>> +\t\tparams_.rawp.bpp_denom = 4;\n>> +\t\tparams_.rawp.r_pos = 0;\n>> +\t\tbreak;\n>> +\n> \n> There are no blank lines between formats of the same group for other\n> groups, I'd follow the same pattern here.\n> \n>> +\tcase libcamera::formats::SGRBG10_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 5;\n>> +\t\tparams_.rawp.bpp_denom = 4;\n>> +\t\tparams_.rawp.r_pos = 1;\n>> +\t\tbreak;\n>> +\n>> +\tcase libcamera::formats::SGBRG10_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 5;\n>> +\t\tparams_.rawp.bpp_denom = 4;\n>> +\t\tparams_.rawp.r_pos = 2;\n>> +\t\tbreak;\n>> +\n>> +\tcase libcamera::formats::SBGGR10_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 5;\n>> +\t\tparams_.rawp.bpp_denom = 4;\n>> +\t\tparams_.rawp.r_pos = 3;\n>> +\t\tbreak;\n>> +\n>> +\tcase libcamera::formats::SRGGB12_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 3;\t/* 1.5 bytes per pixel */\n>> +\t\tparams_.rawp.bpp_denom = 2;\n>> +\t\tparams_.rawp.r_pos = 0;\n>> +\t\tbreak;\n>> +\n>> +\tcase libcamera::formats::SGRBG12_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 3;\n>> +\t\tparams_.rawp.bpp_denom = 2;\n>> +\t\tparams_.rawp.r_pos = 1;\n>> +\t\tbreak;\n>> +\n>> +\tcase libcamera::formats::SGBRG12_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 3;\n>> +\t\tparams_.rawp.bpp_denom = 2;\n>> +\t\tparams_.rawp.r_pos = 2;\n>> +\t\tbreak;\n>> +\n>> +\tcase libcamera::formats::SBGGR12_CSI2P:\n>> +\t\tformatFamily_ = RAW_CSI2P;\n>> +\t\tparams_.rawp.bpp_numer = 3;\n>> +\t\tparams_.rawp.bpp_denom = 2;\n>> +\t\tparams_.rawp.r_pos = 3;\n>> +\t\tbreak;\n>> +\n>> \tcase libcamera::formats::MJPEG:\n>> \t\tformatFamily_ = MJPEG;\n>> \t\tbreak;\n>> @@ -144,6 +201,45 @@ int FormatConverter::configure(const libcamera::PixelFormat &format,\n>> \twidth_ = size.width();\n>> \theight_ = size.height();\n>> \n>> +\tif (formatFamily_ == RAW_CSI2P) {\n>> +\t\t/*\n>> +\t\t * For RAW_CSI2P the assumption is that height_ and width_\n>> +\t\t * are even numbers.\n>> +\t\t */\n>> +\t\tif ( width_ % 2 != 0 || height_ % 2 != 0 ) {\n> \n> No space after ( and before ). You can also drop the != 0.\n> \n>> +\t\t\tqWarning() << \"Image width or height isn't even number\";\n>> +\t\t\treturn -EINVAL;\n>> +\t\t}\n>> +\n>> +\t\tparams_.rawp.srcLineLength = width_ * params_.rawp.bpp_numer /\n>> +\t\t\t\t\t params_.rawp.bpp_denom;\n>> +\n>> +\t\t/*\n>> +\t\t * Calculate [g1,g2,b]_pos based on r_pos.\n>> +\t\t * On entrance, r_pos is the position of red pixel in the\n> \n> s/entrace/entry/ (Kieran can correct me if this isn't right)\n> s/red pixel/the red pixel/\n> \n>> +\t\t * 2-by-2 pixel Bayer pattern, and is from 0 to 3 range:\n> \n> \"is in the 0 to 3 range\" or \"is in the [0, 3] range\"\n> \n>> +\t\t * +---+---+\n>> +\t\t * | 0 | 1 |\n>> +\t\t * +---+---+\n>> +\t\t * | 2 | 3 |\n>> +\t\t * +---+---+\n>> +\t\t * At exit, [r,g1,g2,b]_pos are offsetts of the color\n> \n> s/offsetts/the offsets/\n> \n>> +\t\t * values in the source buffer.\n>> +\t\t */\n>> +\t\tif (params_.rawp.r_pos > 1) {\n>> +\t\t\tparams_.rawp.r_pos = params_.rawp.r_pos - 2 +\n>> +\t\t\t\t\t params_.rawp.srcLineLength;\n>> +\t\t\tparams_.rawp.b_pos = 3 - params_.rawp.r_pos;\n>> +\t\t} else {\n>> +\t\t\tparams_.rawp.b_pos = 1 - params_.rawp.r_pos +\n>> +\t\t\t\t\t params_.rawp.srcLineLength;\n>> +\t\t}\n>> +\t\tparams_.rawp.g1_pos = (params_.rawp.r_pos == 0 ||\n>> +\t\t\t\t params_.rawp.b_pos == 0) ? 1 : 0;\n>> +\t\tparams_.rawp.g2_pos = 1 - params_.rawp.g1_pos +\n>> +\t\t\t\t params_.rawp.srcLineLength;\n>> +\t}\n>> +\n>> \treturn 0;\n>> }\n>> \n>> @@ -163,9 +259,51 @@ void FormatConverter::convert(const unsigned char *src, size_t size,\n>> \tcase NV:\n>> \t\tconvertNV(src, dst->bits());\n>> \t\tbreak;\n>> +\tcase RAW_CSI2P:\n>> +\t\tconvertRawCSI2P(src, dst->bits());\n>> +\t\tbreak;\n>> \t};\n>> }\n>> \n>> +void FormatConverter::convertRawCSI2P(const unsigned char *src,\n>> +\t\t\t\t unsigned char *dst)\n>> +{\n>> +\tunsigned int g;\n>> +\tstatic unsigned char dst_buf[8] = { 0, 0, 0, 0xff };\n>> +\tunsigned int dstLineLength = width_ * 4;\n>> +\n>> +\tfor (unsigned int y = 0; y < height_; y += 2) {\n>> +\t\tfor (unsigned x = 0; x < width_; x += params_.rawp.bpp_denom) {\n>> +\t\t\tfor (unsigned int i = 0; i < params_.rawp.bpp_denom ;\n> \n> s/ ;/;/\n> \n>> +\t\t\t i += 2) {\n> \n> I wouldn't wrap this to the next line, it looks weird.\n> \n>> +\t\t\t\t/*\n>> +\t\t\t\t * Process the current 2x2 group.\n>> +\t\t\t\t * Use the average of the two green pixels as\n>> +\t\t\t\t * the green value for all the pixels in the\n>> +\t\t\t\t * group.\n>> +\t\t\t\t */\n>> +\t\t\t\tdst_buf[0] = src[params_.rawp.b_pos];\n>> +\t\t\t\tg = src[params_.rawp.g1_pos];\n>> +\t\t\t\tg = (g + src[params_.rawp.g2_pos]) >> 1;\n>> +\t\t\t\tdst_buf[1] = (unsigned char)g;\n>> +\t\t\t\tdst_buf[2] = src[params_.rawp.r_pos];\n>> +\t\t\t\tsrc += 2;\n>> +\n>> +\t\t\t\tmemcpy(dst_buf + 4, dst_buf, 4);\n>> +\t\t\t\tmemcpy(dst, dst_buf, 8);\n>> +\t\t\t\tmemcpy(dst + dstLineLength, dst_buf, 8);\n> \n> The memcpy() calls look really awkward. I suppose this is what you've\n> mentioned as a performance improvement in the cover letter ?\n\nYep, v2 looks worse here, but works a bit faster.\n\nThe thing is that after setting the green value for all the pixels in the 2x2 group\nto the average of the two green pixels in v2, all the 4 corresponding RGB pixels in\nthe dst_buf got the same color values. So we wouldn't lose any information if we\nmerged these 4 pixels into one, and scaled down the dst[] to width_/2 by height_/2.\nThis would not only save the extra memory writes to dst[], but also might speed up\nthe scaling done by QT to match the qcam window size. Is it something that worth trying?\n\nI am also thinking about trying linear interpolation (using from 2 to 4 nearest Bayer pixels\nof the same color) - it will be slower, but will prevent these 2x2 clusters with the same\ncolor values (thus improving the image quality a bit).\n\n> I'm\n> concerned someone will forget about it in the future and refactor this\n> code. We need at least a comment that explains what's going on.\n> \n> I wonder if you won't get better performances from splitting this\n> function in two, and unrolling the inner loop for the 10bpp case.\n\nI checked this before introducing the inner loop, and the fps decreased just slightly\nafter the inner loop was created (the difference can't be seen by just looking at the\nprinted fps numbers - without calculating the average fps over ~20 of the fps numbers).\nSo I decided to go with the inner loop as it allows to reuse the same code for 10bpp, and\n12bpp (and 14bpp if we ever add it) - it would be easier to maintain/modify in the future.\n\nThanks,\nAndrey\n\n>> +\t\t\t\tdst += 8;\n>> +\t\t\t}\n>> +\t\t\tsrc += params_.rawp.bpp_numer - params_.rawp.bpp_denom;\n>> +\t\t}\n> \n> Can you add a blank line here ?\n> \n>> +\t\t/* odd lines are copies of the even lines they follow: */\n> \n> s/odd/Odd/\n> s/follow:/follow/.\n> \n>> +\t\tmemcpy(dst, dst-dstLineLength, dstLineLength);\n> \n> Blank line here too.\n> \n>> +\t\t/* move to the next even line: */\n> \n> s/move/Move/\n> s/line:/line./\n> \n>> +\t\tsrc += params_.rawp.srcLineLength;\n>> +\t\tdst += dstLineLength;\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>> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h\n>> index c53fa28..dc20e0d 100644\n>> --- a/src/qcam/format_converter.h\n>> +++ b/src/qcam/format_converter.h\n>> @@ -26,11 +26,13 @@ private:\n>> \tenum FormatFamily {\n>> \t\tMJPEG,\n>> \t\tNV,\n>> +\t\tRAW_CSI2P,\n> \n> I'd name this RawCSI2Packed.\n> \n>> \t\tRGB,\n>> \t\tYUV,\n>> \t};\n>> \n>> \tvoid convertNV(const unsigned char *src, unsigned char *dst);\n>> +\tvoid convertRawCSI2P(const unsigned char *src, unsigned char *dst);\n> \n> And s/CSI2P/CSI2Packed/\n> \n>> \tvoid convertRGB(const unsigned char *src, unsigned char *dst);\n>> \tvoid convertYUV(const unsigned char *src, unsigned char *dst);\n>> \n>> @@ -48,6 +50,25 @@ private:\n>> \t\t\tbool nvSwap;\n>> \t\t} nv;\n>> \n>> +\t\t/* RAW Bayer CSI2P parameters */\n>> +\t\tstruct raw_csi2p_params {\n>> +\t\t\t/*\n>> +\t\t\t * Bytes per pixel is a fractional number, and is\n>> +\t\t\t * represented by integer numerator and denominator.\n>> +\t\t\t */\n>> +\t\t\tunsigned int bpp_numer;\n>> +\t\t\tunsigned int bpp_denom;\n>> +\t\t\tunsigned int r_pos;\n>> +\t\t\t/*\n>> +\t\t\t * The fields below are used to hold the values computed\n>> +\t\t\t * in configure() based on r_pos and width_.\n>> +\t\t\t */\n>> +\t\t\tunsigned int g1_pos;\n>> +\t\t\tunsigned int g2_pos;\n>> +\t\t\tunsigned int b_pos;\n>> +\t\t\tunsigned int srcLineLength;\n>> +\t\t} rawp;\n> \n> These parameters are applicable to all Bayer formats, not only the\n> packed ones. And they're specific to Bayer formats, they're not\n> applicable to other color filter array patterns (or other non-CFA raw\n> formats). I would thus rename the structure from raw_csi2p_params to\n> bayer_params and the member from rawp to bayer.\n> \n>> +\n>> \t\t/* RGB parameters */\n>> \t\tstruct rgb_params {\n>> \t\t\tunsigned int bpp;\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 49FAEC3B5C\n\tfor ;\n\tTue, 22 Sep 2020 20:36:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B4DF562FDC;\n\tTue, 22 Sep 2020 22:36:54 +0200 (CEST)","from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5758C6036A\n\tfor ;\n\tTue, 22 Sep 2020 22:36:53 +0200 (CEST)","by mail-lf1-x141.google.com with SMTP id x69so19515988lff.3\n\tfor ;\n\tTue, 22 Sep 2020 13:36:53 -0700 (PDT)","from [192.168.118.216] (37-144-159-139.broadband.corbina.ru.\n\t[37.144.159.139]) by smtp.gmail.com with ESMTPSA id\n\tf19sm3876712lfk.53.2020.09.22.13.36.51\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tTue, 22 Sep 2020 13:36:51 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"LJoTw7H/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=subject:to:cc:references:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-language:content-transfer-encoding; \n\tbh=YsY9c3HfyiGg5t2984Xf4yxSEtVwThz4RkNfLbxpFoI=;\n\tb=LJoTw7H/8hExXsswaqYofRxqhNaOXnPwh1NorwhIuXwNdIu1SJFzeO8vSJ1TH+a//l\n\tdouameiVNnKwjQnSF3Min2N1yKjwlfvCKoZpqhKnRQma+Cu8zylgswQ5qmNWhNF2dMfa\n\tXHtAmyVQ6IiyiipuJv/3xsGloTzFhhvKVkCTOMpoqa9LkFsH0775PeA4MdKy2VVwOUnw\n\tYVi0r9mVrks4MayQhc7d9RPkAeSLZQHqlWx1owuNK3AnQK0sDK9FpKfuiWHsLXQf1Ddm\n\t/Gzq6wxpUYN8BWYX59k42VC0kLiqUbJjdTQbJ+DaLYL1BvDYj4C50fIUTl27lTLoO5DL\n\tjIEg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=YsY9c3HfyiGg5t2984Xf4yxSEtVwThz4RkNfLbxpFoI=;\n\tb=qu5jLe2ioH4e9tVZw1+92m+3NcozywG8PejMu/7j4LwEb2Y+rTk2oSU2HKYN1YflWR\n\tkdcG2AXR/lAqffTIktsielysNmP779/gWq0byDmj+Bzbykn1JuuIkVkng5BsvII2lmz2\n\tvcIAz1dBI+ICZ/fPVptaqKX3Dp+9Lt107s9jwFavMdvYvPAwbKQJYiCYc8GS0IJLgKBb\n\tnn6cl9eWPaBBt9iufK+eJcsUb0ebkqOZHwWF0if0Zb/xKVQBtDxgNa2XdxoX3/zIXb98\n\tL2N6W5xobHvVRTFMpoOg45UcggdrqQZ/XlrHtOB0haRxUj8ZA2pxdhYw8QCt+guvoIqL\n\tsvww==","X-Gm-Message-State":"AOAM531rm9EPgoFyL+hLDFEedp99vM/Roy2EZ51AXiMGjQ1xV4w0WJHa\n\tlDY17Cy3XBq+Er1gHZuSLeNKvw==","X-Google-Smtp-Source":"ABdhPJz0QWtCtO9bN1ulP4EDh4Z2GbSeY7L0nCM9bVX42qo7Pa3LiVJ86QlD549ZmEJlUtbPGwFAOg==","X-Received":"by 2002:ac2:4c11:: with SMTP id\n\tt17mr2419043lfq.260.1600807012579; \n\tTue, 22 Sep 2020 13:36:52 -0700 (PDT)","To":"Laurent Pinchart ","References":"<20200908150739.1552-1-andrey.konovalov@linaro.org>\n\t<20200908150739.1552-3-andrey.konovalov@linaro.org>\n\t<20200912010331.GI6808@pendragon.ideasonboard.com>","From":"Andrey Konovalov ","Message-ID":"","Date":"Tue, 22 Sep 2020 23:36:50 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200912010331.GI6808@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH v2 2/2] qcam: format_converter:\n\tadd 10 and 12 bit packed raw Bayer formats","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]