[{"id":12461,"web_url":"https://patchwork.libcamera.org/comment/12461/","msgid":"<20200912004419.GH6808@pendragon.ideasonboard.com>","date":"2020-09-12T00:44:19","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/2] qcam: format_converter:\n\tput parameters of different formats into union","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:38PM +0300, Andrey Konovalov wrote:\n> As the number of supported formats in the converter grows, having the\n> format-specific parameters in a union makes the code cleaner, and save\n> some amount of memory.\n>\n> Signed-off-by: Andrey Konovalov \n> ---\n> src/qcam/format_converter.cpp | 133 ++++++++++++++++++----------------\n> src/qcam/format_converter.h | 37 ++++++----\n> 2 files changed, 92 insertions(+), 78 deletions(-)\n> \n> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp\n> index 4b9722d..3481330 100644\n> --- a/src/qcam/format_converter.cpp\n> +++ b/src/qcam/format_converter.cpp\n> @@ -33,103 +33,103 @@ int FormatConverter::configure(const libcamera::PixelFormat &format,\n> \tswitch (format) {\n> \tcase libcamera::formats::NV12:\n> \t\tformatFamily_ = NV;\n> -\t\thorzSubSample_ = 2;\n> -\t\tvertSubSample_ = 2;\n> -\t\tnvSwap_ = false;\n> +\t\tparams_.nv.horzSubSample = 2;\n> +\t\tparams_.nv.vertSubSample = 2;\n> +\t\tparams_.nv.nvSwap = false;\n> \t\tbreak;\n> \tcase libcamera::formats::NV21:\n> \t\tformatFamily_ = NV;\n> -\t\thorzSubSample_ = 2;\n> -\t\tvertSubSample_ = 2;\n> -\t\tnvSwap_ = true;\n> +\t\tparams_.nv.horzSubSample = 2;\n> +\t\tparams_.nv.vertSubSample = 2;\n> +\t\tparams_.nv.nvSwap = true;\n> \t\tbreak;\n> \tcase libcamera::formats::NV16:\n> \t\tformatFamily_ = NV;\n> -\t\thorzSubSample_ = 2;\n> -\t\tvertSubSample_ = 1;\n> -\t\tnvSwap_ = false;\n> +\t\tparams_.nv.horzSubSample = 2;\n> +\t\tparams_.nv.vertSubSample = 1;\n> +\t\tparams_.nv.nvSwap = false;\n> \t\tbreak;\n> \tcase libcamera::formats::NV61:\n> \t\tformatFamily_ = NV;\n> -\t\thorzSubSample_ = 2;\n> -\t\tvertSubSample_ = 1;\n> -\t\tnvSwap_ = true;\n> +\t\tparams_.nv.horzSubSample = 2;\n> +\t\tparams_.nv.vertSubSample = 1;\n> +\t\tparams_.nv.nvSwap = true;\n> \t\tbreak;\n> \tcase libcamera::formats::NV24:\n> \t\tformatFamily_ = NV;\n> -\t\thorzSubSample_ = 1;\n> -\t\tvertSubSample_ = 1;\n> -\t\tnvSwap_ = false;\n> +\t\tparams_.nv.horzSubSample = 1;\n> +\t\tparams_.nv.vertSubSample = 1;\n> +\t\tparams_.nv.nvSwap = false;\n> \t\tbreak;\n> \tcase libcamera::formats::NV42:\n> \t\tformatFamily_ = NV;\n> -\t\thorzSubSample_ = 1;\n> -\t\tvertSubSample_ = 1;\n> -\t\tnvSwap_ = true;\n> +\t\tparams_.nv.horzSubSample = 1;\n> +\t\tparams_.nv.vertSubSample = 1;\n> +\t\tparams_.nv.nvSwap = true;\n> \t\tbreak;\n> \n> \tcase libcamera::formats::RGB888:\n> \t\tformatFamily_ = RGB;\n> -\t\tr_pos_ = 2;\n> -\t\tg_pos_ = 1;\n> -\t\tb_pos_ = 0;\n> -\t\tbpp_ = 3;\n> +\t\tparams_.rgb.r_pos = 2;\n> +\t\tparams_.rgb.g_pos = 1;\n> +\t\tparams_.rgb.b_pos = 0;\n> +\t\tparams_.rgb.bpp = 3;\n> \t\tbreak;\n> \tcase libcamera::formats::BGR888:\n> \t\tformatFamily_ = RGB;\n> -\t\tr_pos_ = 0;\n> -\t\tg_pos_ = 1;\n> -\t\tb_pos_ = 2;\n> -\t\tbpp_ = 3;\n> +\t\tparams_.rgb.r_pos = 0;\n> +\t\tparams_.rgb.g_pos = 1;\n> +\t\tparams_.rgb.b_pos = 2;\n> +\t\tparams_.rgb.bpp = 3;\n> \t\tbreak;\n> \tcase libcamera::formats::ARGB8888:\n> \t\tformatFamily_ = RGB;\n> -\t\tr_pos_ = 2;\n> -\t\tg_pos_ = 1;\n> -\t\tb_pos_ = 0;\n> -\t\tbpp_ = 4;\n> +\t\tparams_.rgb.r_pos = 2;\n> +\t\tparams_.rgb.g_pos = 1;\n> +\t\tparams_.rgb.b_pos = 0;\n> +\t\tparams_.rgb.bpp = 4;\n> \t\tbreak;\n> \tcase libcamera::formats::RGBA8888:\n> \t\tformatFamily_ = RGB;\n> -\t\tr_pos_ = 3;\n> -\t\tg_pos_ = 2;\n> -\t\tb_pos_ = 1;\n> -\t\tbpp_ = 4;\n> +\t\tparams_.rgb.r_pos = 3;\n> +\t\tparams_.rgb.g_pos = 2;\n> +\t\tparams_.rgb.b_pos = 1;\n> +\t\tparams_.rgb.bpp = 4;\n> \t\tbreak;\n> \tcase libcamera::formats::ABGR8888:\n> \t\tformatFamily_ = RGB;\n> -\t\tr_pos_ = 0;\n> -\t\tg_pos_ = 1;\n> -\t\tb_pos_ = 2;\n> -\t\tbpp_ = 4;\n> +\t\tparams_.rgb.r_pos = 0;\n> +\t\tparams_.rgb.g_pos = 1;\n> +\t\tparams_.rgb.b_pos = 2;\n> +\t\tparams_.rgb.bpp = 4;\n> \t\tbreak;\n> \tcase libcamera::formats::BGRA8888:\n> \t\tformatFamily_ = RGB;\n> -\t\tr_pos_ = 1;\n> -\t\tg_pos_ = 2;\n> -\t\tb_pos_ = 3;\n> -\t\tbpp_ = 4;\n> +\t\tparams_.rgb.r_pos = 1;\n> +\t\tparams_.rgb.g_pos = 2;\n> +\t\tparams_.rgb.b_pos = 3;\n> +\t\tparams_.rgb.bpp = 4;\n> \t\tbreak;\n> \n> \tcase libcamera::formats::VYUY:\n> \t\tformatFamily_ = YUV;\n> -\t\ty_pos_ = 1;\n> -\t\tcb_pos_ = 2;\n> +\t\tparams_.yuv.y_pos = 1;\n> +\t\tparams_.yuv.cb_pos = 2;\n> \t\tbreak;\n> \tcase libcamera::formats::YVYU:\n> \t\tformatFamily_ = YUV;\n> -\t\ty_pos_ = 0;\n> -\t\tcb_pos_ = 3;\n> +\t\tparams_.yuv.y_pos = 0;\n> +\t\tparams_.yuv.cb_pos = 3;\n> \t\tbreak;\n> \tcase libcamera::formats::UYVY:\n> \t\tformatFamily_ = YUV;\n> -\t\ty_pos_ = 1;\n> -\t\tcb_pos_ = 0;\n> +\t\tparams_.yuv.y_pos = 1;\n> +\t\tparams_.yuv.cb_pos = 0;\n> \t\tbreak;\n> \tcase libcamera::formats::YUYV:\n> \t\tformatFamily_ = YUV;\n> -\t\ty_pos_ = 0;\n> -\t\tcb_pos_ = 1;\n> +\t\tparams_.yuv.y_pos = 0;\n> +\t\tparams_.yuv.cb_pos = 1;\n> \t\tbreak;\n> \n> \tcase libcamera::formats::MJPEG:\n> @@ -178,18 +178,20 @@ static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)\n> \n> void FormatConverter::convertNV(const unsigned char *src, unsigned char *dst)\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 c_stride = width_ * (2 / params_.nv.horzSubSample);\n> +\tunsigned int c_inc = params_.nv.horzSubSample == 1 ? 2 : 0;\n> +\tunsigned int cb_pos = params_.nv.nvSwap ? 1 : 0;\n> +\tunsigned int cr_pos = params_.nv.nvSwap ? 0 : 1;\n> \tconst unsigned char *src_c = src + width_ * height_;\n> \tint r, g, b;\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\tconst unsigned char *src_cb = src_c +\n> +\t\t\t\t\t (y / params_.nv.vertSubSample) *\n> \t\t\t\t\t c_stride + cb_pos;\n> -\t\tconst unsigned char *src_cr = src_c + (y / vertSubSample_) *\n> +\t\tconst unsigned char *src_cr = src_c +\n> +\t\t\t\t\t (y / params_.nv.vertSubSample) *\n> \t\t\t\t\t c_stride + cr_pos;\n> \n> \t\tfor (unsigned int x = 0; x < width_; x += 2) {\n> @@ -223,9 +225,9 @@ void FormatConverter::convertRGB(const unsigned char *src, unsigned char *dst)\n> \n> \tfor (y = 0; y < height_; y++) {\n> \t\tfor (x = 0; x < width_; x++) {\n> -\t\t\tr = src[bpp_ * x + r_pos_];\n> -\t\t\tg = src[bpp_ * x + g_pos_];\n> -\t\t\tb = src[bpp_ * x + b_pos_];\n> +\t\t\tr = src[params_.rgb.bpp * x + params_.rgb.r_pos];\n> +\t\t\tg = src[params_.rgb.bpp * x + params_.rgb.g_pos];\n> +\t\t\tb = src[params_.rgb.bpp * x + params_.rgb.b_pos];\n> \n> \t\t\tdst[4 * x + 0] = b;\n> \t\t\tdst[4 * x + 1] = g;\n> @@ -233,7 +235,7 @@ void FormatConverter::convertRGB(const unsigned char *src, unsigned char *dst)\n> \t\t\tdst[4 * x + 3] = 0xff;\n> \t\t}\n> \n> -\t\tsrc += width_ * bpp_;\n> +\t\tsrc += width_ * params_.rgb.bpp;\n> \t\tdst += width_ * 4;\n> \t}\n> }\n> @@ -246,16 +248,18 @@ void FormatConverter::convertYUV(const unsigned char *src, unsigned char *dst)\n> \tunsigned int cr_pos;\n> \tint r, g, b, y, cr, cb;\n> \n> -\tcr_pos = (cb_pos_ + 2) % 4;\n> +\tcr_pos = (params_.yuv.cb_pos + 2) % 4;\n> \tsrc_stride = width_ * 2;\n> \tdst_stride = width_ * 4;\n> \n> \tfor (src_y = 0, dst_y = 0; dst_y < height_; src_y++, dst_y++) {\n> \t\tfor (src_x = 0, dst_x = 0; dst_x < width_; ) {\n> -\t\t\tcb = src[src_y * src_stride + src_x * 4 + cb_pos_];\n> +\t\t\tcb = src[src_y * src_stride + src_x * 4 +\n> +\t\t\t\t params_.yuv.cb_pos];\n> \t\t\tcr = src[src_y * src_stride + src_x * 4 + cr_pos];\n> \n> -\t\t\ty = src[src_y * src_stride + src_x * 4 + y_pos_];\n> +\t\t\ty = src[src_y * src_stride + src_x * 4 +\n> +\t\t\t\tparams_.yuv.y_pos];\n> \t\t\tyuv_to_rgb(y, cb, cr, &r, &g, &b);\n> \t\t\tdst[dst_y * dst_stride + 4 * dst_x + 0] = b;\n> \t\t\tdst[dst_y * dst_stride + 4 * dst_x + 1] = g;\n> @@ -263,7 +267,8 @@ void FormatConverter::convertYUV(const unsigned char *src, unsigned char *dst)\n> \t\t\tdst[dst_y * dst_stride + 4 * dst_x + 3] = 0xff;\n> \t\t\tdst_x++;\n> \n> -\t\t\ty = src[src_y * src_stride + src_x * 4 + y_pos_ + 2];\n> +\t\t\ty = src[src_y * src_stride + src_x * 4 +\n> +\t\t\t\tparams_.yuv.y_pos + 2];\n> \t\t\tyuv_to_rgb(y, cb, cr, &r, &g, &b);\n> \t\t\tdst[dst_y * dst_stride + 4 * dst_x + 0] = b;\n> \t\t\tdst[dst_y * dst_stride + 4 * dst_x + 1] = g;\n> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h\n> index e389b24..c53fa28 100644\n> --- a/src/qcam/format_converter.h\n> +++ b/src/qcam/format_converter.h\n> @@ -40,20 +40,29 @@ 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_;\n> -\tunsigned int g_pos_;\n> -\tunsigned int b_pos_;\n> -\n> -\t/* YUV parameters */\n> -\tunsigned int y_pos_;\n> -\tunsigned int cb_pos_;\n> +\tunion {\n> +\t\t/* NV parameters */\n> +\t\tstruct nv_params {\n> +\t\t\tunsigned int horzSubSample;\n> +\t\t\tunsigned int vertSubSample;\n> +\t\t\tbool nvSwap;\n> +\t\t} nv;\n> +\n> +\t\t/* RGB parameters */\n> +\t\tstruct rgb_params {\n> +\t\t\tunsigned int bpp;\n> +\t\t\tunsigned int r_pos;\n> +\t\t\tunsigned int g_pos;\n> +\t\t\tunsigned int b_pos;\n> +\t\t} rgb;\n> +\n> +\t\t/* YUV parameters */\n> +\t\tstruct yuv_params {\n> +\t\t\tunsigned int y_pos;\n> +\t\t\tunsigned int cb_pos;\n> +\t\t} yuv;\n\nThe NV* formats are YUV too, we should try to find a better name. It's a\npre-existing issue though.\n\n> +\t} params_;\n\nI wonder if it wouldn't make more sense to move all of these to a\nseparate structure, stored in a map indexed by the pixel format. That\nwould greatly simplify FormatConverter::configure(). We actually have\nsuch a structure already, PixelFormatInfo, but it's an internal API.\nExposing it as a public API runs into the issue that not all the fields\nit contains should be exposed. The V4L2 4CC, for instance, should stay\ninternal. There's probably a way to handle that, but I'm also no really\nkeen at this point to committing to a stable API for PixelFormatInfo. I\nthus think it would be better to duplicate the data we need here.\n\nTo avoid blocking Bayer support in qcam on this, how about moving\nforward with patch 2/2 for now without 1/2 ? The small space increase is\nnegligible.\n\n> +\n\nExtra blank line.\n\n> };\n> \n> #endif /* __QCAM_FORMAT_CONVERTER_H__ */","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 5FAC6BF01C\n\tfor ;\n\tSat, 12 Sep 2020 00:44:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C095162DA1;\n\tSat, 12 Sep 2020 02:44:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 301E762C43\n\tfor ;\n\tSat, 12 Sep 2020 02:44:48 +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 A18FF539;\n\tSat, 12 Sep 2020 02:44:47 +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=\"ItVdnPcp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599871487;\n\tbh=7A2SPoDIy3GqK86jYDUQUR09t81XrOUFPZHbi0bJfmk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ItVdnPcpozdqh/UjWJg2DWItO35uT3EDBV4JcINex5U6/XLHmMq+MMBLJXqbgMr3Q\n\tZGEc6m5AZuge5B1ChNraPNk3Agz5oboxnOzt94Z3c5yntzbjDfRyD9OVLDI44lqq59\n\t3/0giptgxBSAWpiLMFZaclPVnScGEFsbBk5FRxLU=","Date":"Sat, 12 Sep 2020 03:44:19 +0300","From":"Laurent Pinchart ","To":"Andrey Konovalov ","Message-ID":"<20200912004419.GH6808@pendragon.ideasonboard.com>","References":"<20200908150739.1552-1-andrey.konovalov@linaro.org>\n\t<20200908150739.1552-2-andrey.konovalov@linaro.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200908150739.1552-2-andrey.konovalov@linaro.org>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/2] qcam: format_converter:\n\tput parameters of different formats into union","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":12645,"web_url":"https://patchwork.libcamera.org/comment/12645/","msgid":"","date":"2020-09-22T20:03:48","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/2] qcam: format_converter:\n\tput parameters of different formats into union","submitter":{"id":25,"url":"https://patchwork.libcamera.org/api/people/25/","name":"Andrey Konovalov","email":"andrey.konovalov@linaro.org"},"content":"Hi Laurent,\n\nThanks for the review.\n\nOn 12.09.2020 03:44, Laurent Pinchart wrote:\n> Hi Andrey,\n> \n> Thank you for the patch.\n> \n> On Tue, Sep 08, 2020 at 06:07:38PM +0300, Andrey Konovalov wrote:\n>> As the number of supported formats in the converter grows, having the\n>> format-specific parameters in a union makes the code cleaner, and save\n>> some amount of memory.\n>>\n>> Signed-off-by: Andrey Konovalov \n>> ---\n>> src/qcam/format_converter.cpp | 133 ++++++++++++++++++----------------\n>> src/qcam/format_converter.h | 37 ++++++----\n>> 2 files changed, 92 insertions(+), 78 deletions(-)\n>>\n>> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp\n>> index 4b9722d..3481330 100644\n>> --- a/src/qcam/format_converter.cpp\n>> +++ b/src/qcam/format_converter.cpp\n>> @@ -33,103 +33,103 @@ int FormatConverter::configure(const libcamera::PixelFormat &format,\n>> \tswitch (format) {\n>> \tcase libcamera::formats::NV12:\n>> \t\tformatFamily_ = NV;\n>> -\t\thorzSubSample_ = 2;\n>> -\t\tvertSubSample_ = 2;\n>> -\t\tnvSwap_ = false;\n>> +\t\tparams_.nv.horzSubSample = 2;\n>> +\t\tparams_.nv.vertSubSample = 2;\n>> +\t\tparams_.nv.nvSwap = false;\n>> \t\tbreak;\n>> \tcase libcamera::formats::NV21:\n>> \t\tformatFamily_ = NV;\n>> -\t\thorzSubSample_ = 2;\n>> -\t\tvertSubSample_ = 2;\n>> -\t\tnvSwap_ = true;\n>> +\t\tparams_.nv.horzSubSample = 2;\n>> +\t\tparams_.nv.vertSubSample = 2;\n>> +\t\tparams_.nv.nvSwap = true;\n>> \t\tbreak;\n>> \tcase libcamera::formats::NV16:\n>> \t\tformatFamily_ = NV;\n>> -\t\thorzSubSample_ = 2;\n>> -\t\tvertSubSample_ = 1;\n>> -\t\tnvSwap_ = false;\n>> +\t\tparams_.nv.horzSubSample = 2;\n>> +\t\tparams_.nv.vertSubSample = 1;\n>> +\t\tparams_.nv.nvSwap = false;\n>> \t\tbreak;\n>> \tcase libcamera::formats::NV61:\n>> \t\tformatFamily_ = NV;\n>> -\t\thorzSubSample_ = 2;\n>> -\t\tvertSubSample_ = 1;\n>> -\t\tnvSwap_ = true;\n>> +\t\tparams_.nv.horzSubSample = 2;\n>> +\t\tparams_.nv.vertSubSample = 1;\n>> +\t\tparams_.nv.nvSwap = true;\n>> \t\tbreak;\n>> \tcase libcamera::formats::NV24:\n>> \t\tformatFamily_ = NV;\n>> -\t\thorzSubSample_ = 1;\n>> -\t\tvertSubSample_ = 1;\n>> -\t\tnvSwap_ = false;\n>> +\t\tparams_.nv.horzSubSample = 1;\n>> +\t\tparams_.nv.vertSubSample = 1;\n>> +\t\tparams_.nv.nvSwap = false;\n>> \t\tbreak;\n>> \tcase libcamera::formats::NV42:\n>> \t\tformatFamily_ = NV;\n>> -\t\thorzSubSample_ = 1;\n>> -\t\tvertSubSample_ = 1;\n>> -\t\tnvSwap_ = true;\n>> +\t\tparams_.nv.horzSubSample = 1;\n>> +\t\tparams_.nv.vertSubSample = 1;\n>> +\t\tparams_.nv.nvSwap = true;\n>> \t\tbreak;\n>> \n>> \tcase libcamera::formats::RGB888:\n>> \t\tformatFamily_ = RGB;\n>> -\t\tr_pos_ = 2;\n>> -\t\tg_pos_ = 1;\n>> -\t\tb_pos_ = 0;\n>> -\t\tbpp_ = 3;\n>> +\t\tparams_.rgb.r_pos = 2;\n>> +\t\tparams_.rgb.g_pos = 1;\n>> +\t\tparams_.rgb.b_pos = 0;\n>> +\t\tparams_.rgb.bpp = 3;\n>> \t\tbreak;\n>> \tcase libcamera::formats::BGR888:\n>> \t\tformatFamily_ = RGB;\n>> -\t\tr_pos_ = 0;\n>> -\t\tg_pos_ = 1;\n>> -\t\tb_pos_ = 2;\n>> -\t\tbpp_ = 3;\n>> +\t\tparams_.rgb.r_pos = 0;\n>> +\t\tparams_.rgb.g_pos = 1;\n>> +\t\tparams_.rgb.b_pos = 2;\n>> +\t\tparams_.rgb.bpp = 3;\n>> \t\tbreak;\n>> \tcase libcamera::formats::ARGB8888:\n>> \t\tformatFamily_ = RGB;\n>> -\t\tr_pos_ = 2;\n>> -\t\tg_pos_ = 1;\n>> -\t\tb_pos_ = 0;\n>> -\t\tbpp_ = 4;\n>> +\t\tparams_.rgb.r_pos = 2;\n>> +\t\tparams_.rgb.g_pos = 1;\n>> +\t\tparams_.rgb.b_pos = 0;\n>> +\t\tparams_.rgb.bpp = 4;\n>> \t\tbreak;\n>> \tcase libcamera::formats::RGBA8888:\n>> \t\tformatFamily_ = RGB;\n>> -\t\tr_pos_ = 3;\n>> -\t\tg_pos_ = 2;\n>> -\t\tb_pos_ = 1;\n>> -\t\tbpp_ = 4;\n>> +\t\tparams_.rgb.r_pos = 3;\n>> +\t\tparams_.rgb.g_pos = 2;\n>> +\t\tparams_.rgb.b_pos = 1;\n>> +\t\tparams_.rgb.bpp = 4;\n>> \t\tbreak;\n>> \tcase libcamera::formats::ABGR8888:\n>> \t\tformatFamily_ = RGB;\n>> -\t\tr_pos_ = 0;\n>> -\t\tg_pos_ = 1;\n>> -\t\tb_pos_ = 2;\n>> -\t\tbpp_ = 4;\n>> +\t\tparams_.rgb.r_pos = 0;\n>> +\t\tparams_.rgb.g_pos = 1;\n>> +\t\tparams_.rgb.b_pos = 2;\n>> +\t\tparams_.rgb.bpp = 4;\n>> \t\tbreak;\n>> \tcase libcamera::formats::BGRA8888:\n>> \t\tformatFamily_ = RGB;\n>> -\t\tr_pos_ = 1;\n>> -\t\tg_pos_ = 2;\n>> -\t\tb_pos_ = 3;\n>> -\t\tbpp_ = 4;\n>> +\t\tparams_.rgb.r_pos = 1;\n>> +\t\tparams_.rgb.g_pos = 2;\n>> +\t\tparams_.rgb.b_pos = 3;\n>> +\t\tparams_.rgb.bpp = 4;\n>> \t\tbreak;\n>> \n>> \tcase libcamera::formats::VYUY:\n>> \t\tformatFamily_ = YUV;\n>> -\t\ty_pos_ = 1;\n>> -\t\tcb_pos_ = 2;\n>> +\t\tparams_.yuv.y_pos = 1;\n>> +\t\tparams_.yuv.cb_pos = 2;\n>> \t\tbreak;\n>> \tcase libcamera::formats::YVYU:\n>> \t\tformatFamily_ = YUV;\n>> -\t\ty_pos_ = 0;\n>> -\t\tcb_pos_ = 3;\n>> +\t\tparams_.yuv.y_pos = 0;\n>> +\t\tparams_.yuv.cb_pos = 3;\n>> \t\tbreak;\n>> \tcase libcamera::formats::UYVY:\n>> \t\tformatFamily_ = YUV;\n>> -\t\ty_pos_ = 1;\n>> -\t\tcb_pos_ = 0;\n>> +\t\tparams_.yuv.y_pos = 1;\n>> +\t\tparams_.yuv.cb_pos = 0;\n>> \t\tbreak;\n>> \tcase libcamera::formats::YUYV:\n>> \t\tformatFamily_ = YUV;\n>> -\t\ty_pos_ = 0;\n>> -\t\tcb_pos_ = 1;\n>> +\t\tparams_.yuv.y_pos = 0;\n>> +\t\tparams_.yuv.cb_pos = 1;\n>> \t\tbreak;\n>> \n>> \tcase libcamera::formats::MJPEG:\n>> @@ -178,18 +178,20 @@ static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)\n>> \n>> void FormatConverter::convertNV(const unsigned char *src, unsigned char *dst)\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 c_stride = width_ * (2 / params_.nv.horzSubSample);\n>> +\tunsigned int c_inc = params_.nv.horzSubSample == 1 ? 2 : 0;\n>> +\tunsigned int cb_pos = params_.nv.nvSwap ? 1 : 0;\n>> +\tunsigned int cr_pos = params_.nv.nvSwap ? 0 : 1;\n>> \tconst unsigned char *src_c = src + width_ * height_;\n>> \tint r, g, b;\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\tconst unsigned char *src_cb = src_c +\n>> +\t\t\t\t\t (y / params_.nv.vertSubSample) *\n>> \t\t\t\t\t c_stride + cb_pos;\n>> -\t\tconst unsigned char *src_cr = src_c + (y / vertSubSample_) *\n>> +\t\tconst unsigned char *src_cr = src_c +\n>> +\t\t\t\t\t (y / params_.nv.vertSubSample) *\n>> \t\t\t\t\t c_stride + cr_pos;\n>> \n>> \t\tfor (unsigned int x = 0; x < width_; x += 2) {\n>> @@ -223,9 +225,9 @@ void FormatConverter::convertRGB(const unsigned char *src, unsigned char *dst)\n>> \n>> \tfor (y = 0; y < height_; y++) {\n>> \t\tfor (x = 0; x < width_; x++) {\n>> -\t\t\tr = src[bpp_ * x + r_pos_];\n>> -\t\t\tg = src[bpp_ * x + g_pos_];\n>> -\t\t\tb = src[bpp_ * x + b_pos_];\n>> +\t\t\tr = src[params_.rgb.bpp * x + params_.rgb.r_pos];\n>> +\t\t\tg = src[params_.rgb.bpp * x + params_.rgb.g_pos];\n>> +\t\t\tb = src[params_.rgb.bpp * x + params_.rgb.b_pos];\n>> \n>> \t\t\tdst[4 * x + 0] = b;\n>> \t\t\tdst[4 * x + 1] = g;\n>> @@ -233,7 +235,7 @@ void FormatConverter::convertRGB(const unsigned char *src, unsigned char *dst)\n>> \t\t\tdst[4 * x + 3] = 0xff;\n>> \t\t}\n>> \n>> -\t\tsrc += width_ * bpp_;\n>> +\t\tsrc += width_ * params_.rgb.bpp;\n>> \t\tdst += width_ * 4;\n>> \t}\n>> }\n>> @@ -246,16 +248,18 @@ void FormatConverter::convertYUV(const unsigned char *src, unsigned char *dst)\n>> \tunsigned int cr_pos;\n>> \tint r, g, b, y, cr, cb;\n>> \n>> -\tcr_pos = (cb_pos_ + 2) % 4;\n>> +\tcr_pos = (params_.yuv.cb_pos + 2) % 4;\n>> \tsrc_stride = width_ * 2;\n>> \tdst_stride = width_ * 4;\n>> \n>> \tfor (src_y = 0, dst_y = 0; dst_y < height_; src_y++, dst_y++) {\n>> \t\tfor (src_x = 0, dst_x = 0; dst_x < width_; ) {\n>> -\t\t\tcb = src[src_y * src_stride + src_x * 4 + cb_pos_];\n>> +\t\t\tcb = src[src_y * src_stride + src_x * 4 +\n>> +\t\t\t\t params_.yuv.cb_pos];\n>> \t\t\tcr = src[src_y * src_stride + src_x * 4 + cr_pos];\n>> \n>> -\t\t\ty = src[src_y * src_stride + src_x * 4 + y_pos_];\n>> +\t\t\ty = src[src_y * src_stride + src_x * 4 +\n>> +\t\t\t\tparams_.yuv.y_pos];\n>> \t\t\tyuv_to_rgb(y, cb, cr, &r, &g, &b);\n>> \t\t\tdst[dst_y * dst_stride + 4 * dst_x + 0] = b;\n>> \t\t\tdst[dst_y * dst_stride + 4 * dst_x + 1] = g;\n>> @@ -263,7 +267,8 @@ void FormatConverter::convertYUV(const unsigned char *src, unsigned char *dst)\n>> \t\t\tdst[dst_y * dst_stride + 4 * dst_x + 3] = 0xff;\n>> \t\t\tdst_x++;\n>> \n>> -\t\t\ty = src[src_y * src_stride + src_x * 4 + y_pos_ + 2];\n>> +\t\t\ty = src[src_y * src_stride + src_x * 4 +\n>> +\t\t\t\tparams_.yuv.y_pos + 2];\n>> \t\t\tyuv_to_rgb(y, cb, cr, &r, &g, &b);\n>> \t\t\tdst[dst_y * dst_stride + 4 * dst_x + 0] = b;\n>> \t\t\tdst[dst_y * dst_stride + 4 * dst_x + 1] = g;\n>> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h\n>> index e389b24..c53fa28 100644\n>> --- a/src/qcam/format_converter.h\n>> +++ b/src/qcam/format_converter.h\n>> @@ -40,20 +40,29 @@ 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_;\n>> -\tunsigned int g_pos_;\n>> -\tunsigned int b_pos_;\n>> -\n>> -\t/* YUV parameters */\n>> -\tunsigned int y_pos_;\n>> -\tunsigned int cb_pos_;\n>> +\tunion {\n>> +\t\t/* NV parameters */\n>> +\t\tstruct nv_params {\n>> +\t\t\tunsigned int horzSubSample;\n>> +\t\t\tunsigned int vertSubSample;\n>> +\t\t\tbool nvSwap;\n>> +\t\t} nv;\n>> +\n>> +\t\t/* RGB parameters */\n>> +\t\tstruct rgb_params {\n>> +\t\t\tunsigned int bpp;\n>> +\t\t\tunsigned int r_pos;\n>> +\t\t\tunsigned int g_pos;\n>> +\t\t\tunsigned int b_pos;\n>> +\t\t} rgb;\n>> +\n>> +\t\t/* YUV parameters */\n>> +\t\tstruct yuv_params {\n>> +\t\t\tunsigned int y_pos;\n>> +\t\t\tunsigned int cb_pos;\n>> +\t\t} yuv;\n> \n> The NV* formats are YUV too, we should try to find a better name. It's a\n> pre-existing issue though.\n> \n>> +\t} params_;\n> \n> I wonder if it wouldn't make more sense to move all of these to a\n> separate structure, stored in a map indexed by the pixel format. That\n> would greatly simplify FormatConverter::configure(). We actually have\n> such a structure already, PixelFormatInfo, but it's an internal API.\n> Exposing it as a public API runs into the issue that not all the fields\n> it contains should be exposed. The V4L2 4CC, for instance, should stay\n> internal. There's probably a way to handle that, but I'm also no really\n> keen at this point to committing to a stable API for PixelFormatInfo. I\n> thus think it would be better to duplicate the data we need here.\n\nYes, this makes sense, and will probably take some more patch review\niterations.\n\n> To avoid blocking Bayer support in qcam on this, how about moving\n> forward with patch 2/2 for now without 1/2 ? The small space increase is\n> negligible.\n\nThat's fine for me. Let's go this way.\n\nIn this case should I still use:\n\n\t/* RAW Bayer parameters */\n\tstruct bayer_params {\n\t\t/*\n\t\t * Bytes per pixel is a fractional number, and is\n\t\t * represented by integer numerator and denominator.\n\t\t */\n\t\tunsigned int bpp_numer;\n\t\tunsigned int bpp_denom;\n\t\tunsigned int r_pos;\n\t\t/*\n\t\t * The fields below are used to hold the values computed\n\t\t * in configure() based on r_pos and width_.\n\t\t */\n\t\tunsigned int g1_pos;\n\t\tunsigned int g2_pos;\n\t\tunsigned int b_pos;\n\t\tunsigned int srcLineLength;\n\t} bayer_;\n\n- as the member of FormatConverter (all Bayer parameters grouped together into one member),\nor should I follow the current pattern and make each of the above parameters a separate member of\nFormatConverter (and invent some other name for r_pos_ and b_pos_ not to clash with the existing\nRGB parameters)?\n\nThanks,\nAndrey\n\n>> +\n> \n> Extra blank line.\n> \n>> };\n>> \n>> #endif /* __QCAM_FORMAT_CONVERTER_H__ */\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 3C62FC3B5B\n\tfor ;\n\tTue, 22 Sep 2020 20:03:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 94AD062FD8;\n\tTue, 22 Sep 2020 22:03:52 +0200 (CEST)","from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 223636036A\n\tfor ;\n\tTue, 22 Sep 2020 22:03:51 +0200 (CEST)","by mail-lf1-x142.google.com with SMTP id 77so12105496lfj.0\n\tfor ;\n\tTue, 22 Sep 2020 13:03:51 -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\ts6sm4150382ljm.8.2020.09.22.13.03.49\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tTue, 22 Sep 2020 13:03:49 -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=\"DNwb/mX1\"; 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=EeqYSjdKQVKfcw533LPpd8oDSk0dsUO0z51PYe5501U=;\n\tb=DNwb/mX1sWMlB4y6xEmbVe9lBKGVSOBuTyFda4qeil6EPt82Hcdllz1LTBk5b/dE6j\n\truTLvASQ1KfIHhu7gKrLU+9g+Muob9SYmK6WoNOcfMdz9H+i0VH66DEAnXVXHS0yDiY3\n\twdwP8e+96Atk94tWpEEn7G7LY6PjJ29fTok//uF/50txF6lY9JQi383iywzmIN++ga4B\n\tQxaSY8PdsiXYU7jDWyISU2RrF0hF5B+UEBNsOZY77s+AmDiesgGpebJvVvd1nKXe30qS\n\t4lX3XeTsAesLbmWuq299G30aGyjCYxmZlaDQ5JfIwHTA75GrXr4LAVuSGgLtZfApJixo\n\tbq4A==","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=EeqYSjdKQVKfcw533LPpd8oDSk0dsUO0z51PYe5501U=;\n\tb=NSVRBDFtC/SSZqAa9EOeKoH1zi1lMSEFrjvA2GFxAJT8Drj19X9eSm/qx6XxULHArB\n\todrZqdG06fxGEjgIIy0BvVTFwX53uCkJqpfdRXprjeBvNXc+/+Lcz1G+CCtc2rL06k+c\n\t8qplddMpFY3cbgp3WM7+oNjiQUPKqNiDCTVcqsEPPaK8blUHuFezlBXebhIsKrqgxzOO\n\toFFS848nh+1kN/J+IFRwlhdZesQv1XiuJukdLGFdWd23WgXbdUrx+Neu3lf9p+DhhgN6\n\to74/6xPVfDxEzR3KIE5u74Vvc+GecSKB947ifx4B4BmIxubAOxjeTXGI8HQe4RmNtNto\n\teaVQ==","X-Gm-Message-State":"AOAM530ZudOK+ywqUMruogF4F/KJKFq3wQPyr/OiTzfRcOlidQLdzrRr\n\toOVI+dX/bZ0YlE/2/lMsYnnZwg==","X-Google-Smtp-Source":"ABdhPJwHlbht3y4l0DZGik6A8YZLjmnCdmTWIPnvA0iyp/VDtNW31pBxq5pTW6QuqFAqZOWZCTSUOQ==","X-Received":"by 2002:a19:4085:: with SMTP id\n\tn127mr2075014lfa.425.1600805030166; \n\tTue, 22 Sep 2020 13:03:50 -0700 (PDT)","To":"Laurent Pinchart ","References":"<20200908150739.1552-1-andrey.konovalov@linaro.org>\n\t<20200908150739.1552-2-andrey.konovalov@linaro.org>\n\t<20200912004419.GH6808@pendragon.ideasonboard.com>","From":"Andrey Konovalov ","Message-ID":"","Date":"Tue, 22 Sep 2020 23:03:48 +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":"<20200912004419.GH6808@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/2] qcam: format_converter:\n\tput parameters of different formats into union","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\" "}}]