[{"id":22327,"web_url":"https://patchwork.libcamera.org/comment/22327/","msgid":"<20220321082158.jr2ye2cy46c3intl@uno.localdomain>","date":"2022-03-21T08:21:58","subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Christian,\n thanks for the patch.\n\nWould you mind re-sending the patch with git send-email instead of\nattaching it, otherwise it would be hard for us to review and reply.\n\nThanks\n j\n\nOn Thu, Mar 17, 2022 at 02:48:47PM +0000, Christian Rauch via libcamera-devel wrote:\n> Hello,\n>\n> The attached patch fixes the ControlInfo access of Span Control (see\n> https://bugs.libcamera.org/show_bug.cgi?id=101).\n>\n> The ControlInfo for Spans was defined in scalar values (i.e. single\n> values instead of arrays). Thus, while a Control was defined as Span,\n> its corresponding ControlInfo was defined as scalar, causing access errors.\n>\n> Best,\n> Christian","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 C7172BDE17\n\tfor ;\n\tMon, 21 Mar 2022 08:22:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 066B4604D4;\n\tMon, 21 Mar 2022 09:22:02 +0100 (CET)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::223])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E8232604C4\n\tfor ;\n\tMon, 21 Mar 2022 09:22:00 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id F2DEA60009;\n\tMon, 21 Mar 2022 08:21:59 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647850922;\n\tbh=6up69Y3T8/03MjSjhBdOUlfzzzgZac+MvzJnM545nXs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ONpOMdfJNgUwoFGsk2XZ4HsM2EN7K35eTPwdl5W2jGkUIeymorZocwN6xBkYKVXGW\n\tzbQYpmMnNRzlzjCbKwt7d0oSYDjaOIR+Zy+fsahu4lq3n/R20lZK5Mbuipg6h5O3sa\n\tfeg6zBKwAzuh/TaDd1wRhHhAQhlufpty/3o3cgtqzjLepOpT8ryaojlO23EsExdCJQ\n\tK3PHzLpPw+zj+PdvoCTwx3DMcGaSHfkuZTZyZtTMxoONEZttO4zTj4Fp6PyEeuJ06Q\n\tyUHM+IpQaUDqdQjZcXnnedoxbSBUcluOl8Cuthgg8EXYfMTacinVuec60+1E7EdaXQ\n\tFmBz9lDRsmkIg==","Date":"Mon, 21 Mar 2022 09:21:58 +0100","To":"Christian Rauch ","Message-ID":"<20220321082158.jr2ye2cy46c3intl@uno.localdomain>","References":"","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22343,"web_url":"https://patchwork.libcamera.org/comment/22343/","msgid":"","date":"2022-03-21T13:47:19","subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Christian,\n\nThank you for the patch, and sorry for the late reply.\n\nAs Jacopo mentioned, patches should be sent inline and not as\nattachments, but I can still review this version.\n\nOn Thu, Mar 17, 2022 at 02:48:47PM +0000, Christian Rauch via libcamera-devel wrote:\n> Hello,\n> \n> The attached patch fixes the ControlInfo access of Span Control (see\n> https://bugs.libcamera.org/show_bug.cgi?id=101).\n> \n> The ControlInfo for Spans was defined in scalar values (i.e. single\n> values instead of arrays). Thus, while a Control was defined as Span,\n> its corresponding ControlInfo was defined as scalar, causing access errors.\n> \n> Best,\n> Christian\n\n> From 13052bf04a0eccfc6a896bba47ae18c4ad8c6ebb Mon Sep 17 00:00:00 2001\n> From: Christian Rauch \n> Date: Thu, 17 Mar 2022 01:58:10 +0000\n> Subject: [PATCH 1/3] fix ControlInfo for Span Controls\n> \n> Some control properties are typed with a Span to store an array of values.\n> Currently those are ColourGains, SensorBlackLevels, ColourCorrectionMatrix\n> and FrameDurationLimits. The value range and defaults in the ControlInfo of\n> those Controls is currently defined as scalar. This prevents accessing the\n> ControlInfo via the native Span type.\n> \n> By defining the ControlInfo in terms of Spans, we can avoid this issue.\n> ---\n> include/libcamera/ipa/raspberrypi.h | 52 ++++++++++++-------\n> src/ipa/ipu3/ipu3.cpp | 6 +--\n> .../ipa_data_serializer_test.cpp | 8 +--\n> 3 files changed, 40 insertions(+), 26 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 7f705e49..fb5188a1 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -27,26 +27,38 @@ namespace RPi {\n> * and the pipeline handler may be reverted so that it aborts when an\n> * unsupported control is encountered.\n> */\n> -static const ControlInfoMap Controls({\n> -\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n> -\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> -\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> -\t\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> -\t\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> -\t\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> -\t\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> -\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> -\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> -\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> -\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> -\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> -\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> -\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> -\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> -\t\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> -\t\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> -\t\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> -\t}, controls::controls);\n> +static const ControlInfoMap Controls(\n> +\t{ { &controls::AeEnable, ControlInfo(false, true) },\n\nThe change in indentation makes the patch difficult to review, as it\nmexes indentiation changes with other changes. If you think the\nformatting is wrong, please send a patch to fix that first, without any\nfunctional change, and a second patch with the functional changes. Both\ncan be sent as part of the same series (git format-patch --cover-letter\nhelps creating a series with a cover letter).\n\n> +\t { &controls::ExposureTime, ControlInfo(0, 999999) },\n> +\t { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> +\t { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> +\t { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> +\t { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> +\t { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> +\t { &controls::AwbEnable, ControlInfo(false, true) },\n> +\t { &controls::ColourGains, ControlInfo{\n> +\t\t\t\t\t Span({ 0, 0 }),\n> +\t\t\t\t\t Span({ 32, 32 }),\n> +\t\t\t\t\t Span({ 0, 0 }),\n> +\t\t\t\t } },\n> +\t { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> +\t { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> +\t { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> +\t { &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> +\t { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> +\t { &controls::ColourCorrectionMatrix, ControlInfo{\n> +\t\t\t\t\t\t Span({ -16, -16, -16, -16, -16, -16, -16, -16, -16 }),\n> +\t\t\t\t\t\t Span({ 16, 16, 16, 16, 16, 16, 16, 16, 16 }),\n> +\t\t\t\t\t\t Span({ 1, 0, 0, 0, 1, 0, 0, 0, 1 }),\n\nWe have a first issue here, if we were to define an array control that\nhad, let's say, 256 elements, initialization would be very inconvenient.\nWe probably need a bit of a better syntax for the common case where all\nvalues are equal. I also think it would be nice to avoid the Span cast,\npossibly by using initializer lists instead, but I'm not entirely sure\nhow that would look like, and if it's actually doable.\n\nThere's also the question of how to handle arrays with a variable number\nof elements (although I don't think we have use cases for that yet, and\nmaybe we never will).\n\nThe second issue, which is probably more important, is how we define\nminimum, default and maximum values for array controls. At the moment,\nwe report those values for array elements, not for the whole array. Your\npatch shifts that to covering the whole array. For the default value,\nthis allows expressing defaults where all elements are not identical,\nwhich is very nice. For the minimum and maximum values, however, it's a\nbit more complicated.\n\nHow do we define minimum and maximum values for an array ? Is it per\nelement, or do we introduce a concept of minimum and maximum values for\nthe array as a whole ? The latter is likely quite ill-defined, but the\nformer isn't without issues either. If the minimum and maximum values\nfor each element are taken in isolation, the array minimum and maximum\nin the ControlInfo may not be valid values for the control. For\ninstance, we could have an array control with two elements, where each\nelement can be between -16 and 16, but with a requirement that the sum\nof the elements is always equal to 0. The minimum and maximum, could\nthen be { -16, -16 } and { 16, 16 }, but neither would be value values\nfor the control.\n\nThis is likely unsolvable in a generic manner, so I think we need to\nfocus on the main use case(s), and see what applications using this API\nneed. The documentation for ControlInfo should be updated accordingly,\nto clarify what minimum, default and maximum mean for array controls.\n\n> +\t\t\t\t\t } },\n> +\t { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> +\t { &controls::FrameDurationLimits, ControlInfo{\n> +\t\t\t\t\t\t Span({ 1000, 1000 }),\n> +\t\t\t\t\t\t Span({ 1000000000, 1000000000 }),\n> +\t\t\t\t\t\t Span({ 1000, 1000 }),\n> +\t\t\t\t\t } },\n> +\t { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) } },\n> +\tcontrols::controls);\n> \n> } /* namespace RPi */\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 1ea2c898..e64fc2bb 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -267,9 +267,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,\n> \t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> \t}\n> \n> -\tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> -\t\t\t\t\t\t\t frameDurations[1],\n> -\t\t\t\t\t\t\t frameDurations[2]);\n> +\tcontrols[&controls::FrameDurationLimits] = ControlInfo{ Span({ frameDurations[0], frameDurations[0] }),\n> +\t\t\t\t\t\t\t\tSpan({ frameDurations[1], frameDurations[1] }),\n> +\t\t\t\t\t\t\t\tSpan({ frameDurations[2], frameDurations[2] }) };\n> \n> \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n> }\n> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp\n> index d2050a86..5503cc8a 100644\n> --- a/test/serialization/ipa_data_serializer_test.cpp\n> +++ b/test/serialization/ipa_data_serializer_test.cpp\n> @@ -32,13 +32,15 @@\n> using namespace std;\n> using namespace libcamera;\n> \n> -static const ControlInfoMap Controls = ControlInfoMap({\n> +static const ControlInfoMap Controls = ControlInfoMap(\n> +\t{\n> \t\t{ &controls::AeEnable, ControlInfo(false, true) },\n> \t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> \t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> -\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> +\t\t{ &controls::ColourGains, ControlInfo{ Span({ 0, 0 }), Span({ 32, 32 }), Span({ 0, 0 }) } },\n> \t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> -\t}, controls::controls);\n> +\t},\n> +\tcontrols::controls);\n> \n> namespace libcamera {\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 5F355BD80A\n\tfor ;\n\tMon, 21 Mar 2022 13:47:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D83B604DC;\n\tMon, 21 Mar 2022 14:47:38 +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 2B8E9604C6\n\tfor ;\n\tMon, 21 Mar 2022 14:47:37 +0100 (CET)","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 96839291;\n\tMon, 21 Mar 2022 14:47:36 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647870458;\n\tbh=novh5JyGFazn3zxFGT0kU9uhxYbNuHiojeXE6Cu6OTI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=mEL7ET2AmWp02V9pmq0zBjzNq5j2moaHnzrSVPtofIN8oFEMVrJEJIOHBAt5nTyvA\n\tCrYM2N3FUdjIfdr9TJkcFGXQ6pGybz82V5y2HoPxbPOkQcjf0wAW3xT4jyIvDkONQ8\n\taolkh+2WYuyaWQ0slrrDg8kJcauHJCT5ON0O6uRVsVuR75oFNavDBLiNZ/UFqmHL0k\n\tRM0herimuHVNi4IgNklqfmWxEryjXzbUUgWlfJ6Zh7htGL1gdULlBMnVXOHijglm8Q\n\td6Kmx3EByMtJWMVrT1fVlZ+sdmhDN5N7MENmHt28CGirXQ9N8WCM853Y0xydjtlV9M\n\tbU6YchmrBRpTw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647870456;\n\tbh=novh5JyGFazn3zxFGT0kU9uhxYbNuHiojeXE6Cu6OTI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Pys1euDNf4DFXJrR2SehmdtHTcf2QY1Hk5b4m1VR+bH62uGC4FHIlBkagmdMC3mp0\n\tj8vqsBP2nF9NAgPlpH/pViZKDHm1nAtydAnG0DxLn6C0BEQjHcd4bDSYwr05OL2Ct3\n\t9rtrXoxR6sVxXAUMyYZL6lrh2wbENfNqMFsSMIsU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Pys1euDN\"; dkim-atps=neutral","Date":"Mon, 21 Mar 2022 15:47:19 +0200","To":"Christian Rauch ","Message-ID":"","References":"","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22348,"web_url":"https://patchwork.libcamera.org/comment/22348/","msgid":"","date":"2022-03-21T14:24:25","subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hello,\n\nSorry, I am not familiar with sending patches via mailing-lists. I\nusually use web frontends like GitLab or GitHub. I tried to configure my\nsystem as described in the \"PulseAudio How to Use git send-email\" [1]\nbut it fails with \"Unable to initialize SMTP properly.\". The libcamera\ndoc at [2] does not provide much information on how to submit patches.\n\nI just copy&pasted the patch file content at the end of my message. This\nmay look broken because of automatic line breaks.\n\nIn the future, will there be a way to contribute to libcamrea via a web\nfrontend? I've seen pull-requests sent to a GitHub project [3] that got\nreviewed there.\n\nBest,\nChristian\n\n[1]\nhttps://www.freedesktop.org/wiki/Software/PulseAudio/HowToUseGitSendEmail/\n[2] https://libcamera.org/contributing.html\n[3] https://github.com/kbingham/libcamera\n\n\n\n----------------------------------------------------------------------\n\nFrom d2e7eec801a0c77bd2b411a0b8b4c5bbe112da99 Mon Sep 17 00:00:00 2001\n\nFrom: Christian Rauch \n\nDate: Thu, 17 Mar 2022 01:58:10 +0000\n\nSubject: [PATCH] fix ControlInfo for Span Controls\n\n\n\nSome control properties are typed with a Span to store an array of values.\n\nCurrently those are ColourGains, SensorBlackLevels, ColourCorrectionMatrix\n\nand FrameDurationLimits. The value range and defaults in the ControlInfo of\n\nthose Controls is currently defined as scalar. This prevents accessing the\n\nControlInfo via the native Span type.\n\n\n\nBy defining the ControlInfo in terms of Spans, we can avoid this issue.\n\n---\n\n include/libcamera/ipa/raspberrypi.h | 52 ++++++++++++-------\n\n src/ipa/ipu3/ipu3.cpp | 6 +--\n\n .../ipa_data_serializer_test.cpp | 8 +--\n\n 3 files changed, 40 insertions(+), 26 deletions(-)\n\n\n\ndiff --git a/include/libcamera/ipa/raspberrypi.h\nb/include/libcamera/ipa/raspberrypi.h\n\nindex 7f705e49..fb5188a1 100644\n\n--- a/include/libcamera/ipa/raspberrypi.h\n\n+++ b/include/libcamera/ipa/raspberrypi.h\n\n@@ -27,26 +27,38 @@ namespace RPi {\n\n * and the pipeline handler may be reverted so that it aborts when an\n\n * unsupported control is encountered.\n\n */\n\n-static const ControlInfoMap Controls({\n\n-\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n\n-\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n\n-\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n\n-\t\t{ &controls::AeMeteringMode,\nControlInfo(controls::AeMeteringModeValues) },\n\n-\t\t{ &controls::AeConstraintMode,\nControlInfo(controls::AeConstraintModeValues) },\n\n-\t\t{ &controls::AeExposureMode,\nControlInfo(controls::AeExposureModeValues) },\n\n-\t\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n\n-\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n\n-\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n\n-\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n\n-\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n\n-\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n\n-\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n\n-\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n\n-\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n\n-\t\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535,\n65535, 65535, 65535), Rectangle{}) },\n\n-\t\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000),\nINT64_C(1000000000)) },\n\n-\t\t{ &controls::draft::NoiseReductionMode,\nControlInfo(controls::draft::NoiseReductionModeValues) }\n\n-\t}, controls::controls);\n\n+static const ControlInfoMap Controls(\n\n+\t{ { &controls::AeEnable, ControlInfo(false, true) },\n\n+\t { &controls::ExposureTime, ControlInfo(0, 999999) },\n\n+\t { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n\n+\t { &controls::AeMeteringMode,\nControlInfo(controls::AeMeteringModeValues) },\n\n+\t { &controls::AeConstraintMode,\nControlInfo(controls::AeConstraintModeValues) },\n\n+\t { &controls::AeExposureMode,\nControlInfo(controls::AeExposureModeValues) },\n\n+\t { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n\n+\t { &controls::AwbEnable, ControlInfo(false, true) },\n\n+\t { &controls::ColourGains, ControlInfo{\n\n+\t\t\t\t\t Span({ 0, 0 }),\n\n+\t\t\t\t\t Span({ 32, 32 }),\n\n+\t\t\t\t\t Span({ 0, 0 }),\n\n+\t\t\t\t } },\n\n+\t { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n\n+\t { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n\n+\t { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n\n+\t { &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n\n+\t { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n\n+\t { &controls::ColourCorrectionMatrix, ControlInfo{\n\n+\t\t\t\t\t\t Span({ -16, -16, -16, -16, -16, -16, -16,\n-16, -16 }),\n\n+\t\t\t\t\t\t Span({ 16, 16, 16, 16, 16, 16, 16, 16, 16 }),\n\n+\t\t\t\t\t\t Span({ 1, 0, 0, 0, 1, 0, 0, 0, 1 }),\n\n+\t\t\t\t\t } },\n\n+\t { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535,\n65535, 65535, 65535), Rectangle{}) },\n\n+\t { &controls::FrameDurationLimits, ControlInfo{\n\n+\t\t\t\t\t\t Span({ 1000, 1000 }),\n\n+\t\t\t\t\t\t Span({ 1000000000, 1000000000 }),\n\n+\t\t\t\t\t\t Span({ 1000, 1000 }),\n\n+\t\t\t\t\t } },\n\n+\t { &controls::draft::NoiseReductionMode,\nControlInfo(controls::draft::NoiseReductionModeValues) } },\n\n+\tcontrols::controls);\n\n\n\n } /* namespace RPi */\n\n\n\ndiff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n\nindex 1ea2c898..e64fc2bb 100644\n\n--- a/src/ipa/ipu3/ipu3.cpp\n\n+++ b/src/ipa/ipu3/ipu3.cpp\n\n@@ -267,9 +267,9 @@ void IPAIPU3::updateControls(const\nIPACameraSensorInfo &sensorInfo,\n\n \t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n\n \t}\n\n\n\n-\tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n\n-\t\t\t\t\t\t\t frameDurations[1],\n\n-\t\t\t\t\t\t\t frameDurations[2]);\n\n+\tcontrols[&controls::FrameDurationLimits] = ControlInfo{ Span({ frameDurations[0], frameDurations[0] }),\n\n+\t\t\t\t\t\t\t\tSpan({ frameDurations[1], frameDurations[1] }),\n\n+\t\t\t\t\t\t\t\tSpan({ frameDurations[2], frameDurations[2] }) };\n\n\n\n \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n\n }\n\ndiff --git a/test/serialization/ipa_data_serializer_test.cpp\nb/test/serialization/ipa_data_serializer_test.cpp\n\nindex d2050a86..5503cc8a 100644\n\n--- a/test/serialization/ipa_data_serializer_test.cpp\n\n+++ b/test/serialization/ipa_data_serializer_test.cpp\n\n@@ -32,13 +32,15 @@\n\n using namespace std;\n\n using namespace libcamera;\n\n\n\n-static const ControlInfoMap Controls = ControlInfoMap({\n\n+static const ControlInfoMap Controls = ControlInfoMap(\n\n+\t{\n\n \t\t{ &controls::AeEnable, ControlInfo(false, true) },\n\n \t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n\n \t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n\n-\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n\n+\t\t{ &controls::ColourGains, ControlInfo{ Span({ 0, 0 }),\nSpan({ 32, 32 }), Span({ 0, 0 }) } },\n\n \t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n\n-\t}, controls::controls);\n\n+\t},\n\n+\tcontrols::controls);\n\n\n\n namespace libcamera {\n\n\n\n--\n\n2.25.1\n\n\n\n\nAm 21.03.22 um 13:47 schrieb Laurent Pinchart:\n> Hi Christian,\n>\n> Thank you for the patch, and sorry for the late reply.\n>\n> As Jacopo mentioned, patches should be sent inline and not as\n> attachments, but I can still review this version.\n>\n> On Thu, Mar 17, 2022 at 02:48:47PM +0000, Christian Rauch via libcamera-devel wrote:\n>> Hello,\n>>\n>> The attached patch fixes the ControlInfo access of Span Control (see\n>> https://bugs.libcamera.org/show_bug.cgi?id=101).\n>>\n>> The ControlInfo for Spans was defined in scalar values (i.e. single\n>> values instead of arrays). Thus, while a Control was defined as Span,\n>> its corresponding ControlInfo was defined as scalar, causing access errors.\n>>\n>> Best,\n>> Christian\n>\n>> From 13052bf04a0eccfc6a896bba47ae18c4ad8c6ebb Mon Sep 17 00:00:00 2001\n>> From: Christian Rauch \n>> Date: Thu, 17 Mar 2022 01:58:10 +0000\n>> Subject: [PATCH 1/3] fix ControlInfo for Span Controls\n>>\n>> Some control properties are typed with a Span to store an array of values.\n>> Currently those are ColourGains, SensorBlackLevels, ColourCorrectionMatrix\n>> and FrameDurationLimits. The value range and defaults in the ControlInfo of\n>> those Controls is currently defined as scalar. This prevents accessing the\n>> ControlInfo via the native Span type.\n>>\n>> By defining the ControlInfo in terms of Spans, we can avoid this issue.\n>> ---\n>> include/libcamera/ipa/raspberrypi.h | 52 ++++++++++++-------\n>> src/ipa/ipu3/ipu3.cpp | 6 +--\n>> .../ipa_data_serializer_test.cpp | 8 +--\n>> 3 files changed, 40 insertions(+), 26 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n>> index 7f705e49..fb5188a1 100644\n>> --- a/include/libcamera/ipa/raspberrypi.h\n>> +++ b/include/libcamera/ipa/raspberrypi.h\n>> @@ -27,26 +27,38 @@ namespace RPi {\n>> * and the pipeline handler may be reverted so that it aborts when an\n>> * unsupported control is encountered.\n>> */\n>> -static const ControlInfoMap Controls({\n>> -\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n>> -\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n>> -\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n>> -\t\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n>> -\t\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n>> -\t\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n>> -\t\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n>> -\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n>> -\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>> -\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n>> -\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n>> -\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n>> -\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n>> -\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n>> -\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n>> -\t\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n>> -\t\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n>> -\t\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n>> -\t}, controls::controls);\n>> +static const ControlInfoMap Controls(\n>> +\t{ { &controls::AeEnable, ControlInfo(false, true) },\n>\n> The change in indentation makes the patch difficult to review, as it\n> mexes indentiation changes with other changes. If you think the\n> formatting is wrong, please send a patch to fix that first, without any\n> functional change, and a second patch with the functional changes. Both\n> can be sent as part of the same series (git format-patch --cover-letter\n> helps creating a series with a cover letter).\n>\n>> +\t { &controls::ExposureTime, ControlInfo(0, 999999) },\n>> +\t { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n>> +\t { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n>> +\t { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n>> +\t { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n>> +\t { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n>> +\t { &controls::AwbEnable, ControlInfo(false, true) },\n>> +\t { &controls::ColourGains, ControlInfo{\n>> +\t\t\t\t\t Span({ 0, 0 }),\n>> +\t\t\t\t\t Span({ 32, 32 }),\n>> +\t\t\t\t\t Span({ 0, 0 }),\n>> +\t\t\t\t } },\n>> +\t { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n>> +\t { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n>> +\t { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n>> +\t { &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n>> +\t { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n>> +\t { &controls::ColourCorrectionMatrix, ControlInfo{\n>> +\t\t\t\t\t\t Span({ -16, -16, -16, -16, -16, -16, -16, -16, -16 }),\n>> +\t\t\t\t\t\t Span({ 16, 16, 16, 16, 16, 16, 16, 16, 16 }),\n>> +\t\t\t\t\t\t Span({ 1, 0, 0, 0, 1, 0, 0, 0, 1 }),\n>\n> We have a first issue here, if we were to define an array control that\n> had, let's say, 256 elements, initialization would be very inconvenient.\n> We probably need a bit of a better syntax for the common case where all\n> values are equal. I also think it would be nice to avoid the Span cast,\n> possibly by using initializer lists instead, but I'm not entirely sure\n> how that would look like, and if it's actually doable.\n>\n> There's also the question of how to handle arrays with a variable number\n> of elements (although I don't think we have use cases for that yet, and\n> maybe we never will).\n>\n> The second issue, which is probably more important, is how we define\n> minimum, default and maximum values for array controls. At the moment,\n> we report those values for array elements, not for the whole array. Your\n> patch shifts that to covering the whole array. For the default value,\n> this allows expressing defaults where all elements are not identical,\n> which is very nice. For the minimum and maximum values, however, it's a\n> bit more complicated.\n>\n> How do we define minimum and maximum values for an array ? Is it per\n> element, or do we introduce a concept of minimum and maximum values for\n> the array as a whole ? The latter is likely quite ill-defined, but the\n> former isn't without issues either. If the minimum and maximum values\n> for each element are taken in isolation, the array minimum and maximum\n> in the ControlInfo may not be valid values for the control. For\n> instance, we could have an array control with two elements, where each\n> element can be between -16 and 16, but with a requirement that the sum\n> of the elements is always equal to 0. The minimum and maximum, could\n> then be { -16, -16 } and { 16, 16 }, but neither would be value values\n> for the control.\n>\n> This is likely unsolvable in a generic manner, so I think we need to\n> focus on the main use case(s), and see what applications using this API\n> need. The documentation for ControlInfo should be updated accordingly,\n> to clarify what minimum, default and maximum mean for array controls.\n>\n>> +\t\t\t\t\t } },\n>> +\t { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n>> +\t { &controls::FrameDurationLimits, ControlInfo{\n>> +\t\t\t\t\t\t Span({ 1000, 1000 }),\n>> +\t\t\t\t\t\t Span({ 1000000000, 1000000000 }),\n>> +\t\t\t\t\t\t Span({ 1000, 1000 }),\n>> +\t\t\t\t\t } },\n>> +\t { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) } },\n>> +\tcontrols::controls);\n>>\n>> } /* namespace RPi */\n>>\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 1ea2c898..e64fc2bb 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -267,9 +267,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,\n>> \t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n>> \t}\n>>\n>> -\tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n>> -\t\t\t\t\t\t\t frameDurations[1],\n>> -\t\t\t\t\t\t\t frameDurations[2]);\n>> +\tcontrols[&controls::FrameDurationLimits] = ControlInfo{ Span({ frameDurations[0], frameDurations[0] }),\n>> +\t\t\t\t\t\t\t\tSpan({ frameDurations[1], frameDurations[1] }),\n>> +\t\t\t\t\t\t\t\tSpan({ frameDurations[2], frameDurations[2] }) };\n>>\n>> \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n>> }\n>> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp\n>> index d2050a86..5503cc8a 100644\n>> --- a/test/serialization/ipa_data_serializer_test.cpp\n>> +++ b/test/serialization/ipa_data_serializer_test.cpp\n>> @@ -32,13 +32,15 @@\n>> using namespace std;\n>> using namespace libcamera;\n>>\n>> -static const ControlInfoMap Controls = ControlInfoMap({\n>> +static const ControlInfoMap Controls = ControlInfoMap(\n>> +\t{\n>> \t\t{ &controls::AeEnable, ControlInfo(false, true) },\n>> \t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n>> \t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n>> -\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>> +\t\t{ &controls::ColourGains, ControlInfo{ Span({ 0, 0 }), Span({ 32, 32 }), Span({ 0, 0 }) } },\n>> \t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n>> -\t}, controls::controls);\n>> +\t},\n>> +\tcontrols::controls);\n>>\n>> namespace libcamera {\n>>\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 3EC1DC0F1B\n\tfor ;\n\tMon, 21 Mar 2022 14:24:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D5E7A604E7;\n\tMon, 21 Mar 2022 15:24:27 +0100 (CET)","from mout.gmx.net (mout.gmx.net [212.227.17.21])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8148D604C6\n\tfor ;\n\tMon, 21 Mar 2022 15:24:26 +0100 (CET)","from [192.168.1.209] ([92.10.251.63]) by mail.gmx.net (mrgmx105\n\t[212.227.17.168]) with ESMTPSA (Nemesis) id 1MOzSu-1niUBJ32IG-00PN2L;\n\tMon, 21 Mar 2022 15:24:25 +0100"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647872667;\n\tbh=UyvAF39Rtvw4jmLyApLYUJJw+TAEAdIDzzx3jUDYq1o=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=lRLu09mEO/bbcUoO/zPSgQsbo41q3od53RJRt3ofR6Jbty6P6lsXHawnVWBNSZs2H\n\tBwj9Y3zkyZtN88eAunbiBA4aHryvPcW7scK5LX6Af+oOs1IqhYhLC4JIRW9BU37Uk/\n\taH9dtiEEJNZzPhVn0P+Kuc8CGuQWi1Tffd+5yvd1KZHj/xgqI+lLNF52vORE/oWq/n\n\t0+pD4plICcmlQgxRMpol/AS6vsil5bhd2q7bR/Wl8HMopFkn8HYMZdbLUr3xkMKz1u\n\t9G5VQB+X4HQ1CmtxG1/fTLILyD6eE6jn5NqFO9AJCvkwhHLRFJijoYbSfTudhBFt4K\n\te3aYdztF10qWA==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1647872666;\n\tbh=UyvAF39Rtvw4jmLyApLYUJJw+TAEAdIDzzx3jUDYq1o=;\n\th=X-UI-Sender-Class:Date:Subject:To:Cc:References:From:In-Reply-To;\n\tb=lwO9/OeDuu1RZlW1xsxKFCkD+pzPlWmscoMRO81ebYVQhCYmwahF95fIXY6pRCN03\n\tLimLZnoENJ/6sv347aSLGM8tFHQsDKFvDibtyXXOLzI4oHtwMMZKKMEKxY/hY9QDfw\n\tZD4JdPw7syGqjAjVoTE9BSdZn2aFerhP0iwDwlRs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"lwO9/OeD\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"","Date":"Mon, 21 Mar 2022 14:24:25 +0000","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.5.0","Content-Language":"en-CA","To":"Laurent Pinchart ","References":"\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:sb0LWQJygwJCj/tjoocD9uWpRxzDUpmLtuDQM7vi4T8OSzy06KA\n\tL6Em/RJ4JRevApVHA+97fALRpwAe7+N+Ihn4IakXHnIky1dySFjVR+9z4IXyfY2tlOqCePi\n\tJGCgZajFpQx5qDsy5gCvzQIrYCDohSZ4OmXKiPedXSs5M41YvU6wzyQTxEIFF3OzH7xD950\n\tEWWjS2SMQDDQyqN7w9sPA==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:WdLK++GokPA=:jn1Vqlh31LXX5VykEdY0N1\n\tdUZp6kxucJcux9pga9yeieNrrceKrfftJ3Y0c3x5DKCl6ld0QU8vJaFcfqUZJG1NSNh50/Hn1\n\tnNh9fWVhG/0jXDjBU5Py4DA/ggHLPthVCD1xRV+bMUkxqFybaS4PzCixEF5oiiEqOB1pXKtDO\n\tamo/2r1xaatM9h8xaXaW5YwuG4heAXyCMC+Xz0vBiuM9wy165n4V3CTeZdnVQrF4Q5jpTr7Sy\n\tqwN6lr436zy8oCSD0fLvtR3U4afXXcMjgh/G7dTWXd91YrqM00ggDHS8PZBHnEFG8/C0QnHnT\n\tewQKEIEzrXhAujv40JAIFuApa9HTYOk8MF2i2xnV4DcgABh9Ch1kiaLGHyHKbS8ebXbr6TxjO\n\tC63K1XK/v9OqZkp4tMCvopqOgKMjjKGJetWkD8adJ6x5WhYxqimLkZiAQu2N9HdWh0CqSbdcA\n\tXS/JLW/oRL5mKLL2BG+Dbmzv0PDSxXPfHpRO7Uu/9dw0IWxhiU2RFsCznlg/j2qVvyp2PnTJf\n\t7MkF3hGxdhKGHKBcNZNoXtJgsd+2rxRSGfmSZVmPXQyxsNyJcOZleKRAJ7zQmx+21XCpUDL6D\n\tANT7KlBrm9og4vEZQdX376CvXb+xoluCIvapkiaDQkJGTI7qD1rZoZRCb0Tl/AccZm7tZCdLv\n\tmpuj3oKzIwGvkO0iFuRsZPD+N7CmRDndt5ECjHt2fVuChP4aHmXobK9qW0jyqcUIFGTREKi2P\n\teHmMCO2BKGgKQWYCFAZydu1HOuN9L7Kc+4JZ8eP0v6Z9n3DcBV1YJMAH4Gn06dgY3JCbp0eTQ\n\trEWjaHTfeSroZAUBXIjpk5QdpP0MajuDeE6cfbZxHtSCMcRGnl7dIeTbJL+Jw8UAWU5Jgjaix\n\tj6lx2Qi0i058itaxKlVZoS/mofXA/rgq2Pdx6/gFmwmLAr2bEZ23H2wnI8jmQHWWLXC1cwDyx\n\tQVv1ekM7T4DRpnz5ohAHE0aB5mCmNBbwMcGfmNzAK3q/8dPN/wFJRrL3YTYGwh5K5lIe5xmI+\n\tpt4Yr0JAeL9QCpnWUro93sM7xhOGiJYbqDMPBQV0VhUEWUpu39Q6aqL4XV3FFka0fBqcv4GYs\n\tgeqFOCwHiA1zzM=","Subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","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","From":"Christian Rauch via libcamera-devel\n\t","Reply-To":"Christian Rauch ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22349,"web_url":"https://patchwork.libcamera.org/comment/22349/","msgid":"","date":"2022-03-21T14:47:42","subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Christian,\n\nOn Mon, Mar 21, 2022 at 02:24:25PM +0000, Christian Rauch wrote:\n> Hello,\n> \n> Sorry, I am not familiar with sending patches via mailing-lists. I\n> usually use web frontends like GitLab or GitHub. I tried to configure my\n> system as described in the \"PulseAudio How to Use git send-email\" [1]\n> but it fails with \"Unable to initialize SMTP properly.\". The libcamera\n> doc at [2] does not provide much information on how to submit patches.\n\nWould this help\nhttps://stackoverflow.com/questions/68238912/how-to-configure-and-use-git-send-email-to-work-with-gmail-to-email-patches-to\n? If it works, we should update [2] with additional instructions, as\ngmail is quite common.\n\n> I just copy&pasted the patch file content at the end of my message. This\n> may look broken because of automatic line breaks.\n\nYes, that's not great, copy&paste will most likely result in broken\npatches with most e-mail clients (and especially the gmail web UI).\n\n> In the future, will there be a way to contribute to libcamrea via a web\n> frontend? I've seen pull-requests sent to a GitHub project [3] that got\n> reviewed there.\n\nThat's not planned at this point. Submission through a web UI would be\nnice, but it would need to end up on the mailing list for review. The\ngit..b review web UIs are barely more than a joke I'm afraid.\n\nIt would be nice to have a service to automate submission of patches, by\npushing to a git tree, but if we made that open without authentication\nthere would be a risk of abuse (and spamming in particular). I don't\nknow if it's possible to have a repository on gitlab where any\nauthenticated user could push, with a script that would then posted\npatches to the mailing list. If someone likes playing with gitlab, it\ncould be interesting to develop that.\n\n> [1] https://www.freedesktop.org/wiki/Software/PulseAudio/HowToUseGitSendEmail/\n> [2] https://libcamera.org/contributing.html\n> [3] https://github.com/kbingham/libcamera","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 36DA8BD80A\n\tfor ;\n\tMon, 21 Mar 2022 14:48:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 85826604DC;\n\tMon, 21 Mar 2022 15:48:01 +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 E9427604C6\n\tfor ;\n\tMon, 21 Mar 2022 15:47:59 +0100 (CET)","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 69D50291;\n\tMon, 21 Mar 2022 15:47:59 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647874081;\n\tbh=eonhtUkzeg549cONzGzDVScKgRIyBPJ7MjFid19tIe0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ALFKJp8itu/5ymx+CbUZAVb/sV9dlrlGl1XMLP+cGt1hmSNl5httdTzdzZH9GFWGS\n\tJf/w3SMLUCkM3knRkE0keYvBum2qhF7eB2bg2hLtKjxC38gvQ5Cwlw3p9GB4sMUNn/\n\tYR1B5L/e91LpxFTgGVnnrXCqC90ow6SKDu0DmlI4VOtmJ1lm97J8COF1rc7PmUjhlP\n\t1S5WueUA91jfUBdKb5T0nECVqRXdH5p3prWhpKXV5exSMF99lWR2/3XnE6QL10Nfq6\n\tmE4V5chhDqfJV7sXulu1TKaItYkgloGziUxXeahN5gxpqwL4FD8K8zSofUgSGAOZNA\n\tcPPya9BP2Q3tQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1647874079;\n\tbh=eonhtUkzeg549cONzGzDVScKgRIyBPJ7MjFid19tIe0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VLGOja57YaJzcbTyhAZd0DYDA2s92P+T6ybhTqZeG/S/vMQMCk70ZScaNyBmKnARb\n\truNCh2ZlrPg8fpbZI1QmHJlT+V/yVIBy3i1u2T6vXbkHMJ20Pk+jmkZf8XAtF6ct5R\n\twi7iQpFITpW437ih8muznEKvPULyQaYaKN2kqKO4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VLGOja57\"; dkim-atps=neutral","Date":"Mon, 21 Mar 2022 16:47:42 +0200","To":"Christian Rauch ","Message-ID":"","References":"\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22351,"web_url":"https://patchwork.libcamera.org/comment/22351/","msgid":"<277a2261-a869-ae2a-ac93-7faac6b465d4@gmx.de>","date":"2022-03-21T23:12:05","subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Dear Laurent,\n\nYes, that worked. Thanks. It turned out that I was just using the wrong\nport. I adapted the configuration from Thunderbird, which uses SSL/TLS\nport 465. For \"git send-email\" I had to use 587.\n\nAdding information about how to configure and use \"git send-email\" to\nthe contribution documentation would help a lot for new contributors\nthat are not familiar with this way of contributing patches.\n\nThanks,\nChristian\n\n\nAm 21.03.22 um 14:47 schrieb Laurent Pinchart:\n> Hi Christian,\n>\n> On Mon, Mar 21, 2022 at 02:24:25PM +0000, Christian Rauch wrote:\n>> Hello,\n>>\n>> Sorry, I am not familiar with sending patches via mailing-lists. I\n>> usually use web frontends like GitLab or GitHub. I tried to configure my\n>> system as described in the \"PulseAudio How to Use git send-email\" [1]\n>> but it fails with \"Unable to initialize SMTP properly.\". The libcamera\n>> doc at [2] does not provide much information on how to submit patches.\n>\n> Would this help\n> https://stackoverflow.com/questions/68238912/how-to-configure-and-use-git-send-email-to-work-with-gmail-to-email-patches-to\n> ? If it works, we should update [2] with additional instructions, as\n> gmail is quite common.\n>\n>> I just copy&pasted the patch file content at the end of my message. This\n>> may look broken because of automatic line breaks.\n>\n> Yes, that's not great, copy&paste will most likely result in broken\n> patches with most e-mail clients (and especially the gmail web UI).\n>\n>> In the future, will there be a way to contribute to libcamrea via a web\n>> frontend? I've seen pull-requests sent to a GitHub project [3] that got\n>> reviewed there.\n>\n> That's not planned at this point. Submission through a web UI would be\n> nice, but it would need to end up on the mailing list for review. The\n> git..b review web UIs are barely more than a joke I'm afraid.\n>\n> It would be nice to have a service to automate submission of patches, by\n> pushing to a git tree, but if we made that open without authentication\n> there would be a risk of abuse (and spamming in particular). I don't\n> know if it's possible to have a repository on gitlab where any\n> authenticated user could push, with a script that would then posted\n> patches to the mailing list. If someone likes playing with gitlab, it\n> could be interesting to develop that.\n>\n>> [1] https://www.freedesktop.org/wiki/Software/PulseAudio/HowToUseGitSendEmail/\n>> [2] https://libcamera.org/contributing.html\n>> [3] https://github.com/kbingham/libcamera\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 4A2ABC0F1B\n\tfor ;\n\tMon, 21 Mar 2022 23:12:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A72B3604C7;\n\tTue, 22 Mar 2022 00:12:07 +0100 (CET)","from mout.gmx.net (mout.gmx.net [212.227.17.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7654A604C7\n\tfor ;\n\tTue, 22 Mar 2022 00:12:06 +0100 (CET)","from [192.168.1.209] ([92.10.251.63]) by mail.gmx.net (mrgmx104\n\t[212.227.17.168]) with ESMTPSA (Nemesis) id 1MmlTC-1nyeTj366o-00jqIk;\n\tTue, 22 Mar 2022 00:12:05 +0100"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1647904327;\n\tbh=/Um2PUzQgFTfBxOMpyPeDqOmXp7sDjixOGGZZOwg9H0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=zFgc8iqNoBK0KZ2T+tE5Txx5iGm5LoxOjfrypjIrlF3HQuFCGZPm12ts/44GTnwIe\n\tBip7+D/E5CKg5P/zt/baIgy2EfZo8EfecpMXq920xipXPgmBNcFMRR6j+c4qTENTrH\n\t1SCa+yAuW9/lED9pnDATymPEGzLa3diflFQiWNJlnNrvCMHL44TI5PQ4N6IAU+7AJa\n\tdDS044xUmN/XEacofT8/L/BU5i/rfH7NBiXIonnBHTF8Uz6/w8eIwB1GdRAGyB8wSW\n\tzcs/w32jzgdCARV5LoLg/3wrBMIj+AQ2aXrTHZ/xI+chrQHvGqt0b545/lHOJ7h1Gt\n\tdGCs+L/pYM95Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1647904325;\n\tbh=/Um2PUzQgFTfBxOMpyPeDqOmXp7sDjixOGGZZOwg9H0=;\n\th=X-UI-Sender-Class:Date:Subject:To:Cc:References:From:In-Reply-To;\n\tb=lh/UMBSCiwyGFsMDNI2SB7K/miCz23AwzPxDmA413wy9zPmVx/WcCQM7hEJ9EQYTx\n\tIW2c7Bf6DFWm0bh/0gyMH4O3uA8uRJxP7h/jfX3TiW7lxoEetVnBPR+OCQge6tMQqw\n\tB8h1c/9AmMbAJO/4n/5+QC+M378y1JWBUsrvSSVI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"lh/UMBSC\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"<277a2261-a869-ae2a-ac93-7faac6b465d4@gmx.de>","Date":"Mon, 21 Mar 2022 23:12:05 +0000","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.5.0","Content-Language":"en-CA","To":"Laurent Pinchart ","References":"\n\t\n\t\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:I/6O3ewezKuu+gehyTAeqM9Lk3rL7dDUuHFrYxi67ooqqbKI5qb\n\tTuo4bWe95Wo8cB8evDs/Qai0vlIEFTWf1eOtwHXzzwRSTcYFFeX4gS6V4OcPEs10rnA09US\n\t0W+NevJH7WuP/PGhLEEpca2oPBMZdn3UwXqzswgP9rvp4fXxN24tYuXdB28uHwX3LwLYWeB\n\tcOqxj1CbkPUOOM/m+nQUw==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:3ZDb6xXAVkY=:Jni4GT1bgzEBy4qYGzyZ8J\n\tFp5soSwOi9c9byqmOhuhkOTGVVuhKBck8btiyni0hCiCv6lykdoZgN95rqeNjKKaZq9dZ4luR\n\t3m/Gg1ihNiQBadzx2jQ18Fi0gUiZoAYZGm3cbB9x7+n5LSyKt5Vypl3HXCL7MmQZrRbettUXk\n\tJgufrMO88yGpZeZL1PW74B2B/jip3wifihdheYtqg9ejRKFIYfT8Qs0+SheYdZTMAd/NhxjwH\n\t3pRuHuRj+Oy9EY4WLR0PE+6Hi9vITQQY2VlQMiJJ1P4XwQeR832ZNz7nJKp5hIb4hwcfgK/pA\n\tlHtT6BjDjm8axAiPAAxYBFYZOG5WmlwakpPt5uAPk1vL3L383F48+kyg2Goh3nKe9Y0TOCHGS\n\tEfFvwWuGFABYIdR4As5cES91CScXJhPjSBP8gH3QQ7bPEkZs91cQem62NctuqD142MUYNlimk\n\tis9aBMkKKrPaJAQUc1je1jU6gmk4XzsRPpc4yl9VEuPQ3nRH4ldVdiMUgJPsnkN9mJikV1Yy/\n\tzLH0ALY/6kw5QWQUzu1kFZfL95rf1Z1un8pAQXD0ccQskbX+0NZUfxWZDwtjPcjNIcR/hQ5dU\n\tyoJ6ZedV0kvTZEOLADj42PKde2BGCneA0Vf2SStTHE/eIWE7Ba5V0Auv71EZIR1frfmwBne+H\n\tTnC/iWsjeUQhjpDZYJhKu380yz7h0c507ktaCbVZKwXzJVgvjI7u5FIDfnyEGKi+QZRZJvBil\n\tuSWUYlITYGdLrc8YAjgjErai0Tx6xn2DOQKANJWxHYFdVSNxZE2R0MbMvnu82Qbh5zMs2+TIQ\n\tzxknTbsOgvcxrQONuRn2af26xyqcbmQ0g3+ciW2w5qb5UYYhVW/HKbS29SPwB9dnuYUb5Gl67\n\tgR395uu8qRSJ9hSZIK8wAfe63W9xByZPgWzKDA108+p1BCI6JxP+9QN8mWe9HvLMCz37FWW6n\n\tugsPsrm82XnIXPqFZ74Y/uEi1rZ7iny3xrbjYYC6ccRdrTkzaXxhw32e+yRxJMDdZ+olf5S+e\n\tig3n/B/h5xEDzn+BIs/nVuHMCdXeGp0jKdUbaKsmYdI72ifP2qE7BtPTKkW+/43B9Kawh5I6b\n\tLaqdjdlEnAGNK4=","Subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","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","From":"Christian Rauch via libcamera-devel\n\t","Reply-To":"Christian Rauch ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22532,"web_url":"https://patchwork.libcamera.org/comment/22532/","msgid":"","date":"2022-03-30T23:14:30","subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Dear Laurent,\n\nI initially overlooked your review of my patch because of the inline\nresponse.\n\nThere are a lot of questions raised about the definition and\ninterpretation of the limits and default values in the ControlInfo.\n\nThe main purpose of this patch is to fully define the ControlInfo with\nthe same type and dimensionality as the Control it is defined for. The\nreason for this is that otherwise, there is no information about the\ndimensionality of a Control.\n\nI am going to answer your remaining questions inline below.\n\n\nAm 21.03.22 um 13:47 schrieb Laurent Pinchart:\n> Hi Christian,\n>\n> Thank you for the patch, and sorry for the late reply.\n>\n> As Jacopo mentioned, patches should be sent inline and not as\n> attachments, but I can still review this version.\n>\n> On Thu, Mar 17, 2022 at 02:48:47PM +0000, Christian Rauch via libcamera-devel wrote:\n>> Hello,\n>>\n>> The attached patch fixes the ControlInfo access of Span Control (see\n>> https://bugs.libcamera.org/show_bug.cgi?id=101).\n>>\n>> The ControlInfo for Spans was defined in scalar values (i.e. single\n>> values instead of arrays). Thus, while a Control was defined as Span,\n>> its corresponding ControlInfo was defined as scalar, causing access errors.\n>>\n>> Best,\n>> Christian\n>\n>> From 13052bf04a0eccfc6a896bba47ae18c4ad8c6ebb Mon Sep 17 00:00:00 2001\n>> From: Christian Rauch \n>> Date: Thu, 17 Mar 2022 01:58:10 +0000\n>> Subject: [PATCH 1/3] fix ControlInfo for Span Controls\n>>\n>> Some control properties are typed with a Span to store an array of values.\n>> Currently those are ColourGains, SensorBlackLevels, ColourCorrectionMatrix\n>> and FrameDurationLimits. The value range and defaults in the ControlInfo of\n>> those Controls is currently defined as scalar. This prevents accessing the\n>> ControlInfo via the native Span type.\n>>\n>> By defining the ControlInfo in terms of Spans, we can avoid this issue.\n>> ---\n>> include/libcamera/ipa/raspberrypi.h | 52 ++++++++++++-------\n>> src/ipa/ipu3/ipu3.cpp | 6 +--\n>> .../ipa_data_serializer_test.cpp | 8 +--\n>> 3 files changed, 40 insertions(+), 26 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n>> index 7f705e49..fb5188a1 100644\n>> --- a/include/libcamera/ipa/raspberrypi.h\n>> +++ b/include/libcamera/ipa/raspberrypi.h\n>> @@ -27,26 +27,38 @@ namespace RPi {\n>> * and the pipeline handler may be reverted so that it aborts when an\n>> * unsupported control is encountered.\n>> */\n>> -static const ControlInfoMap Controls({\n>> -\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n>> -\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n>> -\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n>> -\t\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n>> -\t\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n>> -\t\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n>> -\t\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n>> -\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n>> -\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>> -\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n>> -\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n>> -\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n>> -\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n>> -\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n>> -\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n>> -\t\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n>> -\t\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n>> -\t\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n>> -\t}, controls::controls);\n>> +static const ControlInfoMap Controls(\n>> +\t{ { &controls::AeEnable, ControlInfo(false, true) },\n>\n> The change in indentation makes the patch difficult to review, as it\n> mexes indentiation changes with other changes. If you think the\n> formatting is wrong, please send a patch to fix that first, without any\n> functional change, and a second patch with the functional changes. Both\n> can be sent as part of the same series (git format-patch --cover-letter\n> helps creating a series with a cover letter).\n\nParts of this change is caused by the applied .clang-format style. I\nwill split those changes once I send a new version of the patches.\n\n>\n>> +\t { &controls::ExposureTime, ControlInfo(0, 999999) },\n>> +\t { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n>> +\t { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n>> +\t { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n>> +\t { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n>> +\t { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n>> +\t { &controls::AwbEnable, ControlInfo(false, true) },\n>> +\t { &controls::ColourGains, ControlInfo{\n>> +\t\t\t\t\t Span({ 0, 0 }),\n>> +\t\t\t\t\t Span({ 32, 32 }),\n>> +\t\t\t\t\t Span({ 0, 0 }),\n>> +\t\t\t\t } },\n>> +\t { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n>> +\t { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n>> +\t { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n>> +\t { &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n>> +\t { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n>> +\t { &controls::ColourCorrectionMatrix, ControlInfo{\n>> +\t\t\t\t\t\t Span({ -16, -16, -16, -16, -16, -16, -16, -16, -16 }),\n>> +\t\t\t\t\t\t Span({ 16, 16, 16, 16, 16, 16, 16, 16, 16 }),\n>> +\t\t\t\t\t\t Span({ 1, 0, 0, 0, 1, 0, 0, 0, 1 }),\n>\n> We have a first issue here, if we were to define an array control that\n> had, let's say, 256 elements, initialization would be very inconvenient.\n> We probably need a bit of a better syntax for the common case where all\n> values are equal. I also think it would be nice to avoid the Span cast,\n> possibly by using initializer lists instead, but I'm not entirely sure\n> how that would look like, and if it's actually doable.\n\nTo initialise a large Span with the same values, we could implement a\nfunction that just creates a Span given a length and a value.\nInitialising a ControlInfo via an initialiser list would still require\nexplicit casts. We should avoid casting every element like:\nControlInfo({int(1), int(2), int(3), ...}, {...}, {...}).\n\n>\n> There's also the question of how to handle arrays with a variable number\n> of elements (although I don't think we have use cases for that yet, and\n> maybe we never will).\n\nAll those Spans are already variable-sized. They do not have a fixed\nsize. This is the main reason why I propose to initialise them with the\ncorrect length. Otherwise, there is no user-visible information about\nthe dimensions of these spans. Information about the length of Spans is\nonly directly available via the \"control_ids.yaml\", but this information\nis not used by the \"gen-controls.py\" script, and implicitly by reading\nthrough the code.\n\n>\n> The second issue, which is probably more important, is how we define\n> minimum, default and maximum values for array controls. At the moment,\n> we report those values for array elements, not for the whole array. Your\n> patch shifts that to covering the whole array. For the default value,\n> this allows expressing defaults where all elements are not identical,\n> which is very nice. For the minimum and maximum values, however, it's a\n> bit more complicated.\n\nWhile using scalar values makes the ControlInfo definition simpler, it\nis not well defined and loses a lot of its meaning.\n\nExamples:\n- The 3x3 ColourCorrectionMatrix by default should be an identity\nmapping that does not apply a correction. But what does this mean in\nterms of a scalar value? Would this be 0 (as in no mapping) or 1 (as in\nidentity)? Defining this as a 3x3 identity matrix makes this crystal\nclear without any ambiguous formulation.\n- The FrameDurationLimits stores a minimum and maximum frame duration\n(i.e. the \"FrameDuration min/max\") between which a sensor can choose its\nframe duration from. Those two values should not have the same\n\"ControlInfo min/max\" limits as this would allow setting the\nFrameDurationLimits min and max to the same values.\n\nAlso semantically, I think that the value range of a control property\nshould have the same dimensionality as the control property itself.\n\n>\n> How do we define minimum and maximum values for an array ? Is it per\n> element, or do we introduce a concept of minimum and maximum values for\n> the array as a whole ? The latter is likely quite ill-defined, but the\n> former isn't without issues either. If the minimum and maximum values\n> for each element are taken in isolation, the array minimum and maximum\n> in the ControlInfo may not be valid values for the control. For\n> instance, we could have an array control with two elements, where each\n> element can be between -16 and 16, but with a requirement that the sum\n> of the elements is always equal to 0. The minimum and maximum, could\n> then be { -16, -16 } and { 16, 16 }, but neither would be value values\n> for the control.\n\nThis case (\"sum of elements is equal to 0\") could also not be handled by\nthe current definitions. Those cases that cannot be expressed by lower\nand upper bounds must be handled \"somewhere\" else. There are already\nconfigurations, such as setting an \"ExposureValue\" while\n\"AeExposureMode\" is on, which cannot be handled by the current API. If\nthis invalid configuration state happens, the request is currently just\ncancelled.\n\n>\n> This is likely unsolvable in a generic manner, so I think we need to\n> focus on the main use case(s), and see what applications using this API\n> need. The documentation for ControlInfo should be updated accordingly,\n> to clarify what minimum, default and maximum mean for array controls.\n\nI agree that a lot of those questions cannot be answered right now, and\nthey are definitely out-of-scope of this patch.\n\n>\n>> +\t\t\t\t\t } },\n>> +\t { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n>> +\t { &controls::FrameDurationLimits, ControlInfo{\n>> +\t\t\t\t\t\t Span({ 1000, 1000 }),\n>> +\t\t\t\t\t\t Span({ 1000000000, 1000000000 }),\n>> +\t\t\t\t\t\t Span({ 1000, 1000 }),\n>> +\t\t\t\t\t } },\n>> +\t { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) } },\n>> +\tcontrols::controls);\n>>\n>> } /* namespace RPi */\n>>\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 1ea2c898..e64fc2bb 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -267,9 +267,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,\n>> \t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n>> \t}\n>>\n>> -\tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n>> -\t\t\t\t\t\t\t frameDurations[1],\n>> -\t\t\t\t\t\t\t frameDurations[2]);\n>> +\tcontrols[&controls::FrameDurationLimits] = ControlInfo{ Span({ frameDurations[0], frameDurations[0] }),\n>> +\t\t\t\t\t\t\t\tSpan({ frameDurations[1], frameDurations[1] }),\n>> +\t\t\t\t\t\t\t\tSpan({ frameDurations[2], frameDurations[2] }) };\n>>\n>> \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n>> }\n>> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp\n>> index d2050a86..5503cc8a 100644\n>> --- a/test/serialization/ipa_data_serializer_test.cpp\n>> +++ b/test/serialization/ipa_data_serializer_test.cpp\n>> @@ -32,13 +32,15 @@\n>> using namespace std;\n>> using namespace libcamera;\n>>\n>> -static const ControlInfoMap Controls = ControlInfoMap({\n>> +static const ControlInfoMap Controls = ControlInfoMap(\n>> +\t{\n>> \t\t{ &controls::AeEnable, ControlInfo(false, true) },\n>> \t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n>> \t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n>> -\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>> +\t\t{ &controls::ColourGains, ControlInfo{ Span({ 0, 0 }), Span({ 32, 32 }), Span({ 0, 0 }) } },\n>> \t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n>> -\t}, controls::controls);\n>> +\t},\n>> +\tcontrols::controls);\n>>\n>> namespace libcamera {\n>>\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 0CF3EC0F1B\n\tfor ;\n\tWed, 30 Mar 2022 23:14:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF0C9633A7;\n\tThu, 31 Mar 2022 01:14:33 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.17.20])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DB681604BC\n\tfor ;\n\tThu, 31 Mar 2022 01:14:31 +0200 (CEST)","from [192.168.1.209] ([92.10.251.63]) by mail.gmx.net (mrgmx104\n\t[212.227.17.168]) with ESMTPSA (Nemesis) id 1Mof9F-1oOQcI19lx-00p2S3\n\tfor\n\t; Thu, 31 Mar 2022 01:14:31 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648682074;\n\tbh=c5R64PWgMkmoadsvg04zxRTGpOFJCM253px3aQInozI=;\n\th=Date:Cc:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=dJWIUcQydYXCgSt7Q++5jaleRgqIMPKqsu1QhRzkNFv9Wob812SN4UTbCrH86D07Z\n\tJy41u3LGP4MKZP6A4d7sff8c8LjVO3RmQyfPPPv37hbfHn2smuoR0aRjhh/Bjyfflf\n\t1iVaNUmfogRMphzfkIaQsyb3rc2+9VKPU9e4bqpgKHWKzXYMGoJiGEpEnVwNnUC/HB\n\tC4m5D2X9JJpyQa6b4XiCGlYc0/AETcrx2JgeYeLxJk5293+aJS7AOaYBAuSMw4cxws\n\t5ao7+AC9uND8DZSUlbUNDeKtHIryGFPi0ZpG4ailvF+s43GP1EqqeA6QwoTAP9zsPG\n\tz7ROa3Kb0rz7g==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1648682071;\n\tbh=c5R64PWgMkmoadsvg04zxRTGpOFJCM253px3aQInozI=;\n\th=X-UI-Sender-Class:Date:Subject:Cc:References:From:In-Reply-To;\n\tb=lHjZQHAoG5GCiqfzHoZKVeZGNLW2KXo/5oDl6ds+FEgDEAMzLo3aE0JJ58I1+cOYt\n\tDjKiKSDyuxC749mnvwwBQLvHrDNHHJkVadORHJALElCKG7r10A9Z0zFP4L/iXCg605\n\t4+udVqcMmV5SMakgu5i2eMhn0zniNEPb2J1cVthQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"lHjZQHAo\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"","Date":"Thu, 31 Mar 2022 00:14:30 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.7.0","Content-Language":"en-GB","Cc":"libcamera-devel@lists.libcamera.org","References":"\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:N53Mb9qaloSpVFicN0w1sTwYocvQrN6rp6LiHAMqd8amZylcla8\n\tuha882J/GjyZ54DxA6SicjGiGsmCc/tLtz0B89r6V4qACl8z2WO37ofyfDcD8A4k8zxNpfD\n\tTOIua1ot91ieyx/rfDWPTCdVIUS0N6KpuonUSVfNZsYyRyBcrBJ47LH6PWpgUTlxkayeUyy\n\tLjP/N8DSfukX89nwdfeEw==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:zkj8eI9oiVc=:2l0/CdYw4ZwKO6ch78C1/q\n\tVZbjbwAxgKoW2bIr+avGb/qDWPxAJn1E+gTNxu2c3uBr4Bku9C7J+iIrKGPCbCfEm103wY2Od\n\t73a7UCnHbb35l8X0JUvtfViYlzWSjW2jYMp5rZFJx4KgntaQ/ZHzl6oY6mCPNKysJFmWY6ktL\n\tuVzdLWoXNA2U2K2TOLDkbNj+4+L21+xU+8VQe9AsKTIZVnAKXXAZCoVt2WjENWhFQb/XBhbnc\n\tkN3zlCioa1gFfHS80rich6dTodQQYJ2qPAcQkyX2XCL2f3A3QA3MTTc/7u7OUwJ/+s6Rlzknj\n\tWxM4vMgCsViKkbQXvn523jPnwIFqBPRi+iTIRj7EZfUC1mUDryFqM5tvGihCKd9TonIEUpsHB\n\tplGyM4TxgLGmUdlxlodTQS931qBnUNgJNAKwUNeZhENvklIICRzW8aoop6JZC5u6+dBaqw1rj\n\tZVR2dAAZHCOfhRHikz0k9tazWXBe54i2yHbqdjvJKCtUoIGcj9117Y7BeGjdEu68kAUyJlZw1\n\tR+DFTP5u7Cds8mu1vZwmadHdAL4z6mD8/bTt+EwS5/uTm6sqEKpAkK6J7tWNriiSk07ygrnCs\n\tO2s+/5miPotbwSkoo5zYIFHRChCe7IjwsYuQQG1UQ2FDMJY3ZD0NyNXq7H4kHHttkWtTlN6jt\n\tkKNTSeK+VxbVfpDxS+2iiJCsCpeb6UqjyVx9QUPvNSW4AhEo+qy1qotAOKEnYgZQvytwnZ8L0\n\toqcHbBklRAJDMjrqaYMTEh3fH2feE75oCEsPRcjrWRknH5aW1GWQ/Ng6/DYTiF2rPoVP5vH5P\n\tTQRHlhKyXlmT0/FkB3hLmymzooPJ/lpPH71soTIbM6UM2sknxBOjCwcLe3TZLXxGLserVGn9P\n\tUHb8EToSDLv+EhO13dosfufgmyj9l564A7Yk2nDOD6xtNZlxPrholGRZK8c/1tFJJlHQDE1Ey\n\tPrsFQelbsFIaNtYReb+inu8zzc59prfQH38qwqKr56x01zKePfFa0kudhQ8ri9Zo4zw0jAXZZ\n\tFfEL8xn+YTmvcDwXC2Jc0CjN+5De9icLS8yMOY1klt4WD9t5p9DN3TDcmDt0FwwbCGoPATRZs\n\t4k1gaYUC2yDXzo=","Subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","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","From":"Christian Rauch via libcamera-devel\n\t","Reply-To":"Christian Rauch ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22582,"web_url":"https://patchwork.libcamera.org/comment/22582/","msgid":"","date":"2022-04-05T17:17:14","subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Christian,\n\nOn Thu, Mar 31, 2022 at 12:14:30AM +0100, Christian Rauch wrote:\n> Dear Laurent,\n> \n> I initially overlooked your review of my patch because of the inline\n> response.\n\nNo worries :-)\n\n> There are a lot of questions raised about the definition and\n> interpretation of the limits and default values in the ControlInfo.\n> \n> The main purpose of this patch is to fully define the ControlInfo with\n> the same type and dimensionality as the Control it is defined for. The\n> reason for this is that otherwise, there is no information about the\n> dimensionality of a Control.\n> \n> I am going to answer your remaining questions inline below.\n> \n> Am 21.03.22 um 13:47 schrieb Laurent Pinchart:\n> > Hi Christian,\n> >\n> > Thank you for the patch, and sorry for the late reply.\n> >\n> > As Jacopo mentioned, patches should be sent inline and not as\n> > attachments, but I can still review this version.\n> >\n> > On Thu, Mar 17, 2022 at 02:48:47PM +0000, Christian Rauch via libcamera-devel wrote:\n> >> Hello,\n> >>\n> >> The attached patch fixes the ControlInfo access of Span Control (see\n> >> https://bugs.libcamera.org/show_bug.cgi?id=101).\n> >>\n> >> The ControlInfo for Spans was defined in scalar values (i.e. single\n> >> values instead of arrays). Thus, while a Control was defined as Span,\n> >> its corresponding ControlInfo was defined as scalar, causing access errors.\n> >>\n> >> Best,\n> >> Christian\n> >\n> >> From 13052bf04a0eccfc6a896bba47ae18c4ad8c6ebb Mon Sep 17 00:00:00 2001\n> >> From: Christian Rauch \n> >> Date: Thu, 17 Mar 2022 01:58:10 +0000\n> >> Subject: [PATCH 1/3] fix ControlInfo for Span Controls\n> >>\n> >> Some control properties are typed with a Span to store an array of values.\n> >> Currently those are ColourGains, SensorBlackLevels, ColourCorrectionMatrix\n> >> and FrameDurationLimits. The value range and defaults in the ControlInfo of\n> >> those Controls is currently defined as scalar. This prevents accessing the\n> >> ControlInfo via the native Span type.\n> >>\n> >> By defining the ControlInfo in terms of Spans, we can avoid this issue.\n> >> ---\n> >> include/libcamera/ipa/raspberrypi.h | 52 ++++++++++++-------\n> >> src/ipa/ipu3/ipu3.cpp | 6 +--\n> >> .../ipa_data_serializer_test.cpp | 8 +--\n> >> 3 files changed, 40 insertions(+), 26 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> >> index 7f705e49..fb5188a1 100644\n> >> --- a/include/libcamera/ipa/raspberrypi.h\n> >> +++ b/include/libcamera/ipa/raspberrypi.h\n> >> @@ -27,26 +27,38 @@ namespace RPi {\n> >> * and the pipeline handler may be reverted so that it aborts when an\n> >> * unsupported control is encountered.\n> >> */\n> >> -static const ControlInfoMap Controls({\n> >> -\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n> >> -\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> >> -\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> >> -\t\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> >> -\t\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> >> -\t\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> >> -\t\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> >> -\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> >> -\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> >> -\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> >> -\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> >> -\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> >> -\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> >> -\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> >> -\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> >> -\t\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> >> -\t\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> >> -\t\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> >> -\t}, controls::controls);\n> >> +static const ControlInfoMap Controls(\n> >> +\t{ { &controls::AeEnable, ControlInfo(false, true) },\n> >\n> > The change in indentation makes the patch difficult to review, as it\n> > mexes indentiation changes with other changes. If you think the\n> > formatting is wrong, please send a patch to fix that first, without any\n> > functional change, and a second patch with the functional changes. Both\n> > can be sent as part of the same series (git format-patch --cover-letter\n> > helps creating a series with a cover letter).\n> \n> Parts of this change is caused by the applied .clang-format style. I\n> will split those changes once I send a new version of the patches.\n\nPlease note that the .clang-format file tries to match our preferred\ncoding style as closely as possibly, but that's not a 100% match. There\nare exceptions that improve readability (at least in our opinion) and\ncan't be expressed in .clang-format (or we haven't found a way to do\nso). The output of checkstyle.py is thus informative, it doesn't have to\nbe followed 100% (especially when refactoring existing code, already\nexisting departure from what clang-format produces are likely\nintentional).\n\n> >> +\t { &controls::ExposureTime, ControlInfo(0, 999999) },\n> >> +\t { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> >> +\t { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> >> +\t { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> >> +\t { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> >> +\t { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n> >> +\t { &controls::AwbEnable, ControlInfo(false, true) },\n> >> +\t { &controls::ColourGains, ControlInfo{\n> >> +\t\t\t\t\t Span({ 0, 0 }),\n> >> +\t\t\t\t\t Span({ 32, 32 }),\n> >> +\t\t\t\t\t Span({ 0, 0 }),\n> >> +\t\t\t\t } },\n> >> +\t { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> >> +\t { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> >> +\t { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> >> +\t { &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> >> +\t { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> >> +\t { &controls::ColourCorrectionMatrix, ControlInfo{\n> >> +\t\t\t\t\t\t Span({ -16, -16, -16, -16, -16, -16, -16, -16, -16 }),\n> >> +\t\t\t\t\t\t Span({ 16, 16, 16, 16, 16, 16, 16, 16, 16 }),\n> >> +\t\t\t\t\t\t Span({ 1, 0, 0, 0, 1, 0, 0, 0, 1 }),\n> >\n> > We have a first issue here, if we were to define an array control that\n> > had, let's say, 256 elements, initialization would be very inconvenient.\n> > We probably need a bit of a better syntax for the common case where all\n> > values are equal. I also think it would be nice to avoid the Span cast,\n> > possibly by using initializer lists instead, but I'm not entirely sure\n> > how that would look like, and if it's actually doable.\n> \n> To initialise a large Span with the same values, we could implement a\n> function that just creates a Span given a length and a value.\n\nThat could possibly be a specialized ControlInfo constructor, but I'm\nnot sure it's possible to express all of our needs as different\nconstructor overloads that could be resolved without ambiguity. A\nseparate function could be an option too, but I'm not sure where it\nshould then live, as I'd like to avoid departing from the std::span API\nfor the Span class.\n\n> Initialising a ControlInfo via an initialiser list would still require\n> explicit casts. We should avoid casting every element like:\n> ControlInfo({int(1), int(2), int(3), ...}, {...}, {...}).\n\nThat would look awful indeed.\n\n> > There's also the question of how to handle arrays with a variable number\n> > of elements (although I don't think we have use cases for that yet, and\n> > maybe we never will).\n> \n> All those Spans are already variable-sized. They do not have a fixed\n> size. This is the main reason why I propose to initialise them with the\n> correct length. Otherwise, there is no user-visible information about\n> the dimensions of these spans. Information about the length of Spans is\n> only directly available via the \"control_ids.yaml\", but this information\n> is not used by the \"gen-controls.py\" script, and implicitly by reading\n> through the code.\n\nThat's right, we don't expose that information programmatically. That's\nmostly because we haven't had any need to do so yet :-) I'm certainly\ninterested in fixing that (thanks for your patch series that starts\nworking in that direction), although I'm not entirely sure what the use\ncases are. If the only tangible result is that compile-time type\nchecking is improved, that's already a win.\n\n> > The second issue, which is probably more important, is how we define\n> > minimum, default and maximum values for array controls. At the moment,\n> > we report those values for array elements, not for the whole array. Your\n> > patch shifts that to covering the whole array. For the default value,\n> > this allows expressing defaults where all elements are not identical,\n> > which is very nice. For the minimum and maximum values, however, it's a\n> > bit more complicated.\n> \n> While using scalar values makes the ControlInfo definition simpler, it\n> is not well defined and loses a lot of its meaning.\n> \n> Examples:\n> - The 3x3 ColourCorrectionMatrix by default should be an identity\n> mapping that does not apply a correction. But what does this mean in\n> terms of a scalar value? Would this be 0 (as in no mapping) or 1 (as in\n> identity)? Defining this as a 3x3 identity matrix makes this crystal\n> clear without any ambiguous formulation.\n\nI fully agree with you that, for default values, having a full array has\nvery interesting use cases (such as the one you mention here). It will\nincrease memory consumption for large arrays, but as long as we don't\ncopy ControlInfo instances, that's fine.\n\n> - The FrameDurationLimits stores a minimum and maximum frame duration\n> (i.e. the \"FrameDuration min/max\") between which a sensor can choose its\n> frame duration from. Those two values should not have the same\n> \"ControlInfo min/max\" limits as this would allow setting the\n> FrameDurationLimits min and max to the same values.\n\nActually that's useful, to express fixed frame rates.\n\n> Also semantically, I think that the value range of a control property\n> should have the same dimensionality as the control property itself.\n\nI don't disagree. I still think minimum and maximum are quite\nill-defined in that case (as discussed below), but they're also\nill-defined when expressed as scalars.\n\n> > How do we define minimum and maximum values for an array ? Is it per\n> > element, or do we introduce a concept of minimum and maximum values for\n> > the array as a whole ? The latter is likely quite ill-defined, but the\n> > former isn't without issues either. If the minimum and maximum values\n> > for each element are taken in isolation, the array minimum and maximum\n> > in the ControlInfo may not be valid values for the control. For\n> > instance, we could have an array control with two elements, where each\n> > element can be between -16 and 16, but with a requirement that the sum\n> > of the elements is always equal to 0. The minimum and maximum, could\n> > then be { -16, -16 } and { 16, 16 }, but neither would be value values\n> > for the control.\n> \n> This case (\"sum of elements is equal to 0\") could also not be handled by\n> the current definitions. Those cases that cannot be expressed by lower\n> and upper bounds must be handled \"somewhere\" else. There are already\n> configurations, such as setting an \"ExposureValue\" while\n> \"AeExposureMode\" is on, which cannot be handled by the current API. If\n> this invalid configuration state happens, the request is currently just\n> cancelled.\n> \n> > This is likely unsolvable in a generic manner, so I think we need to\n> > focus on the main use case(s), and see what applications using this API\n> > need. The documentation for ControlInfo should be updated accordingly,\n> > to clarify what minimum, default and maximum mean for array controls.\n> \n> I agree that a lot of those questions cannot be answered right now, and\n> they are definitely out-of-scope of this patch.\n\nWe seem to agree, good :-)\n\n> >> +\t\t\t\t\t } },\n> >> +\t { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> >> +\t { &controls::FrameDurationLimits, ControlInfo{\n> >> +\t\t\t\t\t\t Span({ 1000, 1000 }),\n> >> +\t\t\t\t\t\t Span({ 1000000000, 1000000000 }),\n> >> +\t\t\t\t\t\t Span({ 1000, 1000 }),\n> >> +\t\t\t\t\t } },\n> >> +\t { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) } },\n> >> +\tcontrols::controls);\n> >>\n> >> } /* namespace RPi */\n> >>\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index 1ea2c898..e64fc2bb 100644\n> >> --- a/src/ipa/ipu3/ipu3.cpp\n> >> +++ b/src/ipa/ipu3/ipu3.cpp\n> >> @@ -267,9 +267,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,\n> >> \t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n> >> \t}\n> >>\n> >> -\tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n> >> -\t\t\t\t\t\t\t frameDurations[1],\n> >> -\t\t\t\t\t\t\t frameDurations[2]);\n> >> +\tcontrols[&controls::FrameDurationLimits] = ControlInfo{ Span({ frameDurations[0], frameDurations[0] }),\n> >> +\t\t\t\t\t\t\t\tSpan({ frameDurations[1], frameDurations[1] }),\n> >> +\t\t\t\t\t\t\t\tSpan({ frameDurations[2], frameDurations[2] }) };\n> >>\n> >> \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n> >> }\n> >> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp\n> >> index d2050a86..5503cc8a 100644\n> >> --- a/test/serialization/ipa_data_serializer_test.cpp\n> >> +++ b/test/serialization/ipa_data_serializer_test.cpp\n> >> @@ -32,13 +32,15 @@\n> >> using namespace std;\n> >> using namespace libcamera;\n> >>\n> >> -static const ControlInfoMap Controls = ControlInfoMap({\n> >> +static const ControlInfoMap Controls = ControlInfoMap(\n> >> +\t{\n> >> \t\t{ &controls::AeEnable, ControlInfo(false, true) },\n> >> \t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> >> \t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> >> -\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> >> +\t\t{ &controls::ColourGains, ControlInfo{ Span({ 0, 0 }), Span({ 32, 32 }), Span({ 0, 0 }) } },\n> >> \t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n> >> -\t}, controls::controls);\n> >> +\t},\n> >> +\tcontrols::controls);\n> >>\n> >> namespace libcamera {\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 8A9D8C3256\n\tfor ;\n\tTue, 5 Apr 2022 17:17:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC7DA6563F;\n\tTue, 5 Apr 2022 19:17:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2145C604BB\n\tfor ;\n\tTue, 5 Apr 2022 19:17:18 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9C9E05D;\n\tTue, 5 Apr 2022 19:17:17 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649179039;\n\tbh=aB7SjBFlrDjkhG33Kh7S/63bjQa226rhwkhMidgmtdE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=OkxnLPvv5Q9X0Tt+Q1QuRSFEHVvG+baPPE9sAG7SEJNF7N8sLsk9g3ZYXAbJDFnE5\n\tpNHoX0S7vVDIYtpiJbxEuu4Y6yEmkxvKKUCRBLqFRb1A1NkbcKT4rgS/fV6wDYEgM7\n\tTA8CeCkxuBx7BTtqzRYjfy688Lbm9oOHt/YpGRsR3XmH68yrVBh0GpyyS7ENDnZkZo\n\tfkjBpw/3xWno5XlPbUNWcAqsvgADOhk1Z3m4rRELs49yTycMQsG6y+yk2YviGMh9PG\n\tCM+1qakldbv70rmF5388dd9DA4iCmyRZrrtY/1h2lRMbULdZsLvtAiiog40gTSLAWk\n\tPfzkCb/xfIPWQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649179037;\n\tbh=aB7SjBFlrDjkhG33Kh7S/63bjQa226rhwkhMidgmtdE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YazhhNhpBkzJS6P9iwzogfsyAwJd+sTiWBN5bxkh2hFkc+wRAZLo03zDNwtZ3hCZf\n\tqR2G6OHlIRtT6a3JlrdNTPO/h8tVkZnru6jwejxkeg18Tr4ao3vg6PEDesCa1iBUjk\n\tY8O34XqWn/Ua/eGaySlCr2R/2bWaprttxDE5RO3g="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YazhhNhp\"; dkim-atps=neutral","Date":"Tue, 5 Apr 2022 20:17:14 +0300","To":"Christian Rauch ","Message-ID":"","References":"\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22636,"web_url":"https://patchwork.libcamera.org/comment/22636/","msgid":"<39e16507-2622-9976-ff56-eaf2b787eecf@gmx.de>","date":"2022-04-06T23:48:12","subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi Laurent,\n\nTo give you a bit of background on my two patch sets:\n\nI am interested in fully specifying the Control dimensions via the\nuser-visible API to present all Controls in a generic way to a user. I\ncould hard code the dimensions of control on my side, but this would not\nscale if new controls are added or the control types change.\nEssentially, I want to iterate over the \"Camera::controls()\" and present\nall controls to a user. In order to let a user choose values and set\nthem on the camera, I have to know the full control type (scalar type,\nSpan, size). Currently, this information is not available via the API.\n\nI also left a couple of inline comments below.\n\n\nAm 05.04.22 um 18:17 schrieb Laurent Pinchart:\n> Hi Christian,\n>\n> On Thu, Mar 31, 2022 at 12:14:30AM +0100, Christian Rauch wrote:\n>> Dear Laurent,\n>>\n>> I initially overlooked your review of my patch because of the inline\n>> response.\n>\n> No worries :-)\n>\n>> There are a lot of questions raised about the definition and\n>> interpretation of the limits and default values in the ControlInfo.\n>>\n>> The main purpose of this patch is to fully define the ControlInfo with\n>> the same type and dimensionality as the Control it is defined for. The\n>> reason for this is that otherwise, there is no information about the\n>> dimensionality of a Control.\n>>\n>> I am going to answer your remaining questions inline below.\n>>\n>> Am 21.03.22 um 13:47 schrieb Laurent Pinchart:\n>>> Hi Christian,\n>>>\n>>> Thank you for the patch, and sorry for the late reply.\n>>>\n>>> As Jacopo mentioned, patches should be sent inline and not as\n>>> attachments, but I can still review this version.\n>>>\n>>> On Thu, Mar 17, 2022 at 02:48:47PM +0000, Christian Rauch via libcamera-devel wrote:\n>>>> Hello,\n>>>>\n>>>> The attached patch fixes the ControlInfo access of Span Control (see\n>>>> https://bugs.libcamera.org/show_bug.cgi?id=101).\n>>>>\n>>>> The ControlInfo for Spans was defined in scalar values (i.e. single\n>>>> values instead of arrays). Thus, while a Control was defined as Span,\n>>>> its corresponding ControlInfo was defined as scalar, causing access errors.\n>>>>\n>>>> Best,\n>>>> Christian\n>>>\n>>>> From 13052bf04a0eccfc6a896bba47ae18c4ad8c6ebb Mon Sep 17 00:00:00 2001\n>>>> From: Christian Rauch \n>>>> Date: Thu, 17 Mar 2022 01:58:10 +0000\n>>>> Subject: [PATCH 1/3] fix ControlInfo for Span Controls\n>>>>\n>>>> Some control properties are typed with a Span to store an array of values.\n>>>> Currently those are ColourGains, SensorBlackLevels, ColourCorrectionMatrix\n>>>> and FrameDurationLimits. The value range and defaults in the ControlInfo of\n>>>> those Controls is currently defined as scalar. This prevents accessing the\n>>>> ControlInfo via the native Span type.\n>>>>\n>>>> By defining the ControlInfo in terms of Spans, we can avoid this issue.\n>>>> ---\n>>>> include/libcamera/ipa/raspberrypi.h | 52 ++++++++++++-------\n>>>> src/ipa/ipu3/ipu3.cpp | 6 +--\n>>>> .../ipa_data_serializer_test.cpp | 8 +--\n>>>> 3 files changed, 40 insertions(+), 26 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n>>>> index 7f705e49..fb5188a1 100644\n>>>> --- a/include/libcamera/ipa/raspberrypi.h\n>>>> +++ b/include/libcamera/ipa/raspberrypi.h\n>>>> @@ -27,26 +27,38 @@ namespace RPi {\n>>>> * and the pipeline handler may be reverted so that it aborts when an\n>>>> * unsupported control is encountered.\n>>>> */\n>>>> -static const ControlInfoMap Controls({\n>>>> -\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n>>>> -\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n>>>> -\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n>>>> -\t\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n>>>> -\t\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n>>>> -\t\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n>>>> -\t\t{ &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n>>>> -\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n>>>> -\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>>>> -\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n>>>> -\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n>>>> -\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n>>>> -\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n>>>> -\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n>>>> -\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n>>>> -\t\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n>>>> -\t\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n>>>> -\t\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n>>>> -\t}, controls::controls);\n>>>> +static const ControlInfoMap Controls(\n>>>> +\t{ { &controls::AeEnable, ControlInfo(false, true) },\n>>>\n>>> The change in indentation makes the patch difficult to review, as it\n>>> mexes indentiation changes with other changes. If you think the\n>>> formatting is wrong, please send a patch to fix that first, without any\n>>> functional change, and a second patch with the functional changes. Both\n>>> can be sent as part of the same series (git format-patch --cover-letter\n>>> helps creating a series with a cover letter).\n>>\n>> Parts of this change is caused by the applied .clang-format style. I\n>> will split those changes once I send a new version of the patches.\n>\n> Please note that the .clang-format file tries to match our preferred\n> coding style as closely as possibly, but that's not a 100% match. There\n> are exceptions that improve readability (at least in our opinion) and\n> can't be expressed in .clang-format (or we haven't found a way to do\n> so). The output of checkstyle.py is thus informative, it doesn't have to\n> be followed 100% (especially when refactoring existing code, already\n> existing departure from what clang-format produces are likely\n> intentional).\n\nI am going to drop these changes in a future version of this patch.\n\n>\n>>>> +\t { &controls::ExposureTime, ControlInfo(0, 999999) },\n>>>> +\t { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n>>>> +\t { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n>>>> +\t { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n>>>> +\t { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n>>>> +\t { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },\n>>>> +\t { &controls::AwbEnable, ControlInfo(false, true) },\n>>>> +\t { &controls::ColourGains, ControlInfo{\n>>>> +\t\t\t\t\t Span({ 0, 0 }),\n>>>> +\t\t\t\t\t Span({ 32, 32 }),\n>>>> +\t\t\t\t\t Span({ 0, 0 }),\n>>>> +\t\t\t\t } },\n>>>> +\t { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n>>>> +\t { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n>>>> +\t { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n>>>> +\t { &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n>>>> +\t { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n>>>> +\t { &controls::ColourCorrectionMatrix, ControlInfo{\n>>>> +\t\t\t\t\t\t Span({ -16, -16, -16, -16, -16, -16, -16, -16, -16 }),\n>>>> +\t\t\t\t\t\t Span({ 16, 16, 16, 16, 16, 16, 16, 16, 16 }),\n>>>> +\t\t\t\t\t\t Span({ 1, 0, 0, 0, 1, 0, 0, 0, 1 }),\n>>>\n>>> We have a first issue here, if we were to define an array control that\n>>> had, let's say, 256 elements, initialization would be very inconvenient.\n>>> We probably need a bit of a better syntax for the common case where all\n>>> values are equal. I also think it would be nice to avoid the Span cast,\n>>> possibly by using initializer lists instead, but I'm not entirely sure\n>>> how that would look like, and if it's actually doable.\n>>\n>> To initialise a large Span with the same values, we could implement a\n>> function that just creates a Span given a length and a value.\n>\n> That could possibly be a specialized ControlInfo constructor, but I'm\n> not sure it's possible to express all of our needs as different\n> constructor overloads that could be resolved without ambiguity. A\n> separate function could be an option too, but I'm not sure where it\n> should then live, as I'd like to avoid departing from the std::span API\n> for the Span class.\n\nI think the \"std::span\" class also works with \"std::array\". A custom\nfunction could just create a std::array with identical values.\n\n>\n>> Initialising a ControlInfo via an initialiser list would still require\n>> explicit casts. We should avoid casting every element like:\n>> ControlInfo({int(1), int(2), int(3), ...}, {...}, {...}).\n>\n> That would look awful indeed.\n>\n>>> There's also the question of how to handle arrays with a variable number\n>>> of elements (although I don't think we have use cases for that yet, and\n>>> maybe we never will).\n>>\n>> All those Spans are already variable-sized. They do not have a fixed\n>> size. This is the main reason why I propose to initialise them with the\n>> correct length. Otherwise, there is no user-visible information about\n>> the dimensions of these spans. Information about the length of Spans is\n>> only directly available via the \"control_ids.yaml\", but this information\n>> is not used by the \"gen-controls.py\" script, and implicitly by reading\n>> through the code.\n>\n> That's right, we don't expose that information programmatically. That's\n> mostly because we haven't had any need to do so yet :-) I'm certainly\n> interested in fixing that (thanks for your patch series that starts\n> working in that direction), although I'm not entirely sure what the use\n> cases are. If the only tangible result is that compile-time type\n> checking is improved, that's already a win.\n\nSetting the ControlInfo with the full type (Span + size) is one way to\ninform the user about the expected Control dimensions. I am not sure\nthose can be checked at compile time. For a compile-time check, my other\npatch series is required.\n\n>\n>>> The second issue, which is probably more important, is how we define\n>>> minimum, default and maximum values for array controls. At the moment,\n>>> we report those values for array elements, not for the whole array. Your\n>>> patch shifts that to covering the whole array. For the default value,\n>>> this allows expressing defaults where all elements are not identical,\n>>> which is very nice. For the minimum and maximum values, however, it's a\n>>> bit more complicated.\n>>\n>> While using scalar values makes the ControlInfo definition simpler, it\n>> is not well defined and loses a lot of its meaning.\n>>\n>> Examples:\n>> - The 3x3 ColourCorrectionMatrix by default should be an identity\n>> mapping that does not apply a correction. But what does this mean in\n>> terms of a scalar value? Would this be 0 (as in no mapping) or 1 (as in\n>> identity)? Defining this as a 3x3 identity matrix makes this crystal\n>> clear without any ambiguous formulation.\n>\n> I fully agree with you that, for default values, having a full array has\n> very interesting use cases (such as the one you mention here). It will\n> increase memory consumption for large arrays, but as long as we don't\n> copy ControlInfo instances, that's fine.\n>\n>> - The FrameDurationLimits stores a minimum and maximum frame duration\n>> (i.e. the \"FrameDuration min/max\") between which a sensor can choose its\n>> frame duration from. Those two values should not have the same\n>> \"ControlInfo min/max\" limits as this would allow setting the\n>> FrameDurationLimits min and max to the same values.\n>\n> Actually that's useful, to express fixed frame rates.\n\nSetting the limits to the same value will definitely force any \"auto\nmode\" to select the specified frame duration. But for this specific use\ncase, there is already the \"FrameDuration\" control. It's a bit\nconfusing, but my interpreation is now that the min/max limits of\ncontrols are for users, e.g. a user can chose a fixed \"FrameDuration\"\nwithin the controls min/max bounds, but the \"FrameDurationLimits\" are\nuser chosen bounds another algorithm (e.g. auto exposure) can chose\nfrom, where the min/max bounds define the \"physical\" sensor limits.\n\n>\n>> Also semantically, I think that the value range of a control property\n>> should have the same dimensionality as the control property itself.\n>\n> I don't disagree. I still think minimum and maximum are quite\n> ill-defined in that case (as discussed below), but they're also\n> ill-defined when expressed as scalars.\n>\n>>> How do we define minimum and maximum values for an array ? Is it per\n>>> element, or do we introduce a concept of minimum and maximum values for\n>>> the array as a whole ? The latter is likely quite ill-defined, but the\n>>> former isn't without issues either. If the minimum and maximum values\n>>> for each element are taken in isolation, the array minimum and maximum\n>>> in the ControlInfo may not be valid values for the control. For\n>>> instance, we could have an array control with two elements, where each\n>>> element can be between -16 and 16, but with a requirement that the sum\n>>> of the elements is always equal to 0. The minimum and maximum, could\n>>> then be { -16, -16 } and { 16, 16 }, but neither would be value values\n>>> for the control.\n>>\n>> This case (\"sum of elements is equal to 0\") could also not be handled by\n>> the current definitions. Those cases that cannot be expressed by lower\n>> and upper bounds must be handled \"somewhere\" else. There are already\n>> configurations, such as setting an \"ExposureValue\" while\n>> \"AeExposureMode\" is on, which cannot be handled by the current API. If\n>> this invalid configuration state happens, the request is currently just\n>> cancelled.\n>>\n>>> This is likely unsolvable in a generic manner, so I think we need to\n>>> focus on the main use case(s), and see what applications using this API\n>>> need. The documentation for ControlInfo should be updated accordingly,\n>>> to clarify what minimum, default and maximum mean for array controls.\n>>\n>> I agree that a lot of those questions cannot be answered right now, and\n>> they are definitely out-of-scope of this patch.\n>\n> We seem to agree, good :-)\n>\n>>>> +\t\t\t\t\t } },\n>>>> +\t { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n>>>> +\t { &controls::FrameDurationLimits, ControlInfo{\n>>>> +\t\t\t\t\t\t Span({ 1000, 1000 }),\n>>>> +\t\t\t\t\t\t Span({ 1000000000, 1000000000 }),\n>>>> +\t\t\t\t\t\t Span({ 1000, 1000 }),\n>>>> +\t\t\t\t\t } },\n>>>> +\t { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) } },\n>>>> +\tcontrols::controls);\n>>>>\n>>>> } /* namespace RPi */\n>>>>\n>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>> index 1ea2c898..e64fc2bb 100644\n>>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>>> @@ -267,9 +267,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,\n>>>> \t\tframeDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);\n>>>> \t}\n>>>>\n>>>> -\tcontrols[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],\n>>>> -\t\t\t\t\t\t\t frameDurations[1],\n>>>> -\t\t\t\t\t\t\t frameDurations[2]);\n>>>> +\tcontrols[&controls::FrameDurationLimits] = ControlInfo{ Span({ frameDurations[0], frameDurations[0] }),\n>>>> +\t\t\t\t\t\t\t\tSpan({ frameDurations[1], frameDurations[1] }),\n>>>> +\t\t\t\t\t\t\t\tSpan({ frameDurations[2], frameDurations[2] }) };\n>>>>\n>>>> \t*ipaControls = ControlInfoMap(std::move(controls), controls::controls);\n>>>> }\n>>>> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp\n>>>> index d2050a86..5503cc8a 100644\n>>>> --- a/test/serialization/ipa_data_serializer_test.cpp\n>>>> +++ b/test/serialization/ipa_data_serializer_test.cpp\n>>>> @@ -32,13 +32,15 @@\n>>>> using namespace std;\n>>>> using namespace libcamera;\n>>>>\n>>>> -static const ControlInfoMap Controls = ControlInfoMap({\n>>>> +static const ControlInfoMap Controls = ControlInfoMap(\n>>>> +\t{\n>>>> \t\t{ &controls::AeEnable, ControlInfo(false, true) },\n>>>> \t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n>>>> \t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n>>>> -\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>>>> +\t\t{ &controls::ColourGains, ControlInfo{ Span({ 0, 0 }), Span({ 32, 32 }), Span({ 0, 0 }) } },\n>>>> \t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f) },\n>>>> -\t}, controls::controls);\n>>>> +\t},\n>>>> +\tcontrols::controls);\n>>>>\n>>>> namespace libcamera {\n>>>>\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 4B3FDC3256\n\tfor ;\n\tWed, 6 Apr 2022 23:48:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 02A5E6563F;\n\tThu, 7 Apr 2022 01:48:16 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.17.21])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D336B604BB\n\tfor ;\n\tThu, 7 Apr 2022 01:48:13 +0200 (CEST)","from [192.168.1.209] ([92.10.251.63]) by mail.gmx.net (mrgmx105\n\t[212.227.17.168]) with ESMTPSA (Nemesis) id 1MoO6C-1oMZXg3Y5I-00omWl\n\tfor\n\t; Thu, 07 Apr 2022 01:48:12 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649288896;\n\tbh=8D7BTX+AIvIkxXxi5IjpB5mhmgWPqvtwd+Kv/RO0Ubg=;\n\th=Date:Cc:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=dg/Nm8aGCK47SrjIEGz1dsoFTMMm2HFfbJ39Gh49BFteZjXDot3a3FLs1YcYCuPQX\n\tJvfRjVk5LWEoeMeDXzW7/jciHSO46YKOA4gIJU+a2YTrwVPpGkvbyKMS3pIMe4c1v0\n\tMI0K4XYXBvnzpAMgY2PEibmmCC4g2idv5Lt+s2pGuiiM9pBV0CAjQfg7vK64HRLgNo\n\t4P7xmtv5Tk+H9VBRZLakweBjYyYZAL8D4hlF2KAxKtXkHxGsi10J9YtY6wNrdrkfkH\n\tiJN/6o0nu8IktRZkXOW0Wgw60A16dojvLhkYpTRlTrN0pAp4KBecWFFolkEUSJZcrH\n\t5euGe3P72RXSA==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1649288893;\n\tbh=8D7BTX+AIvIkxXxi5IjpB5mhmgWPqvtwd+Kv/RO0Ubg=;\n\th=X-UI-Sender-Class:Date:Subject:Cc:References:From:In-Reply-To;\n\tb=JDSk6qwOSCU05ozZuPZ0U4/WtHUq5Qp9wakGo5pxfVhb7dojVICJw4W8TCESwjPRw\n\tOu8NB/8PgKobVDmeVxCqG1WETR4fKe8N5kUxKau6lismPjAG1qfK4etg/UyS6kfADX\n\t1STrj+RjYvqvgtgy6PgEUs5HUKCzh/ccKUjipaWo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"JDSk6qwO\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"<39e16507-2622-9976-ff56-eaf2b787eecf@gmx.de>","Date":"Thu, 7 Apr 2022 00:48:12 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.7.0","Content-Language":"en-GB","Cc":"libcamera-devel@lists.libcamera.org","References":"\n\t\n\t\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:3JBWkHcbHzdroN1+zTKZusb/JxEzEFjWVBIvK9teYoGC/0AHQeE\n\ttOUz4/Kz1fA8Qx95uVPOz8WW4hLIEZDc+rxvt1ySDVIPGOiahvl4gGzMk4t9UAX8SilLGOx\n\tMt9p7AVAyYPOW1n/jFwcGnBvMHgNmxXSVD9yNEm/CzeqFGArMg+Js5Zn3aFGufspwQ42mTA\n\t4mfLBZOvSv9GZ2oGoUOhg==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:VDGWXgXK4u4=:+zdzLtVvTlAYtshbqEMpWl\n\t8+HEXHA338WqDHPCmj7r3+h4m5USCgogL6Rc5A8xPDjzTxA6rGahQdu0BzXZrzvE7/Ys6rurc\n\tOlCRjq0Wm404feY+Jgv6Nu0VxN+iWYdYsisZjrPv/7XJY0qrM3E1FFkbmZDEYyxjWRsVh7Z/X\n\trV7lp2Zn8s+DBs5Q1GUEYXxocBFsBJFl20GZ+ZK1YO3BOIbtzgweIs33/fQwF76zM57c6qSwu\n\t5BWs/gQE9DaKMa+w9GsBA7ifvkJNJBLAN/4b5K289WCZv/AtMDonNKmn9sz/ar0gLeMjnXmZi\n\tuNiLBjjl/lBKVnMq2e0HSYrWPYVpvw3E9gNlOCtUXjR2+zWEsQ3MtxRxxavCfQXCRXdpPZvIT\n\tCzjyyILbRQx7XOIL2yIO+7Np5SoK8/P+z2C/5Th+4WwCqMljNdZwShwwjXsXXc2RyRAf4CqIL\n\tmPI+enTPt12H618l4s+p1OcgdIbeWbOqCNL9+UW5/sbJLEJ9wcXIfK61KvCo2Vs+G7XNbvfpl\n\tBbcUPO7WIVanbkG5EEy2B9ddec5uEaIGT1kBSPwAPnhhpnH8q55QjGNE/QsPe+3bhiAMv6VtF\n\thH3n4AEG3Zk+khMmPJtivplFxWW9d2d4+vVxGgooXaknN1oxOhABU5Fp9r439TdcedsHdSPPx\n\tmatq0n4QkIA59Pq/RZRJO0CjE9HBjctQxEcLAVqEfwc1qZW5nJNXN7SosOfeQIWZTzh9nVA65\n\tVgecegeTI5t0undRnNtO0rM6ldxrAo7RpmD4gY7GbHA8KXDBnQGzbVcSPDLO3MiqhidnRTYWE\n\tVcnl8a5biCnFOxL1PxP4GZ5RY0jqeMblX2uN0B/2DKAhTWYD53XBxfnRTGxzkPYkNz/VBsXg9\n\tqpfxqTHUfT3QNmtJjXBq1adqJ3MFxXyK0jU3Wy93JYSQotSa/z7jvV/x59tJAomlD4g6BktVS\n\taCiZ2TEhraLLHNHgpFKL4X1Gl8s9ohLn1Yn9wO5rdS1c1an81lNdosZqYrGlKCq1523YsWzK1\n\tXp+eaSzRl/FaLY9d2R1779qmX7XZ0CisbdT66HDSPcGvjvX2wIFQLRrDTmBC06uioXuJIKjDP\n\tu1GEYDNCPwNi4I=","Subject":"Re: [libcamera-devel] fix ControlInfo for Span Controls","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","From":"Christian Rauch via libcamera-devel\n\t","Reply-To":"Christian Rauch ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]