[{"id":27210,"web_url":"https://patchwork.libcamera.org/comment/27210/","msgid":"","date":"2023-06-01T10:33:15","subject":"Re: [libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to\n\thyperfocal on startup","submitter":{"id":130,"url":"https://patchwork.libcamera.org/api/people/130/","name":"Nick Hollinghurst","email":"nick.hollinghurst@raspberrypi.com"},"content":"On Thu, 1 Jun 2023 at 10:56, Naushir Patuck wrote:\n>\n> On the first ipa->configure() call, set the lens position (if a lens is\n> present) to the default position. Typically this would be the hyperfocal\n> position based on the tuning data.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 13 +++++++++++++\n> 1 file changed, 13 insertions(+)\n>\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index 599ad146a863..4413689b7bef 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -199,6 +199,19 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> agcStatus.shutterTime = defaultExposureTime;\n> agcStatus.analogueGain = defaultAnalogueGain;\n> applyAGC(&agcStatus, ctrls);\n> +\n> + /* Set the lens to the default (typically hyperfocal) position on first start. */\n> + RPiController::AfAlgorithm *af =\n> + dynamic_cast(controller_.getAlgorithm(\"af\"));\n> + if (lensPresent_ && af) {\n> + float defaultPos = ipaAfControls.at(&controls::LensPosition).def().get();\n> + ControlList lensCtrl(lensCtrls_);\n> + int32_t hwpos;\n> +\n> + af->setLensPosition(defaultPos, &hwpos);\n> + lensCtrl.set(V4L2_CID_FOCUS_ABSOLUTE, hwpos);\n> + result->lensControls = std::move(lensCtrl);\n> + }\n> }\n>\n> result->sensorControls = std::move(ctrls);\n> --\n> 2.34.1\n>\n\nI think this is good, but it suggests future cleanups:\n\nThere's an opportunity to simplify the RPI AF algorithm, which\ndeliberately supports an initial state in which it doesn't touch the\nlens and doesn't know where it is. If lens position is always\ninitialized, a flag and some code paths can be removed there. But\nthere's currently no way to retrieve \"hwpos\" without either setting a\nlens position or processing a frame; so it might entail a change to\nAfAlgorithm interface.\n\nBased on the existing interface, this patch is doing the best thing.\n\nThere's a complication with the concept of \"default\" too: The tuning\nfile can define defaults for each range (\"normal\", \"macro\", \"full\"),\nbut none of those match the static default and limits in the\nControlInfo. This might be part of a more general issue with control\nlimits, so I don't think it needs addressing here.\n\n Nick","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 D1882C3200\n\tfor ;\n\tThu, 1 Jun 2023 10:33:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5511B62720;\n\tThu, 1 Jun 2023 12:33:29 +0200 (CEST)","from mail-ed1-x531.google.com (mail-ed1-x531.google.com\n\t[IPv6:2a00:1450:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1324860388\n\tfor ;\n\tThu, 1 Jun 2023 12:33:28 +0200 (CEST)","by mail-ed1-x531.google.com with SMTP id\n\t4fb4d7f45d1cf-5149c76f4dbso1086375a12.1\n\tfor ;\n\tThu, 01 Jun 2023 03:33:28 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685615609;\n\tbh=tfUqLkmMHAq0FvGPHf59MPKObMEdJZzxjgmOJ6DbSxk=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=vCXmN6e0Jz6UnaqnrcCemPtSltPD1Pi4AHpEgpHl/edVGD5qSxMyaXf/LuEQJh09J\n\tHmKwGGVK12vqx6wYTHs7WeJAWpUBDaLOqJXXr6w5iSx02pdIuS+aFDYqHmZ1Tan1S9\n\t1sX54OiYo77ag7XDSllKivlUzx5KrDRdrAhF75dVSv2sbi8VYQmvR11uOT4Vidyx6U\n\tdnWUu3jx77Iwp4r+BxDg92cz0mJ4/mPgo7593lbpiD1wi8qzWAKnHY/Sefyxx/oVjV\n\tCciDBo95KvdzpIdmwoCo3t7KWd/ebJcNDnxq86cApZa2DQaTHxWWvc42p+It9dZMrs\n\tjt3NEXTaglVpg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1685615607; x=1688207607;\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=gcXrC1NFYly+6b8HbfxZdqEciCqa3iPP4L1JN1eJolo=;\n\tb=mWRLVTSSRbjbJIr3aBPIJXX9I+9naqFtFtTkvlAWtijswIZfQVayXa/Xl/E5fLNaoj\n\tEd58U89qIN5rc4+kZukfVO3sMU2osPygjP9+9btAFmKZlNrKKPyynKknRFJedQKzki5S\n\tQgJqs3/7GCha3xxAdpMovI7w8VZS5zu22MJGQJnyCcLHQQEN7tBZMQ3loJil/MZJPN65\n\tdCXFZc0GkyQf7G55hToM9v/S1MqGqXwP1o5ulmp60UbmPC+A/T451A4Dad2Xilu10bDf\n\t5GDJO7JaRXxDZcYnj8EEGq5XIdprpDLp6MEJVc9N57rZkr0yy5qHIeSHRtaJ5vWQqbPe\n\tzFGw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"mWRLVTSS\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685615607; x=1688207607;\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=gcXrC1NFYly+6b8HbfxZdqEciCqa3iPP4L1JN1eJolo=;\n\tb=CehZ/QsupUZo8d7+EdPblRcKypg35XSgfghxsIPh7Ysd8Lwyr6RHUnhuJ6z1lPEuRL\n\tn8aFUN4hRWmxIl9vMtFzfyPeYnY13M/iaQYx/3XXyFIjsI0rkfn5v/BgE7gHXO52ba3v\n\twHDlLMnPEaA7Qz78ZiVxUyUlRG3oQ9xmGUFLsYtzApm+OcockKGLcniCjmw7pJgF1jR6\n\tn2fLWltutL+uk6/zxwBgfN3QVtBOrZxR1MAh9e0QkDXITdJXfQvOPzOUPPMMtkFb6Evi\n\tpddbmnWZRAJfFCo1pOHnYLFeG6aJghdITMiTjAPOyNetP/DxdU5rggCsCg+l+8DTMNtT\n\trTyQ==","X-Gm-Message-State":"AC+VfDwSE94Zl5N2wwTR9VeOglfxp5avY+A+zppXW5fOorbi1b61HggE\n\tMlD5cVOwm/Li+xTihCAeYpe17cH9CKqiC7zEKcFMtQ==","X-Google-Smtp-Source":"ACHHUZ4beiJt9i/8avPMeTfBbNaPG+3518uiMfJ2pga4Ly1UCu9YD0XVU8nAzDOEWKcf3T8sSJ3ZXsEfEk0KnYphBuU=","X-Received":"by 2002:aa7:d850:0:b0:514:9dd7:4bcc with SMTP id\n\tf16-20020aa7d850000000b005149dd74bccmr5616830eds.14.1685615607628;\n\tThu, 01 Jun 2023 03:33:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20230601095630.25443-1-naush@raspberrypi.com>\n\t<20230601095630.25443-4-naush@raspberrypi.com>","In-Reply-To":"<20230601095630.25443-4-naush@raspberrypi.com>","Date":"Thu, 1 Jun 2023 11:33:15 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to\n\thyperfocal on startup","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":"Nick Hollinghurst via libcamera-devel\n\t","Reply-To":"Nick Hollinghurst ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27217,"web_url":"https://patchwork.libcamera.org/comment/27217/","msgid":"<20230602064312.GS22609@pendragon.ideasonboard.com>","date":"2023-06-02T06:43:12","subject":"Re: [libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to\n\thyperfocal on startup","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Thu, Jun 01, 2023 at 11:33:15AM +0100, Nick Hollinghurst via libcamera-devel wrote:\n> On Thu, 1 Jun 2023 at 10:56, Naushir Patuck wrote:\n> >\n> > On the first ipa->configure() call, set the lens position (if a lens is\n> > present) to the default position. Typically this would be the hyperfocal\n> > position based on the tuning data.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/rpi/common/ipa_base.cpp | 13 +++++++++++++\n> > 1 file changed, 13 insertions(+)\n> >\n> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > index 599ad146a863..4413689b7bef 100644\n> > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > @@ -199,6 +199,19 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> > agcStatus.shutterTime = defaultExposureTime;\n> > agcStatus.analogueGain = defaultAnalogueGain;\n> > applyAGC(&agcStatus, ctrls);\n> > +\n> > + /* Set the lens to the default (typically hyperfocal) position on first start. */\n> > + RPiController::AfAlgorithm *af =\n> > + dynamic_cast(controller_.getAlgorithm(\"af\"));\n> > + if (lensPresent_ && af) {\n\nYou could avoid looking up the AF algorithm is lensPresent_ is false.\nThat's a small optimization in a non-hot path, so it's not mandatory.\n\n> > + float defaultPos = ipaAfControls.at(&controls::LensPosition).def().get();\n> > + ControlList lensCtrl(lensCtrls_);\n> > + int32_t hwpos;\n> > +\n> > + af->setLensPosition(defaultPos, &hwpos);\n> > + lensCtrl.set(V4L2_CID_FOCUS_ABSOLUTE, hwpos);\n> > + result->lensControls = std::move(lensCtrl);\n\nWill this work as expected if the user sets the AF mode to continuous at\nstart time ? Would it cause the lens to first be moved to the hyperfocal\ndistance, to be overridden right after ?\n\n> > + }\n> > }\n> >\n> > result->sensorControls = std::move(ctrls);\n> \n> I think this is good, but it suggests future cleanups:\n> \n> There's an opportunity to simplify the RPI AF algorithm, which\n> deliberately supports an initial state in which it doesn't touch the\n> lens and doesn't know where it is. If lens position is always\n> initialized, a flag and some code paths can be removed there. But\n> there's currently no way to retrieve \"hwpos\" without either setting a\n> lens position or processing a frame; so it might entail a change to\n> AfAlgorithm interface.\n> \n> Based on the existing interface, this patch is doing the best thing.\n> \n> There's a complication with the concept of \"default\" too: The tuning\n> file can define defaults for each range (\"normal\", \"macro\", \"full\"),\n> but none of those match the static default and limits in the\n> ControlInfo. This might be part of a more general issue with control\n> limits, so I don't think it needs addressing here.","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 E692AC31E9\n\tfor ;\n\tFri, 2 Jun 2023 06:43:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2896D62754;\n\tFri, 2 Jun 2023 08:43:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 99C1E626F8\n\tfor ;\n\tFri, 2 Jun 2023 08:43:13 +0200 (CEST)","from pendragon.ideasonboard.com (om126156168104.26.openmobile.ne.jp\n\t[126.156.168.104])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 38E5FA39;\n\tFri, 2 Jun 2023 08:42:49 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685688195;\n\tbh=NhVHSQ6NF5rqnZ5SZcy6jD3K175+Xu4Ypt5eLSqSCIU=;\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=Idw3HBiyHPIO1vfGVhVhvGYvv9Q0DfWDWCXcXR7zPhTkP2TF0TuBOKEgy9Z5xHLu9\n\tgwv2TfSBjmRFAWFbPRvFrWTFUc76vVqwGaPe6wIIJHpeGkWAJEH8u54+ckaigKjt3O\n\t9TTmXTvS8pO/ZGXchcrjC+r7A4omwOrhlvv0ao4Aj0VVl1EIXBYNG1N1V3cDPaoSLE\n\tWYZNyRrUSPQjYjngBl0Ki7X3KWPD826THQA7b9KXqqC3NmH/YsVwB5Lihr2gJZ1KGh\n\trPoDHiYbtFNs/8y60Cebtxo7oAEJ2lcw+zKr9WGcR/mvmvWJG4c+Hg5zeF6iRXF/sv\n\tDtidjHKAtO6gQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685688171;\n\tbh=NhVHSQ6NF5rqnZ5SZcy6jD3K175+Xu4Ypt5eLSqSCIU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VCFgKMtiJ9PF3r9AuxcJW/3Xxx5LlxopZ+pfQbbQQYbgZk91AqUdzr8W71lQkgALq\n\tklbfHe0yOhqPyn2a8+qwTfjuPBHaeS9MJ7mEv2shfXFmd2JCn1AQ7Td2LwGQdj8ZrD\n\tKtmEPjWdZp2zfr9lNea0ts5X+fFNKE1Ipcf0ulGY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VCFgKMti\"; dkim-atps=neutral","Date":"Fri, 2 Jun 2023 09:43:12 +0300","To":"Nick Hollinghurst ","Message-ID":"<20230602064312.GS22609@pendragon.ideasonboard.com>","References":"<20230601095630.25443-1-naush@raspberrypi.com>\n\t<20230601095630.25443-4-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to\n\thyperfocal on startup","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27223,"web_url":"https://patchwork.libcamera.org/comment/27223/","msgid":"","date":"2023-06-02T08:39:30","subject":"Re: [libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to\n\thyperfocal on startup","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for the feedback!\n\nOn Fri, 2 Jun 2023 at 07:43, Laurent Pinchart\n wrote:\n>\n> Hello,\n>\n> On Thu, Jun 01, 2023 at 11:33:15AM +0100, Nick Hollinghurst via libcamera-devel wrote:\n> > On Thu, 1 Jun 2023 at 10:56, Naushir Patuck wrote:\n> > >\n> > > On the first ipa->configure() call, set the lens position (if a lens is\n> > > present) to the default position. Typically this would be the hyperfocal\n> > > position based on the tuning data.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/ipa/rpi/common/ipa_base.cpp | 13 +++++++++++++\n> > > 1 file changed, 13 insertions(+)\n> > >\n> > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > index 599ad146a863..4413689b7bef 100644\n> > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > @@ -199,6 +199,19 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> > > agcStatus.shutterTime = defaultExposureTime;\n> > > agcStatus.analogueGain = defaultAnalogueGain;\n> > > applyAGC(&agcStatus, ctrls);\n> > > +\n> > > + /* Set the lens to the default (typically hyperfocal) position on first start. */\n> > > + RPiController::AfAlgorithm *af =\n> > > + dynamic_cast(controller_.getAlgorithm(\"af\"));\n> > > + if (lensPresent_ && af) {\n>\n> You could avoid looking up the AF algorithm is lensPresent_ is false.\n> That's a small optimization in a non-hot path, so it's not mandatory.\n\nThat's fine to do. Would you want me to post another patch or can it be done\nwhen applying?\n\n>\n> > > + float defaultPos = ipaAfControls.at(&controls::LensPosition).def().get();\n> > > + ControlList lensCtrl(lensCtrls_);\n> > > + int32_t hwpos;\n> > > +\n> > > + af->setLensPosition(defaultPos, &hwpos);\n> > > + lensCtrl.set(V4L2_CID_FOCUS_ABSOLUTE, hwpos);\n> > > + result->lensControls = std::move(lensCtrl);\n>\n> Will this work as expected if the user sets the AF mode to continuous at\n> start time ? Would it cause the lens to first be moved to the hyperfocal\n> distance, to be overridden right after ?\n\nYes, it does exactly that. It's only a minor inconvenience, and in reality the\nuser is not going to get to see this movement as the startup frames are dropped\nanyway.\n\nRegards,\nNaush\n\n>\n> > > + }\n> > > }\n> > >\n> > > result->sensorControls = std::move(ctrls);\n> >\n> > I think this is good, but it suggests future cleanups:\n> >\n> > There's an opportunity to simplify the RPI AF algorithm, which\n> > deliberately supports an initial state in which it doesn't touch the\n> > lens and doesn't know where it is. If lens position is always\n> > initialized, a flag and some code paths can be removed there. But\n> > there's currently no way to retrieve \"hwpos\" without either setting a\n> > lens position or processing a frame; so it might entail a change to\n> > AfAlgorithm interface.\n> >\n> > Based on the existing interface, this patch is doing the best thing.\n> >\n> > There's a complication with the concept of \"default\" too: The tuning\n> > file can define defaults for each range (\"normal\", \"macro\", \"full\"),\n> > but none of those match the static default and limits in the\n> > ControlInfo. This might be part of a more general issue with control\n> > limits, so I don't think it needs addressing here.\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 23D81C3200\n\tfor ;\n\tFri, 2 Jun 2023 08:39:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 84D6E62720;\n\tFri, 2 Jun 2023 10:39:37 +0200 (CEST)","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 DE5FA626F8\n\tfor ;\n\tFri, 2 Jun 2023 10:39:35 +0200 (CEST)","by mail-yb1-xb32.google.com with SMTP id\n\t3f1490d57ef6-ba827a34ba8so1963682276.0\n\tfor ;\n\tFri, 02 Jun 2023 01:39:35 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685695177;\n\tbh=G2Iwmpn/FpcZNRomwWHY329zrwAgNOKStzs5AxI0jeE=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=kZ3NcChXCRzAreeTb5HBCqSTscsOTNur7plST14m+S9DlHV2AcMSWmcqJsAB9Z1Ms\n\tBbsEP8S/M+kurrEKl5kCiFUjD2EzTVdf02jMJFNynQEBVrExes8O3/m3ExqJoJkv6M\n\toLgNprbQN+o2YCXh+OHjdRTqLH2zSevSHNiNw9RHEmesUs6hl3p/SRMk8jVnc5iMnM\n\twc+d7cgcL25cx+jKlWAwIkMO0Ndxn/TbyKYpfyQfQBCkbJ0ZDm2t38FgFSpH93T5ny\n\t8GW7WRvAQXgqfR5ENBfWhd88nDQ7R8lFSzncWHJ/flMiWpJ8pjVMFw6HwKbFGRNVfk\n\tXcFyhIALVzDMg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1685695174; x=1688287174;\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=HaaHQA7e+hMEtE2c3hxzYZqXUuFxN6H+mQ3L4zDAl40=;\n\tb=Bh7z5qEys1gmBWcjERTc8oAwNVK/jjaMQO2ke466qaQPm72unoqWf1xsYGTajnCv6z\n\t7r7vbOQvEQy24f2FICsHKYqOx1ImZdhXD7NyjddXglu2M+JkGVPzZxj7AMGJu02eRmXq\n\tIRCPQrJ0Z851jbIWDG47ZQ/l7M4jvXYeyJXIdDqa8NDw59Srk88C5Uv7470HlX/cnj/5\n\thutggETi1BVEK/9j2oMQKihL4yG5NmhlPuD8o8j7+UJ91HNaHsrGiA3nGmr4QCm4aHhF\n\taCBJjWT5arimCgQMEloqA61yOXfof6Oi17IYeR9UiiVP20c4Jjs43/FtKoBCRgiuiG5M\n\tyfgw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Bh7z5qEy\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685695174; x=1688287174;\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=HaaHQA7e+hMEtE2c3hxzYZqXUuFxN6H+mQ3L4zDAl40=;\n\tb=ihKykUYRQ0qbhG++X7ZStF7Y0+kdLWNX0lC0utXFab9pVc8btLt7vy7M5ODA6hChbN\n\tZ8BvH1eXDiHKVSIqNswx1is73AYmIzHEiyaDTh4YW4q9lS8j4QFei72yQkWWpaj5cYBe\n\t21DnE6p/yqp6Z+kqo9RcP1ySWH2jv3upBamEPoKsSKp+v9SwWxRCWCFcmQCun4RQQW9I\n\t4z4r73QYJ0cwXjZJBEgjt7TB0vKvl72bDaenSK/8xEF1csyhShXrHpVnhgnfyui1r4CA\n\tAu32DohTIcg5jBjuYJCzjcq9ZlsPCVPbRzTj4n8pduXNNnTUraj1Qnmku0152xA8ZUEs\n\tB13A==","X-Gm-Message-State":"AC+VfDy6BfURasqm2ZGIfEgWP+dRaZROqnDOvcqUddJKwqBVg8gYHxUU\n\tVulA/jA9CG0OMC79DhQcTqcVXKeDl9uOUEa9us05kUjBEQDR6Dx17AvwDA==","X-Google-Smtp-Source":"ACHHUZ6kf1uvYrb4g+RsGAVWFjUpowdm1EgS+dWG5qCCwU15kKuAOWhFY1wJSvIWmd3EycVuqwZ5IGETUvDxOFWAkTo=","X-Received":"by 2002:a0d:edc1:0:b0:552:a63e:18d1 with SMTP id\n\tw184-20020a0dedc1000000b00552a63e18d1mr10090814ywe.52.1685695174647;\n\tFri, 02 Jun 2023 01:39:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20230601095630.25443-1-naush@raspberrypi.com>\n\t<20230601095630.25443-4-naush@raspberrypi.com>\n\t\n\t<20230602064312.GS22609@pendragon.ideasonboard.com>","In-Reply-To":"<20230602064312.GS22609@pendragon.ideasonboard.com>","Date":"Fri, 2 Jun 2023 09:39:30 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to\n\thyperfocal on startup","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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27248,"web_url":"https://patchwork.libcamera.org/comment/27248/","msgid":"<20230605075328.GB30841@pendragon.ideasonboard.com>","date":"2023-06-05T07:53:28","subject":"Re: [libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to\n\thyperfocal on startup","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Fri, Jun 02, 2023 at 09:39:30AM +0100, Naushir Patuck wrote:\n> On Fri, 2 Jun 2023 at 07:43, Laurent Pinchart wrote:\n> > On Thu, Jun 01, 2023 at 11:33:15AM +0100, Nick Hollinghurst via libcamera-devel wrote:\n> > > On Thu, 1 Jun 2023 at 10:56, Naushir Patuck wrote:\n> > > >\n> > > > On the first ipa->configure() call, set the lens position (if a lens is\n> > > > present) to the default position. Typically this would be the hyperfocal\n> > > > position based on the tuning data.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > src/ipa/rpi/common/ipa_base.cpp | 13 +++++++++++++\n> > > > 1 file changed, 13 insertions(+)\n> > > >\n> > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > index 599ad146a863..4413689b7bef 100644\n> > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > @@ -199,6 +199,19 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> > > > agcStatus.shutterTime = defaultExposureTime;\n> > > > agcStatus.analogueGain = defaultAnalogueGain;\n> > > > applyAGC(&agcStatus, ctrls);\n> > > > +\n> > > > + /* Set the lens to the default (typically hyperfocal) position on first start. */\n> > > > + RPiController::AfAlgorithm *af =\n> > > > + dynamic_cast(controller_.getAlgorithm(\"af\"));\n> > > > + if (lensPresent_ && af) {\n> >\n> > You could avoid looking up the AF algorithm is lensPresent_ is false.\n> > That's a small optimization in a non-hot path, so it's not mandatory.\n> \n> That's fine to do. Would you want me to post another patch or can it be done\n> when applying?\n\nAs it changes the structure of the code a bit, I'd prefer a new version.\nI don't want to risk making a mistake with an untested change (I've made\nenough of those already).\n\n> > > > + float defaultPos = ipaAfControls.at(&controls::LensPosition).def().get();\n> > > > + ControlList lensCtrl(lensCtrls_);\n> > > > + int32_t hwpos;\n> > > > +\n> > > > + af->setLensPosition(defaultPos, &hwpos);\n> > > > + lensCtrl.set(V4L2_CID_FOCUS_ABSOLUTE, hwpos);\n> > > > + result->lensControls = std::move(lensCtrl);\n> >\n> > Will this work as expected if the user sets the AF mode to continuous at\n> > start time ? Would it cause the lens to first be moved to the hyperfocal\n> > distance, to be overridden right after ?\n> \n> Yes, it does exactly that. It's only a minor inconvenience, and in reality the\n> user is not going to get to see this movement as the startup frames are dropped\n> anyway.\n\nI suppose we can enhance it later if needed.\n\n> > > > + }\n> > > > }\n> > > >\n> > > > result->sensorControls = std::move(ctrls);\n> > >\n> > > I think this is good, but it suggests future cleanups:\n> > >\n> > > There's an opportunity to simplify the RPI AF algorithm, which\n> > > deliberately supports an initial state in which it doesn't touch the\n> > > lens and doesn't know where it is. If lens position is always\n> > > initialized, a flag and some code paths can be removed there. But\n> > > there's currently no way to retrieve \"hwpos\" without either setting a\n> > > lens position or processing a frame; so it might entail a change to\n> > > AfAlgorithm interface.\n> > >\n> > > Based on the existing interface, this patch is doing the best thing.\n> > >\n> > > There's a complication with the concept of \"default\" too: The tuning\n> > > file can define defaults for each range (\"normal\", \"macro\", \"full\"),\n> > > but none of those match the static default and limits in the\n> > > ControlInfo. This might be part of a more general issue with control\n> > > limits, so I don't think it needs addressing here.","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 03CBCC31E9\n\tfor ;\n\tMon, 5 Jun 2023 07:53:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC64F6287F;\n\tMon, 5 Jun 2023 09:53:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5BD3C60579\n\tfor ;\n\tMon, 5 Jun 2023 09:53:36 +0200 (CEST)","from pendragon.ideasonboard.com (om126156242094.26.openmobile.ne.jp\n\t[126.156.242.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 161632BC;\n\tMon, 5 Jun 2023 09:53:07 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685951617;\n\tbh=ByFuIdYpXe1/ZA/X3XKMDPuEulEK7GIMrMK1eaxMpN0=;\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=JdQF5vQCE611LmDWM3yr6dWJf7InjdzRoV3GPojy7Xa/kcsfakiOR48x08dYa/o7/\n\t9AYR+9ADI+hLymTiz/UtEbd3q+MCZudqybgt0WKZbDyd2YROIpZzmlb96esJVPl5M6\n\tJXd/EOgSJ6vIqVkwEwDQBzNBYnYpuHwr57koufWyiFPOL4fZtEFniDtrVUIwDRbxa8\n\tN+lQiwtL9HdMLbFp1ph9XEk+Q9fdt+tn1jBB7Oh1L163YT8DjDzMOgud+IYZQeczw8\n\tpNxPvRc6CVwxyZwuAODlpwbODtuijM+mjr5cHae8LiCjO3YTSLqnbBxuHtXvfHJ/mC\n\thqyPp8i6jJTPA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685951591;\n\tbh=ByFuIdYpXe1/ZA/X3XKMDPuEulEK7GIMrMK1eaxMpN0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=F6Rw4TMXGZZlY7ksvbFf8PJAqjI1ei10EeSft2Md4b4oY3m0yRbBPc36KlwBxi2ay\n\tR6myekFQdSTVGkrZo64zLF9Ni9mkddrcCdg8ZZ+h/118EyQ9N4JD2OiIrgFcErGdpZ\n\tAbgmGHMzbs488yEz0cpLZI60PqGRfY6Nv2/A6iQQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"F6Rw4TMX\"; dkim-atps=neutral","Date":"Mon, 5 Jun 2023 10:53:28 +0300","To":"Naushir Patuck ","Message-ID":"<20230605075328.GB30841@pendragon.ideasonboard.com>","References":"<20230601095630.25443-1-naush@raspberrypi.com>\n\t<20230601095630.25443-4-naush@raspberrypi.com>\n\t\n\t<20230602064312.GS22609@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to\n\thyperfocal on startup","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27316,"web_url":"https://patchwork.libcamera.org/comment/27316/","msgid":"","date":"2023-06-12T09:08:43","subject":"Re: [libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to\n\thyperfocal on startup","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nI just wanted to pick up Nick's points, which I think are valid.\n\nThe default behaviour that you might want will often be hyperfocal,\nbut not always. Particularly if, as Nick said, the AfRange were set to\nmacro you'd probably want to define a specific default if the AF\nalgorithm is put into Macro on startup.\n\nI'm always a bit paranoid about unnecessary lens movements too. As\nthings stand, if you wanted a different lens position, you couldn't\navoid sending the lens to hyperfocal, and then to the place that you\nwanted. Could we delay the default movement until the camera starts,\nthen we can replace this by a single movement? It also lets us do\nthings like define a default which is appropriate for the range that\nis being set, and gives an application control of it.\n\nIt would also help when you wanted to restart your camera app, but\nleave the lens wherever it was when the app last finished. Delaying\nthe default movement until the user has a chance to set the lens\nposition where it was previously means you could avoid moving it\naltogether. (There might still be a slight problem in finding out\nwhere the lens ended up \"last time\"?)\n\nAny thoughts?\n\nThanks!\nDavid\n\nOn Mon, 5 Jun 2023 at 08:53, Laurent Pinchart via libcamera-devel\n wrote:\n>\n> Hi Naush,\n>\n> On Fri, Jun 02, 2023 at 09:39:30AM +0100, Naushir Patuck wrote:\n> > On Fri, 2 Jun 2023 at 07:43, Laurent Pinchart wrote:\n> > > On Thu, Jun 01, 2023 at 11:33:15AM +0100, Nick Hollinghurst via libcamera-devel wrote:\n> > > > On Thu, 1 Jun 2023 at 10:56, Naushir Patuck wrote:\n> > > > >\n> > > > > On the first ipa->configure() call, set the lens position (if a lens is\n> > > > > present) to the default position. Typically this would be the hyperfocal\n> > > > > position based on the tuning data.\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > ---\n> > > > > src/ipa/rpi/common/ipa_base.cpp | 13 +++++++++++++\n> > > > > 1 file changed, 13 insertions(+)\n> > > > >\n> > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > index 599ad146a863..4413689b7bef 100644\n> > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > @@ -199,6 +199,19 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> > > > > agcStatus.shutterTime = defaultExposureTime;\n> > > > > agcStatus.analogueGain = defaultAnalogueGain;\n> > > > > applyAGC(&agcStatus, ctrls);\n> > > > > +\n> > > > > + /* Set the lens to the default (typically hyperfocal) position on first start. */\n> > > > > + RPiController::AfAlgorithm *af =\n> > > > > + dynamic_cast(controller_.getAlgorithm(\"af\"));\n> > > > > + if (lensPresent_ && af) {\n> > >\n> > > You could avoid looking up the AF algorithm is lensPresent_ is false.\n> > > That's a small optimization in a non-hot path, so it's not mandatory.\n> >\n> > That's fine to do. Would you want me to post another patch or can it be done\n> > when applying?\n>\n> As it changes the structure of the code a bit, I'd prefer a new version.\n> I don't want to risk making a mistake with an untested change (I've made\n> enough of those already).\n>\n> > > > > + float defaultPos = ipaAfControls.at(&controls::LensPosition).def().get();\n> > > > > + ControlList lensCtrl(lensCtrls_);\n> > > > > + int32_t hwpos;\n> > > > > +\n> > > > > + af->setLensPosition(defaultPos, &hwpos);\n> > > > > + lensCtrl.set(V4L2_CID_FOCUS_ABSOLUTE, hwpos);\n> > > > > + result->lensControls = std::move(lensCtrl);\n> > >\n> > > Will this work as expected if the user sets the AF mode to continuous at\n> > > start time ? Would it cause the lens to first be moved to the hyperfocal\n> > > distance, to be overridden right after ?\n> >\n> > Yes, it does exactly that. It's only a minor inconvenience, and in reality the\n> > user is not going to get to see this movement as the startup frames are dropped\n> > anyway.\n>\n> I suppose we can enhance it later if needed.\n>\n> > > > > + }\n> > > > > }\n> > > > >\n> > > > > result->sensorControls = std::move(ctrls);\n> > > >\n> > > > I think this is good, but it suggests future cleanups:\n> > > >\n> > > > There's an opportunity to simplify the RPI AF algorithm, which\n> > > > deliberately supports an initial state in which it doesn't touch the\n> > > > lens and doesn't know where it is. If lens position is always\n> > > > initialized, a flag and some code paths can be removed there. But\n> > > > there's currently no way to retrieve \"hwpos\" without either setting a\n> > > > lens position or processing a frame; so it might entail a change to\n> > > > AfAlgorithm interface.\n> > > >\n> > > > Based on the existing interface, this patch is doing the best thing.\n> > > >\n> > > > There's a complication with the concept of \"default\" too: The tuning\n> > > > file can define defaults for each range (\"normal\", \"macro\", \"full\"),\n> > > > but none of those match the static default and limits in the\n> > > > ControlInfo. This might be part of a more general issue with control\n> > > > limits, so I don't think it needs addressing here.\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 78F9EC3220\n\tfor ;\n\tMon, 12 Jun 2023 09:08:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BB2BA61E4B;\n\tMon, 12 Jun 2023 11:08:56 +0200 (CEST)","from mail-oa1-x36.google.com (mail-oa1-x36.google.com\n\t[IPv6:2001:4860:4864:20::36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E0F4561E48\n\tfor ;\n\tMon, 12 Jun 2023 11:08:55 +0200 (CEST)","by mail-oa1-x36.google.com with SMTP id\n\t586e51a60fabf-1a694ac3238so382155fac.3\n\tfor ;\n\tMon, 12 Jun 2023 02:08:55 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1686560936;\n\tbh=DNYoE8KW8b9zEl4ERxBqOfdiXhWySUlvzC5I5EBwCMM=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=kxqdZvEv66C+hXWDiNRDpzCgelicrXg9PFa+3EVSRqL00H1QLBjWmuM97O6S1Qhv5\n\tTfw7ziq40aObC/zWxpSpGmXfdNtIhMyAqTrYOEaKms9JADaBe+6FPKkEprq7i+n04W\n\t87b29qaGP+jjcHpIB2teUz24SemPxcqO70Zpz120ry1h1qCd40Ohf2TL20Yl6GaS4M\n\tWp84oJFZWxXQk1PMfHy4glrRuThkor27BmmrdaJA8EIhn8ppNpjNip3IQH17pnbpK8\n\tXWFUSHAux1AKWIgrq/l9UtnYu9Is8zr6ewovfCac83SvyL4pimKh0h4/iGohYTAs8W\n\tMsd/japhFFPCA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1686560934; x=1689152934;\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=wcZaluByeb/EKOb9mXbcrKZYdhTV+G5d+YHX+ADDRgE=;\n\tb=cXhutkO4ghrwdW0LmcI362g5oLcT2FdvK1tS0T6CqSxYXMkDQjYT9bUYS5scfZP3bd\n\tQl6LMpg6GUCG+MDbDhLOPY4AwWWOxEjTlbU3+vO66KXYkWw1Rs4/kGL+Dq3DmisbGw1d\n\ta/lZUtXnB9Sp0IiACmAxDOXwY0Z7d8YPL4kJDJz+uM6TxOeivBGc+dlhq2ejtpBSIavu\n\tNSF6TFQaVEOezOePlky1gAZJM+GWenMW7AcUvHFbL8utca/91ewHeckiSGWCJicez9TB\n\tjL6kHHwOlueFGwDzY7KbAMD++vqfpnVSq1hQehuxfyMymQgclijGZjyaXX1JfkVz0hg2\n\tVKtA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"cXhutkO4\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1686560934; x=1689152934;\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=wcZaluByeb/EKOb9mXbcrKZYdhTV+G5d+YHX+ADDRgE=;\n\tb=MLiFdI94R7svKMYPQTqzS+FUDh9N9WCKZnzYouASL7RuKiuC1t5PiA5XZPxFL5M5mg\n\tW4WU+NI2+Doi9g38AzngyuyfjlCQLD976KpZkTLyxN9pty55M8IrXQqHcI2nzJRutCXl\n\tpUeaJXP+vmSIz6BxWbMP7ZyxPK+WgbSpgiPyhI0gf6wzcuhlbWnvIUJtjDBxh6FJSd5h\n\tfMPkzk3tC6oaKHCSGxDPmBgLWTlWzohQMC1mqMJwsz1Ytm5bPI2TU7x2+uHzn3VfyBQ2\n\t+zBfKulpdgqVq5fb7vz4Zp8ZPJh8JRsggZ9Pd7PM72gd2kABNJYRi10juzrs/AaESJ7q\n\t3P+g==","X-Gm-Message-State":"AC+VfDzebOl1E9zCq+G/heSq1LeRPRhg5f0JMxYxj15X9EbxhVklr48+\n\toNroYLXq3iLLmsK6QJ5RcSd+LHswjFWPCpsikxemiw==","X-Google-Smtp-Source":"ACHHUZ40XTwxfgvfM4hJ1i3rdVGN5WG+EMwuRSJM1hFeM3AC1tpqStgtTvgEtyoAP9vRFvSD+IzHSW/m3aNnaO1k4Fs=","X-Received":"by 2002:a05:6870:d450:b0:180:857:d47d with SMTP id\n\tj16-20020a056870d45000b001800857d47dmr5163165oag.57.1686560934587;\n\tMon, 12 Jun 2023 02:08:54 -0700 (PDT)","MIME-Version":"1.0","References":"<20230601095630.25443-1-naush@raspberrypi.com>\n\t<20230601095630.25443-4-naush@raspberrypi.com>\n\t\n\t<20230602064312.GS22609@pendragon.ideasonboard.com>\n\t\n\t<20230605075328.GB30841@pendragon.ideasonboard.com>","In-Reply-To":"<20230605075328.GB30841@pendragon.ideasonboard.com>","Date":"Mon, 12 Jun 2023 10:08:43 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to\n\thyperfocal on startup","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":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27341,"web_url":"https://patchwork.libcamera.org/comment/27341/","msgid":"<168673771098.2889415.7227611072964129625@Monstersaurus>","date":"2023-06-14T10:15:10","subject":"Re: [libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to\n\thyperfocal on startup","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman via libcamera-devel (2023-06-12 10:08:43)\n> Hi everyone\n> \n> I just wanted to pick up Nick's points, which I think are valid.\n> \n> The default behaviour that you might want will often be hyperfocal,\n> but not always. Particularly if, as Nick said, the AfRange were set to\n> macro you'd probably want to define a specific default if the AF\n> algorithm is put into Macro on startup.\n> \n> I'm always a bit paranoid about unnecessary lens movements too. As\n> things stand, if you wanted a different lens position, you couldn't\n> avoid sending the lens to hyperfocal, and then to the place that you\n> wanted. Could we delay the default movement until the camera starts,\n\nIt sounds reasonable that the first update to the lens position could be\nset during camera->start() - perhaps by having a hook into something\nlike CameraLens->start() which would be the first point any actual\nchange is made?\n\n(I wonder if this means we should also have a CameraSensor->start() ....\nwould that let us clean up controls that currently get passed during\ncamera->start()?)\n\nBut are there any conditions where this would be undesirable? Do we need\nto wait for the lens to settle before handling any 3a for instance? I\nexpect 'most' of the AWB/AEGC shouldn't be too affected by the focus...\n\n> then we can replace this by a single movement? It also lets us do\n> things like define a default which is appropriate for the range that\n> is being set, and gives an application control of it.\n\nI could expect that during configure the CameraLens class could be\nconfigured accordingly indeed. It would then cache the most recent state\nuntil start, and would mean multiple configure calls wouldn't actually\ncause an unnecessary movement...\n\n\n> It would also help when you wanted to restart your camera app, but\n> leave the lens wherever it was when the app last finished. Delaying\n> the default movement until the user has a chance to set the lens\n> position where it was previously means you could avoid moving it\n> altogether. (There might still be a slight problem in finding out\n> where the lens ended up \"last time\"?)\n\nI think that would be up to the camera application to save metadata\nstate and continue from there if it desired.\n\n--\nKieran\n\n\n> \n> Any thoughts?\n> \n> Thanks!\n> David\n> \n> On Mon, 5 Jun 2023 at 08:53, Laurent Pinchart via libcamera-devel\n> wrote:\n> >\n> > Hi Naush,\n> >\n> > On Fri, Jun 02, 2023 at 09:39:30AM +0100, Naushir Patuck wrote:\n> > > On Fri, 2 Jun 2023 at 07:43, Laurent Pinchart wrote:\n> > > > On Thu, Jun 01, 2023 at 11:33:15AM +0100, Nick Hollinghurst via libcamera-devel wrote:\n> > > > > On Thu, 1 Jun 2023 at 10:56, Naushir Patuck wrote:\n> > > > > >\n> > > > > > On the first ipa->configure() call, set the lens position (if a lens is\n> > > > > > present) to the default position. Typically this would be the hyperfocal\n> > > > > > position based on the tuning data.\n> > > > > >\n> > > > > > Signed-off-by: Naushir Patuck \n> > > > > > ---\n> > > > > > src/ipa/rpi/common/ipa_base.cpp | 13 +++++++++++++\n> > > > > > 1 file changed, 13 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > index 599ad146a863..4413689b7bef 100644\n> > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > > > @@ -199,6 +199,19 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> > > > > > agcStatus.shutterTime = defaultExposureTime;\n> > > > > > agcStatus.analogueGain = defaultAnalogueGain;\n> > > > > > applyAGC(&agcStatus, ctrls);\n> > > > > > +\n> > > > > > + /* Set the lens to the default (typically hyperfocal) position on first start. */\n> > > > > > + RPiController::AfAlgorithm *af =\n> > > > > > + dynamic_cast(controller_.getAlgorithm(\"af\"));\n> > > > > > + if (lensPresent_ && af) {\n> > > >\n> > > > You could avoid looking up the AF algorithm is lensPresent_ is false.\n> > > > That's a small optimization in a non-hot path, so it's not mandatory.\n> > >\n> > > That's fine to do. Would you want me to post another patch or can it be done\n> > > when applying?\n> >\n> > As it changes the structure of the code a bit, I'd prefer a new version.\n> > I don't want to risk making a mistake with an untested change (I've made\n> > enough of those already).\n> >\n> > > > > > + float defaultPos = ipaAfControls.at(&controls::LensPosition).def().get();\n> > > > > > + ControlList lensCtrl(lensCtrls_);\n> > > > > > + int32_t hwpos;\n> > > > > > +\n> > > > > > + af->setLensPosition(defaultPos, &hwpos);\n> > > > > > + lensCtrl.set(V4L2_CID_FOCUS_ABSOLUTE, hwpos);\n> > > > > > + result->lensControls = std::move(lensCtrl);\n> > > >\n> > > > Will this work as expected if the user sets the AF mode to continuous at\n> > > > start time ? Would it cause the lens to first be moved to the hyperfocal\n> > > > distance, to be overridden right after ?\n> > >\n> > > Yes, it does exactly that. It's only a minor inconvenience, and in reality the\n> > > user is not going to get to see this movement as the startup frames are dropped\n> > > anyway.\n> >\n> > I suppose we can enhance it later if needed.\n> >\n> > > > > > + }\n> > > > > > }\n> > > > > >\n> > > > > > result->sensorControls = std::move(ctrls);\n> > > > >\n> > > > > I think this is good, but it suggests future cleanups:\n> > > > >\n> > > > > There's an opportunity to simplify the RPI AF algorithm, which\n> > > > > deliberately supports an initial state in which it doesn't touch the\n> > > > > lens and doesn't know where it is. If lens position is always\n> > > > > initialized, a flag and some code paths can be removed there. But\n> > > > > there's currently no way to retrieve \"hwpos\" without either setting a\n> > > > > lens position or processing a frame; so it might entail a change to\n> > > > > AfAlgorithm interface.\n> > > > >\n> > > > > Based on the existing interface, this patch is doing the best thing.\n> > > > >\n> > > > > There's a complication with the concept of \"default\" too: The tuning\n> > > > > file can define defaults for each range (\"normal\", \"macro\", \"full\"),\n> > > > > but none of those match the static default and limits in the\n> > > > > ControlInfo. This might be part of a more general issue with control\n> > > > > limits, so I don't think it needs addressing here.\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 57F07C31E9\n\tfor ;\n\tWed, 14 Jun 2023 10:15:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B27D0628B6;\n\tWed, 14 Jun 2023 12:15:16 +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 3332A61E4A\n\tfor ;\n\tWed, 14 Jun 2023 12:15:15 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 557DD838;\n\tWed, 14 Jun 2023 12:14:44 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1686737716;\n\tbh=AOo45IbRiTsXx5ZHPx4vohJY04hOhdnGprWHS3yO5sM=;\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:Cc:\n\tFrom;\n\tb=4cbIdydQoHp2Rx5336kT7vey5GmE3mARI73t7UbAmARLISGk3JbjBqWYhdhGthQsK\n\tF7AeUoDUuKM9XDeEcHo3CeGPw3YfXvmLWy4MVgjdawFVWTh7SHAN9akKUYLNHrgZz7\n\tQY8gbzQO7J2mr+zkUCy4qPM3MhHyZZ1FPzWCKkhjeVhWlqJDPcmvBiKutIDFGdVs7G\n\tsazhU4hWpv44iEIm7DMOuMNI/gOl6vvh8ogfjsS1Qv0HWiL5/dUNX9FL521f+VeKdd\n\tEL2W7tAWMqXHu+yPCQtn7S00fkkwaBKcIktgj+SOlEP4xUGa5ZtAZ/pJ7QTiTVVp+h\n\tEHl2Q8qETaAlg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1686737684;\n\tbh=AOo45IbRiTsXx5ZHPx4vohJY04hOhdnGprWHS3yO5sM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=J7fKCgHFynhzd/ybuRMhXD6qO0EMg+DNhWwJ8yOJuq8ieYFcRIMe8MVHREA6o7vN+\n\tNqCihb4mNvUJXaFuWfYrH6OWnCjvu72MfirFCZ8Osv2DQLBLD9qaRC9A39scPHXwsZ\n\tzaoBIHJnTml14dMrF/NIf8IxLAxdOUqO8SWtZLhE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"J7fKCgHF\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230601095630.25443-1-naush@raspberrypi.com>\n\t<20230601095630.25443-4-naush@raspberrypi.com>\n\t\n\t<20230602064312.GS22609@pendragon.ideasonboard.com>\n\t\n\t<20230605075328.GB30841@pendragon.ideasonboard.com>\n\t","To":"David Plowman ,\n\tDavid Plowman via libcamera-devel , \n\tLaurent Pinchart ","Date":"Wed, 14 Jun 2023 11:15:10 +0100","Message-ID":"<168673771098.2889415.7227611072964129625@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1 3/3] ipa: rpi: Set lens position to\n\thyperfocal on startup","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 ","Cc":"Nick Hollinghurst ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]