[{"id":20620,"web_url":"https://patchwork.libcamera.org/comment/20620/","msgid":"<4f3745e7-4892-3b4c-2542-d95c2ca28551@ideasonboard.com>","date":"2021-10-28T18:33:54","subject":"Re: [libcamera-devel] [PATCH 3/6] ipu3: Set statistics with the\n\teffective AE AiqResults","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\nOn 10/28/21 3:33 PM, Han-Lin Chen wrote:\n> Set the statistics with the latest AE AiqResults which has the same\n> exposure time and analog gain. The patch reduces the AE haunting during\n> the converging process.\n>\n> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.com>\n> ---\n>   aiq/aiq_input_parameters.cpp |  4 +--\n>   ipu3.cpp                     | 66 ++++++++++++++++++++++++++++++------\n>   2 files changed, 57 insertions(+), 13 deletions(-)\n>\n> diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp\n> index bc87b31..5dd2f6c 100644\n> --- a/aiq/aiq_input_parameters.cpp\n> +++ b/aiq/aiq_input_parameters.cpp\n> @@ -130,7 +130,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe\n>   \n>   void AiqInputParameters::setAeAwbAfDefaults()\n>   {\n> -\t/*Ae Params */\n> +\t/* Ae Params */\n>   \taeInputParams.num_exposures = NUM_EXPOSURES;\n>   \taeInputParams.frame_use = ia_aiq_frame_use_preview;\n>   \taeInputParams.flash_mode = ia_aiq_flash_mode_off;\n> @@ -166,7 +166,7 @@ void AiqInputParameters::setAeAwbAfDefaults()\n>   \t\tia_aiq_af_range_normal,\n>   \t\tia_aiq_af_metering_mode_auto,\n>   \t\tia_aiq_flash_mode_off,\n> -\t\tNULL, NULL, false\n> +\t\t&focusRect, &manualFocusParams, false\n\n\nShould this be moved to focus patch one instead? I find this odd to be \npresent in this patch, since it's about AE results?\n\n>   \t};\n>   \n>   \t/* GBCE Params */\n> diff --git a/ipu3.cpp b/ipu3.cpp\n> index 8126e9d..b7de901 100644\n> --- a/ipu3.cpp\n> +++ b/ipu3.cpp\n> @@ -59,7 +59,8 @@ private:\n>   \tvoid fillParams(unsigned int frame, ipu3_uapi_params *params);\n>   \tvoid parseStatistics(unsigned int frame,\n>   \t\t\t     int64_t frameTimestamp,\n> -\t\t\t     const ipu3_uapi_stats_3a *stats);\n> +\t\t\t     const ipu3_uapi_stats_3a *stats,\n> +\t\t\t     const ControlList& sensorCtrls);\n>   \n>   \tvoid setControls(unsigned int frame);\n>   \n> @@ -84,6 +85,7 @@ private:\n>   \t/* Temporary storage until we have a FrameContext object / struct */\n>   \taiq::AiqInputParameters aiqInputParams_;\n>   \taiq::AiqResults results_;\n> +\taiq::AiqResultsRingBuffer resultsHistory_;\n>   \n>   \tBinaryData aiqb_;\n>   \tBinaryData nvm_;\n> @@ -282,6 +284,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n>   \t/* Upate the camera controls using the new sensor settings. */\n>   \tupdateControls(sensorInfo_, ctrls_, ipaControls);\n>   \n> +\tresultsHistory_.Reset();\n> +\n>   \treturn 0;\n>   }\n>   \n> @@ -327,7 +331,10 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n>   \t\tconst ipu3_uapi_stats_3a *stats =\n>   \t\t\treinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());\n>   \n> -\t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n> +\t\tparseStatistics(event.frame,\n> +\t\t\t\t\t\t\t\t\t\tevent.frameTimestamp,\n> +\t\t\t\t\t\t\t\t\t\tstats,\n> +\t\t\t\t\t\t\t\t\t\tevent.sensorControls);\n\n\nindentation\n\n>   \t\tbreak;\n>   \t}\n>   \tcase EventFillParams: {\n> @@ -374,14 +381,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>   \t*/\n>   \n>   \t/* Run algorithms into/using this context structure */\n> -\tif (frame % 10 == 0)\n> -\t\taiq_.run2a(frame, aiqInputParams_, results_);\n> +\taiq_.run2a(frame, aiqInputParams_, results_);\n>   \n>   \taic_.updateRuntimeParams(results_);\n>   \taic_.run(params);\n>   \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> +\tresultsHistory_.Push(results_);\n\n\nPossibly usage of ringbuffer to highlight how it's used, what it \nprovides (a high level documentation on it's 'need', the 'when' and \n'why' of previous algo-runs results lookup) would be good to understand \nthe context I believe. All this can be a separate patch.\n\n> +\n>   \tsetControls(frame);\n>   \n>   \tIPU3Action op;\n> @@ -392,7 +401,8 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>   \n>   void IPAIPU3::parseStatistics(unsigned int frame,\n>   \t\t\t      int64_t frameTimestamp,\n> -\t\t\t      const ipu3_uapi_stats_3a *stats)\n> +\t\t\t      const ipu3_uapi_stats_3a *stats,\n> +\t\t\t      const ControlList& sensorCtrls)\n>   {\n>   \tControlList ctrls(controls::controls);\n>   \n> @@ -403,10 +413,43 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>   \t */\n>   \n>   \tASSERT (frameTimestamp > 0);\n> -\taiq_.setStatistics(frame, frameTimestamp, results_, stats);\n> +\n> +\t/* Ae algorithm expects the statistics to be set with it's corresponding Ae\n> +\t * result, i.e., the Ae result should match the exposure time/analog gain\n> +\t * with the the effective sensor controls of the statistics.\n> +\t * Search the required Ae result in the result history and combine it with\n> +\t * the latest result as the input to AIQ::setStatistics().\n> +\t */\n\n\nIs there a bit more descriptive documentation I can refer to? Maybe in \nthe ia_aiq documentation to understand 'when' and 'how' the aiq results \nare meant to be re-used? I think I wrote my last comment a bit hastily\n\n> +\n> +\tint32_t effectiveExpo = 0;\n> +\tint32_t effectiveGain = 0;\n> +\tControlValue ctrlValue;\n> +\n> +\tctrlValue = sensorCtrls.get(V4L2_CID_EXPOSURE);\n> +\tif (!ctrlValue.isNone())\n> +\t\teffectiveExpo = ctrlValue.get<int32_t>();\n> +\n> +\tctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN);\n> +\tif (!ctrlValue.isNone())\n> +\t\teffectiveGain = ctrlValue.get<int32_t>();\n> +\n> +\tauto pred = [effectiveExpo, effectiveGain] (aiq::AiqResults& result) {\n> +\t\tia_aiq_exposure_sensor_parameters* sensorExposure =\n> +\t\t\tresult.ae()->exposures[0].sensor_exposure;\n> +\n> +\t\treturn (effectiveExpo == sensorExposure->coarse_integration_time ||\n> +\t\t\teffectiveGain == sensorExposure->analog_gain_code_global);\n> +\t};\n> +\taiq::AiqResults& aeMatchedResults = resultsHistory_.searchBackward(pred, results_);\n> +\taiq::AiqResults combinedResults = results_;\n> +\tcombinedResults.setAe(aeMatchedResults.ae());\n> +\n> +\t/* Aiq library expects timestamp in microseconds */\n> +\taiq_.setStatistics(frame, (frameTimestamp / 1000), combinedResults, stats);\n>   \n>   \t/* Set frame durations from exposure results */\n> -\tia_aiq_exposure_sensor_parameters *sensorExposure = results_.ae()->exposures->sensor_exposure;\n> +\tia_aiq_exposure_sensor_parameters *sensorExposure = combinedResults.ae()->exposures->sensor_exposure;\n>   \tint64_t frameDuration = (sensorExposure->line_length_pixels * sensorExposure->frame_length_lines) /\n>   \t\t\t\t(sensorInfo_.pixelRate / 1e6);\n>   \tctrls.set(controls::FrameDuration, frameDuration);\n> @@ -423,10 +466,11 @@ void IPAIPU3::setControls(unsigned int frame)\n>   \tIPU3Action op;\n>   \top.op = ActionSetSensorControls;\n>   \n> -\tControlList ctrls(ctrls_);\n> -\tctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n> -\tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));\n> -\top.controls = ctrls;\n> +\tControlList sensorCtrls(ctrls_);\n> +\tsensorCtrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n> +\tsensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));\n> +\n> +\top.sensorControls = sensorCtrls;\n>   \n>   \tqueueFrameAction.emit(frame, op);\n>   }","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 358DFBDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 28 Oct 2021 18:34:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F4E2600BC;\n\tThu, 28 Oct 2021 20:34:04 +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 9AECA600B5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 28 Oct 2021 20:34:02 +0200 (CEST)","from [192.168.43.203] (unknown [152.57.66.23])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CA2A0513;\n\tThu, 28 Oct 2021 20:34:00 +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=\"o+NS6NKu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635446042;\n\tbh=nzqIEQbk51UanW75+uhWFOJDMVHgtM9lOm1Lry4X2q0=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=o+NS6NKuyuCJJw1N8/4HIxegL4VDLX43t+zuZ/W1jWEQ2WzTEZf+f14RmDM6s+25H\n\tlkZlEZIU/KE9z6SwnX+ylXY4eoebYAZPcq1/pshC07zX2v6G9NlEhPrK4ZfgX9zsVh\n\tBGRNGmKkRf+klTS1DNAKXS+iIY0EngYTf4n0e5cs=","To":"Han-Lin Chen <hanlinchen@chromium.org>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211028100349.1098545-1-hanlinchen@chromium.org>\n\t<20211028100349.1098545-3-hanlinchen@chromium.org>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<4f3745e7-4892-3b4c-2542-d95c2ca28551@ideasonboard.com>","Date":"Fri, 29 Oct 2021 00:03:54 +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-3-hanlinchen@chromium.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 3/6] ipu3: Set statistics with the\n\teffective AE AiqResults","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"Han-Lin Chen <hanlinchen@chromium.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20621,"web_url":"https://patchwork.libcamera.org/comment/20621/","msgid":"<0b5755e1-5f52-f343-3fcb-68b244c41325@ideasonboard.com>","date":"2021-10-28T18:38:05","subject":"Re: [libcamera-devel] [PATCH 3/6] ipu3: Set statistics with the\n\teffective AE AiqResults","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Han-Lin,\n\nOne more comment,\n\nOn 10/28/21 3:33 PM, Han-Lin Chen wrote:\n> Set the statistics with the latest AE AiqResults which has the same\n> exposure time and analog gain. The patch reduces the AE haunting during\n> the converging process.\n>\n> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.com>\n> ---\n>   aiq/aiq_input_parameters.cpp |  4 +--\n>   ipu3.cpp                     | 66 ++++++++++++++++++++++++++++++------\n>   2 files changed, 57 insertions(+), 13 deletions(-)\n>\n> diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp\n> index bc87b31..5dd2f6c 100644\n> --- a/aiq/aiq_input_parameters.cpp\n> +++ b/aiq/aiq_input_parameters.cpp\n> @@ -130,7 +130,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe\n>   \n>   void AiqInputParameters::setAeAwbAfDefaults()\n>   {\n> -\t/*Ae Params */\n> +\t/* Ae Params */\n>   \taeInputParams.num_exposures = NUM_EXPOSURES;\n>   \taeInputParams.frame_use = ia_aiq_frame_use_preview;\n>   \taeInputParams.flash_mode = ia_aiq_flash_mode_off;\n> @@ -166,7 +166,7 @@ void AiqInputParameters::setAeAwbAfDefaults()\n>   \t\tia_aiq_af_range_normal,\n>   \t\tia_aiq_af_metering_mode_auto,\n>   \t\tia_aiq_flash_mode_off,\n> -\t\tNULL, NULL, false\n> +\t\t&focusRect, &manualFocusParams, false\n>   \t};\n>   \n>   \t/* GBCE Params */\n> diff --git a/ipu3.cpp b/ipu3.cpp\n> index 8126e9d..b7de901 100644\n> --- a/ipu3.cpp\n> +++ b/ipu3.cpp\n> @@ -59,7 +59,8 @@ private:\n>   \tvoid fillParams(unsigned int frame, ipu3_uapi_params *params);\n>   \tvoid parseStatistics(unsigned int frame,\n>   \t\t\t     int64_t frameTimestamp,\n> -\t\t\t     const ipu3_uapi_stats_3a *stats);\n> +\t\t\t     const ipu3_uapi_stats_3a *stats,\n> +\t\t\t     const ControlList& sensorCtrls);\n>   \n>   \tvoid setControls(unsigned int frame);\n>   \n> @@ -84,6 +85,7 @@ private:\n>   \t/* Temporary storage until we have a FrameContext object / struct */\n>   \taiq::AiqInputParameters aiqInputParams_;\n>   \taiq::AiqResults results_;\n> +\taiq::AiqResultsRingBuffer resultsHistory_;\n>   \n>   \tBinaryData aiqb_;\n>   \tBinaryData nvm_;\n> @@ -282,6 +284,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n>   \t/* Upate the camera controls using the new sensor settings. */\n>   \tupdateControls(sensorInfo_, ctrls_, ipaControls);\n>   \n> +\tresultsHistory_.Reset();\n> +\n>   \treturn 0;\n>   }\n>   \n> @@ -327,7 +331,10 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n>   \t\tconst ipu3_uapi_stats_3a *stats =\n>   \t\t\treinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());\n>   \n> -\t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n> +\t\tparseStatistics(event.frame,\n> +\t\t\t\t\t\t\t\t\t\tevent.frameTimestamp,\n> +\t\t\t\t\t\t\t\t\t\tstats,\n> +\t\t\t\t\t\t\t\t\t\tevent.sensorControls);\n>   \t\tbreak;\n>   \t}\n>   \tcase EventFillParams: {\n> @@ -374,14 +381,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>   \t*/\n>   \n>   \t/* Run algorithms into/using this context structure */\n> -\tif (frame % 10 == 0)\n> -\t\taiq_.run2a(frame, aiqInputParams_, results_);\n> +\taiq_.run2a(frame, aiqInputParams_, results_);\n\n\nIs this intended, I believe we'll run all algorithms for each frame now. \nI vaguely remember chrome use to run algorthims for every X frames once \n(which as 4 if I wasn't mistaken). Although with 4, I was getting \noscillations and so we arbitrarily bumped up to 10.\n\nI might be wrong about this though. Do we need to run every algorithms \nfor each frame until convergence ? or a fixed set before/after that?\n\n>   \n>   \taic_.updateRuntimeParams(results_);\n>   \taic_.run(params);\n>   \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> +\tresultsHistory_.Push(results_);\n> +\n>   \tsetControls(frame);\n>   \n>   \tIPU3Action op;\n> @@ -392,7 +401,8 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>   \n>   void IPAIPU3::parseStatistics(unsigned int frame,\n>   \t\t\t      int64_t frameTimestamp,\n> -\t\t\t      const ipu3_uapi_stats_3a *stats)\n> +\t\t\t      const ipu3_uapi_stats_3a *stats,\n> +\t\t\t      const ControlList& sensorCtrls)\n>   {\n>   \tControlList ctrls(controls::controls);\n>   \n> @@ -403,10 +413,43 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>   \t */\n>   \n>   \tASSERT (frameTimestamp > 0);\n> -\taiq_.setStatistics(frame, frameTimestamp, results_, stats);\n> +\n> +\t/* Ae algorithm expects the statistics to be set with it's corresponding Ae\n> +\t * result, i.e., the Ae result should match the exposure time/analog gain\n> +\t * with the the effective sensor controls of the statistics.\n> +\t * Search the required Ae result in the result history and combine it with\n> +\t * the latest result as the input to AIQ::setStatistics().\n> +\t */\n> +\n> +\tint32_t effectiveExpo = 0;\n> +\tint32_t effectiveGain = 0;\n> +\tControlValue ctrlValue;\n> +\n> +\tctrlValue = sensorCtrls.get(V4L2_CID_EXPOSURE);\n> +\tif (!ctrlValue.isNone())\n> +\t\teffectiveExpo = ctrlValue.get<int32_t>();\n> +\n> +\tctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN);\n> +\tif (!ctrlValue.isNone())\n> +\t\teffectiveGain = ctrlValue.get<int32_t>();\n> +\n> +\tauto pred = [effectiveExpo, effectiveGain] (aiq::AiqResults& result) {\n> +\t\tia_aiq_exposure_sensor_parameters* sensorExposure =\n> +\t\t\tresult.ae()->exposures[0].sensor_exposure;\n> +\n> +\t\treturn (effectiveExpo == sensorExposure->coarse_integration_time ||\n> +\t\t\teffectiveGain == sensorExposure->analog_gain_code_global);\n> +\t};\n> +\taiq::AiqResults& aeMatchedResults = resultsHistory_.searchBackward(pred, results_);\n> +\n> +\taiq::AiqResults combinedResults = results_;\n> +\tcombinedResults.setAe(aeMatchedResults.ae());\n> +\n> +\t/* Aiq library expects timestamp in microseconds */\n> +\taiq_.setStatistics(frame, (frameTimestamp / 1000), combinedResults, stats);\n>   \n>   \t/* Set frame durations from exposure results */\n> -\tia_aiq_exposure_sensor_parameters *sensorExposure = results_.ae()->exposures->sensor_exposure;\n> +\tia_aiq_exposure_sensor_parameters *sensorExposure = combinedResults.ae()->exposures->sensor_exposure;\n>   \tint64_t frameDuration = (sensorExposure->line_length_pixels * sensorExposure->frame_length_lines) /\n>   \t\t\t\t(sensorInfo_.pixelRate / 1e6);\n>   \tctrls.set(controls::FrameDuration, frameDuration);\n> @@ -423,10 +466,11 @@ void IPAIPU3::setControls(unsigned int frame)\n>   \tIPU3Action op;\n>   \top.op = ActionSetSensorControls;\n>   \n> -\tControlList ctrls(ctrls_);\n> -\tctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n> -\tctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));\n> -\top.controls = ctrls;\n> +\tControlList sensorCtrls(ctrls_);\n> +\tsensorCtrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n> +\tsensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));\n> +\n> +\top.sensorControls = sensorCtrls;\n>   \n>   \tqueueFrameAction.emit(frame, op);\n>   }","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 6CACBBF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 28 Oct 2021 18:38:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB968600BA;\n\tThu, 28 Oct 2021 20:38:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07E3D600B5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 28 Oct 2021 20:38:13 +0200 (CEST)","from [192.168.43.203] (unknown [152.57.66.23])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 28F32513;\n\tThu, 28 Oct 2021 20:38:10 +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=\"roKA01KB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635446292;\n\tbh=sHm5U2Yc0dT3TeIranyciiLyfYgHRIHIXjfLXqyIZcw=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=roKA01KBvhYKV5AhRsunNZUHphlwn/KRQWLPIQe0k5t9N63kvPllL9aG8iHblouwW\n\tuQlyt7TSFEx09N1cY6PovqckcQFV27vq6H6taSNRtpGM4eYiL7U4a1MIrywgT+ueb5\n\tUfaNJAFwN3oDsqqxsmxqgRda3LjGAb8KZi89fCYE=","To":"Han-Lin Chen <hanlinchen@chromium.org>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211028100349.1098545-1-hanlinchen@chromium.org>\n\t<20211028100349.1098545-3-hanlinchen@chromium.org>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<0b5755e1-5f52-f343-3fcb-68b244c41325@ideasonboard.com>","Date":"Fri, 29 Oct 2021 00:08:05 +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-3-hanlinchen@chromium.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 3/6] ipu3: Set statistics with the\n\teffective AE AiqResults","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"Han-Lin Chen <hanlinchen@chromium.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20636,"web_url":"https://patchwork.libcamera.org/comment/20636/","msgid":"<CAJAuwMn13wNBu_-EA6GpaT8ttWiijuwyNhge8zu6cwUeU8fT-w@mail.gmail.com>","date":"2021-10-29T10:07:01","subject":"Re: [libcamera-devel] [PATCH 3/6] ipu3: Set statistics with the\n\teffective AE AiqResults","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:38 AM Umang Jain <umang.jain@ideasonboard.com> wrote:\n>\n> Hi Han-Lin,\n>\n> One more comment,\n>\n> On 10/28/21 3:33 PM, Han-Lin Chen wrote:\n> > Set the statistics with the latest AE AiqResults which has the same\n> > exposure time and analog gain. The patch reduces the AE haunting during\n> > the converging process.\n> >\n> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.com>\n> > ---\n> >   aiq/aiq_input_parameters.cpp |  4 +--\n> >   ipu3.cpp                     | 66 ++++++++++++++++++++++++++++++------\n> >   2 files changed, 57 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp\n> > index bc87b31..5dd2f6c 100644\n> > --- a/aiq/aiq_input_parameters.cpp\n> > +++ b/aiq/aiq_input_parameters.cpp\n> > @@ -130,7 +130,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe\n> >\n> >   void AiqInputParameters::setAeAwbAfDefaults()\n> >   {\n> > -     /*Ae Params */\n> > +     /* Ae Params */\n> >       aeInputParams.num_exposures = NUM_EXPOSURES;\n> >       aeInputParams.frame_use = ia_aiq_frame_use_preview;\n> >       aeInputParams.flash_mode = ia_aiq_flash_mode_off;\n> > @@ -166,7 +166,7 @@ void AiqInputParameters::setAeAwbAfDefaults()\n> >               ia_aiq_af_range_normal,\n> >               ia_aiq_af_metering_mode_auto,\n> >               ia_aiq_flash_mode_off,\n> > -             NULL, NULL, false\n> > +             &focusRect, &manualFocusParams, false\n> >       };\n> >\n> >       /* GBCE Params */\n> > diff --git a/ipu3.cpp b/ipu3.cpp\n> > index 8126e9d..b7de901 100644\n> > --- a/ipu3.cpp\n> > +++ b/ipu3.cpp\n> > @@ -59,7 +59,8 @@ private:\n> >       void fillParams(unsigned int frame, ipu3_uapi_params *params);\n> >       void parseStatistics(unsigned int frame,\n> >                            int64_t frameTimestamp,\n> > -                          const ipu3_uapi_stats_3a *stats);\n> > +                          const ipu3_uapi_stats_3a *stats,\n> > +                          const ControlList& sensorCtrls);\n> >\n> >       void setControls(unsigned int frame);\n> >\n> > @@ -84,6 +85,7 @@ private:\n> >       /* Temporary storage until we have a FrameContext object / struct */\n> >       aiq::AiqInputParameters aiqInputParams_;\n> >       aiq::AiqResults results_;\n> > +     aiq::AiqResultsRingBuffer resultsHistory_;\n> >\n> >       BinaryData aiqb_;\n> >       BinaryData nvm_;\n> > @@ -282,6 +284,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> >       /* Upate the camera controls using the new sensor settings. */\n> >       updateControls(sensorInfo_, ctrls_, ipaControls);\n> >\n> > +     resultsHistory_.Reset();\n> > +\n> >       return 0;\n> >   }\n> >\n> > @@ -327,7 +331,10 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> >               const ipu3_uapi_stats_3a *stats =\n> >                       reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());\n> >\n> > -             parseStatistics(event.frame, event.frameTimestamp, stats);\n> > +             parseStatistics(event.frame,\n> > +                                                                             event.frameTimestamp,\n> > +                                                                             stats,\n> > +                                                                             event.sensorControls);\n> >               break;\n> >       }\n> >       case EventFillParams: {\n> > @@ -374,14 +381,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> >       */\n> >\n> >       /* Run algorithms into/using this context structure */\n> > -     if (frame % 10 == 0)\n> > -             aiq_.run2a(frame, aiqInputParams_, results_);\n> > +     aiq_.run2a(frame, aiqInputParams_, results_);\n>\n>\n> Is this intended, I believe we'll run all algorithms for each frame now.\n> I vaguely remember chrome use to run algorthims for every X frames once\n> (which as 4 if I wasn't mistaken). Although with 4, I was getting\n> oscillations and so we arbitrarily bumped up to 10.\nI noticed the oscillations too ;-). In fact, the \"frame_use\" and\n\"AiqResults History\" patches are to fix the oscillations problem.\nI don't remember whether chrome handles it though. In my\nunderstanding, Intel HAL runs the algorithm for all frames.\nRunning an algorithm for every X frames may slow down the AE/AF\nconverging time, like X times?\n>\n> I might be wrong about this though. Do we need to run every algorithms\n> for each frame until convergence ? or a fixed set before/after that?\nBecause AE/AF may rescan once the scenes are changed in Auto mode,\nand only AIQ knows when the scenes are changed and reports the lens\ncontrols, I suppose we cannot stop running it after it's converged.\n>\n> >\n> >       aic_.updateRuntimeParams(results_);\n> >       aic_.run(params);\n> >\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> > +     resultsHistory_.Push(results_);\n> > +\n> >       setControls(frame);\n> >\n> >       IPU3Action op;\n> > @@ -392,7 +401,8 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> >\n> >   void IPAIPU3::parseStatistics(unsigned int frame,\n> >                             int64_t frameTimestamp,\n> > -                           const ipu3_uapi_stats_3a *stats)\n> > +                           const ipu3_uapi_stats_3a *stats,\n> > +                           const ControlList& sensorCtrls)\n> >   {\n> >       ControlList ctrls(controls::controls);\n> >\n> > @@ -403,10 +413,43 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> >        */\n> >\n> >       ASSERT (frameTimestamp > 0);\n> > -     aiq_.setStatistics(frame, frameTimestamp, results_, stats);\n> > +\n> > +     /* Ae algorithm expects the statistics to be set with it's corresponding Ae\n> > +      * result, i.e., the Ae result should match the exposure time/analog gain\n> > +      * with the the effective sensor controls of the statistics.\n> > +      * Search the required Ae result in the result history and combine it with\n> > +      * the latest result as the input to AIQ::setStatistics().\n> > +      */\n> > +\n> > +     int32_t effectiveExpo = 0;\n> > +     int32_t effectiveGain = 0;\n> > +     ControlValue ctrlValue;\n> > +\n> > +     ctrlValue = sensorCtrls.get(V4L2_CID_EXPOSURE);\n> > +     if (!ctrlValue.isNone())\n> > +             effectiveExpo = ctrlValue.get<int32_t>();\n> > +\n> > +     ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN);\n> > +     if (!ctrlValue.isNone())\n> > +             effectiveGain = ctrlValue.get<int32_t>();\n> > +\n> > +     auto pred = [effectiveExpo, effectiveGain] (aiq::AiqResults& result) {\n> > +             ia_aiq_exposure_sensor_parameters* sensorExposure =\n> > +                     result.ae()->exposures[0].sensor_exposure;\n> > +\n> > +             return (effectiveExpo == sensorExposure->coarse_integration_time ||\n> > +                     effectiveGain == sensorExposure->analog_gain_code_global);\n> > +     };\n> > +     aiq::AiqResults& aeMatchedResults = resultsHistory_.searchBackward(pred, results_);\n> > +\n> > +     aiq::AiqResults combinedResults = results_;\n> > +     combinedResults.setAe(aeMatchedResults.ae());\n> > +\n> > +     /* Aiq library expects timestamp in microseconds */\n> > +     aiq_.setStatistics(frame, (frameTimestamp / 1000), combinedResults, stats);\n> >\n> >       /* Set frame durations from exposure results */\n> > -     ia_aiq_exposure_sensor_parameters *sensorExposure = results_.ae()->exposures->sensor_exposure;\n> > +     ia_aiq_exposure_sensor_parameters *sensorExposure = combinedResults.ae()->exposures->sensor_exposure;\n> >       int64_t frameDuration = (sensorExposure->line_length_pixels * sensorExposure->frame_length_lines) /\n> >                               (sensorInfo_.pixelRate / 1e6);\n> >       ctrls.set(controls::FrameDuration, frameDuration);\n> > @@ -423,10 +466,11 @@ void IPAIPU3::setControls(unsigned int frame)\n> >       IPU3Action op;\n> >       op.op = ActionSetSensorControls;\n> >\n> > -     ControlList ctrls(ctrls_);\n> > -     ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n> > -     ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));\n> > -     op.controls = ctrls;\n> > +     ControlList sensorCtrls(ctrls_);\n> > +     sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n> > +     sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));\n> > +\n> > +     op.sensorControls = sensorCtrls;\n> >\n> >       queueFrameAction.emit(frame, op);\n> >   }","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 28539BDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 29 Oct 2021 10:07:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50539600BD;\n\tFri, 29 Oct 2021 12:07:15 +0200 (CEST)","from mail-ot1-x32a.google.com (mail-ot1-x32a.google.com\n\t[IPv6:2607:f8b0:4864:20::32a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 45A30600B8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 29 Oct 2021 12:07:13 +0200 (CEST)","by mail-ot1-x32a.google.com with SMTP id\n\ts23-20020a056830125700b00553e2ca2dccso8021034otp.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 29 Oct 2021 03:07:13 -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=\"SUzSx2w/\"; 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=UqxXf0gwVlD0FkuHiAYHuRgrpJ8cgBTyVhQZqsNKBuA=;\n\tb=SUzSx2w/ryAxthA7Nmcn0A/X6+BtzFIIc3hraxA0WCH1Dk1bw4mIL+lNeICnsfiQBZ\n\tYKloRKnuNqmgAmXrCMUrICxp3R9KiRdrfKeLCVzvH06A1cHen52C10w5xS5cYI5Yr+Wq\n\tAz/3dYUt3yfEvStGRrDgC7Z9iu3RZRzwhMfBw=","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=UqxXf0gwVlD0FkuHiAYHuRgrpJ8cgBTyVhQZqsNKBuA=;\n\tb=4ONhDkff7aV8NdnuqvsauMM9l66lER0jllpDzi2PKwJ8moFiXztG94/apXv5h6S0pu\n\thWurBPa+P0a6fPE897dB3ImlzMyqo8GXmOLSl+h6J/PC6hAhY5sksb6bRRxfp/KR2il6\n\ta8a//gxO1/WS1qeEGKIvtkYmqbmeiFC1ET0SizrDjU3iIj9WCz3ds2UBtFb0QvYtahiq\n\tZ8ycoxtzl3s0gPPVfv0Bm2cKGubriWlFTBYjY10RjnP65rIZ/OxkEVnCkbd5IO7uiY7I\n\t5Fxe7FqQtJZuUQc0hZKW3hHamhkE350yn41V7CgblsoD9yK7F9or48SVS0Yv25UtgJXv\n\tRHLQ==","X-Gm-Message-State":"AOAM531A+LIzbJGeGsub5KWrBbo1Ur2P5hflCl2fArLhfBAIcMlm5xbx\n\t9QSFvElbjwxZHz2Syj63EOCuB5LJuHyfMJLx5MBNSBghoDwqm6f/","X-Google-Smtp-Source":"ABdhPJyXaKv0OD8ADZBicFlGaMrOAmW/Yf0jOELOspTbR+D/26eQI8yCj/32utvL/RiPIW7CpXM8tAGd7NUkVGTGNa4=","X-Received":"by 2002:a9d:53c4:: with SMTP id i4mr7468918oth.176.1635502031945;\n\tFri, 29 Oct 2021 03:07:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20211028100349.1098545-1-hanlinchen@chromium.org>\n\t<20211028100349.1098545-3-hanlinchen@chromium.org>\n\t<0b5755e1-5f52-f343-3fcb-68b244c41325@ideasonboard.com>","In-Reply-To":"<0b5755e1-5f52-f343-3fcb-68b244c41325@ideasonboard.com>","From":"Hanlin Chen <hanlinchen@chromium.org>","Date":"Fri, 29 Oct 2021 18:07:01 +0800","Message-ID":"<CAJAuwMn13wNBu_-EA6GpaT8ttWiijuwyNhge8zu6cwUeU8fT-w@mail.gmail.com>","To":"Umang Jain <umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 3/6] ipu3: Set statistics with the\n\teffective AE AiqResults","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20638,"web_url":"https://patchwork.libcamera.org/comment/20638/","msgid":"<CAJAuwMk7Kpus9tWH+JVLSKJztOkw4ZyUXrvLZe__FknNONzwLQ@mail.gmail.com>","date":"2021-10-29T11:55:45","subject":"Re: [libcamera-devel] [PATCH 3/6] ipu3: Set statistics with the\n\teffective AE AiqResults","submitter":{"id":98,"url":"https://patchwork.libcamera.org/api/people/98/","name":"Hanlin Chen","email":"hanlinchen@chromium.org"},"content":"On Fri, Oct 29, 2021 at 2:34 AM Umang Jain <umang.jain@ideasonboard.com> wrote:\n>\n> Hi Han-Lin,\n>\n> Thank you for the patch.\n>\n> On 10/28/21 3:33 PM, Han-Lin Chen wrote:\n> > Set the statistics with the latest AE AiqResults which has the same\n> > exposure time and analog gain. The patch reduces the AE haunting during\n> > the converging process.\n> >\n> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.com>\n> > ---\n> >   aiq/aiq_input_parameters.cpp |  4 +--\n> >   ipu3.cpp                     | 66 ++++++++++++++++++++++++++++++------\n> >   2 files changed, 57 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp\n> > index bc87b31..5dd2f6c 100644\n> > --- a/aiq/aiq_input_parameters.cpp\n> > +++ b/aiq/aiq_input_parameters.cpp\n> > @@ -130,7 +130,7 @@ AiqInputParameters &AiqInputParameters::operator=(const AiqInputParameters &othe\n> >\n> >   void AiqInputParameters::setAeAwbAfDefaults()\n> >   {\n> > -     /*Ae Params */\n> > +     /* Ae Params */\n> >       aeInputParams.num_exposures = NUM_EXPOSURES;\n> >       aeInputParams.frame_use = ia_aiq_frame_use_preview;\n> >       aeInputParams.flash_mode = ia_aiq_flash_mode_off;\n> > @@ -166,7 +166,7 @@ void AiqInputParameters::setAeAwbAfDefaults()\n> >               ia_aiq_af_range_normal,\n> >               ia_aiq_af_metering_mode_auto,\n> >               ia_aiq_flash_mode_off,\n> > -             NULL, NULL, false\n> > +             &focusRect, &manualFocusParams, false\n>\n>\n> Should this be moved to focus patch one instead? I find this odd to be\n> present in this patch, since it's about AE results?\nYes, you're right ;-).\n>\n> >       };\n> >\n> >       /* GBCE Params */\n> > diff --git a/ipu3.cpp b/ipu3.cpp\n> > index 8126e9d..b7de901 100644\n> > --- a/ipu3.cpp\n> > +++ b/ipu3.cpp\n> > @@ -59,7 +59,8 @@ private:\n> >       void fillParams(unsigned int frame, ipu3_uapi_params *params);\n> >       void parseStatistics(unsigned int frame,\n> >                            int64_t frameTimestamp,\n> > -                          const ipu3_uapi_stats_3a *stats);\n> > +                          const ipu3_uapi_stats_3a *stats,\n> > +                          const ControlList& sensorCtrls);\n> >\n> >       void setControls(unsigned int frame);\n> >\n> > @@ -84,6 +85,7 @@ private:\n> >       /* Temporary storage until we have a FrameContext object / struct */\n> >       aiq::AiqInputParameters aiqInputParams_;\n> >       aiq::AiqResults results_;\n> > +     aiq::AiqResultsRingBuffer resultsHistory_;\n> >\n> >       BinaryData aiqb_;\n> >       BinaryData nvm_;\n> > @@ -282,6 +284,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,\n> >       /* Upate the camera controls using the new sensor settings. */\n> >       updateControls(sensorInfo_, ctrls_, ipaControls);\n> >\n> > +     resultsHistory_.Reset();\n> > +\n> >       return 0;\n> >   }\n> >\n> > @@ -327,7 +331,10 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> >               const ipu3_uapi_stats_3a *stats =\n> >                       reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());\n> >\n> > -             parseStatistics(event.frame, event.frameTimestamp, stats);\n> > +             parseStatistics(event.frame,\n> > +                                                                             event.frameTimestamp,\n> > +                                                                             stats,\n> > +                                                                             event.sensorControls);\n>\n>\n> indentation\n>\n> >               break;\n> >       }\n> >       case EventFillParams: {\n> > @@ -374,14 +381,16 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> >       */\n> >\n> >       /* Run algorithms into/using this context structure */\n> > -     if (frame % 10 == 0)\n> > -             aiq_.run2a(frame, aiqInputParams_, results_);\n> > +     aiq_.run2a(frame, aiqInputParams_, results_);\n> >\n> >       aic_.updateRuntimeParams(results_);\n> >       aic_.run(params);\n> >\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> > +     resultsHistory_.Push(results_);\n>\n>\n> Possibly usage of ringbuffer to highlight how it's used, what it\n> provides (a high level documentation on it's 'need', the 'when' and\n> 'why' of previous algo-runs results lookup) would be good to understand\n> the context I believe. All this can be a separate patch.\n>\n> > +\n> >       setControls(frame);\n> >\n> >       IPU3Action op;\n> > @@ -392,7 +401,8 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> >\n> >   void IPAIPU3::parseStatistics(unsigned int frame,\n> >                             int64_t frameTimestamp,\n> > -                           const ipu3_uapi_stats_3a *stats)\n> > +                           const ipu3_uapi_stats_3a *stats,\n> > +                           const ControlList& sensorCtrls)\n> >   {\n> >       ControlList ctrls(controls::controls);\n> >\n> > @@ -403,10 +413,43 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> >        */\n> >\n> >       ASSERT (frameTimestamp > 0);\n> > -     aiq_.setStatistics(frame, frameTimestamp, results_, stats);\n> > +\n> > +     /* Ae algorithm expects the statistics to be set with it's corresponding Ae\n> > +      * result, i.e., the Ae result should match the exposure time/analog gain\n> > +      * with the the effective sensor controls of the statistics.\n> > +      * Search the required Ae result in the result history and combine it with\n> > +      * the latest result as the input to AIQ::setStatistics().\n> > +      */\n>\n>\n> Is there a bit more descriptive documentation I can refer to? Maybe in\n> the ia_aiq documentation to understand 'when' and 'how' the aiq results\n> are meant to be re-used? I think I wrote my last comment a bit hastily\nSadly, there are no documents about it. I collect the restriction from\nIntel's HAL implementation and experiments.\n>\n> > +\n> > +     int32_t effectiveExpo = 0;\n> > +     int32_t effectiveGain = 0;\n> > +     ControlValue ctrlValue;\n> > +\n> > +     ctrlValue = sensorCtrls.get(V4L2_CID_EXPOSURE);\n> > +     if (!ctrlValue.isNone())\n> > +             effectiveExpo = ctrlValue.get<int32_t>();\n> > +\n> > +     ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN);\n> > +     if (!ctrlValue.isNone())\n> > +             effectiveGain = ctrlValue.get<int32_t>();\n> > +\n> > +     auto pred = [effectiveExpo, effectiveGain] (aiq::AiqResults& result) {\n> > +             ia_aiq_exposure_sensor_parameters* sensorExposure =\n> > +                     result.ae()->exposures[0].sensor_exposure;\n> > +\n> > +             return (effectiveExpo == sensorExposure->coarse_integration_time ||\n> > +                     effectiveGain == sensorExposure->analog_gain_code_global);\n> > +     };\n> > +     aiq::AiqResults& aeMatchedResults = resultsHistory_.searchBackward(pred, results_);\n> > +     aiq::AiqResults combinedResults = results_;\n> > +     combinedResults.setAe(aeMatchedResults.ae());\n> > +\n> > +     /* Aiq library expects timestamp in microseconds */\n> > +     aiq_.setStatistics(frame, (frameTimestamp / 1000), combinedResults, stats);\n> >\n> >       /* Set frame durations from exposure results */\n> > -     ia_aiq_exposure_sensor_parameters *sensorExposure = results_.ae()->exposures->sensor_exposure;\n> > +     ia_aiq_exposure_sensor_parameters *sensorExposure = combinedResults.ae()->exposures->sensor_exposure;\n> >       int64_t frameDuration = (sensorExposure->line_length_pixels * sensorExposure->frame_length_lines) /\n> >                               (sensorInfo_.pixelRate / 1e6);\n> >       ctrls.set(controls::FrameDuration, frameDuration);\n> > @@ -423,10 +466,11 @@ void IPAIPU3::setControls(unsigned int frame)\n> >       IPU3Action op;\n> >       op.op = ActionSetSensorControls;\n> >\n> > -     ControlList ctrls(ctrls_);\n> > -     ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n> > -     ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));\n> > -     op.controls = ctrls;\n> > +     ControlList sensorCtrls(ctrls_);\n> > +     sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));\n> > +     sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));\n> > +\n> > +     op.sensorControls = sensorCtrls;\n> >\n> >       queueFrameAction.emit(frame, op);\n> >   }","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 05800BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 29 Oct 2021 11:56:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 51B45600BD;\n\tFri, 29 Oct 2021 13:55:59 +0200 (CEST)","from mail-oi1-x22f.google.com (mail-oi1-x22f.google.com\n\t[IPv6:2607:f8b0:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C7D3B600B8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 29 Oct 2021 13:55:57 +0200 (CEST)","by mail-oi1-x22f.google.com with SMTP id n63so12983201oif.7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 29 Oct 2021 04:55:57 -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=\"OaJfe5tC\"; 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=22tTqyH1yUjb50ujoF1Bx4CbTpPlkg286aZcnde9f4E=;\n\tb=OaJfe5tCd0jglg3NUqU86ds7DLqPkDfeqNN2MudbNH60PUvPcvzyqPgGpPVche3L8R\n\tlcOVKtn37s3q0bQqrUaxNlNZdIx4t4xQ6nCHeBKOJWFh0Ymykh7KDkikBDc+reqNZYpu\n\tvOI9NpatKck9TZKQv5gKYA6HpmssY1+7xCrHU=","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=22tTqyH1yUjb50ujoF1Bx4CbTpPlkg286aZcnde9f4E=;\n\tb=TAD4wNqCXvIIRf+mWIkOt+KGwdEu08krTqE+gXfZpwxwST/5zKjyBtMqmhXLVcF6sE\n\tcCa7Bn9xAB8Zk246WAnowpXZsZa1+Se8q9+W34KNcSeQ45zPeHOBFJBQQADsoIrsMHXh\n\ttOmDgfjRJF+0niFRIxeAijYym37B5Q66Q0EaZl4zOBkEJi9cvb7TKBurVs1jF76g1LNq\n\tjxx0uCKUsSUHWQsZ7b9323GfT8RnBdQiw7WLNHelF1swIAuvyfAmhm0+S6nrUZVb2Ztk\n\t9lYO3tEt0cJRNXsf/97caHhDET9Y8oOKjTviEyj1VLEbq/uyKLzWCpfvOoCj5nLz5PCa\n\tmAPw==","X-Gm-Message-State":"AOAM530+BI+TgG//EY7ie9Dtdi1+B/8mWUSU1myJTp8bGDwOPCKZ73jw\n\t59Qc3nkQODayKuple2UFCywMEbKxKU2uxG+AbUjJM4xtSnwZgw==","X-Google-Smtp-Source":"ABdhPJwFYoBWU6BxWw1RL1G78AjrrLUwsD0GIKzGNBNLWRtqkj5NaIVWR1e0J5uEbiVLWRxIfLqC14HBhNvPNA9Dkb0=","X-Received":"by 2002:a54:4f8f:: with SMTP id\n\tg15mr7449614oiy.169.1635508556477; \n\tFri, 29 Oct 2021 04:55:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20211028100349.1098545-1-hanlinchen@chromium.org>\n\t<20211028100349.1098545-3-hanlinchen@chromium.org>\n\t<4f3745e7-4892-3b4c-2542-d95c2ca28551@ideasonboard.com>","In-Reply-To":"<4f3745e7-4892-3b4c-2542-d95c2ca28551@ideasonboard.com>","From":"Hanlin Chen <hanlinchen@chromium.org>","Date":"Fri, 29 Oct 2021 19:55:45 +0800","Message-ID":"<CAJAuwMk7Kpus9tWH+JVLSKJztOkw4ZyUXrvLZe__FknNONzwLQ@mail.gmail.com>","To":"Umang Jain <umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 3/6] ipu3: Set statistics with the\n\teffective AE AiqResults","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]