[{"id":12057,"web_url":"https://patchwork.libcamera.org/comment/12057/","msgid":"<20200820081135.ybgcvwtbp2s72cgb@uno.localdomain>","date":"2020-08-20T08:11:35","subject":"Re: [libcamera-devel] [PATCH 02/13] libcamera: pipeline: rkisp1:\n\tBreakout mainpath size and format constraints","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Aug 13, 2020 at 02:52:35AM +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> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 32 +++++++++++++-----------\n> 1 file changed, 17 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index e2b703fc09f7afda..a0e36a44d8d91260 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -37,6 +37,19 @@ namespace libcamera {\n>\n> LOG_DEFINE_CATEGORY(RkISP1)\n>\n> +static const Size RKISP1_RSZ_MP_SRC_MIN = { 32, 16 };\n> +static const Size RKISP1_RSZ_MP_SRC_MAX = { 4416, 3312 };\n\nYou could list-initialize these\n\n> +static const std::array RKISP1_RSZ_MP_FORMATS{\n\nAll of these can be made constexprs\nAlso, we usually prefer an anonymous namespace in place of static\n\nAlso, are you sure about the ALL_CAPITAL_NAME of the array of formats ?\n\n> +\tformats::YUYV,\n> +\tformats::YVYU,\n> +\tformats::VYUY,\n> +\tformats::NV16,\n> +\tformats::NV61,\n> +\tformats::NV21,\n> +\tformats::NV12,\n> +\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> +};\n> +\n> class PipelineHandlerRkISP1;\n> class RkISP1ActionQueueBuffers;\n>\n> @@ -461,17 +474,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 +489,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 +527,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\nYou could concatenate the two\n cfg.size.boundTo().expandTo();\n\nNits apart, the patch itself looks good!\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> \tif (cfg.size != size) {\n> \t\tLOG(RkISP1, Debug)\n> --\n> 2.28.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 346FDBE173\n\tfor ;\n\tThu, 20 Aug 2020 08:07:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5D6E61F61;\n\tThu, 20 Aug 2020 10:07:54 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 686BF61ED9\n\tfor ;\n\tThu, 20 Aug 2020 10:07:53 +0200 (CEST)","from uno.localdomain (host-82-52-18-94.retail.telecomitalia.it\n\t[82.52.18.94]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 8F898200006;\n\tThu, 20 Aug 2020 08:07:52 +0000 (UTC)"],"Date":"Thu, 20 Aug 2020 10:11:35 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200820081135.ybgcvwtbp2s72cgb@uno.localdomain>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200813005246.3265807-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 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\" "}},{"id":12069,"web_url":"https://patchwork.libcamera.org/comment/12069/","msgid":"","date":"2020-08-20T12:14:50","subject":"Re: [libcamera-devel] [PATCH 02/13] libcamera: pipeline: rkisp1:\n\tBreakout mainpath size and format constraints","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 20/08/2020 09:11, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Thu, Aug 13, 2020 at 02:52:35AM +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>> ---\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 32 +++++++++++++-----------\n>> 1 file changed, 17 insertions(+), 15 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index e2b703fc09f7afda..a0e36a44d8d91260 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -37,6 +37,19 @@ namespace libcamera {\n>>\n>> LOG_DEFINE_CATEGORY(RkISP1)\n>>\n>> +static const Size RKISP1_RSZ_MP_SRC_MIN = { 32, 16 };\n>> +static const Size RKISP1_RSZ_MP_SRC_MAX = { 4416, 3312 };\n> \n> You could list-initialize these\n> \n>> +static const std::array RKISP1_RSZ_MP_FORMATS{\n> \n> All of these can be made constexprs\n> Also, we usually prefer an anonymous namespace in place of static\n> \n> Also, are you sure about the ALL_CAPITAL_NAME of the array of formats ?\n> \n>> +\tformats::YUYV,\n>> +\tformats::YVYU,\n>> +\tformats::VYUY,\n>> +\tformats::NV16,\n>> +\tformats::NV61,\n>> +\tformats::NV21,\n>> +\tformats::NV12,\n>> +\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n>> +};\n>> +\n>> class PipelineHandlerRkISP1;\n>> class RkISP1ActionQueueBuffers;\n>>\n>> @@ -461,17 +474,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 +489,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 +527,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> You could concatenate the two\n> cfg.size.boundTo().expandTo();\n> \n\nThat looks neat, but does it also indicate we need a\n Size::clamp(min, max) ?\n\nOr would that not make sense given the multi-dimensional properties of a\nSize...\n\n\n> Nits apart, the patch itself looks good!\n> Reviewed-by: Jacopo Mondi \n\nWith fixups above:\n\nReviewed-by: Kieran Bingham \n\n> \n> Thanks\n> j\n> \n>>\n>> \tif (cfg.size != size) {\n>> \t\tLOG(RkISP1, Debug)\n>> --\n>> 2.28.0\n>>\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 12C97BE173\n\tfor ;\n\tThu, 20 Aug 2020 12:14:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D561361FBB;\n\tThu, 20 Aug 2020 14:14: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 476ED6038C\n\tfor ;\n\tThu, 20 Aug 2020 14:14:55 +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 ECD6723D;\n\tThu, 20 Aug 2020 14:14: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=\"W4ne6qcA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1597925694;\n\tbh=UQyrb/MoWRfpixNqyoDRxeh2193ybwWD9vO61urqlFw=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=W4ne6qcACWV4R5jr3crciSUbC78VlryiJ5ffWbDBjR1cJppZYTDVtO/NNOD0v0D2+\n\tsX27Z/tJurusQ9LTVtx2gW/qrAGp4QFOGYWCTsNlQSn6BzaW/5Y5CymXcm+QmnmPU1\n\tDY3pkqejlQF635ginxaydZOW90OYJhQbdoS5MJkw=","To":"Jacopo Mondi , =?utf-8?q?Niklas_S=C3=B6derlund?=\n\t","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-3-niklas.soderlund@ragnatech.se>\n\t<20200820081135.ybgcvwtbp2s72cgb@uno.localdomain>","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":"Thu, 20 Aug 2020 13:14: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":"<20200820081135.ybgcvwtbp2s72cgb@uno.localdomain>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 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","Reply-To":"kieran.bingham@ideasonboard.com","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\" "}},{"id":12482,"web_url":"https://patchwork.libcamera.org/comment/12482/","msgid":"<20200913124723.GJ695456@oden.dyn.berto.se>","date":"2020-09-13T12:47:23","subject":"Re: [libcamera-devel] [PATCH 02/13] libcamera: pipeline: rkisp1:\n\tBreakout mainpath size and format constraints","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your feedback.\n\nOn 2020-08-20 10:11:35 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Thu, Aug 13, 2020 at 02:52:35AM +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> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 32 +++++++++++++-----------\n> > 1 file changed, 17 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index e2b703fc09f7afda..a0e36a44d8d91260 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -37,6 +37,19 @@ namespace libcamera {\n> >\n> > LOG_DEFINE_CATEGORY(RkISP1)\n> >\n> > +static const Size RKISP1_RSZ_MP_SRC_MIN = { 32, 16 };\n> > +static const Size RKISP1_RSZ_MP_SRC_MAX = { 4416, 3312 };\n> \n> You could list-initialize these\n> \n> > +static const std::array RKISP1_RSZ_MP_FORMATS{\n> \n> All of these can be made constexprs\n> Also, we usually prefer an anonymous namespace in place of static\n\nI like both suggestions above.\n\n> \n> Also, are you sure about the ALL_CAPITAL_NAME of the array of formats ?\n\nI have picked the names directly from the kernel driver sources to make \nit easier to map between the two. I'm open to pick something else but I \nstill find value in keeping a clear mapping between the two so I think I \nwill keep it as is for now.\n\n> \n> > +\tformats::YUYV,\n> > +\tformats::YVYU,\n> > +\tformats::VYUY,\n> > +\tformats::NV16,\n> > +\tformats::NV61,\n> > +\tformats::NV21,\n> > +\tformats::NV12,\n> > +\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> > +};\n> > +\n> > class PipelineHandlerRkISP1;\n> > class RkISP1ActionQueueBuffers;\n> >\n> > @@ -461,17 +474,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 +489,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 +527,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> You could concatenate the two\n> cfg.size.boundTo().expandTo();\n\nI could but I find the above more readable ;-)\n\n> \n> Nits apart, the patch itself looks good!\n> Reviewed-by: Jacopo Mondi \n> \n> Thanks\n> j\n> \n> >\n> > \tif (cfg.size != size) {\n> > \t\tLOG(RkISP1, Debug)\n> > --\n> > 2.28.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 254D6C3B5B\n\tfor ;\n\tSun, 13 Sep 2020 12:47:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E68A62D99;\n\tSun, 13 Sep 2020 14:47:26 +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 6181562901\n\tfor ;\n\tSun, 13 Sep 2020 14:47:25 +0200 (CEST)","by mail-lf1-x142.google.com with SMTP id y2so10500110lfy.10\n\tfor ;\n\tSun, 13 Sep 2020 05:47:25 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\to18sm2275101lfl.282.2020.09.13.05.47.23\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 13 Sep 2020 05:47:24 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"arSAdbW6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=nI8dL3tHNF4A9tnWr0j2jHgyBYoJKdJxtePUeL1aUOo=;\n\tb=arSAdbW60Kp23u5jJw4xqvNBmh/qMqK7OwlcsGL7uL3Zbo2zSLKTHr7HGtQR/7RKaz\n\tfJsplXqpNa9XSAWyvOSx0d9Gg/QL/+cFst/G1LIFDhNTnGBNApNjUinJyUmOOtA50jjF\n\tbDWbA1s/XaNy+vvxDuE3D5j+h7Ap7XfeVBB7X1/OKmV9atUBOxvhSYEVKq0RmHRzZu7D\n\t/Y1Y/P+v0L8mvj6qkoVRm7N15xCI3A+p/62tf9voRus3lDE8sRB4CJrf9WTZpe2w0QpL\n\tS0KBYpzqfpO9eooruy99mspve0Xknc+C6enG+st+JwHxXoYzDTFtC4ktg4KFbxoDdYSV\n\tmSIA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=nI8dL3tHNF4A9tnWr0j2jHgyBYoJKdJxtePUeL1aUOo=;\n\tb=c+BNq/4B4uAwsf25B4idFespE2VahXSJsdimP/np11DdZQm3GpwlALu35jUOdFbk8J\n\tpjcRYNbWN5TJbwXhS3453biYeNHekHm7FrPf9tVmKADTikPalBtZeFXHYIUz2UMQPlxv\n\tnhc5IpW6T/opYwb8ALXrY2ZnMvysiHLiKNDF4RKm5LG13OSdldE7pZ/G3AhrFvSwRskO\n\teXCckOWYPgYzP72tPEP+ON/ocOHVT4eJQBrAsPeJ0BeZ24Lb+BW8051l+8pGrr/t0+Iu\n\tj7WenX9HFOX1GS3vfeS5txp5/Hw2AU25Oc635IUPJjpbAVatTM6p32H0hkX+kb7xBz6+\n\td7aQ==","X-Gm-Message-State":"AOAM533YRrcWrMu2kfL7xsM1lveN1YMr5RnrDP5gS60z9Wzi9NOVYPey\n\tEEuQy/gzLQJ8dmnB5CquuEGs87cEsIBxjA==","X-Google-Smtp-Source":"ABdhPJzKOqA5Ubj0FZcTxabpPeB0utbb2hbDrB44sa0wKoNq6lih2RAJKJEabuQRxssYZG0lIlSdTw==","X-Received":"by 2002:a19:dd5:: with SMTP id 204mr2767827lfn.551.1600001244751;\n\tSun, 13 Sep 2020 05:47:24 -0700 (PDT)","Date":"Sun, 13 Sep 2020 14:47:23 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200913124723.GJ695456@oden.dyn.berto.se>","References":"<20200813005246.3265807-1-niklas.soderlund@ragnatech.se>\n\t<20200813005246.3265807-3-niklas.soderlund@ragnatech.se>\n\t<20200820081135.ybgcvwtbp2s72cgb@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200820081135.ybgcvwtbp2s72cgb@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]