[{"id":24730,"web_url":"https://patchwork.libcamera.org/comment/24730/","msgid":"<166120512201.3484129.4999114544766245166@Monstersaurus>","date":"2022-08-22T21:52:02","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)\n> Explicitly set the default value for all ControlInfo to 0, false or minimum.\n> \n> Signed-off-by: Christian Rauch \n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 22 ++++++++++---------\n> src/ipa/rkisp1/rkisp1.cpp | 10 ++++-----\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> 4 files changed, 19 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 69c73f8c..c6360a51 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> \n> /* List of controls handled by the Raspberry Pi IPA */\n> static const ControlInfoMap::Map ipaControls{\n> - { &controls::AeEnable, ControlInfo(false, true) },\n> - { &controls::ExposureTime, ControlInfo(0, 66666) },\n> - { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n> + { &controls::AeEnable, ControlInfo(false, true, false) },\n\nI would expect most of the time AutoExposure would be enabled ... so a\ndefault of false seems misleading... Yet - perhaps that's what the 0 was\nalready implying? So - does that mean a default is unreasonable here?\n\n\n> + { &controls::ExposureTime, ControlInfo(0, 66666, 0) },\n> + { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },\n\nThis default is less than the minimum.\n\n\n> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> - { &controls::AwbEnable, ControlInfo(false, true) },\n> - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n\nSame as AeEnable.... it doesn't seem reasonble to say the default is\ndisabled? I think applications probably expect the default IPA behaviour\nto have AE/AWB enabled, unless it is disabled explicitly ?\n\n\n> + { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },\n> { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },\n> { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> - { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n> + { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },\n\nAgain, this is less than the minimum. A default should probably be\nspecific to the mode of the sensor too ?\n\n\n> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> };\n> \n> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;\n> ctrlMap[&controls::FrameDurationLimits] =\n> ControlInfo(static_cast(minSensorFrameDuration.get()),\n> - static_cast(maxSensorFrameDuration.get()));\n> + static_cast(maxSensorFrameDuration.get()),\n> + int64_t(0));\n\nThis is smaller than the minimum. Perhaps at least here during configure\nit could be set to something more specific about the camera mode\ncapability - or something that is more 'default' like 30 FPS ? (33333 ?)\n\n> \n> ctrlMap[&controls::AnalogueGain] =\n> - ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)));\n> + ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)), 0.0f);\n\nLess than minimum. I think 1.0f makes sense here too as that's unity\ngain.\n\n> \n> /*\n> * Calculate the max exposure limit from the frame duration limit as V4L2\n> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> \n> ctrlMap[&controls::ExposureTime] =\n> ControlInfo(static_cast(helper_->exposure(exposureMin).get()),\n> - static_cast(maxShutter.get()));\n> + static_cast(maxShutter.get()),\n> + int32_t(0));\n\nI'm not sure an exposure time of 0 makes sense ?, but I'm not sure what\na reasonable default is here either. Maybe something like the maximum of the frame\nduration, and maxShutter ?\n\n> \n> result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> return 0;\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 27b4212b..2121bfd2 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -91,12 +91,12 @@ namespace {\n> \n> /* List of controls handled by the RkISP1 IPA */\n> const ControlInfoMap::Map rkisp1Controls{\n> - { &controls::AeEnable, ControlInfo(false, true) },\n> - { &controls::AwbEnable, ControlInfo(false, true) },\n> + { &controls::AeEnable, ControlInfo(false, true, false) },\n> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n\nSame issue as in RPi.\n\n\n> { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },\n> - { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },\n> - { &controls::Contrast, ControlInfo(0.0f, 1.993f) },\n> - { &controls::Saturation, ControlInfo(0.0f, 1.993f) },\n> + { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },\n> + { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },\n> + { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },\n\nI think 0.0 makes sense here though\n\n> { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> };\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 9df24603..a1bcfe49 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -42,7 +42,7 @@ namespace libcamera {\n> LOG_DEFINE_CATEGORY(IPU3)\n> \n> static const ControlInfoMap::Map IPU3Controls = {\n> - { &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n> + { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },\n\n\nThis needs looking at. The PipelineDepth isn't something the application\ncan set. So a default doesn't makes sense, and I'm not sure a min/max\ndoes either.\n\n\n\n> };\n> \n> class IPU3CameraData : public Camera::Private\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index e895584d..8fd7634d 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> /* Add the ScalerCrop control limits based on the current mode. */\n> Rectangle ispMinCrop(data->ispMinCropSize_);\n> ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);\n> - ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));\n> + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);\n\nI think a ScalerCrop would probably expect a default of the largest size\npossible, not the smallest ? (At least if I were to think about\n'resetting' the ScalerCrop that is).\n\nI suspect forcing a default value might be better than implicitly\nleaving it zero - but it means we need to give more thought to what the\ndefaults mean or represent - or provide a way for the default to be\n'unset' if it's not appropriate to have one.\n\n--\nKieran\n\n\n> \n> data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n> \n> --\n> 2.34.1\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 CDE18C3272\n\tfor ;\n\tMon, 22 Aug 2022 21:52:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E100861FC0;\n\tMon, 22 Aug 2022 23:52:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8679E61FBB\n\tfor ;\n\tMon, 22 Aug 2022 23:52:04 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0C0222B3;\n\tMon, 22 Aug 2022 23:52:04 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661205126;\n\tbh=BOTSj6W5A1NYitbziWDPFEmQbCOWByB5UZUu3WBvgaU=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=KvjWJkN2CLQ59vOo8u8j1cGRTcs1ea54+B5CEPpGWklz+C9OZFROpf7FjAkEijDGN\n\tVPMlRs52ZRzcKnrCzagXnrQJMf3/vPpBU83MT+9w95pKtGHwpAJ4YxQNH0UgvfzLFO\n\tWs+57DEwA6OqqrGaMMYB2+tskAch+qL1ZD6fvh5utIsmRc5QXMsa/43LtokXBohYB5\n\tLpIx9A2ajAQr6MogThSdHy0mKtGE3dvwKnz1B0PuLntmBTksr4h8dU1HFetc2tWQMR\n\tgodtzece29jOoIBg/GqCRo5tmHgj+ztN7fDil9Vxv+FM4uuJjIWoSLR4rGbjL1aw4C\n\tFU3Dao1qicQ5A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661205124;\n\tbh=BOTSj6W5A1NYitbziWDPFEmQbCOWByB5UZUu3WBvgaU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=oYVVY3RkgV947N9ukQ4gEqhvGWKyKzNTmDUgKsI1dPTG8ew2NXHywVmF0zQRgI5uz\n\tT8EfSVMEsabfK1biQgrlSle8EkF3wKnpi7WkAw5iCnrp3srwQL9avjh5+Y4zbEFHMk\n\tH807VJbTMk4DDYTYSRw+RD3+Fip0/f1x8yJUu7q0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"oYVVY3Rk\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220821124343.487963-2-Rauch.Christian@gmx.de>","References":"<20220821124343.487963-1-Rauch.Christian@gmx.de>\n\t<20220821124343.487963-2-Rauch.Christian@gmx.de>","To":"Christian Rauch ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 22 Aug 2022 22:52:02 +0100","Message-ID":"<166120512201.3484129.4999114544766245166@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24731,"web_url":"https://patchwork.libcamera.org/comment/24731/","msgid":"<4868aa82-33e8-7492-0e83-b50bb9b66d48@gmx.de>","date":"2022-08-22T22:23:45","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi Kieran,\n\nRegarding all your comments, I simply replaced the \"default\" 0 for \"def\"\nfrom the old constructor with explicitly setting it 0 (or its binary\ncounterpart 'false') to match the types. If 0 is the wrong \"def\" value\nfor those controls, then they were already wrong before :-) They were\njust implicitly wrong. Now you have to explicitly specify a \"def\" value,\nand I kept 0 for backward compatibility.\n\nFeel free to change the default values. You should also be able to merge\nthis patch (2/2) without the first (1/2).\n\nBest,\nChristian\n\n\nAm 22.08.22 um 23:52 schrieb Kieran Bingham:\n> Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)\n>> Explicitly set the default value for all ControlInfo to 0, false or minimum.\n>>\n>> Signed-off-by: Christian Rauch \n>> ---\n>> src/ipa/raspberrypi/raspberrypi.cpp | 22 ++++++++++---------\n>> src/ipa/rkisp1/rkisp1.cpp | 10 ++++-----\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n>> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n>> 4 files changed, 19 insertions(+), 17 deletions(-)\n>>\n>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>> index 69c73f8c..c6360a51 100644\n>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n>>\n>> /* List of controls handled by the Raspberry Pi IPA */\n>> static const ControlInfoMap::Map ipaControls{\n>> - { &controls::AeEnable, ControlInfo(false, true) },\n>> - { &controls::ExposureTime, ControlInfo(0, 66666) },\n>> - { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n>> + { &controls::AeEnable, ControlInfo(false, true, false) },\n>\n> I would expect most of the time AutoExposure would be enabled ... so a\n> default of false seems misleading... Yet - perhaps that's what the 0 was\n> already implying? So - does that mean a default is unreasonable here?\n>\n>\n>> + { &controls::ExposureTime, ControlInfo(0, 66666, 0) },\n>> + { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },\n>\n> This default is less than the minimum.\n>\n>\n>> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n>> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n>> { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n>> { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n>> - { &controls::AwbEnable, ControlInfo(false, true) },\n>> - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n>\n> Same as AeEnable.... it doesn't seem reasonble to say the default is\n> disabled? I think applications probably expect the default IPA behaviour\n> to have AE/AWB enabled, unless it is disabled explicitly ?\n>\n>\n>> + { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },\n>> { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n>> { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n>> { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n>> { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n>> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n>> - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n>> + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },\n>> { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n>> - { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n>> + { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },\n>\n> Again, this is less than the minimum. A default should probably be\n> specific to the mode of the sensor too ?\n>\n>\n>> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n>> };\n>>\n>> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>> const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;\n>> ctrlMap[&controls::FrameDurationLimits] =\n>> ControlInfo(static_cast(minSensorFrameDuration.get()),\n>> - static_cast(maxSensorFrameDuration.get()));\n>> + static_cast(maxSensorFrameDuration.get()),\n>> + int64_t(0));\n>\n> This is smaller than the minimum. Perhaps at least here during configure\n> it could be set to something more specific about the camera mode\n> capability - or something that is more 'default' like 30 FPS ? (33333 ?)\n>\n>>\n>> ctrlMap[&controls::AnalogueGain] =\n>> - ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)));\n>> + ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)), 0.0f);\n>\n> Less than minimum. I think 1.0f makes sense here too as that's unity\n> gain.\n>\n>>\n>> /*\n>> * Calculate the max exposure limit from the frame duration limit as V4L2\n>> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>>\n>> ctrlMap[&controls::ExposureTime] =\n>> ControlInfo(static_cast(helper_->exposure(exposureMin).get()),\n>> - static_cast(maxShutter.get()));\n>> + static_cast(maxShutter.get()),\n>> + int32_t(0));\n>\n> I'm not sure an exposure time of 0 makes sense ?, but I'm not sure what\n> a reasonable default is here either. Maybe something like the maximum of the frame\n> duration, and maxShutter ?\n>\n>>\n>> result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);\n>> return 0;\n>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>> index 27b4212b..2121bfd2 100644\n>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>> @@ -91,12 +91,12 @@ namespace {\n>>\n>> /* List of controls handled by the RkISP1 IPA */\n>> const ControlInfoMap::Map rkisp1Controls{\n>> - { &controls::AeEnable, ControlInfo(false, true) },\n>> - { &controls::AwbEnable, ControlInfo(false, true) },\n>> + { &controls::AeEnable, ControlInfo(false, true, false) },\n>> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n>\n> Same issue as in RPi.\n>\n>\n>> { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },\n>> - { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },\n>> - { &controls::Contrast, ControlInfo(0.0f, 1.993f) },\n>> - { &controls::Saturation, ControlInfo(0.0f, 1.993f) },\n>> + { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },\n>> + { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },\n>> + { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },\n>\n> I think 0.0 makes sense here though\n>\n>> { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n>> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n>> };\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 9df24603..a1bcfe49 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -42,7 +42,7 @@ namespace libcamera {\n>> LOG_DEFINE_CATEGORY(IPU3)\n>>\n>> static const ControlInfoMap::Map IPU3Controls = {\n>> - { &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n>> + { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },\n>\n>\n> This needs looking at. The PipelineDepth isn't something the application\n> can set. So a default doesn't makes sense, and I'm not sure a min/max\n> does either.\n>\n>\n>\n>> };\n>>\n>> class IPU3CameraData : public Camera::Private\n>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> index e895584d..8fd7634d 100644\n>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n>> /* Add the ScalerCrop control limits based on the current mode. */\n>> Rectangle ispMinCrop(data->ispMinCropSize_);\n>> ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);\n>> - ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));\n>> + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);\n>\n> I think a ScalerCrop would probably expect a default of the largest size\n> possible, not the smallest ? (At least if I were to think about\n> 'resetting' the ScalerCrop that is).\n>\n> I suspect forcing a default value might be better than implicitly\n> leaving it zero - but it means we need to give more thought to what the\n> defaults mean or represent - or provide a way for the default to be\n> 'unset' if it's not appropriate to have one.\n>\n> --\n> Kieran\n>\n>\n>>\n>> data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n>>\n>> --\n>> 2.34.1\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 CAF20C3272\n\tfor ;\n\tMon, 22 Aug 2022 22:23:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1F81361FC0;\n\tTue, 23 Aug 2022 00:23:51 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.15.19])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA5DE61FA3\n\tfor ;\n\tTue, 23 Aug 2022 00:23:48 +0200 (CEST)","from [192.168.0.158] ([88.152.184.103]) by mail.gmx.net (mrgmx004\n\t[212.227.17.190]) with ESMTPSA (Nemesis) id\n\t1MdNY2-1ozaaK0YOe-00ZSln for\n\t; Tue, 23 Aug 2022 00:23:48 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661207031;\n\tbh=laLUj0sMNLv3AfjKC3jQWucFIQ6glEBG+xhnYy1OLjM=;\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:\n\tFrom;\n\tb=027O5fI0zOJ5Q3GeVNU+RTq1i4rToaYT1544dR4u0PR3iVglLxUy/jNfl52BDF6/y\n\t1ntmpJCuEbcGFpE0uEkn1PAPq4ef+ix/S2ShVEjSqAgty7UMy8UUz0XCEh4z/yXv5h\n\tQsWhL63/TTe7ms8aixUtniw7d1AfjKkCu51Z/HW1quBVqxIoWHmPepXTiTD7dwa1Xi\n\t1F/a7YNaFb0lszBvd70+/ltbFuPrPUlQF4nM8hhkFMe52XWvkrCfGiuYnTSJ5P29jJ\n\tln5tXF8L/aw8MPwglj/gxAmdR6qxefWHL3sqV+taSqdD/lVtXpAOEO+KNNeINsIq0O\n\tOqLFo5ImUNcJw==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1661207028;\n\tbh=laLUj0sMNLv3AfjKC3jQWucFIQ6glEBG+xhnYy1OLjM=;\n\th=X-UI-Sender-Class:Date:Subject:To:References:From:In-Reply-To;\n\tb=WOHmAstkjyeXdPNuLNpxaLrM1zA9CmLaBioQF9Qxq0PDDGFPC7IsPwZdlEsBIjge7\n\tA2SjNj0FqYY8poJKV2CP1AMTeULn2gv23JkI0KJ0d9PqhHUywLGzhsgorqRRdNqkCJ\n\t4dYXEvftHtR+f5yPM8mCCwP5Z4Fw8OQCMnz6WOAw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"WOHmAstk\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"<4868aa82-33e8-7492-0e83-b50bb9b66d48@gmx.de>","Date":"Tue, 23 Aug 2022 00:23:45 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-GB","To":"libcamera-devel@lists.libcamera.org","References":"<20220821124343.487963-1-Rauch.Christian@gmx.de>\n\t<20220821124343.487963-2-Rauch.Christian@gmx.de>\n\t<166120512201.3484129.4999114544766245166@Monstersaurus>","In-Reply-To":"<166120512201.3484129.4999114544766245166@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:Mt9pnNiteKtzOi6q/sZfgHjcVUjgzxVvuuBLwhshDceE1Sb1dbd\n\tkvF5/BntN2VEuk3NedGi4e6S/Tb7xU6HfQSDKq+DqJmCzvKwb6fezQpWpSmmqr+Mbu5B1wb\n\t8E/eUcGI17wYS5P+fk1VEIwyl3QwRoDAzkpnU6zBuxek2Kp5AwHsYKbwc3t8mMVNUlVBvxu\n\tLpN+1KlPAz7dG4TStFhKA==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:0HH30ZOMaQU=:6WzL4PWcAkmXV5lOZnG2qL\n\tUlGzX+iB3Ob4aLvuI61CgNUDPsDD6Oj5Crhd3R7RQ+1IParClEnCfl8LpUimRZe6RrHF9bXc+\n\tINLGcYD2XZaqfuuf9KjJy4lIpueb2IV7qY9ZyMz5wTiqKlxilycHkSs1hQiLUSlw5WvfoRQXa\n\tGhn8sxn8RqLinLl/iwZeKjGavT3ZpFMQjlig3mS5ooHw7h1j7jISsSOc1uosrA9QWfWH99j+E\n\tdPdrKxKpZ/2nul+3bkyqhZMpIuP7EloSVcOoP5R3b9wHZsYSgor5TrT+Nzcw+c2QjhmR+3T8R\n\tk2nQL6ayFbEDKdyTDDVxJvsnnVYGn/6dVEMRsgXkc5kCZwkDYUzwW5vqNsk11hnARihKxwqcA\n\td8ZDBEpZMmOvcw2vNHToIyJJu8L2IlHs2cB/U9y+zXEO2y7lzFv/SRH8vvjWU18qckfBIrBgd\n\t2j667N5i3bxmJwZhoJNx2NSDuTA8n9Y6blno/qefljjPTExaXfcFhpe0+72PPNb1dGzdamo1E\n\tHASvKSwzYZnSkVHmELK7ue6O7264xvazoOUh4T6CCoI43HLSicpHNQ6JP+qrV0vJ2QoDft09v\n\tJ73x/ux03nbGb9kzgXMm14s1kbgdl1FGLo8yc+l9f9su87Ay3pU/2VqI4tPIVl0gMVpu3nCu/\n\t2Cc4McZF3BbpS1aaLEIo0Wqn48cno9GDHO1QyNWtjn0Z6PindWwiNWAARg+noRXaeSNKCa4mo\n\tPECmOgaBL/SVDmn6bjnivSVeVz7uWtZ1Ips7fKftqkARRZpPaUN1us/69VdWVlJEisKUjJVn6\n\tBDMCbEjBz1wygVRmjl8jCVT+qXK0MDooudQBxQ1RN+iVbiIBMe5/HhEDJ05SxjtKTrfAk+mLe\n\thtmziq0i1psaOdJieMKJbNVk09pj2xq6BwbnsmSr44FsFuV9al7lGBIxCcZxWBnz9z19w+bbX\n\t6bMVzYtQw4W8QRjq4huBb2o1t7Xkn0pNA0oqKrFDLKXoelQtqR2RFIqYh52H7DW198QFKxh46\n\t+WwyqnssAGXHVwdOosz9PBN4Q3WW7SnoRyEcOJa3ecrFiddt/PfS7j/lXqcdfiU1dHPhtCLwf\n\tl9SxEIyK+ZzOvsW2E0kIblLU9pHuUAxPzjTE775K/rD0qS0sJq+0nTdog==","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","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":24736,"web_url":"https://patchwork.libcamera.org/comment/24736/","msgid":"<166120927268.3515432.13365033869379884518@Monstersaurus>","date":"2022-08-22T23:01:12","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Christian Rauch via libcamera-devel (2022-08-22 23:23:45)\n> Hi Kieran,\n> \n> Regarding all your comments, I simply replaced the \"default\" 0 for \"def\"\n> from the old constructor with explicitly setting it 0 (or its binary\n> counterpart 'false') to match the types. If 0 is the wrong \"def\" value\n> for those controls, then they were already wrong before :-) They were\n> just implicitly wrong. Now you have to explicitly specify a \"def\" value,\n> and I kept 0 for backward compatibility.\n\nYes, they may have been implicitly incorrect before, but we shouldn't\nset them to be explictly incorrect. So this proposed series means we\nneed to consider each one carefully.\n\nAs mentioned though, I think we need to work out if all controls make\nsense to have a default value, and if not - how to express that it isn't\navailable. I suspect that previously the omision of a default value was\nthe expression that the default was 'redundant' (or unnecessary), thus\nby making it required, you need to explicitly set each one.\n\n--\nKieran\n\n\n\n> Feel free to change the default values. You should also be able to merge\n> this patch (2/2) without the first (1/2).\n> \n> Best,\n> Christian\n> \n> \n> Am 22.08.22 um 23:52 schrieb Kieran Bingham:\n> > Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)\n> >> Explicitly set the default value for all ControlInfo to 0, false or minimum.\n> >>\n> >> Signed-off-by: Christian Rauch \n> >> ---\n> >> src/ipa/raspberrypi/raspberrypi.cpp | 22 ++++++++++---------\n> >> src/ipa/rkisp1/rkisp1.cpp | 10 ++++-----\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> >> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> >> 4 files changed, 19 insertions(+), 17 deletions(-)\n> >>\n> >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> index 69c73f8c..c6360a51 100644\n> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> >>\n> >> /* List of controls handled by the Raspberry Pi IPA */\n> >> static const ControlInfoMap::Map ipaControls{\n> >> - { &controls::AeEnable, ControlInfo(false, true) },\n> >> - { &controls::ExposureTime, ControlInfo(0, 66666) },\n> >> - { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n> >> + { &controls::AeEnable, ControlInfo(false, true, false) },\n> >\n> > I would expect most of the time AutoExposure would be enabled ... so a\n> > default of false seems misleading... Yet - perhaps that's what the 0 was\n> > already implying? So - does that mean a default is unreasonable here?\n> >\n> >\n> >> + { &controls::ExposureTime, ControlInfo(0, 66666, 0) },\n> >> + { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },\n> >\n> > This default is less than the minimum.\n> >\n> >\n> >> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> >> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> >> { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> >> { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> >> - { &controls::AwbEnable, ControlInfo(false, true) },\n> >> - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> >> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n> >\n> > Same as AeEnable.... it doesn't seem reasonble to say the default is\n> > disabled? I think applications probably expect the default IPA behaviour\n> > to have AE/AWB enabled, unless it is disabled explicitly ?\n> >\n> >\n> >> + { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },\n> >> { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> >> { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> >> { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> >> { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> >> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> >> - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> >> + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },\n> >> { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> >> - { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n> >> + { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },\n> >\n> > Again, this is less than the minimum. A default should probably be\n> > specific to the mode of the sensor too ?\n> >\n> >\n> >> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> >> };\n> >>\n> >> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> >> const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;\n> >> ctrlMap[&controls::FrameDurationLimits] =\n> >> ControlInfo(static_cast(minSensorFrameDuration.get()),\n> >> - static_cast(maxSensorFrameDuration.get()));\n> >> + static_cast(maxSensorFrameDuration.get()),\n> >> + int64_t(0));\n> >\n> > This is smaller than the minimum. Perhaps at least here during configure\n> > it could be set to something more specific about the camera mode\n> > capability - or something that is more 'default' like 30 FPS ? (33333 ?)\n> >\n> >>\n> >> ctrlMap[&controls::AnalogueGain] =\n> >> - ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)));\n> >> + ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)), 0.0f);\n> >\n> > Less than minimum. I think 1.0f makes sense here too as that's unity\n> > gain.\n> >\n> >>\n> >> /*\n> >> * Calculate the max exposure limit from the frame duration limit as V4L2\n> >> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> >>\n> >> ctrlMap[&controls::ExposureTime] =\n> >> ControlInfo(static_cast(helper_->exposure(exposureMin).get()),\n> >> - static_cast(maxShutter.get()));\n> >> + static_cast(maxShutter.get()),\n> >> + int32_t(0));\n> >\n> > I'm not sure an exposure time of 0 makes sense ?, but I'm not sure what\n> > a reasonable default is here either. Maybe something like the maximum of the frame\n> > duration, and maxShutter ?\n> >\n> >>\n> >> result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> >> return 0;\n> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> >> index 27b4212b..2121bfd2 100644\n> >> --- a/src/ipa/rkisp1/rkisp1.cpp\n> >> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> >> @@ -91,12 +91,12 @@ namespace {\n> >>\n> >> /* List of controls handled by the RkISP1 IPA */\n> >> const ControlInfoMap::Map rkisp1Controls{\n> >> - { &controls::AeEnable, ControlInfo(false, true) },\n> >> - { &controls::AwbEnable, ControlInfo(false, true) },\n> >> + { &controls::AeEnable, ControlInfo(false, true, false) },\n> >> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n> >\n> > Same issue as in RPi.\n> >\n> >\n> >> { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },\n> >> - { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },\n> >> - { &controls::Contrast, ControlInfo(0.0f, 1.993f) },\n> >> - { &controls::Saturation, ControlInfo(0.0f, 1.993f) },\n> >> + { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },\n> >> + { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },\n> >> + { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },\n> >\n> > I think 0.0 makes sense here though\n> >\n> >> { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n> >> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> >> };\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index 9df24603..a1bcfe49 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -42,7 +42,7 @@ namespace libcamera {\n> >> LOG_DEFINE_CATEGORY(IPU3)\n> >>\n> >> static const ControlInfoMap::Map IPU3Controls = {\n> >> - { &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n> >> + { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },\n> >\n> >\n> > This needs looking at. The PipelineDepth isn't something the application\n> > can set. So a default doesn't makes sense, and I'm not sure a min/max\n> > does either.\n> >\n> >\n> >\n> >> };\n> >>\n> >> class IPU3CameraData : public Camera::Private\n> >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> index e895584d..8fd7634d 100644\n> >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> >> /* Add the ScalerCrop control limits based on the current mode. */\n> >> Rectangle ispMinCrop(data->ispMinCropSize_);\n> >> ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);\n> >> - ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));\n> >> + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);\n> >\n> > I think a ScalerCrop would probably expect a default of the largest size\n> > possible, not the smallest ? (At least if I were to think about\n> > 'resetting' the ScalerCrop that is).\n> >\n> > I suspect forcing a default value might be better than implicitly\n> > leaving it zero - but it means we need to give more thought to what the\n> > defaults mean or represent - or provide a way for the default to be\n> > 'unset' if it's not appropriate to have one.\n> >\n> > --\n> > Kieran\n> >\n> >\n> >>\n> >> data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n> >>\n> >> --\n> >> 2.34.1\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 701A5BE173\n\tfor ;\n\tMon, 22 Aug 2022 23:01:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BFFC661FBC;\n\tTue, 23 Aug 2022 01:01:17 +0200 (CEST)","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 02A6A60E25\n\tfor ;\n\tTue, 23 Aug 2022 01:01:15 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 85AE32B3;\n\tTue, 23 Aug 2022 01:01:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661209277;\n\tbh=659fZIXp78yrXXA0TC3pootT1no+ADeaGzS8/GdJUIo=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=h86NnNazVXmCdCiEWebjDwpTgNJdGcao9MNcEdkZc/ampXSf3cf9lPTNKwB5jU26E\n\tvJ4nYHU6VXCIhJCrTD7ceMQwqSQc3SdQ4Jar3B+i3r5zohqW4NDyrqi4xCGHFKCa6N\n\t4lYadvgtF5JDIPI6MpyAnRJ3D8udROQTcsIQzaiGxwrIrCB/qusEFdsMQXkT8ki7DI\n\tFDuTO8sMyy9Cshtwa0A8o1k7ULpp2ebGEg6rXnCRLR0+UleKtvV5JTz40zRFB6tYpN\n\tP+Y07n0CAHq5DAQFvOMfzwAjIfKbQ8QimoZ49OMJjRrzn/RHG2DMt4Izo8eWrOXw0z\n\tI3zz8EZAqO7Ag==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661209275;\n\tbh=659fZIXp78yrXXA0TC3pootT1no+ADeaGzS8/GdJUIo=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=oJ5vT+hA4pTdixbfBhN9LtNo6lumrAREsCZNjfI3WTpOagcfOp/5ImU9WGcR3as+X\n\tl5xpvdcNEXC1MH+DhF714gBoe88uTNRNcbWBwSE3dGQoyxxG4LOOmJildpsKyE6SGD\n\tUFd96dD5Ahx6FeCJ0zmAtKsK6bvIXHEY9R/7ceYg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"oJ5vT+hA\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<4868aa82-33e8-7492-0e83-b50bb9b66d48@gmx.de>","References":"<20220821124343.487963-1-Rauch.Christian@gmx.de>\n\t<20220821124343.487963-2-Rauch.Christian@gmx.de>\n\t<166120512201.3484129.4999114544766245166@Monstersaurus>\n\t<4868aa82-33e8-7492-0e83-b50bb9b66d48@gmx.de>","To":"Christian Rauch ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 23 Aug 2022 00:01:12 +0100","Message-ID":"<166120927268.3515432.13365033869379884518@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24746,"web_url":"https://patchwork.libcamera.org/comment/24746/","msgid":"","date":"2022-08-23T17:50:40","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi Kieran,\n\nAm 23.08.22 um 01:01 schrieb Kieran Bingham:\n> Quoting Christian Rauch via libcamera-devel (2022-08-22 23:23:45)\n>> Hi Kieran,\n>>\n>> Regarding all your comments, I simply replaced the \"default\" 0 for \"def\"\n>> from the old constructor with explicitly setting it 0 (or its binary\n>> counterpart 'false') to match the types. If 0 is the wrong \"def\" value\n>> for those controls, then they were already wrong before :-) They were\n>> just implicitly wrong. Now you have to explicitly specify a \"def\" value,\n>> and I kept 0 for backward compatibility.\n>\n> Yes, they may have been implicitly incorrect before, but we shouldn't\n> set them to be explictly incorrect. So this proposed series means we\n> need to consider each one carefully.\n>\n> As mentioned though, I think we need to work out if all controls make\n> sense to have a default value, and if not - how to express that it isn't\n> available. I suspect that previously the omision of a default value was\n> the expression that the default was 'redundant' (or unnecessary), thus\n> by making it required, you need to explicitly set each one.\n\nDeliberating the right choice of default values will take a lot of time\nand I am probably also not the right person to make these choices. My\nintention with this patch series was to not change the current\nbehaviour, but just to make it a requirement that the min/max/def values\nare either explicitly set fully, or not set at all.\n\nIf the current (and proposed) behaviour of initialising by 0 is not\ndesired, then I would suggest that this should be handled in a separate\npatch series that involves more stakeholders that know better than me\nwhich default values are sensible.\n\nBest,\nChristian\n\n>\n> --\n> Kieran\n>\n>\n>\n>> Feel free to change the default values. You should also be able to merge\n>> this patch (2/2) without the first (1/2).\n>>\n>> Best,\n>> Christian\n>>\n>>\n>> Am 22.08.22 um 23:52 schrieb Kieran Bingham:\n>>> Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)\n>>>> Explicitly set the default value for all ControlInfo to 0, false or minimum.\n>>>>\n>>>> Signed-off-by: Christian Rauch \n>>>> ---\n>>>> src/ipa/raspberrypi/raspberrypi.cpp | 22 ++++++++++---------\n>>>> src/ipa/rkisp1/rkisp1.cpp | 10 ++++-----\n>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n>>>> 4 files changed, 19 insertions(+), 17 deletions(-)\n>>>>\n>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>>>> index 69c73f8c..c6360a51 100644\n>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>>>> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n>>>>\n>>>> /* List of controls handled by the Raspberry Pi IPA */\n>>>> static const ControlInfoMap::Map ipaControls{\n>>>> - { &controls::AeEnable, ControlInfo(false, true) },\n>>>> - { &controls::ExposureTime, ControlInfo(0, 66666) },\n>>>> - { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n>>>> + { &controls::AeEnable, ControlInfo(false, true, false) },\n>>>\n>>> I would expect most of the time AutoExposure would be enabled ... so a\n>>> default of false seems misleading... Yet - perhaps that's what the 0 was\n>>> already implying? So - does that mean a default is unreasonable here?\n>>>\n>>>\n>>>> + { &controls::ExposureTime, ControlInfo(0, 66666, 0) },\n>>>> + { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },\n>>>\n>>> This default is less than the minimum.\n>>>\n>>>\n>>>> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n>>>> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n>>>> { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n>>>> { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n>>>> - { &controls::AwbEnable, ControlInfo(false, true) },\n>>>> - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>>>> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n>>>\n>>> Same as AeEnable.... it doesn't seem reasonble to say the default is\n>>> disabled? I think applications probably expect the default IPA behaviour\n>>> to have AE/AWB enabled, unless it is disabled explicitly ?\n>>>\n>>>\n>>>> + { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },\n>>>> { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n>>>> { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n>>>> { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n>>>> { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n>>>> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n>>>> - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n>>>> + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },\n>>>> { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n>>>> - { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n>>>> + { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },\n>>>\n>>> Again, this is less than the minimum. A default should probably be\n>>> specific to the mode of the sensor too ?\n>>>\n>>>\n>>>> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n>>>> };\n>>>>\n>>>> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>>>> const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;\n>>>> ctrlMap[&controls::FrameDurationLimits] =\n>>>> ControlInfo(static_cast(minSensorFrameDuration.get()),\n>>>> - static_cast(maxSensorFrameDuration.get()));\n>>>> + static_cast(maxSensorFrameDuration.get()),\n>>>> + int64_t(0));\n>>>\n>>> This is smaller than the minimum. Perhaps at least here during configure\n>>> it could be set to something more specific about the camera mode\n>>> capability - or something that is more 'default' like 30 FPS ? (33333 ?)\n>>>\n>>>>\n>>>> ctrlMap[&controls::AnalogueGain] =\n>>>> - ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)));\n>>>> + ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)), 0.0f);\n>>>\n>>> Less than minimum. I think 1.0f makes sense here too as that's unity\n>>> gain.\n>>>\n>>>>\n>>>> /*\n>>>> * Calculate the max exposure limit from the frame duration limit as V4L2\n>>>> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>>>>\n>>>> ctrlMap[&controls::ExposureTime] =\n>>>> ControlInfo(static_cast(helper_->exposure(exposureMin).get()),\n>>>> - static_cast(maxShutter.get()));\n>>>> + static_cast(maxShutter.get()),\n>>>> + int32_t(0));\n>>>\n>>> I'm not sure an exposure time of 0 makes sense ?, but I'm not sure what\n>>> a reasonable default is here either. Maybe something like the maximum of the frame\n>>> duration, and maxShutter ?\n>>>\n>>>>\n>>>> result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);\n>>>> return 0;\n>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>>>> index 27b4212b..2121bfd2 100644\n>>>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>>>> @@ -91,12 +91,12 @@ namespace {\n>>>>\n>>>> /* List of controls handled by the RkISP1 IPA */\n>>>> const ControlInfoMap::Map rkisp1Controls{\n>>>> - { &controls::AeEnable, ControlInfo(false, true) },\n>>>> - { &controls::AwbEnable, ControlInfo(false, true) },\n>>>> + { &controls::AeEnable, ControlInfo(false, true, false) },\n>>>> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n>>>\n>>> Same issue as in RPi.\n>>>\n>>>\n>>>> { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },\n>>>> - { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },\n>>>> - { &controls::Contrast, ControlInfo(0.0f, 1.993f) },\n>>>> - { &controls::Saturation, ControlInfo(0.0f, 1.993f) },\n>>>> + { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },\n>>>> + { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },\n>>>> + { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },\n>>>\n>>> I think 0.0 makes sense here though\n>>>\n>>>> { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n>>>> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n>>>> };\n>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> index 9df24603..a1bcfe49 100644\n>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> @@ -42,7 +42,7 @@ namespace libcamera {\n>>>> LOG_DEFINE_CATEGORY(IPU3)\n>>>>\n>>>> static const ControlInfoMap::Map IPU3Controls = {\n>>>> - { &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n>>>> + { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },\n>>>\n>>>\n>>> This needs looking at. The PipelineDepth isn't something the application\n>>> can set. So a default doesn't makes sense, and I'm not sure a min/max\n>>> does either.\n>>>\n>>>\n>>>\n>>>> };\n>>>>\n>>>> class IPU3CameraData : public Camera::Private\n>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>> index e895584d..8fd7634d 100644\n>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n>>>> /* Add the ScalerCrop control limits based on the current mode. */\n>>>> Rectangle ispMinCrop(data->ispMinCropSize_);\n>>>> ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);\n>>>> - ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));\n>>>> + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);\n>>>\n>>> I think a ScalerCrop would probably expect a default of the largest size\n>>> possible, not the smallest ? (At least if I were to think about\n>>> 'resetting' the ScalerCrop that is).\n>>>\n>>> I suspect forcing a default value might be better than implicitly\n>>> leaving it zero - but it means we need to give more thought to what the\n>>> defaults mean or represent - or provide a way for the default to be\n>>> 'unset' if it's not appropriate to have one.\n>>>\n>>> --\n>>> Kieran\n>>>\n>>>\n>>>>\n>>>> data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n>>>>\n>>>> --\n>>>> 2.34.1\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 864AAC3272\n\tfor ;\n\tTue, 23 Aug 2022 17:50:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CFF7061FBD;\n\tTue, 23 Aug 2022 19:50:42 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.15.18])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 26613603E3\n\tfor ;\n\tTue, 23 Aug 2022 19:50:41 +0200 (CEST)","from [192.168.0.158] ([88.152.184.103]) by mail.gmx.net (mrgmx004\n\t[212.227.17.190]) with ESMTPSA (Nemesis) id\n\t1Mo6v3-1pFa2d37mN-00pa7o for\n\t; Tue, 23 Aug 2022 19:50:40 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661277042;\n\tbh=98IuU7GMAYxT++CRwMw+VsBlp6sUyWutqQuyCjMyKwk=;\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:\n\tFrom;\n\tb=OL80UxYugADRoeQqcfoxw2QeMgwEtD57aI11alEX2DzRlgshsv74pkXQ8d/SK4Q/O\n\tBjjdqONFrxdT3F3T5kBQ3M+651c7JW2Oq4iJrm4/3JHf8JuWRi3xJjHD5396MQU7DL\n\tHoidnVJHTONlVEPW25AM/1EDzTGqcXFN2pMdAL9yBOseZLJh+OWOp1X3cpOTrqdZLx\n\tv0CWfftwY1srRd9iSnWRCmYDsZBk1e4ndDukACaQviiVeMkQmyYcEmYVod4/s7/bW0\n\t0MenI3usHVGcPPV1A8d6D2K7o2itkiW7tyrTGdDmwgUMrRj+jmHEDqE5YG6VjH5izt\n\tHZENfN05QF5ag==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1661277040;\n\tbh=98IuU7GMAYxT++CRwMw+VsBlp6sUyWutqQuyCjMyKwk=;\n\th=X-UI-Sender-Class:Date:Subject:To:References:From:In-Reply-To;\n\tb=eXWqOQ2gobyX5ehL8BkzJhS8AgUeO4iKWHd1OhBWamVKb7op9IjnmXmg+z3Ybq2iO\n\tVW58f4HRxqSJ+cuBRDej1uP8WkCguoNP85ybjOuvCBuL4WXIsOlV6mbBKcazfUQI2u\n\tadbyVPuVdKmjE6NgkoQsIbSG+fnnzj4M+Zjrzzkc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"eXWqOQ2g\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"","Date":"Tue, 23 Aug 2022 19:50:40 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-GB","To":"libcamera-devel@lists.libcamera.org","References":"<20220821124343.487963-1-Rauch.Christian@gmx.de>\n\t<20220821124343.487963-2-Rauch.Christian@gmx.de>\n\t<166120512201.3484129.4999114544766245166@Monstersaurus>\n\t<4868aa82-33e8-7492-0e83-b50bb9b66d48@gmx.de>\n\t<166120927268.3515432.13365033869379884518@Monstersaurus>","In-Reply-To":"<166120927268.3515432.13365033869379884518@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:Ibj1aTW9JWmBJwH3n0BUtdD2eoqWXrexODQbXl+9UMY9RRo4tW7\n\tVc6oM7pHPpr4Lnh6+mVpc0sdW2A6TbetXgLBpZCwmcITSONsVsSxsss65LRc8ovg8rmmCrz\n\t9Idmoi8oRjwG1igai28687tsJHbfpp1qFpERMG+bPdwEFyfEUZEhmVPdCTem3RwHure8W2N\n\tyiRs5p6nWfqqrUgV/wmSg==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:N0bbhV1fw3w=:fVGadNm1zDKcOUfrmjCJeN\n\tSE8AwGPA0/BqwPYZc+ERUwYoX8R1HyegLcUyD6CXmnmDNHjri1KQA28nqA5Z6l+8/lnSZrZbA\n\tsqVeL9fT9La6dig6Id8ITn+uyhZo7sJiS7NQjk5FjeJrwwIzno/bK/f95RI7Irl9OnRJZvHK5\n\tyR6LhpOmL6OF3ztJZkXl/Py4nfTPcm/5O26ZYhgZQmy9WMIg3J0Qh8ZFiD6jpK4E7yWOtg8P8\n\tqK4LGXa3+xB69fVQB4teVCY2UnkVNrGDft9G98C4fRz1sBInfolKU4+me1tKLWtn5NpiwV7rY\n\tJ+xa8VhGHAPsSsT4I+foT16Iy6DKh254oE0+KmQdma3WS6IcqZY7LIDv7SmFAZGorppLV5tsc\n\tcJ0d7/S+Sa+fXdUTyQgjd9TVpgKp8N0SdOqfvC5Pzf2ZBPbc/ANo7pMg8/Ag5N4HU33JC9vfR\n\tCN60kbIyWTmAykXyJdISjEa0OS/4s1spnWe8TsF9Z2hIYOBUd7rtzGP3LCkOtEKXU2NDYtLWR\n\taGhWOnGv968JjlP4wlQOWoGPOW+lsyDF2ilwtAwYfNXsfLEE0wgkLZjqZPHuwH0MZHuSe45Py\n\tkHbs1+g1OReSlvStTuGMBjpdPZu/2v+V1r+buGqateOjDGLq+MBnNRq+JKZODDYF0ffJIywPG\n\t/6kQM6zeVsUpKP3cwk3/ZIM1VhMMdpWRNzP+JzgrhK6dqpsaheLdxyS91Z4wzNNCh1m0W/ZrX\n\tpwNDX3JmnraxvZjWd2Q8D1JuZ73VNE/1+m/sSlRqbRS+KtYhC2Z+HjtqboaTikWFiuJleTGWo\n\tz4QbsVWSbRQ1hOXXs3KnerxjAERJG8t5hd/L26nO/wl/bVmaTTK9dUFvXCcYG5B4Vy6fv8TJs\n\t5Pfq4AJ0JG8xOVRMEG+V3imO3CETJ/32fsC/EapMJ0cFsRThPqbOWquyS0CIHld81tn1RMCGp\n\t0LUgtqGg1QfxyGMUXqiSuvVsU3thIOOmk0HBlm1AUPyj4T+hICpzObUI1U1f1HAMr+CxrGQlF\n\thjJUXTPKqclF8YvMf6E5c0da2zc7JWx693HmUXFTMY7qhj/RLa6/NLA4Tu21GcCdSzLHxw8gI\n\tdPSL181PxmG62EDzT6iHdBTe3RyRLUniZfcW65h9bx4a9e2IIvbCyxGtw==","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","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":24818,"web_url":"https://patchwork.libcamera.org/comment/24818/","msgid":"","date":"2022-08-29T20:32:50","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi Kieran,\n\nI replaced the [1/2] patch in this set with a new one (\"libcamera:\ncontrol: initialise control info to ControlTypeNone by default\") that\ndefault constructs the min/max/def ControlValue instead of setting it to\n0. This allows explicitly checking for 'ControlTypeNone' instead of\nrelying on 0 as an implicit representation.\n\nBest,\nChristian\n\n\nAm 23.08.22 um 01:01 schrieb Kieran Bingham:\n> Quoting Christian Rauch via libcamera-devel (2022-08-22 23:23:45)\n>> Hi Kieran,\n>>\n>> Regarding all your comments, I simply replaced the \"default\" 0 for \"def\"\n>> from the old constructor with explicitly setting it 0 (or its binary\n>> counterpart 'false') to match the types. If 0 is the wrong \"def\" value\n>> for those controls, then they were already wrong before :-) They were\n>> just implicitly wrong. Now you have to explicitly specify a \"def\" value,\n>> and I kept 0 for backward compatibility.\n>\n> Yes, they may have been implicitly incorrect before, but we shouldn't\n> set them to be explictly incorrect. So this proposed series means we\n> need to consider each one carefully.\n>\n> As mentioned though, I think we need to work out if all controls make\n> sense to have a default value, and if not - how to express that it isn't\n> available. I suspect that previously the omision of a default value was\n> the expression that the default was 'redundant' (or unnecessary), thus\n> by making it required, you need to explicitly set each one.\n>\n> --\n> Kieran\n>\n>\n>\n>> Feel free to change the default values. You should also be able to merge\n>> this patch (2/2) without the first (1/2).\n>>\n>> Best,\n>> Christian\n>>\n>>\n>> Am 22.08.22 um 23:52 schrieb Kieran Bingham:\n>>> Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)\n>>>> Explicitly set the default value for all ControlInfo to 0, false or minimum.\n>>>>\n>>>> Signed-off-by: Christian Rauch \n>>>> ---\n>>>> src/ipa/raspberrypi/raspberrypi.cpp | 22 ++++++++++---------\n>>>> src/ipa/rkisp1/rkisp1.cpp | 10 ++++-----\n>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n>>>> 4 files changed, 19 insertions(+), 17 deletions(-)\n>>>>\n>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>>>> index 69c73f8c..c6360a51 100644\n>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>>>> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n>>>>\n>>>> /* List of controls handled by the Raspberry Pi IPA */\n>>>> static const ControlInfoMap::Map ipaControls{\n>>>> - { &controls::AeEnable, ControlInfo(false, true) },\n>>>> - { &controls::ExposureTime, ControlInfo(0, 66666) },\n>>>> - { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n>>>> + { &controls::AeEnable, ControlInfo(false, true, false) },\n>>>\n>>> I would expect most of the time AutoExposure would be enabled ... so a\n>>> default of false seems misleading... Yet - perhaps that's what the 0 was\n>>> already implying? So - does that mean a default is unreasonable here?\n>>>\n>>>\n>>>> + { &controls::ExposureTime, ControlInfo(0, 66666, 0) },\n>>>> + { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },\n>>>\n>>> This default is less than the minimum.\n>>>\n>>>\n>>>> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n>>>> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n>>>> { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n>>>> { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n>>>> - { &controls::AwbEnable, ControlInfo(false, true) },\n>>>> - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>>>> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n>>>\n>>> Same as AeEnable.... it doesn't seem reasonble to say the default is\n>>> disabled? I think applications probably expect the default IPA behaviour\n>>> to have AE/AWB enabled, unless it is disabled explicitly ?\n>>>\n>>>\n>>>> + { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },\n>>>> { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n>>>> { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n>>>> { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n>>>> { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n>>>> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n>>>> - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n>>>> + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },\n>>>> { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n>>>> - { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n>>>> + { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },\n>>>\n>>> Again, this is less than the minimum. A default should probably be\n>>> specific to the mode of the sensor too ?\n>>>\n>>>\n>>>> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n>>>> };\n>>>>\n>>>> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>>>> const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;\n>>>> ctrlMap[&controls::FrameDurationLimits] =\n>>>> ControlInfo(static_cast(minSensorFrameDuration.get()),\n>>>> - static_cast(maxSensorFrameDuration.get()));\n>>>> + static_cast(maxSensorFrameDuration.get()),\n>>>> + int64_t(0));\n>>>\n>>> This is smaller than the minimum. Perhaps at least here during configure\n>>> it could be set to something more specific about the camera mode\n>>> capability - or something that is more 'default' like 30 FPS ? (33333 ?)\n>>>\n>>>>\n>>>> ctrlMap[&controls::AnalogueGain] =\n>>>> - ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)));\n>>>> + ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)), 0.0f);\n>>>\n>>> Less than minimum. I think 1.0f makes sense here too as that's unity\n>>> gain.\n>>>\n>>>>\n>>>> /*\n>>>> * Calculate the max exposure limit from the frame duration limit as V4L2\n>>>> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>>>>\n>>>> ctrlMap[&controls::ExposureTime] =\n>>>> ControlInfo(static_cast(helper_->exposure(exposureMin).get()),\n>>>> - static_cast(maxShutter.get()));\n>>>> + static_cast(maxShutter.get()),\n>>>> + int32_t(0));\n>>>\n>>> I'm not sure an exposure time of 0 makes sense ?, but I'm not sure what\n>>> a reasonable default is here either. Maybe something like the maximum of the frame\n>>> duration, and maxShutter ?\n>>>\n>>>>\n>>>> result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);\n>>>> return 0;\n>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>>>> index 27b4212b..2121bfd2 100644\n>>>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>>>> @@ -91,12 +91,12 @@ namespace {\n>>>>\n>>>> /* List of controls handled by the RkISP1 IPA */\n>>>> const ControlInfoMap::Map rkisp1Controls{\n>>>> - { &controls::AeEnable, ControlInfo(false, true) },\n>>>> - { &controls::AwbEnable, ControlInfo(false, true) },\n>>>> + { &controls::AeEnable, ControlInfo(false, true, false) },\n>>>> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n>>>\n>>> Same issue as in RPi.\n>>>\n>>>\n>>>> { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },\n>>>> - { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },\n>>>> - { &controls::Contrast, ControlInfo(0.0f, 1.993f) },\n>>>> - { &controls::Saturation, ControlInfo(0.0f, 1.993f) },\n>>>> + { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },\n>>>> + { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },\n>>>> + { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },\n>>>\n>>> I think 0.0 makes sense here though\n>>>\n>>>> { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n>>>> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n>>>> };\n>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> index 9df24603..a1bcfe49 100644\n>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> @@ -42,7 +42,7 @@ namespace libcamera {\n>>>> LOG_DEFINE_CATEGORY(IPU3)\n>>>>\n>>>> static const ControlInfoMap::Map IPU3Controls = {\n>>>> - { &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n>>>> + { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },\n>>>\n>>>\n>>> This needs looking at. The PipelineDepth isn't something the application\n>>> can set. So a default doesn't makes sense, and I'm not sure a min/max\n>>> does either.\n>>>\n>>>\n>>>\n>>>> };\n>>>>\n>>>> class IPU3CameraData : public Camera::Private\n>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>> index e895584d..8fd7634d 100644\n>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n>>>> /* Add the ScalerCrop control limits based on the current mode. */\n>>>> Rectangle ispMinCrop(data->ispMinCropSize_);\n>>>> ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);\n>>>> - ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));\n>>>> + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);\n>>>\n>>> I think a ScalerCrop would probably expect a default of the largest size\n>>> possible, not the smallest ? (At least if I were to think about\n>>> 'resetting' the ScalerCrop that is).\n>>>\n>>> I suspect forcing a default value might be better than implicitly\n>>> leaving it zero - but it means we need to give more thought to what the\n>>> defaults mean or represent - or provide a way for the default to be\n>>> 'unset' if it's not appropriate to have one.\n>>>\n>>> --\n>>> Kieran\n>>>\n>>>\n>>>>\n>>>> data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n>>>>\n>>>> --\n>>>> 2.34.1\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 81333C0DA4\n\tfor ;\n\tMon, 29 Aug 2022 20:32:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D186961FC0;\n\tMon, 29 Aug 2022 22:32:52 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.15.18])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 346AC61FBA\n\tfor ;\n\tMon, 29 Aug 2022 22:32:51 +0200 (CEST)","from [192.168.0.158] ([88.152.184.103]) by mail.gmx.net (mrgmx005\n\t[212.227.17.190]) with ESMTPSA (Nemesis) id\n\t1MVeMA-1os6JK245k-00Ra17 for\n\t; Mon, 29 Aug 2022 22:32:50 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661805172;\n\tbh=Hq/nZ2vGaK+FGuRfJNuGnyGL4PEXVdmpUxef9lDRMbE=;\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:\n\tFrom;\n\tb=dADqWsy8oTLClDndlXutvc8RPc31ZjlIzEfJvXmyhtFwTleXB7z/qo53043fVRerS\n\tm156xKPWJgrKIZcKls2hToDHtXtqlxve8IcYE5AOI9LOWFfMnaIn/0RU5M2ND1pRYV\n\tgE6t3J9KhL4pFv/3wo+qy6nG00uMavdl8p8aqjGf6n8v5dvO8ceTlyj6sn0jJ7mIBJ\n\t3XTQryvpFfXNDBZ8+pwwGBe2WxoZm3vVn5PKQppE38ZCR2A7/Zh7xl95ws+wLdzb42\n\tfoBfLzPtAUj1EtHg9mO/gavSrVSSfSZqFqkT+4/IM66SBhos8X5RFM5e40qwQxEKwI\n\twYcVDo1AOXqbQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1661805170;\n\tbh=Hq/nZ2vGaK+FGuRfJNuGnyGL4PEXVdmpUxef9lDRMbE=;\n\th=X-UI-Sender-Class:Date:Subject:To:References:From:In-Reply-To;\n\tb=OiUp3hBFmr0r04Ec4KvpmHA0HFJ343Im/mjM0OK2Q+siX27V2B03wcoKzu0vVsHPh\n\teNl+zKYUhzGMywW7fLYR5fHiiT9Vw83RQMfQSQb0bIAGbcflNDGlJqbnoQ4drHVWB5\n\tAkONOY4R80GgnfJBswZNXosg/ezuF7+zn6HGg+is="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"OiUp3hBF\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"","Date":"Mon, 29 Aug 2022 22:32:50 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-GB","To":"libcamera-devel@lists.libcamera.org","References":"<20220821124343.487963-1-Rauch.Christian@gmx.de>\n\t<20220821124343.487963-2-Rauch.Christian@gmx.de>\n\t<166120512201.3484129.4999114544766245166@Monstersaurus>\n\t<4868aa82-33e8-7492-0e83-b50bb9b66d48@gmx.de>\n\t<166120927268.3515432.13365033869379884518@Monstersaurus>","In-Reply-To":"<166120927268.3515432.13365033869379884518@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:MbtfE/caHpe38v322ajaM4qPbd3wOtdg8ShFS14euAu9DgYoQ5n\n\t4tAlLbDS4O+z6DUGB2IrFeuIttbT2Ry8IsFR2JsOCH2UoIXDC15Hdw11gz5OX0C7hKKzJ+h\n\t417QJsSDKrEWDPeX1pkvYdM0LTfiGT/J1hGosjZNRn1CQUUUYNOBRTgPp20+sft4m4v92Uw\n\tVgYbzEh7BtQf/OZCBG/ng==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:/Jl2Kq5tYBo=:0dmSTu5WHWGon3zuX/okeq\n\t5BBEbsFz5H6YecG41o4tQ+ZLQo9DbukRILRjqfsD0ryMKDhdETrxJuBP/Tgl3APw5HnuDgG4v\n\t2YVdL8OfuUXThqSk1r5rB6gmFZTCkom53+DSGJEKn6oVIundKtASGOYn8Y9wPIfA+5xl7oLkH\n\tAXcURtgZwmLRhteRaMvSzyyD4kdRAqfO5yvVpJmnFbCFvlTZLC7gOOs7yZZtdmWn45P5qFAz1\n\tHFI5Aqje/3bcS6dcw6/l6bmixw9o4t2haxaQi1mIMuCwRJ6AELkQZfWMz/2M9bTSYIFcfwA4a\n\tUBMYMNS7hUqPZ7Toqu6ZRwgC4OGkypkmPBzAmQZqE/lPp08HA0tcb2P5FtI/iOCXkt5XxOLri\n\t3VMWRAdCFrgZ9LdeJPK3FCzGlqyeKF6I0ClwEkNejkhWmKW5nzeUZTVn36mF40htBr1a6rjas\n\tiCFVgZvn6NbHgXQzM4rb4F46gEwO98gby/vlfMJ4ZQJanCA6KOVwrp9qCxnJ6TaI4yGYpVM3h\n\tl+Fm/2QU9dNPIJnB2ZmT30H7Lay59Kc2Us14qOiP+0hcvqjAnV3zQqDrD6eR1PMjHIQUt6XOT\n\tJP5Zbrt9CXIP2d97hUnxdOIrbkVpA06nGCG/Rbv5ya0lb3YBHslq6SBOSwko/PxUuTqnarAxj\n\tqtOhfEn4AM0g0VmHtVQ3svYMLRiRSmMEfltXVLLV7z4CklCpldkl6ZecbYlCD+31bQLbE7lca\n\tGRa38we1x2eO4lnixKh26HUaersODR7TaqhCh5v3qWE677LuzTQvkialO5ivzbN7ghMnxfmIC\n\t6UJntsanR1/Z4QyUeJPlrmWQqvR2t99yJUd5TSdQpsfMjDjOaHQ2+zGsyVbZ9c4hTnGKhA4Yo\n\t4Zgv7o2kXiBLkFeWmOMcAYqJhoIlJ5n3ZtMSevBmOSMWWCRA1Tk8XIPUh/Jjhan6CBdtbApYT\n\twWN/wL650SmlQ/j3TdiqIE88/+V3FyZRUl82KZdjaC0i3LrhFTlpVqnkzs/vd3hAPlzboHEih\n\t1mWeJo6DmqLfVqCttyEvufO8GENLTivqyvd3OXdSEa55jCdoYsvDbIs/3Xl3YFXj+0QA6gsHd\n\tbnAGGWKmP5jkG2zeC2TXKCnNv7vxBilHcv2anN+iDytxxIKpGw8jbZ81A==","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","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":24980,"web_url":"https://patchwork.libcamera.org/comment/24980/","msgid":"<4b8e1d5f-1313-96a2-5089-9e4f53379007@gmx.de>","date":"2022-09-12T19:37:21","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","submitter":{"id":111,"url":"https://patchwork.libcamera.org/api/people/111/","name":"Christian Rauch","email":"Rauch.Christian@gmx.de"},"content":"Hi,\n\nNot sure what the \"proper\" way is to close or retract proposed patches\non mailing lists. I want to mark this patch series, and a couple of\nprevious patches touching the default values, as \"obsolete\", such that\nthey do not appear on patchwork anymore and are not considered for\nreview/merging.\n\nBest,\nChristian\n\n\nAm 29.08.22 um 22:32 schrieb Christian Rauch via libcamera-devel:\n> Hi Kieran,\n>\n> I replaced the [1/2] patch in this set with a new one (\"libcamera:\n> control: initialise control info to ControlTypeNone by default\") that\n> default constructs the min/max/def ControlValue instead of setting it to\n> 0. This allows explicitly checking for 'ControlTypeNone' instead of\n> relying on 0 as an implicit representation.\n>\n> Best,\n> Christian\n>\n>\n> Am 23.08.22 um 01:01 schrieb Kieran Bingham:\n>> Quoting Christian Rauch via libcamera-devel (2022-08-22 23:23:45)\n>>> Hi Kieran,\n>>>\n>>> Regarding all your comments, I simply replaced the \"default\" 0 for \"def\"\n>>> from the old constructor with explicitly setting it 0 (or its binary\n>>> counterpart 'false') to match the types. If 0 is the wrong \"def\" value\n>>> for those controls, then they were already wrong before :-) They were\n>>> just implicitly wrong. Now you have to explicitly specify a \"def\" value,\n>>> and I kept 0 for backward compatibility.\n>>\n>> Yes, they may have been implicitly incorrect before, but we shouldn't\n>> set them to be explictly incorrect. So this proposed series means we\n>> need to consider each one carefully.\n>>\n>> As mentioned though, I think we need to work out if all controls make\n>> sense to have a default value, and if not - how to express that it isn't\n>> available. I suspect that previously the omision of a default value was\n>> the expression that the default was 'redundant' (or unnecessary), thus\n>> by making it required, you need to explicitly set each one.\n>>\n>> --\n>> Kieran\n>>\n>>\n>>\n>>> Feel free to change the default values. You should also be able to merge\n>>> this patch (2/2) without the first (1/2).\n>>>\n>>> Best,\n>>> Christian\n>>>\n>>>\n>>> Am 22.08.22 um 23:52 schrieb Kieran Bingham:\n>>>> Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)\n>>>>> Explicitly set the default value for all ControlInfo to 0, false or minimum.\n>>>>>\n>>>>> Signed-off-by: Christian Rauch \n>>>>> ---\n>>>>> src/ipa/raspberrypi/raspberrypi.cpp | 22 ++++++++++---------\n>>>>> src/ipa/rkisp1/rkisp1.cpp | 10 ++++-----\n>>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n>>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n>>>>> 4 files changed, 19 insertions(+), 17 deletions(-)\n>>>>>\n>>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>>>>> index 69c73f8c..c6360a51 100644\n>>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>>>>> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n>>>>>\n>>>>> /* List of controls handled by the Raspberry Pi IPA */\n>>>>> static const ControlInfoMap::Map ipaControls{\n>>>>> - { &controls::AeEnable, ControlInfo(false, true) },\n>>>>> - { &controls::ExposureTime, ControlInfo(0, 66666) },\n>>>>> - { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n>>>>> + { &controls::AeEnable, ControlInfo(false, true, false) },\n>>>>\n>>>> I would expect most of the time AutoExposure would be enabled ... so a\n>>>> default of false seems misleading... Yet - perhaps that's what the 0 was\n>>>> already implying? So - does that mean a default is unreasonable here?\n>>>>\n>>>>\n>>>>> + { &controls::ExposureTime, ControlInfo(0, 66666, 0) },\n>>>>> + { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },\n>>>>\n>>>> This default is less than the minimum.\n>>>>\n>>>>\n>>>>> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n>>>>> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n>>>>> { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n>>>>> { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n>>>>> - { &controls::AwbEnable, ControlInfo(false, true) },\n>>>>> - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>>>>> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n>>>>\n>>>> Same as AeEnable.... it doesn't seem reasonble to say the default is\n>>>> disabled? I think applications probably expect the default IPA behaviour\n>>>> to have AE/AWB enabled, unless it is disabled explicitly ?\n>>>>\n>>>>\n>>>>> + { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },\n>>>>> { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n>>>>> { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n>>>>> { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n>>>>> { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n>>>>> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n>>>>> - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n>>>>> + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },\n>>>>> { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n>>>>> - { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n>>>>> + { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },\n>>>>\n>>>> Again, this is less than the minimum. A default should probably be\n>>>> specific to the mode of the sensor too ?\n>>>>\n>>>>\n>>>>> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n>>>>> };\n>>>>>\n>>>>> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>>>>> const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;\n>>>>> ctrlMap[&controls::FrameDurationLimits] =\n>>>>> ControlInfo(static_cast(minSensorFrameDuration.get()),\n>>>>> - static_cast(maxSensorFrameDuration.get()));\n>>>>> + static_cast(maxSensorFrameDuration.get()),\n>>>>> + int64_t(0));\n>>>>\n>>>> This is smaller than the minimum. Perhaps at least here during configure\n>>>> it could be set to something more specific about the camera mode\n>>>> capability - or something that is more 'default' like 30 FPS ? (33333 ?)\n>>>>\n>>>>>\n>>>>> ctrlMap[&controls::AnalogueGain] =\n>>>>> - ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)));\n>>>>> + ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)), 0.0f);\n>>>>\n>>>> Less than minimum. I think 1.0f makes sense here too as that's unity\n>>>> gain.\n>>>>\n>>>>>\n>>>>> /*\n>>>>> * Calculate the max exposure limit from the frame duration limit as V4L2\n>>>>> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>>>>>\n>>>>> ctrlMap[&controls::ExposureTime] =\n>>>>> ControlInfo(static_cast(helper_->exposure(exposureMin).get()),\n>>>>> - static_cast(maxShutter.get()));\n>>>>> + static_cast(maxShutter.get()),\n>>>>> + int32_t(0));\n>>>>\n>>>> I'm not sure an exposure time of 0 makes sense ?, but I'm not sure what\n>>>> a reasonable default is here either. Maybe something like the maximum of the frame\n>>>> duration, and maxShutter ?\n>>>>\n>>>>>\n>>>>> result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);\n>>>>> return 0;\n>>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>>>>> index 27b4212b..2121bfd2 100644\n>>>>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>>>>> @@ -91,12 +91,12 @@ namespace {\n>>>>>\n>>>>> /* List of controls handled by the RkISP1 IPA */\n>>>>> const ControlInfoMap::Map rkisp1Controls{\n>>>>> - { &controls::AeEnable, ControlInfo(false, true) },\n>>>>> - { &controls::AwbEnable, ControlInfo(false, true) },\n>>>>> + { &controls::AeEnable, ControlInfo(false, true, false) },\n>>>>> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n>>>>\n>>>> Same issue as in RPi.\n>>>>\n>>>>\n>>>>> { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },\n>>>>> - { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },\n>>>>> - { &controls::Contrast, ControlInfo(0.0f, 1.993f) },\n>>>>> - { &controls::Saturation, ControlInfo(0.0f, 1.993f) },\n>>>>> + { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },\n>>>>> + { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },\n>>>>> + { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },\n>>>>\n>>>> I think 0.0 makes sense here though\n>>>>\n>>>>> { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n>>>>> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n>>>>> };\n>>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>>> index 9df24603..a1bcfe49 100644\n>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>>> @@ -42,7 +42,7 @@ namespace libcamera {\n>>>>> LOG_DEFINE_CATEGORY(IPU3)\n>>>>>\n>>>>> static const ControlInfoMap::Map IPU3Controls = {\n>>>>> - { &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n>>>>> + { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },\n>>>>\n>>>>\n>>>> This needs looking at. The PipelineDepth isn't something the application\n>>>> can set. So a default doesn't makes sense, and I'm not sure a min/max\n>>>> does either.\n>>>>\n>>>>\n>>>>\n>>>>> };\n>>>>>\n>>>>> class IPU3CameraData : public Camera::Private\n>>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>>> index e895584d..8fd7634d 100644\n>>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>>> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n>>>>> /* Add the ScalerCrop control limits based on the current mode. */\n>>>>> Rectangle ispMinCrop(data->ispMinCropSize_);\n>>>>> ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);\n>>>>> - ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));\n>>>>> + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);\n>>>>\n>>>> I think a ScalerCrop would probably expect a default of the largest size\n>>>> possible, not the smallest ? (At least if I were to think about\n>>>> 'resetting' the ScalerCrop that is).\n>>>>\n>>>> I suspect forcing a default value might be better than implicitly\n>>>> leaving it zero - but it means we need to give more thought to what the\n>>>> defaults mean or represent - or provide a way for the default to be\n>>>> 'unset' if it's not appropriate to have one.\n>>>>\n>>>> --\n>>>> Kieran\n>>>>\n>>>>\n>>>>>\n>>>>> data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n>>>>>\n>>>>> --\n>>>>> 2.34.1\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 1E822C3272\n\tfor ;\n\tMon, 12 Sep 2022 19:37:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 897EF61F9F;\n\tMon, 12 Sep 2022 21:37:24 +0200 (CEST)","from mout.gmx.net (mout.gmx.net [212.227.17.20])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46AD5609A6\n\tfor ;\n\tMon, 12 Sep 2022 21:37:22 +0200 (CEST)","from [192.168.0.158] ([88.152.184.103]) by mail.gmx.net (mrgmx105\n\t[212.227.17.168]) with ESMTPSA (Nemesis) id\n\t1MSbx3-1ovqP22m2z-00SyOD for\n\t; Mon, 12 Sep 2022 21:37:21 +0200"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663011444;\n\tbh=IFP1AnokRSrvncynWWmjsy/X2HfP5nFIiPdNmFvWOMk=;\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:\n\tFrom;\n\tb=qmgQwq0eaBl+LdiqHz4kWtpO+usGpmTvdKjHJJpLVstKIjNQ6n1b7irpM1c0Yo2EO\n\tbdt1l5o7tfERdHmL5d2OUoYYPFdZt/0ueIOg1kVRvK8yN8n9lYcBQqdWY6TfzqLG7K\n\tMndrbcChgWDmzpFkjN/gUF3UTo3TzaXwUJY4+2tW1s79lWk+obqZvoG3NDs5qpfKD2\n\tf07FvMEUa/+lQ6IHPggvD0WDYliu5nJixcyGDwmf6J0KkErmEq6Xmco7RLCILicP8d\n\tT0dPBzxN996h9QN10KyoWm7NgmEi3DHznUZ/dK/NYzL9qP7s6C+OqwzFJ329fVogVU\n\tPz/y0zlAwbwtA==","v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net;\n\ts=badeba3b8450; t=1663011441;\n\tbh=IFP1AnokRSrvncynWWmjsy/X2HfP5nFIiPdNmFvWOMk=;\n\th=X-UI-Sender-Class:Date:Subject:To:References:From:In-Reply-To;\n\tb=X//mI23RYEVQi8NYOKyVU1YAzPs3Hkt4zZlC7P33C6WXRxa74QpyVpPHPgyHJPVw4\n\t7QGMFOCCzteHZ9aEM1uEC8GfBweAlW44POM6Kbz13KaQb+CJ25kx5n3uYh90n3iFrS\n\tQELy3AMP18TjMgIoX3JR+Gvrm4Ri/rlCnhPePf2Q="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=gmx.net header.i=@gmx.net\n\theader.b=\"X//mI23R\"; dkim-atps=neutral","X-UI-Sender-Class":"01bb95c1-4bf8-414a-932a-4f6e2808ef9c","Message-ID":"<4b8e1d5f-1313-96a2-5089-9e4f53379007@gmx.de>","Date":"Mon, 12 Sep 2022 21:37:21 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-GB","To":"libcamera-devel@lists.libcamera.org","References":"<20220821124343.487963-1-Rauch.Christian@gmx.de>\n\t<20220821124343.487963-2-Rauch.Christian@gmx.de>\n\t<166120512201.3484129.4999114544766245166@Monstersaurus>\n\t<4868aa82-33e8-7492-0e83-b50bb9b66d48@gmx.de>\n\t<166120927268.3515432.13365033869379884518@Monstersaurus>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"quoted-printable","X-Provags-ID":"V03:K1:xYCBefv1hqyjqdKuSgcBSIX97is/DN0NTmZBOOLW0hr7PR1wEBg\n\tUswzcNcxgq4uOit8mUyWpf7WEdXGY8E5k+o0SrKyaM6w8mtaJ26UEZEvfQCchqomGRHk7b/\n\thbQIDrHqER4fh2+eyv2eaCAbNq4MukDHcsignMTe+tqZJ82WUdd9PlGWaZeIdw3XuMHF9qx\n\tH8BPcE4aZQ5NsGU5jOE1Q==","X-Spam-Flag":"NO","X-UI-Out-Filterresults":"notjunk:1; V03:K0:4riBarSRqVU=:HitOMMOK65P/KuXQzt5Lbm\n\tCmB/oImpA0mwlOOZTlil1IEqVhxXTa0IUKcU11eM3GTY5RTohGyl6vrstDgS4n/BiHnf0dbTA\n\teTj2Q4VcIsHeKFvU6P0AYI4tTGxftQLAtzNDMEz7H4OjvdUlYcFKlF7zswxfBiKR7Z5UzGSxI\n\tM1TXglO5/R/SUzlY6KApAwxtywWvq2AwOlR4o36RFY+ucGLuJpVE1oy3FA5GJvP4cbvJ44BFY\n\tZrv4oEPXaI04DzSrzQwxlg99vhz5OLRyEY+Fpu2kn3efxTtPiBh4P+8vjtE9sNpXsl5lpS6eU\n\trHee6g+DBT6tGWNXzJINHJWDOELqEqOqREdogwwxQMzlCEs42PTo2r+PleCHhRaBrKwB0HqNb\n\trIc2MZZqIpzalR3aU/Wx9AIJquGZ4RUMcrgg4gARDbNmaAYIGoHSj9D9FitegQQ/lbUXqHnE5\n\tOvQ7MxW5hjcBQzJNQqgx+ubUYwjtgNcM4EG9bcG7OgBD5+ZRVQ2xL0JH6Y5TiKZcJbkW+yoxP\n\t1MxlP7vaRxLkYPvdQZFoTfi3cB/+6WIjw+zwl+LrGT/F1o0PHRPl9mB75kkkzVBDAbKosttfg\n\t5l062u3l+Fq7pZwcrrX7Nbwn7BDCvxHJyxDXwBPiapPOn/Cjebv2xCtybzA1mQKUZ7E8/Pbbt\n\tQfUEmrs4Fd6zXshExsOCd1rfjuLKrgzxJ9ng8yTL5JxUznwINnP6HPLWOaE8CaXyNdwbnK8bf\n\tKMreujnuco4nahmFG24stzVEujOZ3DUhaheGPxgdjczMv4LH5tdIQRuWBcwe4K1iaGd3+ooRu\n\t7qhRMrFBnmlBUKm+KW1b2MFBHWXSKSnkxmQ9qe01FitYZZCenhU0rQ9AJ5rzU62110HO8XXYb\n\tj3D1fJuY1g7QwbiIbkjCOI0XDZSPx7t+4aYuFKT0HHpDx1I//hH4cTzGw2xTVW1K+aEeIV0Ha\n\tfXXslhSfDk1sudNOghBodlnheflyrRFE5gsxjReylClROpsOCI5YCkPz7wY7540mVfQXMG8IU\n\tb9kAodig4NfZg7333iqu3UXnib4G3EyMfbRhtnz3LO2ahr4r9yOY0Iy0Df1quHx4YunkEheVu\n\tVhcbX6BWOohHYo0VnYQGCl6zdpJPH/mE8tOKpZXihqGuT+Hr/B7xpbp3KJutsAnKCt9ZndvrX\n\txd9STTH0CuZUFp39pYna5IxUCt","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","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":24990,"web_url":"https://patchwork.libcamera.org/comment/24990/","msgid":"<20220916031231.GB1055504@pyrite.rasen.tech>","date":"2022-09-16T03:12:31","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On Mon, Sep 12, 2022 at 09:37:21PM +0200, Christian Rauch via libcamera-devel wrote:\n> Hi,\n> \n> Not sure what the \"proper\" way is to close or retract proposed patches\n> on mailing lists. I want to mark this patch series, and a couple of\n> previous patches touching the default values, as \"obsolete\", such that\n> they do not appear on patchwork anymore and are not considered for\n> review/merging.\n\nI've marked these as \"superseded\" on patchwork.\n\n\nPaul\n\n> \n> \n> Am 29.08.22 um 22:32 schrieb Christian Rauch via libcamera-devel:\n> > Hi Kieran,\n> >\n> > I replaced the [1/2] patch in this set with a new one (\"libcamera:\n> > control: initialise control info to ControlTypeNone by default\") that\n> > default constructs the min/max/def ControlValue instead of setting it to\n> > 0. This allows explicitly checking for 'ControlTypeNone' instead of\n> > relying on 0 as an implicit representation.\n> >\n> > Best,\n> > Christian\n> >\n> >\n> > Am 23.08.22 um 01:01 schrieb Kieran Bingham:\n> >> Quoting Christian Rauch via libcamera-devel (2022-08-22 23:23:45)\n> >>> Hi Kieran,\n> >>>\n> >>> Regarding all your comments, I simply replaced the \"default\" 0 for \"def\"\n> >>> from the old constructor with explicitly setting it 0 (or its binary\n> >>> counterpart 'false') to match the types. If 0 is the wrong \"def\" value\n> >>> for those controls, then they were already wrong before :-) They were\n> >>> just implicitly wrong. Now you have to explicitly specify a \"def\" value,\n> >>> and I kept 0 for backward compatibility.\n> >>\n> >> Yes, they may have been implicitly incorrect before, but we shouldn't\n> >> set them to be explictly incorrect. So this proposed series means we\n> >> need to consider each one carefully.\n> >>\n> >> As mentioned though, I think we need to work out if all controls make\n> >> sense to have a default value, and if not - how to express that it isn't\n> >> available. I suspect that previously the omision of a default value was\n> >> the expression that the default was 'redundant' (or unnecessary), thus\n> >> by making it required, you need to explicitly set each one.\n> >>\n> >> --\n> >> Kieran\n> >>\n> >>\n> >>\n> >>> Feel free to change the default values. You should also be able to merge\n> >>> this patch (2/2) without the first (1/2).\n> >>>\n> >>> Best,\n> >>> Christian\n> >>>\n> >>>\n> >>> Am 22.08.22 um 23:52 schrieb Kieran Bingham:\n> >>>> Quoting Christian Rauch via libcamera-devel (2022-08-21 13:43:43)\n> >>>>> Explicitly set the default value for all ControlInfo to 0, false or minimum.\n> >>>>>\n> >>>>> Signed-off-by: Christian Rauch \n> >>>>> ---\n> >>>>> src/ipa/raspberrypi/raspberrypi.cpp | 22 ++++++++++---------\n> >>>>> src/ipa/rkisp1/rkisp1.cpp | 10 ++++-----\n> >>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> >>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> >>>>> 4 files changed, 19 insertions(+), 17 deletions(-)\n> >>>>>\n> >>>>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> >>>>> index 69c73f8c..c6360a51 100644\n> >>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >>>>> @@ -74,23 +74,23 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> >>>>>\n> >>>>> /* List of controls handled by the Raspberry Pi IPA */\n> >>>>> static const ControlInfoMap::Map ipaControls{\n> >>>>> - { &controls::AeEnable, ControlInfo(false, true) },\n> >>>>> - { &controls::ExposureTime, ControlInfo(0, 66666) },\n> >>>>> - { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n> >>>>> + { &controls::AeEnable, ControlInfo(false, true, false) },\n> >>>>\n> >>>> I would expect most of the time AutoExposure would be enabled ... so a\n> >>>> default of false seems misleading... Yet - perhaps that's what the 0 was\n> >>>> already implying? So - does that mean a default is unreasonable here?\n> >>>>\n> >>>>\n> >>>>> + { &controls::ExposureTime, ControlInfo(0, 66666, 0) },\n> >>>>> + { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 0.0f) },\n> >>>>\n> >>>> This default is less than the minimum.\n> >>>>\n> >>>>\n> >>>>> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> >>>>> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> >>>>> { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> >>>>> { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> >>>>> - { &controls::AwbEnable, ControlInfo(false, true) },\n> >>>>> - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> >>>>> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n> >>>>\n> >>>> Same as AeEnable.... it doesn't seem reasonble to say the default is\n> >>>> disabled? I think applications probably expect the default IPA behaviour\n> >>>> to have AE/AWB enabled, unless it is disabled explicitly ?\n> >>>>\n> >>>>\n> >>>>> + { &controls::ColourGains, ControlInfo(0.0f, 32.0f, 0.0f) },\n> >>>>> { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> >>>>> { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> >>>>> { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> >>>>> { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> >>>>> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> >>>>> - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> >>>>> + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f, 0.0f) },\n> >>>>> { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> >>>>> - { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n> >>>>> + { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000), INT64_C(0)) },\n> >>>>\n> >>>> Again, this is less than the minimum. A default should probably be\n> >>>> specific to the mode of the sensor too ?\n> >>>>\n> >>>>\n> >>>>> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> >>>>> };\n> >>>>>\n> >>>>> @@ -463,10 +463,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> >>>>> const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;\n> >>>>> ctrlMap[&controls::FrameDurationLimits] =\n> >>>>> ControlInfo(static_cast(minSensorFrameDuration.get()),\n> >>>>> - static_cast(maxSensorFrameDuration.get()));\n> >>>>> + static_cast(maxSensorFrameDuration.get()),\n> >>>>> + int64_t(0));\n> >>>>\n> >>>> This is smaller than the minimum. Perhaps at least here during configure\n> >>>> it could be set to something more specific about the camera mode\n> >>>> capability - or something that is more 'default' like 30 FPS ? (33333 ?)\n> >>>>\n> >>>>>\n> >>>>> ctrlMap[&controls::AnalogueGain] =\n> >>>>> - ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)));\n> >>>>> + ControlInfo(1.0f, static_cast(helper_->gain(maxSensorGainCode_)), 0.0f);\n> >>>>\n> >>>> Less than minimum. I think 1.0f makes sense here too as that's unity\n> >>>> gain.\n> >>>>\n> >>>>>\n> >>>>> /*\n> >>>>> * Calculate the max exposure limit from the frame duration limit as V4L2\n> >>>>> @@ -478,7 +479,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> >>>>>\n> >>>>> ctrlMap[&controls::ExposureTime] =\n> >>>>> ControlInfo(static_cast(helper_->exposure(exposureMin).get()),\n> >>>>> - static_cast(maxShutter.get()));\n> >>>>> + static_cast(maxShutter.get()),\n> >>>>> + int32_t(0));\n> >>>>\n> >>>> I'm not sure an exposure time of 0 makes sense ?, but I'm not sure what\n> >>>> a reasonable default is here either. Maybe something like the maximum of the frame\n> >>>> duration, and maxShutter ?\n> >>>>\n> >>>>>\n> >>>>> result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> >>>>> return 0;\n> >>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> >>>>> index 27b4212b..2121bfd2 100644\n> >>>>> --- a/src/ipa/rkisp1/rkisp1.cpp\n> >>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> >>>>> @@ -91,12 +91,12 @@ namespace {\n> >>>>>\n> >>>>> /* List of controls handled by the RkISP1 IPA */\n> >>>>> const ControlInfoMap::Map rkisp1Controls{\n> >>>>> - { &controls::AeEnable, ControlInfo(false, true) },\n> >>>>> - { &controls::AwbEnable, ControlInfo(false, true) },\n> >>>>> + { &controls::AeEnable, ControlInfo(false, true, false) },\n> >>>>> + { &controls::AwbEnable, ControlInfo(false, true, false) },\n> >>>>\n> >>>> Same issue as in RPi.\n> >>>>\n> >>>>\n> >>>>> { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },\n> >>>>> - { &controls::Brightness, ControlInfo(-1.0f, 0.993f) },\n> >>>>> - { &controls::Contrast, ControlInfo(0.0f, 1.993f) },\n> >>>>> - { &controls::Saturation, ControlInfo(0.0f, 1.993f) },\n> >>>>> + { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },\n> >>>>> + { &controls::Contrast, ControlInfo(0.0f, 1.993f, 0.0f) },\n> >>>>> + { &controls::Saturation, ControlInfo(0.0f, 1.993f, 0.0f) },\n> >>>>\n> >>>> I think 0.0 makes sense here though\n> >>>>\n> >>>>> { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n> >>>>> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> >>>>> };\n> >>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>>> index 9df24603..a1bcfe49 100644\n> >>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>>> @@ -42,7 +42,7 @@ namespace libcamera {\n> >>>>> LOG_DEFINE_CATEGORY(IPU3)\n> >>>>>\n> >>>>> static const ControlInfoMap::Map IPU3Controls = {\n> >>>>> - { &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n> >>>>> + { &controls::draft::PipelineDepth, ControlInfo(2, 3, 0) },\n> >>>>\n> >>>>\n> >>>> This needs looking at. The PipelineDepth isn't something the application\n> >>>> can set. So a default doesn't makes sense, and I'm not sure a min/max\n> >>>> does either.\n> >>>>\n> >>>>\n> >>>>\n> >>>>> };\n> >>>>>\n> >>>>> class IPU3CameraData : public Camera::Private\n> >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>> index e895584d..8fd7634d 100644\n> >>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>>>> @@ -954,7 +954,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> >>>>> /* Add the ScalerCrop control limits based on the current mode. */\n> >>>>> Rectangle ispMinCrop(data->ispMinCropSize_);\n> >>>>> ispMinCrop.scaleBy(data->sensorInfo_.analogCrop.size(), data->sensorInfo_.outputSize);\n> >>>>> - ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()));\n> >>>>> + ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, Rectangle(data->sensorInfo_.analogCrop.size()), ispMinCrop);\n> >>>>\n> >>>> I think a ScalerCrop would probably expect a default of the largest size\n> >>>> possible, not the smallest ? (At least if I were to think about\n> >>>> 'resetting' the ScalerCrop that is).\n> >>>>\n> >>>> I suspect forcing a default value might be better than implicitly\n> >>>> leaving it zero - but it means we need to give more thought to what the\n> >>>> defaults mean or represent - or provide a way for the default to be\n> >>>> 'unset' if it's not appropriate to have one.\n> >>>>\n> >>>> --\n> >>>> Kieran\n> >>>>\n> >>>>\n> >>>>>\n> >>>>> data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n> >>>>>\n> >>>>> --\n> >>>>> 2.34.1\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 386E4C3272\n\tfor ;\n\tFri, 16 Sep 2022 03:12:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8815361FB0;\n\tFri, 16 Sep 2022 05:12:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B93361F82\n\tfor ;\n\tFri, 16 Sep 2022 05:12:39 +0200 (CEST)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C2BCB47C;\n\tFri, 16 Sep 2022 05:12:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1663297961;\n\tbh=O/f+WtQ36R4IsV1G27I7ynvDD0aRjdCbeKgCvkhhM/k=;\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=0rJp1h9QSfsW19QmDrj0PEsDGwwzHgoZxW3Sotszi6mZYa6Bf7E0ELcUnQgwksEDY\n\tl2FgXFz60s9dK4VKKFJ0Wj2+l0u8CI3+uXGxiIYPdDFiD19Hu7N33Ddn/vzTZb/30b\n\t+3xvZD/IFdlRipDkyb0wUA/a89xCGZ8g+gcTdE3JEO3DPPByMXPnohf48B3VjXcXwb\n\tgg8Ryp78T58NNx2J/YQH9tU0HZS3Nm7Q82DkQBj44/8GbPAddsatjxOaGAlOTsU5s4\n\tHlv58VTV6w3SzE23reqr3gEGn2hf+1h2zip24mSP2xIpKn0+hyRdInO9iTwLpWMhtt\n\tXJqzFWbtoasSw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1663297958;\n\tbh=O/f+WtQ36R4IsV1G27I7ynvDD0aRjdCbeKgCvkhhM/k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fTW3mboavwgYxLLrs8h7IkWcUx4eFd5AkQfT5IROX91NjI/zZALgJ4IJbE1m7sLBq\n\tywLSWuW/k7J+m0tawXX3I0egSEqWjeejUzCRIWr4/MzpxqAmrkQuptg9x6RA1egU80\n\tOKCqp56RdsGvBnTkLwyfDcIla4DQhSRxKQKoY0R8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fTW3mboa\"; dkim-atps=neutral","Date":"Fri, 16 Sep 2022 12:12:31 +0900","To":"Christian Rauch ","Message-ID":"<20220916031231.GB1055504@pyrite.rasen.tech>","References":"<20220821124343.487963-1-Rauch.Christian@gmx.de>\n\t<20220821124343.487963-2-Rauch.Christian@gmx.de>\n\t<166120512201.3484129.4999114544766245166@Monstersaurus>\n\t<4868aa82-33e8-7492-0e83-b50bb9b66d48@gmx.de>\n\t<166120927268.3515432.13365033869379884518@Monstersaurus>\n\t\n\t<4b8e1d5f-1313-96a2-5089-9e4f53379007@gmx.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<4b8e1d5f-1313-96a2-5089-9e4f53379007@gmx.de>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: control: explicitly\n\tdefine default control values","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":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]