[{"id":29982,"web_url":"https://patchwork.libcamera.org/comment/29982/","msgid":"<171861771981.2248009.6989387662296441134@ping.linuxembedded.co.uk>","date":"2024-06-17T09:48:39","subject":"Re: [PATCH 11/12] ipa: rkisp1: agc: Correctly clamp maximum shutter\n\tspeed","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-06-16 17:39:09)\n> The sensor's maximum shutter speed is clamped by the maximum frame\n> duration specified in requests. If the requested maximum frame duration\n> is lower than the sensor's minimum shutter speed, the Agc::process()\n> function will pass a minimum value higher than the maximum to the\n> setLimits() function, resulting in an assertion failure. Fix it by\n> clamping the value to both the lower and the upper bounds.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 5b917557b887..0018c43f18cf 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -420,8 +420,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> [](uint32_t x) { return x >> 4; });\n> expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> \n> - utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,\n> - frameContext.agc.maxFrameDuration);\n> + utils::Duration maxShutterSpeed =\n> + std::clamp(frameContext.agc.maxFrameDuration,\n> + context.configuration.sensor.minShutterSpeed,\n> + context.configuration.sensor.maxShutterSpeed);\n> setLimits(context.configuration.sensor.minShutterSpeed,\n> maxShutterSpeed,\n\nI'm somehow confused here, as we're passing sensor.minShutterSpeed in\nhere as well. Does that now become redundant? Or could/should this\nclamping be done within setLimits? Does the other call site of setLimits\nexperience the same issue?\n\n\n\n> context.configuration.sensor.minAnalogueGain,\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 2C26DBD87C\n\tfor ;\n\tMon, 17 Jun 2024 09:48:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 505876548A;\n\tMon, 17 Jun 2024 11:48:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9305E65489\n\tfor ;\n\tMon, 17 Jun 2024 11:48:43 +0200 (CEST)","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 A112E39F;\n\tMon, 17 Jun 2024 11:48:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nKAgBcwJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718617706;\n\tbh=7l57VrSFkVQIUPyZW6+PiUsABpzE2SPDvtzjIjaUuH4=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=nKAgBcwJUxYFUrR6gEHkKhojolRw8VT7ffS3yr0p5B7zp/yvVTcL8nXY5K/v8MAZK\n\tzLlQdE/pZoeiX6q4NzD/Ow2Rxi3HieXKYSkupWKrW5yulelIgJu5Glcd5y6xsumwzQ\n\t9jL3D7+On9AViV2WBfCwE6bobh5X+yowHADgV3Ms=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240616163910.5506-12-laurent.pinchart@ideasonboard.com>","References":"<20240616163910.5506-1-laurent.pinchart@ideasonboard.com>\n\t<20240616163910.5506-12-laurent.pinchart@ideasonboard.com>","Subject":"Re: [PATCH 11/12] ipa: rkisp1: agc: Correctly clamp maximum shutter\n\tspeed","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 17 Jun 2024 10:48:39 +0100","Message-ID":"<171861771981.2248009.6989387662296441134@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":29993,"web_url":"https://patchwork.libcamera.org/comment/29993/","msgid":"","date":"2024-06-17T11:52:01","subject":"Re: [PATCH 11/12] ipa: rkisp1: agc: Correctly clamp maximum shutter\n\tspeed","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, Jun 17, 2024 at 10:48:39AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2024-06-16 17:39:09)\n> > The sensor's maximum shutter speed is clamped by the maximum frame\n> > duration specified in requests. If the requested maximum frame duration\n> > is lower than the sensor's minimum shutter speed, the Agc::process()\n> > function will pass a minimum value higher than the maximum to the\n> > setLimits() function, resulting in an assertion failure. Fix it by\n> > clamping the value to both the lower and the upper bounds.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 6 ++++--\n> > 1 file changed, 4 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 5b917557b887..0018c43f18cf 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -420,8 +420,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > [](uint32_t x) { return x >> 4; });\n> > expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> > \n> > - utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,\n> > - frameContext.agc.maxFrameDuration);\n> > + utils::Duration maxShutterSpeed =\n> > + std::clamp(frameContext.agc.maxFrameDuration,\n> > + context.configuration.sensor.minShutterSpeed,\n> > + context.configuration.sensor.maxShutterSpeed);\n> > setLimits(context.configuration.sensor.minShutterSpeed,\n> > maxShutterSpeed,\n> \n> I'm somehow confused here, as we're passing sensor.minShutterSpeed in\n> here as well. Does that now become redundant? Or could/should this\n> clamping be done within setLimits? Does the other call site of setLimits\n> experience the same issue?\n\nsetLimits() is to configure the ExposureModeHelper, so that you can use\nit to split the exposure duration into gain and shutter time.\nsensor.minShutterSpeed is the minimum shutter speed for the exposure\nmode helper, and the maxShutterSpeed is, well, the maximum.\n\nLooks fine to me.\n\nReviewed-by: Paul Elder \n\n> \n> \n> \n> > context.configuration.sensor.minAnalogueGain,","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 E8777BD87C\n\tfor ;\n\tMon, 17 Jun 2024 11:52:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B0E3E65498;\n\tMon, 17 Jun 2024 13:52:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 92FBF61A1C\n\tfor ;\n\tMon, 17 Jun 2024 13:52:08 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6C0CC39F;\n\tMon, 17 Jun 2024 13:51:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"p9SACNg2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718625111;\n\tbh=J2luACWIFjx1g4TSo2VcG3rm1HlFB/mvV/hluXVF7NY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p9SACNg2jv/cOlS1LJ6JP9quDZeb+fgp/kJNOnzeymrYBtYH9GPebQ3MPCn3kRKfX\n\tteoslhrdsQaG4LbcVbtvj+GmtqXGKXCf1nBHbCkzzuHuuhNeBFD+fN1htDgnbfINmh\n\tNbwdK/rUNd1x5ITO7VKIZcmTIfAmHuIUahOFEDUw=","Date":"Mon, 17 Jun 2024 20:52:01 +0900","From":"Paul Elder ","To":"Kieran Bingham ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 11/12] ipa: rkisp1: agc: Correctly clamp maximum shutter\n\tspeed","Message-ID":"","References":"<20240616163910.5506-1-laurent.pinchart@ideasonboard.com>\n\t<20240616163910.5506-12-laurent.pinchart@ideasonboard.com>\n\t<171861771981.2248009.6989387662296441134@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<171861771981.2248009.6989387662296441134@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":29996,"web_url":"https://patchwork.libcamera.org/comment/29996/","msgid":"<20240617120859.GL30324@pendragon.ideasonboard.com>","date":"2024-06-17T12:08:59","subject":"Re: [PATCH 11/12] ipa: rkisp1: agc: Correctly clamp maximum shutter\n\tspeed","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jun 17, 2024 at 08:52:01PM +0900, Paul Elder wrote:\n> On Mon, Jun 17, 2024 at 10:48:39AM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2024-06-16 17:39:09)\n> > > The sensor's maximum shutter speed is clamped by the maximum frame\n> > > duration specified in requests. If the requested maximum frame duration\n> > > is lower than the sensor's minimum shutter speed, the Agc::process()\n> > > function will pass a minimum value higher than the maximum to the\n> > > setLimits() function, resulting in an assertion failure. Fix it by\n> > > clamping the value to both the lower and the upper bounds.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 6 ++++--\n> > > 1 file changed, 4 insertions(+), 2 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index 5b917557b887..0018c43f18cf 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -420,8 +420,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > [](uint32_t x) { return x >> 4; });\n> > > expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> > > \n> > > - utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,\n> > > - frameContext.agc.maxFrameDuration);\n> > > + utils::Duration maxShutterSpeed =\n> > > + std::clamp(frameContext.agc.maxFrameDuration,\n> > > + context.configuration.sensor.minShutterSpeed,\n> > > + context.configuration.sensor.maxShutterSpeed);\n> > > setLimits(context.configuration.sensor.minShutterSpeed,\n> > > maxShutterSpeed,\n> > \n> > I'm somehow confused here, as we're passing sensor.minShutterSpeed in\n> > here as well. Does that now become redundant? Or could/should this\n> > clamping be done within setLimits? Does the other call site of setLimits\n> > experience the same issue?\n> \n> setLimits() is to configure the ExposureModeHelper, so that you can use\n> it to split the exposure duration into gain and shutter time.\n> sensor.minShutterSpeed is the minimum shutter speed for the exposure\n> mode helper, and the maxShutterSpeed is, well, the maximum.\n\nI got confused too, and implemented code to using the minimum\nFrameDurationLimits to compute the minShutterSpeed. The result was\ninteresting, but clearly wrong :-) That's when I realized that we're\ndoing with shutter speeds here, not frame durations. That prompted me to\nrename the variable in patch 11/12.\n\n> Looks fine to me.\n> \n> Reviewed-by: Paul Elder \n> \n> > > context.configuration.sensor.minAnalogueGain,","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 C2F2DC3237\n\tfor ;\n\tMon, 17 Jun 2024 12:09:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BD0C06549E;\n\tMon, 17 Jun 2024 14:09:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 64B8C65498\n\tfor ;\n\tMon, 17 Jun 2024 14:09:21 +0200 (CEST)","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 1DAA93E;\n\tMon, 17 Jun 2024 14:09:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nfj2lq3I\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718626144;\n\tbh=lh663PfXnovlyP9Ee6EP+aO72gzW/LE6IZoArZP6ivM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nfj2lq3IL8wLNKLJ2czEOy2aSknz6iDgruz6qOEbshnmY9JldM4VnVeVwUv/pRYcS\n\t0KyBSci2HnoVcdbqtf9XmsF4cC3km8WbpU3sfkHivCy/As9woK6TUDtYkBdBnCcrjr\n\tk4QR6G21Do3Y9regWa61r5t8TrQLfto0/hA3nuTM=","Date":"Mon, 17 Jun 2024 15:08:59 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 11/12] ipa: rkisp1: agc: Correctly clamp maximum shutter\n\tspeed","Message-ID":"<20240617120859.GL30324@pendragon.ideasonboard.com>","References":"<20240616163910.5506-1-laurent.pinchart@ideasonboard.com>\n\t<20240616163910.5506-12-laurent.pinchart@ideasonboard.com>\n\t<171861771981.2248009.6989387662296441134@ping.linuxembedded.co.uk>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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":29997,"web_url":"https://patchwork.libcamera.org/comment/29997/","msgid":"<20240617121008.GM30324@pendragon.ideasonboard.com>","date":"2024-06-17T12:10:08","subject":"Re: [PATCH 11/12] ipa: rkisp1: agc: Correctly clamp maximum shutter\n\tspeed","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jun 17, 2024 at 03:09:01PM +0300, Laurent Pinchart wrote:\n> On Mon, Jun 17, 2024 at 08:52:01PM +0900, Paul Elder wrote:\n> > On Mon, Jun 17, 2024 at 10:48:39AM +0100, Kieran Bingham wrote:\n> > > Quoting Laurent Pinchart (2024-06-16 17:39:09)\n> > > > The sensor's maximum shutter speed is clamped by the maximum frame\n> > > > duration specified in requests. If the requested maximum frame duration\n> > > > is lower than the sensor's minimum shutter speed, the Agc::process()\n> > > > function will pass a minimum value higher than the maximum to the\n> > > > setLimits() function, resulting in an assertion failure. Fix it by\n> > > > clamping the value to both the lower and the upper bounds.\n> > > > \n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/agc.cpp | 6 ++++--\n> > > > 1 file changed, 4 insertions(+), 2 deletions(-)\n> > > > \n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > index 5b917557b887..0018c43f18cf 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > @@ -420,8 +420,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > [](uint32_t x) { return x >> 4; });\n> > > > expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> > > > \n> > > > - utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,\n> > > > - frameContext.agc.maxFrameDuration);\n> > > > + utils::Duration maxShutterSpeed =\n> > > > + std::clamp(frameContext.agc.maxFrameDuration,\n> > > > + context.configuration.sensor.minShutterSpeed,\n> > > > + context.configuration.sensor.maxShutterSpeed);\n> > > > setLimits(context.configuration.sensor.minShutterSpeed,\n> > > > maxShutterSpeed,\n> > > \n> > > I'm somehow confused here, as we're passing sensor.minShutterSpeed in\n> > > here as well. Does that now become redundant? Or could/should this\n> > > clamping be done within setLimits? Does the other call site of setLimits\n> > > experience the same issue?\n> > \n> > setLimits() is to configure the ExposureModeHelper, so that you can use\n> > it to split the exposure duration into gain and shutter time.\n> > sensor.minShutterSpeed is the minimum shutter speed for the exposure\n> > mode helper, and the maxShutterSpeed is, well, the maximum.\n> \n> I got confused too, and implemented code to using the minimum\n> FrameDurationLimits to compute the minShutterSpeed. The result was\n> interesting, but clearly wrong :-) That's when I realized that we're\n> doing with shutter speeds here, not frame durations. That prompted me to\n> rename the variable in patch 11/12.\n\nI meant 10/12, sorry\n\n> > Looks fine to me.\n> > \n> > Reviewed-by: Paul Elder \n> > \n> > > > context.configuration.sensor.minAnalogueGain,","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 727B0C3237\n\tfor ;\n\tMon, 17 Jun 2024 12:10:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FE586549E;\n\tMon, 17 Jun 2024 14:10:33 +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 1F0F965498\n\tfor ;\n\tMon, 17 Jun 2024 14:10:31 +0200 (CEST)","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 F2B9C2D5;\n\tMon, 17 Jun 2024 14:10:12 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IOf66+/z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718626213;\n\tbh=8c/oleloKuNCHAPjmu38opDbmAR3snhXiLrUO2iFod8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IOf66+/zHPTPyxWS4W1CY+3ENwDiNJzuJsJetHZqoMP0lUs2ca7Az5tzGitomG+oI\n\tJn3DbeRsAHSTVKwGsuaO/LyHLz33Unj0qxTiZ0vqcVozlTeAPVvrMF8d+6JBUQKHDR\n\tj0Z9b/NyCwaQl7xpoxA0NS7Lbmji6UZG2gNY1178=","Date":"Mon, 17 Jun 2024 15:10:08 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 11/12] ipa: rkisp1: agc: Correctly clamp maximum shutter\n\tspeed","Message-ID":"<20240617121008.GM30324@pendragon.ideasonboard.com>","References":"<20240616163910.5506-1-laurent.pinchart@ideasonboard.com>\n\t<20240616163910.5506-12-laurent.pinchart@ideasonboard.com>\n\t<171861771981.2248009.6989387662296441134@ping.linuxembedded.co.uk>\n\t\n\t<20240617120859.GL30324@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240617120859.GL30324@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":30003,"web_url":"https://patchwork.libcamera.org/comment/30003/","msgid":"<171863096727.2248009.13941593366976231199@ping.linuxembedded.co.uk>","date":"2024-06-17T13:29:27","subject":"Re: [PATCH 11/12] ipa: rkisp1: agc: Correctly clamp maximum shutter\n\tspeed","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-06-17 13:10:08)\n> On Mon, Jun 17, 2024 at 03:09:01PM +0300, Laurent Pinchart wrote:\n> > On Mon, Jun 17, 2024 at 08:52:01PM +0900, Paul Elder wrote:\n> > > On Mon, Jun 17, 2024 at 10:48:39AM +0100, Kieran Bingham wrote:\n> > > > Quoting Laurent Pinchart (2024-06-16 17:39:09)\n> > > > > The sensor's maximum shutter speed is clamped by the maximum frame\n> > > > > duration specified in requests. If the requested maximum frame duration\n> > > > > is lower than the sensor's minimum shutter speed, the Agc::process()\n> > > > > function will pass a minimum value higher than the maximum to the\n> > > > > setLimits() function, resulting in an assertion failure. Fix it by\n> > > > > clamping the value to both the lower and the upper bounds.\n> > > > > \n> > > > > Signed-off-by: Laurent Pinchart \n> > > > > ---\n> > > > > src/ipa/rkisp1/algorithms/agc.cpp | 6 ++++--\n> > > > > 1 file changed, 4 insertions(+), 2 deletions(-)\n> > > > > \n> > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > index 5b917557b887..0018c43f18cf 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > @@ -420,8 +420,10 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > [](uint32_t x) { return x >> 4; });\n> > > > > expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> > > > > \n> > > > > - utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,\n> > > > > - frameContext.agc.maxFrameDuration);\n> > > > > + utils::Duration maxShutterSpeed =\n> > > > > + std::clamp(frameContext.agc.maxFrameDuration,\n> > > > > + context.configuration.sensor.minShutterSpeed,\n> > > > > + context.configuration.sensor.maxShutterSpeed);\n> > > > > setLimits(context.configuration.sensor.minShutterSpeed,\n> > > > > maxShutterSpeed,\n> > > > \n> > > > I'm somehow confused here, as we're passing sensor.minShutterSpeed in\n> > > > here as well. Does that now become redundant? Or could/should this\n> > > > clamping be done within setLimits? Does the other call site of setLimits\n> > > > experience the same issue?\n> > > \n> > > setLimits() is to configure the ExposureModeHelper, so that you can use\n> > > it to split the exposure duration into gain and shutter time.\n> > > sensor.minShutterSpeed is the minimum shutter speed for the exposure\n> > > mode helper, and the maxShutterSpeed is, well, the maximum.\n> > \n> > I got confused too, and implemented code to using the minimum\n> > FrameDurationLimits to compute the minShutterSpeed. The result was\n> > interesting, but clearly wrong :-) That's when I realized that we're\n> > doing with shutter speeds here, not frame durations. That prompted me to\n> > rename the variable in patch 11/12.\n> \n> I meant 10/12, sorry\n\nI've checked the other setLimits caller, and there's no equivalent\nrequired there.\n\n\nReviewed-by: Kieran Bingham \n\n> \n> > > Looks fine to me.\n> > > \n> > > Reviewed-by: Paul Elder \n> > > \n> > > > > context.configuration.sensor.minAnalogueGain,\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 AA989BD87C\n\tfor ;\n\tMon, 17 Jun 2024 13:29:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F97265497;\n\tMon, 17 Jun 2024 15:29:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 494076548B\n\tfor ;\n\tMon, 17 Jun 2024 15:29:30 +0200 (CEST)","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 55B4439F;\n\tMon, 17 Jun 2024 15:29:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"A5BzInX1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718630953;\n\tbh=VQIkIWydHOrcf69fYIC9X6Qxru3CxYJPqXjOTC1+V8M=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=A5BzInX1fEgYbM1E0dFnJ7JSBw1ZdZbJS0R68ngwhhgQ5tExmB9VJSmhR8DV/YBV9\n\tWt8MXou6mCcvAc5pG+/Ogr1dCmXqnV8tnMahw0F8r+ZQKeIVzCEPMg70YnnH2LcSWL\n\tNPJclCH+jzjw5cwluPahPUGhMoZRkeJTaQkkNu+U=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240617121008.GM30324@pendragon.ideasonboard.com>","References":"<20240616163910.5506-1-laurent.pinchart@ideasonboard.com>\n\t<20240616163910.5506-12-laurent.pinchart@ideasonboard.com>\n\t<171861771981.2248009.6989387662296441134@ping.linuxembedded.co.uk>\n\t\n\t<20240617120859.GL30324@pendragon.ideasonboard.com>\n\t<20240617121008.GM30324@pendragon.ideasonboard.com>","Subject":"Re: [PATCH 11/12] ipa: rkisp1: agc: Correctly clamp maximum shutter\n\tspeed","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ,\n\tPaul Elder ","Date":"Mon, 17 Jun 2024 14:29:27 +0100","Message-ID":"<171863096727.2248009.13941593366976231199@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\" "}}]