[{"id":29105,"web_url":"https://patchwork.libcamera.org/comment/29105/","msgid":"<20240327183617.GL4721@pendragon.ideasonboard.com>","date":"2024-03-27T18:36:17","subject":"Re: [PATCH v6 15/18] libcamera: debayer_cpu: Add BGR888 output\n\tsupport","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan and Hans,\n\nThank you for the patch.\n\nOn Tue, Mar 19, 2024 at 01:36:02PM +0100, Milan Zamazal wrote:\n> From: Hans de Goede \n> \n> BGR888 is RGB888 with the red and blue pixels swapped, adjust\n> the debayering to swap the red and blue pixels in the bayer pattern\n> to add support for writing formats::BGR888.\n> \n> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek \n> Reviewed-by: Milan Zamazal \n> Signed-off-by: Hans de Goede \n> Reviewed-by: Stefan Klug \n> ---\n> src/libcamera/software_isp/debayer_cpu.cpp | 42 +++++++++++++++++++---\n> src/libcamera/software_isp/debayer_cpu.h | 1 +\n> 2 files changed, 38 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index eb1c2718..a1692693 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -268,7 +268,7 @@ int DebayerCpu::getInputConfig(PixelFormat inputFormat, DebayerInputConfig &conf\n> \t\tconfig.bpp = (bayerFormat.bitDepth + 7) & ~7;\n> \t\tconfig.patternSize.width = 2;\n> \t\tconfig.patternSize.height = 2;\n> -\t\tconfig.outputFormats = std::vector({ formats::RGB888 });\n> +\t\tconfig.outputFormats = std::vector({ formats::RGB888, formats::BGR888 });\n> \t\treturn 0;\n> \t}\n> \n> @@ -278,7 +278,7 @@ int DebayerCpu::getInputConfig(PixelFormat inputFormat, DebayerInputConfig &conf\n> \t\tconfig.bpp = 10;\n> \t\tconfig.patternSize.width = 4; /* 5 bytes per *4* pixels */\n> \t\tconfig.patternSize.height = 2;\n> -\t\tconfig.outputFormats = std::vector({ formats::RGB888 });\n> +\t\tconfig.outputFormats = std::vector({ formats::RGB888, formats::BGR888 });\n> \t\treturn 0;\n> \t}\n> \n> @@ -289,7 +289,7 @@ int DebayerCpu::getInputConfig(PixelFormat inputFormat, DebayerInputConfig &conf\n> \n> int DebayerCpu::getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config)\n> {\n> -\tif (outputFormat == formats::RGB888) {\n> +\tif (outputFormat == formats::RGB888 || outputFormat == formats::BGR888) {\n> \t\tconfig.bpp = 24;\n> \t\treturn 0;\n> \t}\n> @@ -325,13 +325,41 @@ int DebayerCpu::setupStandardBayerOrder(BayerFormat::Order order)\n> \treturn 0;\n> }\n> \n> -/* TODO: this ignores outputFormat since there is only 1 supported outputFormat for now */\n> -int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, [[maybe_unused]] PixelFormat outputFormat)\n> +int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat)\n> {\n> \tBayerFormat bayerFormat =\n> \t\tBayerFormat::fromPixelFormat(inputFormat);\n> \n> \txShift_ = 0;\n> +\tswapRedBlueGains_ = false;\n> +\n> +\tswitch (outputFormat) {\n> +\tcase formats::RGB888:\n> +\t\tbreak;\n> +\tcase formats::BGR888:\n> +\t\t/* Swap R and B in bayer order to generate BGR888 instead of RGB888 */\n> +\t\tswapRedBlueGains_ = true;\n> +\n> +\t\tswitch (bayerFormat.order) {\n> +\t\tcase BayerFormat::BGGR:\n> +\t\t\tbayerFormat.order = BayerFormat::RGGB;\n> +\t\t\tbreak;\n> +\t\tcase BayerFormat::GBRG:\n> +\t\t\tbayerFormat.order = BayerFormat::GRBG;\n> +\t\t\tbreak;\n> +\t\tcase BayerFormat::GRBG:\n> +\t\t\tbayerFormat.order = BayerFormat::GBRG;\n> +\t\t\tbreak;\n> +\t\tcase BayerFormat::RGGB:\n> +\t\t\tbayerFormat.order = BayerFormat::BGGR;\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tgoto invalid_fmt;\n> +\t\t}\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tgoto invalid_fmt;\n\nNo goto please. You can refactor the function to avoid that. One way\nwould be to move the input and output format validation to the caller,\nas the name setDebayerFunctions() doesn't really sound like it should be\nresponsible for validating the formats.\n\n> +\t}\n> \n> \tif ((bayerFormat.bitDepth == 8 || bayerFormat.bitDepth == 10 || bayerFormat.bitDepth == 12) &&\n> \t bayerFormat.packing == BayerFormat::Packing::None &&\n> @@ -378,6 +406,7 @@ int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, [[maybe_unused]] Pi\n> \t\t}\n> \t}\n> \n> +invalid_fmt:\n> \tLOG(Debayer, Error) << \"Unsupported input output format combination\";\n> \treturn -EINVAL;\n> }\n> @@ -661,6 +690,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams\n> \t\tgamma_correction_ = params.gamma;\n> \t}\n> \n> +\tif (swapRedBlueGains_)\n> +\t\tstd::swap(params.gainR, params.gainB);\n> +\n> \tfor (unsigned int i = 0; i < kRGBLookupSize; i++) {\n> \t\tconstexpr unsigned int div =\n> \t\t\tkRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize;\n> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> index fd1fa180..5f44fc65 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -145,6 +145,7 @@ private:\n> \tunsigned int lineBufferIndex_;\n> \tunsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n> \tbool enableInputMemcpy_;\n> +\tbool swapRedBlueGains_;\n> \tfloat gamma_correction_;\n> \tunsigned int measuredFrames_;\n> \tint64_t frameProcessTime_;","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 E111CBD160\n\tfor ;\n\tWed, 27 Mar 2024 18:36:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C47AA63331;\n\tWed, 27 Mar 2024 19:36:27 +0100 (CET)","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 C4C4F61C35\n\tfor ;\n\tWed, 27 Mar 2024 19:36:26 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AD307899;\n\tWed, 27 Mar 2024 19:35:53 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SGvhqvdM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711564553;\n\tbh=/ab9VOBjg+KvX/ojGMFD6iP0jom2vAv/dAsvMO2m3Kg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SGvhqvdMlAE82ZbZURogkMHczeZYlnxYfU4Ra6c4WsoMqFpOJWSdNDIWIX3/drcnN\n\tFz4rK+ICz8HsqyrIElI0huWjZWxA7uVkpSvNo18P53LGiYk5a1pu5Hwd7SnVfcGn0J\n\tjpaYWtyV8Donr5IX+/SuscGk1+lOFDVhOHAazLp0=","Date":"Wed, 27 Mar 2024 20:36:17 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, Hans de Goede ,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tKieran Bingham ,\n\tStefan Klug ","Subject":"Re: [PATCH v6 15/18] libcamera: debayer_cpu: Add BGR888 output\n\tsupport","Message-ID":"<20240327183617.GL4721@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-16-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240319123622.675599-16-mzamazal@redhat.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]