[{"id":12506,"web_url":"https://patchwork.libcamera.org/comment/12506/","msgid":"<20200915003525.GQ15543@pendragon.ideasonboard.com>","date":"2020-09-15T00:35:25","subject":"Re: [libcamera-devel] [PATCH v2 02/13] libcamera: pipeline: rkisp1:\n\tBreakout mainpath size and format constraints","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Mon, Sep 14, 2020 at 04:21:38PM +0200, Niklas Söderlund wrote:\n> Breakout the mainpath size and format constrains as it will be used in\n> more places then just validate(). While at it use the new helpers to\n> validate Size.\n> \n> Signed-off-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> Reviewed-by: Kieran Bingham \n> ---\n> * Changes since v1\n> - Use list initializers\n> - Use constexpr instead of static const\n> - Define constants in anonymoys namespace\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 34 +++++++++++++-----------\n> 1 file changed, 19 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 009d190d3ec828f0..86a3e3c63b17762b 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -37,6 +37,21 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(RkISP1)\n> \n> +namespace {\n> +\tconstexpr Size RKISP1_RSZ_MP_SRC_MIN { 32, 16 };\n> +\tconstexpr Size RKISP1_RSZ_MP_SRC_MAX { 4416, 3312 };\n> +\tconstexpr std::array RKISP1_RSZ_MP_FORMATS{\n\nEither add a space before { here, or remove it on the two lines above.\nWe'll have to tidy up the coding style for list initialization, it's\ninconsistent at the moment.\n\nWe will also need to tidy up the coding style for naming of constants,\nthe existing code base uses a mix of camelCase and SNAKE_CASE. While\nmacros should be SNAKE_CASE, constexpr constants don't need to be. I'm\nleaning for camelCase, possibly with a way to express that they're\nconstants (adding a 'k' prefix is often used for that purpose, even if\nI'm not necessarily fond of it). The debate is open :-)\n\n> +\t\tformats::YUYV,\n> +\t\tformats::YVYU,\n> +\t\tformats::VYUY,\n> +\t\tformats::NV16,\n> +\t\tformats::NV61,\n> +\t\tformats::NV21,\n> +\t\tformats::NV12,\n> +\t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n\nOn a side note, that's formats::R8 :-)\n\n> +\t};\n\nNo need for the extra level of indentation.\n\n> +}\n\n} /* namespace */\n\nReviewed-by: Laurent Pinchart \n\n> +\n> class PipelineHandlerRkISP1;\n> class RkISP1ActionQueueBuffers;\n> \n> @@ -461,17 +476,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> \n> CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> {\n> -\tstatic const std::array formats{\n> -\t\tformats::YUYV,\n> -\t\tformats::YVYU,\n> -\t\tformats::VYUY,\n> -\t\tformats::NV16,\n> -\t\tformats::NV61,\n> -\t\tformats::NV21,\n> -\t\tformats::NV12,\n> -\t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> -\t};\n> -\n> \tconst CameraSensor *sensor = data_->sensor_;\n> \tStatus status = Valid;\n> \n> @@ -487,8 +491,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \tStreamConfiguration &cfg = config_[0];\n> \n> \t/* Adjust the pixel format. */\n> -\tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==\n> -\t formats.end()) {\n> +\tif (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),\n> +\t\t cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {\n> \t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> \t\tcfg.pixelFormat = formats::NV12,\n> \t\tstatus = Adjusted;\n> @@ -525,8 +529,8 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t\t\t/ sensorFormat_.size.width;\n> \t}\n> \n> -\tcfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));\n> -\tcfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));\n> +\tcfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);\n> +\tcfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);\n> \n> \tif (cfg.size != size) {\n> \t\tLOG(RkISP1, Debug)","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 7C389C3B5B\n\tfor ;\n\tTue, 15 Sep 2020 00:35:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0BCFD62DAB;\n\tTue, 15 Sep 2020 02:35:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F30B62901\n\tfor ;\n\tTue, 15 Sep 2020 02:35:54 +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 8BBE6275;\n\tTue, 15 Sep 2020 02:35: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=\"El8VB13C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600130153;\n\tbh=4QLavA4yiZZ1SlOtsWuc7dMpPprqbHFocebP+LeWRoY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=El8VB13CpTrolu9eeJFEePopU7u4+mxV8yIvGoJlKIY4mER5X+E2w9mn8AFgCcBs0\n\tQSV4dAwVf3GAaoiCoWn+ghU0fNPZn9ZrBa2a5AE757KCfnApbMRPHsx4NCqf1/s1/M\n\tFQS0cLd8d9+0qF2sZ8ZL5oabsiD649CvUrUobRqo=","Date":"Tue, 15 Sep 2020 03:35:25 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200915003525.GQ15543@pendragon.ideasonboard.com>","References":"<20200914142149.63857-1-niklas.soderlund@ragnatech.se>\n\t<20200914142149.63857-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200914142149.63857-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v2 02/13] libcamera: pipeline: rkisp1:\n\tBreakout mainpath size and format constraints","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]