[{"id":33289,"web_url":"https://patchwork.libcamera.org/comment/33289/","msgid":"","date":"2025-02-04T16:05:06","subject":"Re: [PATCH v1] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for the patch.\n\nOn Tue, 4 Feb 2025 at 15:09, Naushir Patuck wrote:\n>\n> The existing IPA initialisation code did not set default values for\n> some sensor related controls. This caused a crash using libcamerify\n> when the it was trying to access the default value for\n> controls::FrameDurationLimits as part of a recent change.\n>\n> Ensure controls::FrameDurationLimits, controls::AnalogueGain and\n> controls::ExposureTime advertise default values along with the existing\n> min/max values. The default is set to the min value.\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=253\n> Signed-off-by: Naushir Patuck \n\nI suppose there is a bit of a question as to what these defaults mean,\nif anything. And if they don't mean much, then why do we need them.\nBut this is obviously better than crashing, so:\n\nReviewed-by: David Plowman \n\nThough it also feels like we need some better automated testing!! :)\n\nDavid\n\n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index bd3c22000df5..5924df4c1125 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -259,15 +259,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> ControlInfoMap::Map ctrlMap = ipaControls;\n> ctrlMap[&controls::FrameDurationLimits] =\n> ControlInfo(static_cast(mode_.minFrameDuration.get()),\n> - static_cast(mode_.maxFrameDuration.get()));\n> + static_cast(mode_.maxFrameDuration.get()),\n> + static_cast(mode_.minFrameDuration.get()));\n>\n> ctrlMap[&controls::AnalogueGain] =\n> ControlInfo(static_cast(mode_.minAnalogueGain),\n> - static_cast(mode_.maxAnalogueGain));\n> + static_cast(mode_.maxAnalogueGain),\n> + static_cast(mode_.minAnalogueGain));\n>\n> ctrlMap[&controls::ExposureTime] =\n> ControlInfo(static_cast(mode_.minExposureTime.get()),\n> - static_cast(mode_.maxExposureTime.get()));\n> + static_cast(mode_.maxExposureTime.get()),\n> + static_cast(mode_.minExposureTime.get()));\n>\n> /* Declare colour processing related controls for non-mono sensors. */\n> if (!monoSensor_)\n> --\n> 2.43.0\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 640A7C32F1\n\tfor ;\n\tTue, 4 Feb 2025 16:05:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C6FF685B9;\n\tTue, 4 Feb 2025 17:05:19 +0100 (CET)","from mail-qk1-x732.google.com (mail-qk1-x732.google.com\n\t[IPv6:2607:f8b0:4864:20::732])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDAD9685A7\n\tfor ;\n\tTue, 4 Feb 2025 17:05:17 +0100 (CET)","by mail-qk1-x732.google.com with SMTP id\n\taf79cd13be357-7b6f75f61f9so852697585a.0\n\tfor ;\n\tTue, 04 Feb 2025 08:05:17 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"fvZlHhFt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1738685117; x=1739289917;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=VWeAbLwyh0CX8PuEzEub61AOk3U6BjoFwAnodkptlM4=;\n\tb=fvZlHhFtTP3xFbx6o5zNIRW5jmVPHGvWHVyWcyPoW8RLxev3XwgJDNAB1xP8Sp2d4t\n\t82v0egTA3Ur2qJQtDstfAy0jwRT/i/gmFlr4qTse+/TgCKC5Vy00f5L9H4lcBp8HHPg7\n\tQpH7ZSlwyB/J8qDzNrZcyrAsOxceHGuNCV4ugvlcgC0cNPKNZCn3p9HiCZKd0GFp7zw1\n\teDyNX2gQZwCeMwXwUUs0z5p9ScGY1q4rJKNii0iQFFZrOW6Lsu2Jj4+7BZPUhPcq+WtZ\n\t1rvrr7RfBRvUSFXK4AzLPlBro0EMdaZ5atyVQ2RFLt+M3zoeiKeJB2GtLLayfhFFHI0I\n\tKMZg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1738685117; x=1739289917;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=VWeAbLwyh0CX8PuEzEub61AOk3U6BjoFwAnodkptlM4=;\n\tb=WkSJKFpymafPl7/LGBqhP/8qbhHmwHnV+YO5GWfZPphtEp3SodChCAYEM7C8QAOZpj\n\tGen4aPg3tTThrV+FRnO+4ATh3zHRglhHY3A5QQAcU4K07nkzAeMf9BtxfK5F1bqLkZWP\n\t9dcvJKbPrBTMya+LAvU+O2b/xzmrLoL4G8rhyAJRny7ASWy9hFwixoQgS1o73YLkqnd3\n\tgA6JeOR92mqRnJ9StSwhKhuzHbqWpCKT6QyRqrk2f+qXSKRyHu9QmDOkndUg4DhUU/yE\n\t4sOYC+mJXUsgbeG+nASOqlPnFD83aFoRDm+0hNHYw21xxQnCKTdT8/nmmHq42qmA65AY\n\touHg==","X-Gm-Message-State":"AOJu0YwAvgJ8AjrdxQE/SFmXs+YLJIwGEPqyfT3YwdxJ1jP9wlFZ5VzZ\n\t3DfbVW5gg141LuU7j0GsB/y4iFyBxj2AzOOs8N98m2oJ10uc6c0PwiJPBcnCZuxJrkPOUYFf0Gn\n\tEDOdo1h8brMBBlNMrI6nmeeGR+40R162YZkuwOHtAQOqK7o1w","X-Gm-Gg":"ASbGncvfT+YsgDbHpV8xlzetTEpW0Exre4IsV7s7msqb2tGVAplzAAxA4bjfNNgyFaV\n\thj4+HUbgDMFO3qXylPCd7hEBGVeUd5/8HXUSGZmFgsTK4KghfBElhBafxjJYCUEfbT+biWfWg22\n\t8xb5yY3LpMxnOTAabcTb83SuTCiQNH","X-Google-Smtp-Source":"AGHT+IH0w0UfBAS2TlpRTZyI1Lsnzkgt7o5R9N1FWFB8PhyHWrEpk/w6NxmtTOJYzc9yYcN0mtBHcupSyymw6Zua0ik=","X-Received":"by 2002:a05:620a:240a:b0:7be:3965:7452 with SMTP id\n\taf79cd13be357-7bffcd06b08mr4281611085a.32.1738685116670;\n\tTue, 04 Feb 2025 08:05:16 -0800 (PST)","MIME-Version":"1.0","References":"<20250204150912.962121-1-naush@raspberrypi.com>","In-Reply-To":"<20250204150912.962121-1-naush@raspberrypi.com>","From":"David Plowman ","Date":"Tue, 4 Feb 2025 16:05:06 +0000","X-Gm-Features":"AWEUYZkZEaDtKuBkSzsL_wocWB6zjqHrPD30aTLula1ZEKYeY0e1jgPz8O8df7M","Message-ID":"","Subject":"Re: [PATCH v1] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33291,"web_url":"https://patchwork.libcamera.org/comment/33291/","msgid":"<173869438563.3771432.17895961947920206002@ping.linuxembedded.co.uk>","date":"2025-02-04T18:39:45","subject":"Re: [PATCH v1] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2025-02-04 16:05:06)\n> Hi Naush\n> \n> Thanks for the patch.\n> \n> On Tue, 4 Feb 2025 at 15:09, Naushir Patuck wrote:\n> >\n> > The existing IPA initialisation code did not set default values for\n> > some sensor related controls. This caused a crash using libcamerify\n> > when the it was trying to access the default value for\n> > controls::FrameDurationLimits as part of a recent change.\n> >\n> > Ensure controls::FrameDurationLimits, controls::AnalogueGain and\n> > controls::ExposureTime advertise default values along with the existing\n> > min/max values. The default is set to the min value.\n> >\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253\n> > Signed-off-by: Naushir Patuck \n> \n> I suppose there is a bit of a question as to what these defaults mean,\n> if anything. And if they don't mean much, then why do we need them.\n> But this is obviously better than crashing, so:\n> \n> Reviewed-by: David Plowman \n\nYup, I agree, it's hard to know what the 'right' default should be in\nall of these cases, and I think that's why defaults are not enforced on\nthe Control constructors, because sometimes I'm sure in some controls\nit's really not possible to have a default.\n\nBut as this is better than crashing I think it's fine to merge this\npatch - and we should try to determine which controls /require/ a\ndefault value - and find a way to specify (and validate) such things.\n\nReviewed-by: Kieran Bingham \n\n> \n> Though it also feels like we need some better automated testing!! :)\n> \n> David\n> \n> > ---\n> > src/ipa/rpi/common/ipa_base.cpp | 9 ++++++---\n> > 1 file changed, 6 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > index bd3c22000df5..5924df4c1125 100644\n> > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > @@ -259,15 +259,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> > ControlInfoMap::Map ctrlMap = ipaControls;\n> > ctrlMap[&controls::FrameDurationLimits] =\n> > ControlInfo(static_cast(mode_.minFrameDuration.get()),\n> > - static_cast(mode_.maxFrameDuration.get()));\n> > + static_cast(mode_.maxFrameDuration.get()),\n> > + static_cast(mode_.minFrameDuration.get()));\n> >\n> > ctrlMap[&controls::AnalogueGain] =\n> > ControlInfo(static_cast(mode_.minAnalogueGain),\n> > - static_cast(mode_.maxAnalogueGain));\n> > + static_cast(mode_.maxAnalogueGain),\n> > + static_cast(mode_.minAnalogueGain));\n> >\n> > ctrlMap[&controls::ExposureTime] =\n> > ControlInfo(static_cast(mode_.minExposureTime.get()),\n> > - static_cast(mode_.maxExposureTime.get()));\n> > + static_cast(mode_.maxExposureTime.get()),\n> > + static_cast(mode_.minExposureTime.get()));\n> >\n> > /* Declare colour processing related controls for non-mono sensors. */\n> > if (!monoSensor_)\n> > --\n> > 2.43.0\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 16601C326B\n\tfor ;\n\tTue, 4 Feb 2025 18:39:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4DCA8685C1;\n\tTue, 4 Feb 2025 19:39:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A596685A7\n\tfor ;\n\tTue, 4 Feb 2025 19:39:48 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 49C96CDB;\n\tTue, 4 Feb 2025 19:38:36 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sec/uUKn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738694316;\n\tbh=zjy8s70UuSBqAEg7/XF2AyYZF2jEMGpLpa/p5QGB/Wo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=sec/uUKn8QWvCutk+s6x78jxIVIYpohQ4yz5Kjwu/NH7r3ARkIXxeFDwkCW6/xXJg\n\t2T6xQaGMFB8PEXwDtg70Ynxs9Dh0JtPe4yh06Gq20f9qysOTTyBvNgKwZy9OKohD/s\n\tsBBhTbIjNRSDLqieOI2kzSBWDyiR2YAFvrxeIBVY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20250204150912.962121-1-naush@raspberrypi.com>\n\t","Subject":"Re: [PATCH v1] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"David Plowman ,\n\tNaushir Patuck ","Date":"Tue, 04 Feb 2025 18:39:45 +0000","Message-ID":"<173869438563.3771432.17895961947920206002@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33292,"web_url":"https://patchwork.libcamera.org/comment/33292/","msgid":"<20250204192726.GA28733@pendragon.ideasonboard.com>","date":"2025-02-04T19:27:26","subject":"Re: [PATCH v1] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Feb 04, 2025 at 06:39:45PM +0000, Kieran Bingham wrote:\n> Quoting David Plowman (2025-02-04 16:05:06)\n> > Hi Naush\n> > \n> > Thanks for the patch.\n> > \n> > On Tue, 4 Feb 2025 at 15:09, Naushir Patuck wrote:\n> > >\n> > > The existing IPA initialisation code did not set default values for\n> > > some sensor related controls. This caused a crash using libcamerify\n> > > when the it was trying to access the default value for\n> > > controls::FrameDurationLimits as part of a recent change.\n> > >\n> > > Ensure controls::FrameDurationLimits, controls::AnalogueGain and\n> > > controls::ExposureTime advertise default values along with the existing\n> > > min/max values. The default is set to the min value.\n> > >\n> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253\n> > > Signed-off-by: Naushir Patuck \n> > \n> > I suppose there is a bit of a question as to what these defaults mean,\n> > if anything. And if they don't mean much, then why do we need them.\n> > But this is obviously better than crashing, so:\n> > \n> > Reviewed-by: David Plowman \n> \n> Yup, I agree, it's hard to know what the 'right' default should be in\n> all of these cases, and I think that's why defaults are not enforced on\n> the Control constructors, because sometimes I'm sure in some controls\n> it's really not possible to have a default.\n\nBut the IPA module will use a default when no value is specified. If the\ncamera starts with AGC disabled, without specifying manual gain and\nexposure time, then the IPA will pick a value. Sometimes the default\nwill make clear sense, sometimes it will just be arbitrary, but that's\nwhat the default reported in ControlInfo should be.\n\nDoes this patch report the correct values ? If so, it looks good to me.\n\n> But as this is better than crashing I think it's fine to merge this\n> patch - and we should try to determine which controls /require/ a\n> default value - and find a way to specify (and validate) such things.\n> \n> Reviewed-by: Kieran Bingham \n> \n> > Though it also feels like we need some better automated testing!! :)\n> > \n> > David\n> > \n> > > ---\n> > > src/ipa/rpi/common/ipa_base.cpp | 9 ++++++---\n> > > 1 file changed, 6 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > index bd3c22000df5..5924df4c1125 100644\n> > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > @@ -259,15 +259,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> > > ControlInfoMap::Map ctrlMap = ipaControls;\n> > > ctrlMap[&controls::FrameDurationLimits] =\n> > > ControlInfo(static_cast(mode_.minFrameDuration.get()),\n> > > - static_cast(mode_.maxFrameDuration.get()));\n> > > + static_cast(mode_.maxFrameDuration.get()),\n> > > + static_cast(mode_.minFrameDuration.get()));\n> > >\n> > > ctrlMap[&controls::AnalogueGain] =\n> > > ControlInfo(static_cast(mode_.minAnalogueGain),\n> > > - static_cast(mode_.maxAnalogueGain));\n> > > + static_cast(mode_.maxAnalogueGain),\n> > > + static_cast(mode_.minAnalogueGain));\n> > >\n> > > ctrlMap[&controls::ExposureTime] =\n> > > ControlInfo(static_cast(mode_.minExposureTime.get()),\n> > > - static_cast(mode_.maxExposureTime.get()));\n> > > + static_cast(mode_.maxExposureTime.get()),\n> > > + static_cast(mode_.minExposureTime.get()));\n> > >\n> > > /* Declare colour processing related controls for non-mono sensors. */\n> > > if (!monoSensor_)","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 BC140C32F1\n\tfor ;\n\tTue, 4 Feb 2025 19:27:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E7BC2685C2;\n\tTue, 4 Feb 2025 20:27:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 550FB685A7\n\tfor ;\n\tTue, 4 Feb 2025 20:27:31 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B5589A98;\n\tTue, 4 Feb 2025 20:26:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"NYfGev99\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738697178;\n\tbh=ZE0X4/J3cXOjpFeNx4753OSC+I+L8pSYTLTIZaOKFiI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NYfGev99CSvuP/qanf8m9J3HUAFq4lA1jbafzOCwmWZ47VIzyMJxcJyCog0Whg9Mm\n\t4cXBiJFU+DFpRHWJ44inZr4UgwufEUwz/FbS+2hqDkbDqY+jMhXSPuKbu9qYxq//Zm\n\td+MJqzZyV8ux3qKBn9tq6uw52K0m07LzTTZAYJ+o=","Date":"Tue, 4 Feb 2025 21:27:26 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"David Plowman ,\n\tNaushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","Message-ID":"<20250204192726.GA28733@pendragon.ideasonboard.com>","References":"<20250204150912.962121-1-naush@raspberrypi.com>\n\t\n\t<173869438563.3771432.17895961947920206002@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<173869438563.3771432.17895961947920206002@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33293,"web_url":"https://patchwork.libcamera.org/comment/33293/","msgid":"<20250204193723.GB28733@pendragon.ideasonboard.com>","date":"2025-02-04T19:37:23","subject":"Re: [PATCH v1] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Feb 04, 2025 at 09:27:26PM +0200, Laurent Pinchart wrote:\n> On Tue, Feb 04, 2025 at 06:39:45PM +0000, Kieran Bingham wrote:\n> > Quoting David Plowman (2025-02-04 16:05:06)\n> > > Hi Naush\n> > > \n> > > Thanks for the patch.\n> > > \n> > > On Tue, 4 Feb 2025 at 15:09, Naushir Patuck wrote:\n> > > >\n> > > > The existing IPA initialisation code did not set default values for\n> > > > some sensor related controls. This caused a crash using libcamerify\n> > > > when the it was trying to access the default value for\n> > > > controls::FrameDurationLimits as part of a recent change.\n> > > >\n> > > > Ensure controls::FrameDurationLimits, controls::AnalogueGain and\n> > > > controls::ExposureTime advertise default values along with the existing\n> > > > min/max values. The default is set to the min value.\n> > > >\n> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253\n> > > > Signed-off-by: Naushir Patuck \n> > > \n> > > I suppose there is a bit of a question as to what these defaults mean,\n> > > if anything. And if they don't mean much, then why do we need them.\n> > > But this is obviously better than crashing, so:\n> > > \n> > > Reviewed-by: David Plowman \n> > \n> > Yup, I agree, it's hard to know what the 'right' default should be in\n> > all of these cases, and I think that's why defaults are not enforced on\n> > the Control constructors, because sometimes I'm sure in some controls\n> > it's really not possible to have a default.\n> \n> But the IPA module will use a default when no value is specified. If the\n> camera starts with AGC disabled, without specifying manual gain and\n> exposure time, then the IPA will pick a value. Sometimes the default\n> will make clear sense, sometimes it will just be arbitrary, but that's\n> what the default reported in ControlInfo should be.\n> \n> Does this patch report the correct values ? If so, it looks good to me.\n\nLooking at AgcChannel::switchMode() initial fixed exposure time and\nanalogue gain are calculated and seem to produce different values than\nthe ones below. I think more work is needed.\n\n> > But as this is better than crashing I think it's fine to merge this\n> > patch - and we should try to determine which controls /require/ a\n> > default value - and find a way to specify (and validate) such things.\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > > Though it also feels like we need some better automated testing!! :)\n> > > \n> > > David\n> > > \n> > > > ---\n> > > > src/ipa/rpi/common/ipa_base.cpp | 9 ++++++---\n> > > > 1 file changed, 6 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > index bd3c22000df5..5924df4c1125 100644\n> > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > @@ -259,15 +259,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> > > > ControlInfoMap::Map ctrlMap = ipaControls;\n> > > > ctrlMap[&controls::FrameDurationLimits] =\n> > > > ControlInfo(static_cast(mode_.minFrameDuration.get()),\n> > > > - static_cast(mode_.maxFrameDuration.get()));\n> > > > + static_cast(mode_.maxFrameDuration.get()),\n> > > > + static_cast(mode_.minFrameDuration.get()));\n> > > >\n> > > > ctrlMap[&controls::AnalogueGain] =\n> > > > ControlInfo(static_cast(mode_.minAnalogueGain),\n> > > > - static_cast(mode_.maxAnalogueGain));\n> > > > + static_cast(mode_.maxAnalogueGain),\n> > > > + static_cast(mode_.minAnalogueGain));\n> > > >\n> > > > ctrlMap[&controls::ExposureTime] =\n> > > > ControlInfo(static_cast(mode_.minExposureTime.get()),\n> > > > - static_cast(mode_.maxExposureTime.get()));\n> > > > + static_cast(mode_.maxExposureTime.get()),\n> > > > + static_cast(mode_.minExposureTime.get()));\n> > > >\n> > > > /* Declare colour processing related controls for non-mono sensors. */\n> > > > if (!monoSensor_)","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 1FC81C326B\n\tfor ;\n\tTue, 4 Feb 2025 19:37:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E7C2685C2;\n\tTue, 4 Feb 2025 20:37:30 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 21F98685A7\n\tfor ;\n\tTue, 4 Feb 2025 20:37:28 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8D8DCA98;\n\tTue, 4 Feb 2025 20:36:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QHUJeZfu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738697775;\n\tbh=bOisc/pMjKRO9JmhsnRSQ2dcSwvNb6pdDm3c5hSPE9E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QHUJeZfuN+N/6I75zpo/iPUXJgTd0uEVzLehOvj3MAQMjdC8gE7gqDnBd5mlWjRC6\n\tS0K5qhWWTVQCwDzDUXiguHpdy+UNQe3CwFogBR+Eji/1NjY6HeBXA+vDzRt3i8eRET\n\tMs7lzDLD4UCsrHFUEeQtPkFxsU5kRFtttBF0BtWY=","Date":"Tue, 4 Feb 2025 21:37:23 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"David Plowman ,\n\tNaushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","Message-ID":"<20250204193723.GB28733@pendragon.ideasonboard.com>","References":"<20250204150912.962121-1-naush@raspberrypi.com>\n\t\n\t<173869438563.3771432.17895961947920206002@ping.linuxembedded.co.uk>\n\t<20250204192726.GA28733@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250204192726.GA28733@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33300,"web_url":"https://patchwork.libcamera.org/comment/33300/","msgid":"","date":"2025-02-06T09:11:40","subject":"Re: [PATCH v1] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 4 Feb 2025 at 19:37, Laurent Pinchart\n wrote:\n>\n> On Tue, Feb 04, 2025 at 09:27:26PM +0200, Laurent Pinchart wrote:\n> > On Tue, Feb 04, 2025 at 06:39:45PM +0000, Kieran Bingham wrote:\n> > > Quoting David Plowman (2025-02-04 16:05:06)\n> > > > Hi Naush\n> > > >\n> > > > Thanks for the patch.\n> > > >\n> > > > On Tue, 4 Feb 2025 at 15:09, Naushir Patuck wrote:\n> > > > >\n> > > > > The existing IPA initialisation code did not set default values for\n> > > > > some sensor related controls. This caused a crash using libcamerify\n> > > > > when the it was trying to access the default value for\n> > > > > controls::FrameDurationLimits as part of a recent change.\n> > > > >\n> > > > > Ensure controls::FrameDurationLimits, controls::AnalogueGain and\n> > > > > controls::ExposureTime advertise default values along with the existing\n> > > > > min/max values. The default is set to the min value.\n> > > > >\n> > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253\n> > > > > Signed-off-by: Naushir Patuck \n> > > >\n> > > > I suppose there is a bit of a question as to what these defaults mean,\n> > > > if anything. And if they don't mean much, then why do we need them.\n> > > > But this is obviously better than crashing, so:\n> > > >\n> > > > Reviewed-by: David Plowman \n> > >\n> > > Yup, I agree, it's hard to know what the 'right' default should be in\n> > > all of these cases, and I think that's why defaults are not enforced on\n> > > the Control constructors, because sometimes I'm sure in some controls\n> > > it's really not possible to have a default.\n> >\n> > But the IPA module will use a default when no value is specified. If the\n> > camera starts with AGC disabled, without specifying manual gain and\n> > exposure time, then the IPA will pick a value. Sometimes the default\n> > will make clear sense, sometimes it will just be arbitrary, but that's\n> > what the default reported in ControlInfo should be.\n> >\n> > Does this patch report the correct values ? If so, it looks good to me.\n>\n> Looking at AgcChannel::switchMode() initial fixed exposure time and\n> analogue gain are calculated and seem to produce different values than\n> the ones below. I think more work is needed.\n\nI'm not sure that's correct, but It could be because of my\ninterpretation of the default values...\n\nAgcChannel::switchMode() is called at the time of Camera::start().\nThat point seems too late to set the ControlInfo default value. From\na user's perspective, fetching a control default after the camera has\nstarted does not seem intuitive to me. Also noting that the default\nvalue here is the default in manual mode, which is not the default AGC\nmode :) But again, this might just be my interpretation of how/when\ndefault values are accessed by the user.\n\nHowever, I have missed something in this patch - there are specific\ndefault analogue gain, shutter and frame duration values that are\nglobal to the IPA that get setup in ipa::configure(). Instead of\nusing the min values as default, I should explicitly use those values.\n\nI'll post a v2 patch for folks to consider.\n\nRegards,\nNaush\n\n\n>\n> > > But as this is better than crashing I think it's fine to merge this\n> > > patch - and we should try to determine which controls /require/ a\n> > > default value - and find a way to specify (and validate) such things.\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > Though it also feels like we need some better automated testing!! :)\n> > > >\n> > > > David\n> > > >\n> > > > > ---\n> > > > > src/ipa/rpi/common/ipa_base.cpp | 9 ++++++---\n> > > > > 1 file changed, 6 insertions(+), 3 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > index bd3c22000df5..5924df4c1125 100644\n> > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > @@ -259,15 +259,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> > > > > ControlInfoMap::Map ctrlMap = ipaControls;\n> > > > > ctrlMap[&controls::FrameDurationLimits] =\n> > > > > ControlInfo(static_cast(mode_.minFrameDuration.get()),\n> > > > > - static_cast(mode_.maxFrameDuration.get()));\n> > > > > + static_cast(mode_.maxFrameDuration.get()),\n> > > > > + static_cast(mode_.minFrameDuration.get()));\n> > > > >\n> > > > > ctrlMap[&controls::AnalogueGain] =\n> > > > > ControlInfo(static_cast(mode_.minAnalogueGain),\n> > > > > - static_cast(mode_.maxAnalogueGain));\n> > > > > + static_cast(mode_.maxAnalogueGain),\n> > > > > + static_cast(mode_.minAnalogueGain));\n> > > > >\n> > > > > ctrlMap[&controls::ExposureTime] =\n> > > > > ControlInfo(static_cast(mode_.minExposureTime.get()),\n> > > > > - static_cast(mode_.maxExposureTime.get()));\n> > > > > + static_cast(mode_.maxExposureTime.get()),\n> > > > > + static_cast(mode_.minExposureTime.get()));\n> > > > >\n> > > > > /* Declare colour processing related controls for non-mono sensors. */\n> > > > > if (!monoSensor_)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 DCA55C32EA\n\tfor ;\n\tThu, 6 Feb 2025 09:12:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A360D685D8;\n\tThu, 6 Feb 2025 10:12:20 +0100 (CET)","from mail-yb1-xb32.google.com (mail-yb1-xb32.google.com\n\t[IPv6:2607:f8b0:4864:20::b32])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4C11D6858E\n\tfor ;\n\tThu, 6 Feb 2025 10:12:17 +0100 (CET)","by mail-yb1-xb32.google.com with SMTP id\n\t3f1490d57ef6-e5b15f495c5so53548276.3\n\tfor ;\n\tThu, 06 Feb 2025 01:12:17 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"kMcTjLFh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1738833136; x=1739437936;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=cO/+GJLo7fgjnX1NObYZiRWsZzSIRywuSd9gcB60unA=;\n\tb=kMcTjLFhKacOSNtDrtuTc5B7E55jg/QuB8EJ29XzF555jMJT9qvVLeAAqUsFHg35VI\n\tYL/neV29/B8h5BSvcMuzuOJp26E2ZkxgpFlXRfGZ8gQ5W49VuRuOX7eyIEAe6+WbYEld\n\trEkPM4mtY08sBXh2G30Gs2nmvEQADuza6SmbwCMm3FW1j0PSUKEpQJ73HXHQOGslbM4E\n\tqbGTkCEPStxLM/EmgrKZGbCwKUCpOvbNy0cjQnTDbjxyC7jTn+dd0JQtMyG3cZ4V/c1C\n\tr7UXtL6aNBvA1BEGSXx70O8JrBHTIVEw00FFADxQklzYI2gPx4R3c7Iqn+gjXVQTu2X4\n\t9Tnw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1738833136; x=1739437936;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=cO/+GJLo7fgjnX1NObYZiRWsZzSIRywuSd9gcB60unA=;\n\tb=VRol0lWC03lmQGRz2667Xm8ImMdrH7RzV5Wpa4zSjOBsS/GNJqrA3UQSGa0ytGlkv+\n\tIEywUUwm2OPQlKNmlGJtrf0qMITAgxTODYt/KDXeEdszCoVMaxPg88fyc+/Adb0CFmzd\n\taqdzdxAxKqqe7X1MmDtiPWkrDwz6Vd3KS8cG/vMHouo7OWhYTHLIJA41zM2BOxx9f3kD\n\txe3VVcOgnSFhqD1JS3+2VjPyRr7B5tLBXe/4kybNl5bluLJeGtMt8BZlny9ejVuxD/zh\n\tfAAWFumCwERTs2wb4+JLBm7VUFqWNXYjY8UNM0uRe4CjlYCNHJoRabzDIwb6ITQqzc0O\n\tfnHA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUBSnWMV1U+e9DDrY/25ygXkJuMu12sRuoDsCx8gu70GZm6AbEEYIjPioySGCW/KnWHxSMsjXs96d9uUp03ubk=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YyAHwkoyv3Fhl6NG1B9V+oneVufkWeX7AlI1ds1Fru73UF3iYzs\n\t0tXecmRAbVrRUOGsO7g3ZR2CoJm3P0CWmfIfonaL1nxK4c3z5rnXjgZEB/c+NxPNG4qERRHDERv\n\tlAM+RXtOTZ61eXs2GRAoZ2/XSEoZ6TUoY7MxAdA==","X-Gm-Gg":"ASbGnctwQkEoig286WqgKPILlE+0s/KauRHFm7p3AcwSAmTNYkmAvAC8mn/+fEOKNmD\n\tfvnU6pG/3tYXlEB7twQ+zelYXknN9Pd1cPN2sBvn0VDv5dNcIr3tErUHyCel7saiJPQNor76svp\n\tb26v4MxwMrzuByFLjgorqSE3NeRJc=","X-Google-Smtp-Source":"AGHT+IEfgKVV74rhRn4Bb7Au4wVh5+a+EAxthODd60yKBGPk0SJWVJNCpc2v+tuGikomv6RjGET52t0obX4mUzgxNoM=","X-Received":"by 2002:a05:690c:6d82:b0:6f9:7b1e:2b40 with SMTP id\n\t00721157ae682-6f989f6c321mr20160877b3.6.1738833135887;\n\tThu, 06 Feb 2025 01:12:15 -0800 (PST)","MIME-Version":"1.0","References":"<20250204150912.962121-1-naush@raspberrypi.com>\n\t\n\t<173869438563.3771432.17895961947920206002@ping.linuxembedded.co.uk>\n\t<20250204192726.GA28733@pendragon.ideasonboard.com>\n\t<20250204193723.GB28733@pendragon.ideasonboard.com>","In-Reply-To":"<20250204193723.GB28733@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Thu, 6 Feb 2025 09:11:40 +0000","X-Gm-Features":"AWEUYZnS0JvHFrgbQnarDiTNy3l6yUnRVEEGMD_AEm-W7L9WiEsJ4vXmju_A6rA","Message-ID":"","Subject":"Re: [PATCH v1] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","To":"Laurent Pinchart ","Cc":"Kieran Bingham , \n\tDavid Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]