[{"id":20619,"web_url":"https://patchwork.libcamera.org/comment/20619/","msgid":"<7868213b-1b3f-8ca8-2df9-c69d349ab476@ideasonboard.com>","date":"2021-10-28T18:18:58","subject":"Re: [libcamera-devel] [PATCH 4/6] ipu3: Apply auto focus and send\n\tlens controls to pipeline handler","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Han-Lin,\n\nThank you for the patch.\n\nSome high level comments.\n\nOn 10/28/21 3:33 PM, Han-Lin Chen wrote:\n> Apply auto focus and send lens controls to pipeline handler.\n>\n> Signed-off-by: Han-Lin Chen \n> ---\n> aiq/aiq.cpp | 5 ++++-\n> aiq/aiq.h | 2 +-\n> ipu3.cpp | 27 ++++++++++++++++++++++++++-\n> 3 files changed, 31 insertions(+), 3 deletions(-)\n>\n> diff --git a/aiq/aiq.cpp b/aiq/aiq.cpp\n> index 24c61cb..9c7f9d6 100644\n> --- a/aiq/aiq.cpp\n> +++ b/aiq/aiq.cpp\n> @@ -139,7 +139,7 @@ int AIQ::setStatistics(unsigned int frame,\n> * of the parameter buffer being the only part handled when called for.\n> */\n> int AIQ::run2a(unsigned int frame, AiqInputParameters ¶ms,\n> -\t AiqResults &results)\n> +\t AiqResults &results, int lensPosition, int64_t lensMovementStartTime)\n\n\nSince we are now passing arguments to the algorithm's input parameters, \nI think we should be consider some refactoring in AiqInputParameters to \naccomodate our needs. One quick option I can think of, is to split and \nhave setX(list of args) for (X = each algorithm's input parameters) \nfunctions and then call setDefaults() which shall mimick a \nsetAeAwbAfDefaults() we have today (setDefaults() shall call setX() each \nwith default args)\n\n\n> l\n> {\n> \t(void)frame;\n> \n> @@ -157,6 +157,9 @@ int AIQ::run2a(unsigned int frame, AiqInputParameters ¶ms,\n> \tparams.saParams.awb_results = results.awb();\n> \tshadingAdapterRun(params.saParams, results);\n> \n> +\tparams.afParams.lens_position = lensPosition;\n> +\tparams.afParams.lens_movement_start_timestamp = lensMovementStartTime;\n> +\n\n\nThat's how I see setting these arguments for afParams as well, passing\n\n         setAfParams(...,  int lensPosition, int64_t lensMovementStartTime);\n\nHave you considered any such refactoring in design? I get it that this \nmay well be a quick AF functionality driven patch to test the waters.\n\n> \tafRun(params.afParams, results);\n> \n> \treturn 0;\n> diff --git a/aiq/aiq.h b/aiq/aiq.h\n> index fcd02d2..4f5f868 100644\n> --- a/aiq/aiq.h\n> +++ b/aiq/aiq.h\n> @@ -41,7 +41,7 @@ public:\n> \t\t\t const ipu3_uapi_stats_3a *stats);\n> \n> \tint run2a(unsigned int frame, AiqInputParameters ¶ms,\n> -\t\t AiqResults &results);\n> +\t AiqResults &results, int lensPosition, int64_t lensMovementStartTime);\n> \n> private:\n> \tstd::string decodeError(ia_err err);\n> diff --git a/ipu3.cpp b/ipu3.cpp\n> index b7de901..45330ca 100644\n> --- a/ipu3.cpp\n> +++ b/ipu3.cpp\n> @@ -77,6 +77,10 @@ private:\n> \tuint32_t gain_;\n> \tuint32_t minGain_;\n> \tuint32_t maxGain_;\n> +\tint32_t lensPosition_;\n> +\n> +\t/* Intel AF library relies on timestamp to wait for lens movement */\n> +\tuint64_t lensMovementStartTime_;\n> \n> \t/* Intel Library Instances. */\n> \taiq::AIQ aiq_;\n> @@ -258,6 +262,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> \tmaxGain_ = itGain->second.max().get();\n> \tgain_ = maxGain_;\n> \n> +\tlensMovementStartTime_ = 0;\n> +\tlensPosition_ = 0;\n> +\n> \tint ret;\n> \n> \tret = aiq_.configure();\n> @@ -381,7 +388,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> \t*/\n> \n> \t/* Run algorithms into/using this context structure */\n> -\taiq_.run2a(frame, aiqInputParams_, results_);\n> +\taiq_.run2a(frame, aiqInputParams_, results_, lensPosition_, lensMovementStartTime_);\n> \n> \taic_.updateRuntimeParams(results_);\n> \taic_.run(params);\n> @@ -389,6 +396,19 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> \texposure_ = results_.ae()->exposures[0].sensor_exposure->coarse_integration_time;\n> \tgain_ = results_.ae()->exposures[0].sensor_exposure->analog_gain_code_global;\n> \n> +\t/*\n> +\t* Af algorithm compares the timestamp of start of lens movement start and the\n\n\ns/*/ */  and all for comment lines below.\n\nSentence is a bit confusing. I guess among the 2 'start', one should be \nremoved to be read like:\n\n         * Af algorithm compares the timestamp of start of lens movement and\n\n> +\t* that of the statistics generated to estimate whether next lens positon\n> +\t* should be produced.\n> +\t* Todo: Uses the lens movement start time reported by the pipeline handler.\n\n\ns/Todo: Uses / \\todo: Use /\n\n\n> +\t*/\n> +\tif (lensPosition_ != results_.af()->next_lens_position) {\n> +\t\tutils::time_point time = utils::clock::now();\n> +\t\tuint64_t msecs = std::chrono::duration_cast(time.time_since_epoch()).count();\n> +\t\tlensMovementStartTime_ = msecs;\n> +\t}\n> +\tlensPosition_ = results_.af()->next_lens_position;\n> +\n> \tresultsHistory_.Push(results_);\n> \n> \tsetControls(frame);\n> @@ -472,6 +492,11 @@ void IPAIPU3::setControls(unsigned int frame)\n> \n> \top.sensorControls = sensorCtrls;\n> \n> +\tControlList lensCtrls;\n> +\tlensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, lensPosition_);\n> +\n> +\top.lensControls = lensCtrls;\n> +\n> \tqueueFrameAction.emit(frame, op);\n> }\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 92E09BF415\n\tfor ;\n\tThu, 28 Oct 2021 18:19:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 02503600BA;\n\tThu, 28 Oct 2021 20:19:10 +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 16CD6600B5\n\tfor ;\n\tThu, 28 Oct 2021 20:19:08 +0200 (CEST)","from [192.168.43.203] (unknown [152.57.66.23])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DD4C4513;\n\tThu, 28 Oct 2021 20:19:05 +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=\"l+soqPn2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635445147;\n\tbh=RlplPxcowGkcbhhYrJ/hPkG8b8o2f8NV9iZg4SfbOzI=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=l+soqPn2jVFjaQATxs0l9J8TC9JnZDG8aysKOvQ1Elzk8/TTQFtywHgstB/qkkIGM\n\tSwG80tHOE9kllPv4+4gPHYYEkcXNIydZjSrvWKYDS/zVaXmUKhDk+sAFT21AXgAt0V\n\tv7j8YJ8YIGn1n4/IgqhfxyFPJ+uaOkSNvAMfbyl0=","To":"Han-Lin Chen ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211028100349.1098545-1-hanlinchen@chromium.org>\n\t<20211028100349.1098545-4-hanlinchen@chromium.org>","From":"Umang Jain ","Message-ID":"<7868213b-1b3f-8ca8-2df9-c69d349ab476@ideasonboard.com>","Date":"Thu, 28 Oct 2021 23:48:58 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211028100349.1098545-4-hanlinchen@chromium.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 4/6] ipu3: Apply auto focus and send\n\tlens controls to pipeline handler","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","Cc":"Han-Lin Chen ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20635,"web_url":"https://patchwork.libcamera.org/comment/20635/","msgid":"","date":"2021-10-29T09:48:08","subject":"Re: [libcamera-devel] [PATCH 4/6] ipu3: Apply auto focus and send\n\tlens controls to pipeline handler","submitter":{"id":98,"url":"https://patchwork.libcamera.org/api/people/98/","name":"Hanlin Chen","email":"hanlinchen@chromium.org"},"content":"Hi Umang,\nThanks for the review.\n\nOn Fri, Oct 29, 2021 at 2:19 AM Umang Jain wrote:\n>\n> Hi Han-Lin,\n>\n> Thank you for the patch.\n>\n> Some high level comments.\n>\n> On 10/28/21 3:33 PM, Han-Lin Chen wrote:\n> > Apply auto focus and send lens controls to pipeline handler.\n> >\n> > Signed-off-by: Han-Lin Chen \n> > ---\n> > aiq/aiq.cpp | 5 ++++-\n> > aiq/aiq.h | 2 +-\n> > ipu3.cpp | 27 ++++++++++++++++++++++++++-\n> > 3 files changed, 31 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/aiq/aiq.cpp b/aiq/aiq.cpp\n> > index 24c61cb..9c7f9d6 100644\n> > --- a/aiq/aiq.cpp\n> > +++ b/aiq/aiq.cpp\n> > @@ -139,7 +139,7 @@ int AIQ::setStatistics(unsigned int frame,\n> > * of the parameter buffer being the only part handled when called for.\n> > */\n> > int AIQ::run2a(unsigned int frame, AiqInputParameters ¶ms,\n> > - AiqResults &results)\n> > + AiqResults &results, int lensPosition, int64_t lensMovementStartTime)\n>\n>\n> Since we are now passing arguments to the algorithm's input parameters,\n> I think we should be consider some refactoring in AiqInputParameters to\n> accomodate our needs. One quick option I can think of, is to split and\n> have setX(list of args) for (X = each algorithm's input parameters)\n> functions and then call setDefaults() which shall mimick a\n> setAeAwbAfDefaults() we have today (setDefaults() shall call setX() each\n> with default args)\n>\n>\n> > l\n> > {\n> > (void)frame;\n> >\n> > @@ -157,6 +157,9 @@ int AIQ::run2a(unsigned int frame, AiqInputParameters ¶ms,\n> > params.saParams.awb_results = results.awb();\n> > shadingAdapterRun(params.saParams, results);\n> >\n> > + params.afParams.lens_position = lensPosition;\n> > + params.afParams.lens_movement_start_timestamp = lensMovementStartTime;\n> > +\n>\n>\n> That's how I see setting these arguments for afParams as well, passing\n>\n> setAfParams(..., int lensPosition, int64_t lensMovementStartTime);\n>\n> Have you considered any such refactoring in design? I get it that this\n> may well be a quick AF functionality driven patch to test the waters.\n>\n\nThanks! This does sound great to me.\nI think I will prepare another patch for the refactoring of\nAiqInputParameters, since it's not only related to AF.\n\n> > afRun(params.afParams, results);\n> >\n> > return 0;\n> > diff --git a/aiq/aiq.h b/aiq/aiq.h\n> > index fcd02d2..4f5f868 100644\n> > --- a/aiq/aiq.h\n> > +++ b/aiq/aiq.h\n> > @@ -41,7 +41,7 @@ public:\n> > const ipu3_uapi_stats_3a *stats);\n> >\n> > int run2a(unsigned int frame, AiqInputParameters ¶ms,\n> > - AiqResults &results);\n> > + AiqResults &results, int lensPosition, int64_t lensMovementStartTime);\n> >\n> > private:\n> > std::string decodeError(ia_err err);\n> > diff --git a/ipu3.cpp b/ipu3.cpp\n> > index b7de901..45330ca 100644\n> > --- a/ipu3.cpp\n> > +++ b/ipu3.cpp\n> > @@ -77,6 +77,10 @@ private:\n> > uint32_t gain_;\n> > uint32_t minGain_;\n> > uint32_t maxGain_;\n> > + int32_t lensPosition_;\n> > +\n> > + /* Intel AF library relies on timestamp to wait for lens movement */\n> > + uint64_t lensMovementStartTime_;\n> >\n> > /* Intel Library Instances. */\n> > aiq::AIQ aiq_;\n> > @@ -258,6 +262,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> > maxGain_ = itGain->second.max().get();\n> > gain_ = maxGain_;\n> >\n> > + lensMovementStartTime_ = 0;\n> > + lensPosition_ = 0;\n> > +\n> > int ret;\n> >\n> > ret = aiq_.configure();\n> > @@ -381,7 +388,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> > */\n> >\n> > /* Run algorithms into/using this context structure */\n> > - aiq_.run2a(frame, aiqInputParams_, results_);\n> > + aiq_.run2a(frame, aiqInputParams_, results_, lensPosition_, lensMovementStartTime_);\n> >\n> > aic_.updateRuntimeParams(results_);\n> > aic_.run(params);\n> > @@ -389,6 +396,19 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> > exposure_ = results_.ae()->exposures[0].sensor_exposure->coarse_integration_time;\n> > gain_ = results_.ae()->exposures[0].sensor_exposure->analog_gain_code_global;\n> >\n> > + /*\n> > + * Af algorithm compares the timestamp of start of lens movement start and the\n>\n>\n> s/*/ */ and all for comment lines below.\n>\n> Sentence is a bit confusing. I guess among the 2 'start', one should be\n> removed to be read like:\n>\n> * Af algorithm compares the timestamp of start of lens movement and\n>\n> > + * that of the statistics generated to estimate whether next lens positon\n> > + * should be produced.\n> > + * Todo: Uses the lens movement start time reported by the pipeline handler.\n>\n>\n> s/Todo: Uses / \\todo: Use /\n>\n>\n> > + */\n> > + if (lensPosition_ != results_.af()->next_lens_position) {\n> > + utils::time_point time = utils::clock::now();\n> > + uint64_t msecs = std::chrono::duration_cast(time.time_since_epoch()).count();\n> > + lensMovementStartTime_ = msecs;\n> > + }\n> > + lensPosition_ = results_.af()->next_lens_position;\n> > +\n> > resultsHistory_.Push(results_);\n> >\n> > setControls(frame);\n> > @@ -472,6 +492,11 @@ void IPAIPU3::setControls(unsigned int frame)\n> >\n> > op.sensorControls = sensorCtrls;\n> >\n> > + ControlList lensCtrls;\n> > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, lensPosition_);\n> > +\n> > + op.lensControls = lensCtrls;\n> > +\n> > queueFrameAction.emit(frame, op);\n> > }\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BA878BF415\n\tfor ;\n\tFri, 29 Oct 2021 09:48:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 36FE9600B4;\n\tFri, 29 Oct 2021 11:48:22 +0200 (CEST)","from mail-oi1-x229.google.com (mail-oi1-x229.google.com\n\t[IPv6:2607:f8b0:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7EBE6600B4\n\tfor ;\n\tFri, 29 Oct 2021 11:48:20 +0200 (CEST)","by mail-oi1-x229.google.com with SMTP id g125so12553553oif.9\n\tfor ;\n\tFri, 29 Oct 2021 02:48:20 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"hvu0YHXt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=It4j4RO66u23lK3xSTJM1BNfIApARrfHHgpnm3ylz1U=;\n\tb=hvu0YHXtcoCY1796Kf41SDcNw51/w/XtgsFcpxVHODOrwFYRUnak27CYOm9tDZRQVb\n\tD9RjaLDxXxv6DvXyPan/zxkPOznEswFjM3x5ZBB59mzJA/PD8gdQ2xMQuLXywvDx5dGP\n\tANaPGKeHr4qguSM6wGyo6TVWr3/lobsX+IQh4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=It4j4RO66u23lK3xSTJM1BNfIApARrfHHgpnm3ylz1U=;\n\tb=0k3W+Qy3w0yVaAMsGNydL/6uwg3Rb4p1oQKe6pLm9um+ZQDSHNqu5JM4+Kqdc/Zh3p\n\txFNz8l6XZA8PSA52/6U3oIYKLXsrY4baQnQBQRhin3HOw8Wj3a9a1TnN6DLaLGY7cBzU\n\tvptjCRrj71qo91jxYComj66l5sSDSTFL+zM9YqHvCz87k19BGJkTTb1R/+oNcQFEdPSJ\n\tMmPwdedw+vM7TU7eP1M4Sm1nX9USzjO2p5X16DkbdoPEoDrzEfwPK1x9V3Ggs0/Lx5pz\n\tSLwBOaIJbLskTvpnPb+hylkn1Trg/XXvljA9tksEPEQo/q0TiwDx25T3PexlAlh7t+Dq\n\txgxw==","X-Gm-Message-State":"AOAM5336dAzthc3g+cRSOKSj948ruwGeHznKpsMgd6kp9LXC05287VNR\n\tK6slO6oGCCxi7XKe4Lw2xlOXDI5C/vkxqIYuPZPEgA==","X-Google-Smtp-Source":"ABdhPJw99wkVnNEMoJrzldjqubJNLhDXVAky2uXHMlMTkNugsYYBW8sjF/QZqhwdMVUW6HKF9cKK1IYuz5EMQF4sVNo=","X-Received":"by 2002:a54:4f8f:: with SMTP id\n\tg15mr7000062oiy.169.1635500899078; \n\tFri, 29 Oct 2021 02:48:19 -0700 (PDT)","MIME-Version":"1.0","References":"<20211028100349.1098545-1-hanlinchen@chromium.org>\n\t<20211028100349.1098545-4-hanlinchen@chromium.org>\n\t<7868213b-1b3f-8ca8-2df9-c69d349ab476@ideasonboard.com>","In-Reply-To":"<7868213b-1b3f-8ca8-2df9-c69d349ab476@ideasonboard.com>","From":"Hanlin Chen ","Date":"Fri, 29 Oct 2021 17:48:08 +0800","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 4/6] ipu3: Apply auto focus and send\n\tlens controls to pipeline handler","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","Cc":"libcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]