[{"id":20665,"web_url":"https://patchwork.libcamera.org/comment/20665/","msgid":"<163586149638.275423.17276391440742370199@Monstersaurus>","date":"2021-11-02T13:58:16","subject":"Re: [libcamera-devel] [PATCH v2 3/6] ipu3: Set statistics with the\n\teffective AE AiqResults","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Han-Lin Chen (2021-10-29 12:59:58)\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\ns/haunting/hunting/\n\nAlthough appropriate for Halloween ;-)\n\n> the converging process.\n> \n> Signed-off-by: Han-Lin Chen \n> ---\n> aiq/aiq_input_parameters.cpp | 2 +-\n> ipu3.cpp | 66 ++++++++++++++++++++++++++++++------\n> 2 files changed, 56 insertions(+), 12 deletions(-)\n> \n> diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp\n> index bc87b31..46553a6 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> diff --git a/ipu3.cpp b/ipu3.cpp\n> index 8126e9d..f463805 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\nIs this still needed (results_)? Isn't it just stored in the history ring buffer?\n\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(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\nVery happy to see the artifical run delay removed.\n\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\nAh, ok so you are still using the results_ ... but is this introducing\nunnecessary copies? Can't we just decide / create a new entry in the\nresultsHistory_ (and later rename that to be 'results_') and directly\nrun the run2a call into that active entry?\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 its corresponding\n> + * Ae result, i.e., the Ae result should match the exposure time and\n> + * analog gain with the the effective sensor controls of the statistics.\n> + * Search the required Ae result in the result history and combine it\n> + * with 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();\n> +\n> + ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN);\n> + if (!ctrlValue.isNone())\n> + effectiveGain = ctrlValue.get();\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\nThis is an interesting implementation! (in a good way), though I wonder\nif it's over complicated.\n\nI thought we'd be able to do something simpler, because we know the\nframe number index, so we should be able to associate that with the\nresultsHistory_ and obtain the results from the frame that was queued\nwith the same 'frame' number...\n\nCould you check the frame numbers of when the parameters are queued up,\nagainst the 'frame' that comes in here, and see if it matches up with\nyour implementation as a suitable method to tie the results together?\n\n\n> + aiq::AiqResults combinedResults = results_;\n> + combinedResults.setAe(aeMatchedResults.ae());\n\nOh - that's interesting, so we take the latest calculated results, and\nthe ae() from the matched results..\n\n> +\n> + /* Aiq library expects timestamp in microseconds */\n\nPerhaps std::chrono would help us here in the future. Doesn't have to be\nnow.\n\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(exposure_));\n> - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> - op.controls = ctrls;\n> + ControlList sensorCtrls(ctrls_);\n> + sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> + sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> +\n> + op.sensorControls = sensorCtrls;\n> \n> queueFrameAction.emit(frame, op);\n> }\n> -- \n> 2.33.1.1089.g2158813163f-goog\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 A63F6BF415\n\tfor ;\n\tTue, 2 Nov 2021 13:58:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE9CA60325;\n\tTue, 2 Nov 2021 14:58:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9DD01600B8\n\tfor ;\n\tTue, 2 Nov 2021 14:58:19 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 42CDB3E5;\n\tTue, 2 Nov 2021 14:58:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ODa1nJ8a\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635861499;\n\tbh=0Hn8IPY+3qHsg8A1jgC3LuywL8A3KvaxpaAXThLaIpU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=ODa1nJ8aPYPXBz2GkenWRFOPugq7cWWIHy2pPQSbum60D1na6iNas/bOodV2WzDBR\n\tBUuok2YDwdQl3bkUYM5c590GSXwGV5MH1BzcdqjdovP/CD/DKa8ase4Cuqx6DPHhAN\n\tvZXod/B6ntKLa94txzBtYrRTNmi3Gtk2vbgH+t0I=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211029120001.2469018-3-hanlinchen@chromium.org>","References":"<20211029120001.2469018-1-hanlinchen@chromium.org>\n\t<20211029120001.2469018-3-hanlinchen@chromium.org>","From":"Kieran Bingham ","To":"Han-Lin Chen ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 02 Nov 2021 13:58:16 +0000","Message-ID":"<163586149638.275423.17276391440742370199@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20807,"web_url":"https://patchwork.libcamera.org/comment/20807/","msgid":"","date":"2021-11-10T13:33:13","subject":"Re: [libcamera-devel] [PATCH v2 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 Kieran,\n\nOn Tue, Nov 2, 2021 at 9:58 PM Kieran Bingham\n wrote:\n>\n> Quoting Han-Lin Chen (2021-10-29 12:59:58)\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>\n> s/haunting/hunting/\n>\n> Although appropriate for Halloween ;-)\n>\n> > the converging process.\n> >\n> > Signed-off-by: Han-Lin Chen \n> > ---\n> > aiq/aiq_input_parameters.cpp | 2 +-\n> > ipu3.cpp | 66 ++++++++++++++++++++++++++++++------\n> > 2 files changed, 56 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp\n> > index bc87b31..46553a6 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> > diff --git a/ipu3.cpp b/ipu3.cpp\n> > index 8126e9d..f463805 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>\n> Is this still needed (results_)? Isn't it just stored in the history ring buffer?\n>\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(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> Very happy to see the artifical run delay removed.\n>\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>\n> Ah, ok so you are still using the results_ ... but is this introducing\n> unnecessary copies? Can't we just decide / create a new entry in the\n> resultsHistory_ (and later rename that to be 'results_') and directly\n> run the run2a call into that active entry?\nYes. Will remove the \"results_\".\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 its corresponding\n> > + * Ae result, i.e., the Ae result should match the exposure time and\n> > + * analog gain with the the effective sensor controls of the statistics.\n> > + * Search the required Ae result in the result history and combine it\n> > + * with 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();\n> > +\n> > + ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN);\n> > + if (!ctrlValue.isNone())\n> > + effectiveGain = ctrlValue.get();\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> This is an interesting implementation! (in a good way), though I wonder\n> if it's over complicated.\n>\n> I thought we'd be able to do something simpler, because we know the\n> frame number index, so we should be able to associate that with the\n> resultsHistory_ and obtain the results from the frame that was queued\n> with the same 'frame' number...\n>\n> Could you check the frame numbers of when the parameters are queued up,\n> against the 'frame' that comes in here, and see if it matches up with\n> your implementation as a suitable method to tie the results together?\nYes, I struggled with it too :-).\nThe effective AiqResults may be delayed for an interval which depends\non when the pipeline handler applies controls when cio2 is queued up\nand the delay for the sensor controls to be effective, especially\nexposure and gain.\nIn theory, the delay could be the delay on (applying controls delay +\nsensor effective delay).\nThere were three approaches that were considered.\n\n1. Let IPA assume the implementation and get the delay property of the\nsensor from the pipeline handler and do the calculation.\nIt makes me a little uncomfortable to let IPA assume the\nimplementation details of the pipeline handler, so I skipped this.\n2. Associate the results with an ID and return it back to pipeline\nhandler, and let pipeline send back the effective ID with statistics.\nThis is how Intel HAL in ChromeOS is implemented, but this requires\nchanges on the IPA interface and the DelayedControls class.\n3. Current approach which finds the results backwards.\n\n>\n>\n> > + aiq::AiqResults combinedResults = results_;\n> > + combinedResults.setAe(aeMatchedResults.ae());\n>\n> Oh - that's interesting, so we take the latest calculated results, and\n> the ae() from the matched results..\n>\n> > +\n> > + /* Aiq library expects timestamp in microseconds */\n>\n> Perhaps std::chrono would help us here in the future. Doesn't have to be\n> now.\n>\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(exposure_));\n> > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> > - op.controls = ctrls;\n> > + ControlList sensorCtrls(ctrls_);\n> > + sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> > + sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> > +\n> > + op.sensorControls = sensorCtrls;\n> >\n> > queueFrameAction.emit(frame, op);\n> > }\n> > --\n> > 2.33.1.1089.g2158813163f-goog\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 4575ABDB1C\n\tfor ;\n\tWed, 10 Nov 2021 13:33:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7150160361;\n\tWed, 10 Nov 2021 14:33:27 +0100 (CET)","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 1D4416033C\n\tfor ;\n\tWed, 10 Nov 2021 14:33:26 +0100 (CET)","by mail-oi1-x22f.google.com with SMTP id bf8so5233221oib.6\n\tfor ;\n\tWed, 10 Nov 2021 05:33:26 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"VZzF7R5v\"; 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=FfK9gFSwHLb0KwAJ8DLZB1gdIci+RosDpjlUE0svPIw=;\n\tb=VZzF7R5vZuyoj2rKE4z4U2yhW6clSxaI6eMq0kdHKdOxH444NiFFFnPBGg1iylBY3A\n\tGtU/ZJP1zemqRTyZbk27BvWA5eBKLDHPEmfyY3yZaILwkrr7MMzwZaOJd3Ni4IB2+mYs\n\tISMg+St9DYxGbA5ZuqJUNSPJlwuEh6aw/Xv0k=","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=FfK9gFSwHLb0KwAJ8DLZB1gdIci+RosDpjlUE0svPIw=;\n\tb=y38S7CrYMFXwE0xI90O2ymd114pAiiDi+U0hbe3CjOkK5sHn4q2QrQ70h86gEeKp+R\n\tXum50X16Wt/esTOTcOH3W8TKveAOm6V08V4DxSBlduibXXh8zgCR95nfnAkNEWYzxM27\n\tqZRUBUZ7R/5mJqOT9kDuQh0OSUtpVce8LbTzsEGWdxy1NFpA6BmxO7Dai1mnoKXI8Qwi\n\tZE9FrUOg+jfp7TEouHkTjna6WxVOilK+IeZ+i7E5JgI3n4+cIfXmprmauMSH2i2l5M47\n\tmjVQTYFJAbBSp5mEH/c2Ecr7U6onstcS0zH5twdQToMIH1QpKwfHuvfKbWYHnGqmyaNI\n\tSZ2Q==","X-Gm-Message-State":"AOAM530AkRCToE7qWwDXKgDFJ5shw1kJ60JYf46ImEdw3y3hYTXIGvJJ\n\trrz7jQffxDXc7EFH5BeRzq6N4sGdZ/YbHhAvylBB1oq0UmI=","X-Google-Smtp-Source":"ABdhPJwtGClLArW9g4tXb3JLD8PjDWum7QQt90mTwuMgHfeOSQjWvw52Dr7nnb65b+lyh5fvcn1n6ZHPWtXKkZnidJg=","X-Received":"by 2002:a54:4f8f:: with SMTP id\n\tg15mr12298524oiy.169.1636551204702; \n\tWed, 10 Nov 2021 05:33:24 -0800 (PST)","MIME-Version":"1.0","References":"<20211029120001.2469018-1-hanlinchen@chromium.org>\n\t<20211029120001.2469018-3-hanlinchen@chromium.org>\n\t<163586149638.275423.17276391440742370199@Monstersaurus>","In-Reply-To":"<163586149638.275423.17276391440742370199@Monstersaurus>","From":"Hanlin Chen ","Date":"Wed, 10 Nov 2021 21:33:13 +0800","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20809,"web_url":"https://patchwork.libcamera.org/comment/20809/","msgid":"<163655209209.1946932.9341037718959694294@Monstersaurus>","date":"2021-11-10T13:48:12","subject":"Re: [libcamera-devel] [PATCH v2 3/6] ipu3: Set statistics with the\n\teffective AE AiqResults","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hanlin Chen (2021-11-10 13:33:13)\n> Hi Kieran,\n> \n> On Tue, Nov 2, 2021 at 9:58 PM Kieran Bingham\n> wrote:\n> >\n> > Quoting Han-Lin Chen (2021-10-29 12:59:58)\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> >\n> > s/haunting/hunting/\n> >\n> > Although appropriate for Halloween ;-)\n> >\n> > > the converging process.\n> > >\n> > > Signed-off-by: Han-Lin Chen \n> > > ---\n> > > aiq/aiq_input_parameters.cpp | 2 +-\n> > > ipu3.cpp | 66 ++++++++++++++++++++++++++++++------\n> > > 2 files changed, 56 insertions(+), 12 deletions(-)\n> > >\n> > > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp\n> > > index bc87b31..46553a6 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> > > diff --git a/ipu3.cpp b/ipu3.cpp\n> > > index 8126e9d..f463805 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> >\n> > Is this still needed (results_)? Isn't it just stored in the history ring buffer?\n> >\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(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> > Very happy to see the artifical run delay removed.\n> >\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> >\n> > Ah, ok so you are still using the results_ ... but is this introducing\n> > unnecessary copies? Can't we just decide / create a new entry in the\n> > resultsHistory_ (and later rename that to be 'results_') and directly\n> > run the run2a call into that active entry?\n> Yes. Will remove the \"results_\".\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 its corresponding\n> > > + * Ae result, i.e., the Ae result should match the exposure time and\n> > > + * analog gain with the the effective sensor controls of the statistics.\n> > > + * Search the required Ae result in the result history and combine it\n> > > + * with 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();\n> > > +\n> > > + ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN);\n> > > + if (!ctrlValue.isNone())\n> > > + effectiveGain = ctrlValue.get();\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> > This is an interesting implementation! (in a good way), though I wonder\n> > if it's over complicated.\n> >\n> > I thought we'd be able to do something simpler, because we know the\n> > frame number index, so we should be able to associate that with the\n> > resultsHistory_ and obtain the results from the frame that was queued\n> > with the same 'frame' number...\n> >\n> > Could you check the frame numbers of when the parameters are queued up,\n> > against the 'frame' that comes in here, and see if it matches up with\n> > your implementation as a suitable method to tie the results together?\n> Yes, I struggled with it too :-).\n> The effective AiqResults may be delayed for an interval which depends\n> on when the pipeline handler applies controls when cio2 is queued up\n> and the delay for the sensor controls to be effective, especially\n> exposure and gain.\n> In theory, the delay could be the delay on (applying controls delay +\n> sensor effective delay).\n> There were three approaches that were considered.\n> \n> 1. Let IPA assume the implementation and get the delay property of the\n> sensor from the pipeline handler and do the calculation.\n> It makes me a little uncomfortable to let IPA assume the\n> implementation details of the pipeline handler, so I skipped this.\n> 2. Associate the results with an ID and return it back to pipeline\n> handler, and let pipeline send back the effective ID with statistics.\n> This is how Intel HAL in ChromeOS is implemented, but this requires\n> changes on the IPA interface and the DelayedControls class.\n> 3. Current approach which finds the results backwards.\n\nAs far as I'm aware, 2 is what we also already support. Every operation\nshould have an id/frame number/sequence number which matches the request\nsequence number.\n\nUsing your implementation, you should be able to verify (or vilify) the\nmatching of the results you determine against the ones that were\ngenerated for the same index/sequence number.\n\nRe-reading your text though, am I right to infer that DelayedControls is\nthrowing this option out?\n\n\n\n> \n> >\n> >\n> > > + aiq::AiqResults combinedResults = results_;\n> > > + combinedResults.setAe(aeMatchedResults.ae());\n> >\n> > Oh - that's interesting, so we take the latest calculated results, and\n> > the ae() from the matched results..\n> >\n> > > +\n> > > + /* Aiq library expects timestamp in microseconds */\n> >\n> > Perhaps std::chrono would help us here in the future. Doesn't have to be\n> > now.\n> >\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(exposure_));\n> > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> > > - op.controls = ctrls;\n> > > + ControlList sensorCtrls(ctrls_);\n> > > + sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> > > + sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> > > +\n> > > + op.sensorControls = sensorCtrls;\n> > >\n> > > queueFrameAction.emit(frame, op);\n> > > }\n> > > --\n> > > 2.33.1.1089.g2158813163f-goog\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 56EFDBDB1C\n\tfor ;\n\tWed, 10 Nov 2021 13:48:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9893E6035D;\n\tWed, 10 Nov 2021 14:48:16 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBC956033C\n\tfor ;\n\tWed, 10 Nov 2021 14:48:15 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 86829292;\n\tWed, 10 Nov 2021 14:48:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Nuu/wyHS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636552095;\n\tbh=clWBNS/x6QQqti2tJr0HnoPD9H/RJFqVhbALpMn+ORA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Nuu/wyHSO3kUR7d5XOa2/n2m2T+SejwqevCZ7Gc4C4ES9UshLCGvOTrEdLalyd6Rt\n\tkGyqROHlHOhGkT69xB79RNkubhbVWO2KsYpOqKAjEweVBpoE9fUPAmXnSjg6gj0DvL\n\trFsHeXDqGteEGcFbv9MGaT5FsqRryPOzCsHv1WPY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211029120001.2469018-1-hanlinchen@chromium.org>\n\t<20211029120001.2469018-3-hanlinchen@chromium.org>\n\t<163586149638.275423.17276391440742370199@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Hanlin Chen ","Date":"Wed, 10 Nov 2021 13:48:12 +0000","Message-ID":"<163655209209.1946932.9341037718959694294@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20811,"web_url":"https://patchwork.libcamera.org/comment/20811/","msgid":"","date":"2021-11-10T14:06:30","subject":"Re: [libcamera-devel] [PATCH v2 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 Wed, Nov 10, 2021 at 9:48 PM Kieran Bingham\n wrote:\n>\n> Quoting Hanlin Chen (2021-11-10 13:33:13)\n> > Hi Kieran,\n> >\n> > On Tue, Nov 2, 2021 at 9:58 PM Kieran Bingham\n> > wrote:\n> > >\n> > > Quoting Han-Lin Chen (2021-10-29 12:59:58)\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> > >\n> > > s/haunting/hunting/\n> > >\n> > > Although appropriate for Halloween ;-)\n> > >\n> > > > the converging process.\n> > > >\n> > > > Signed-off-by: Han-Lin Chen \n> > > > ---\n> > > > aiq/aiq_input_parameters.cpp | 2 +-\n> > > > ipu3.cpp | 66 ++++++++++++++++++++++++++++++------\n> > > > 2 files changed, 56 insertions(+), 12 deletions(-)\n> > > >\n> > > > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp\n> > > > index bc87b31..46553a6 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> > > > diff --git a/ipu3.cpp b/ipu3.cpp\n> > > > index 8126e9d..f463805 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> > >\n> > > Is this still needed (results_)? Isn't it just stored in the history ring buffer?\n> > >\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(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> > > Very happy to see the artifical run delay removed.\n> > >\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> > >\n> > > Ah, ok so you are still using the results_ ... but is this introducing\n> > > unnecessary copies? Can't we just decide / create a new entry in the\n> > > resultsHistory_ (and later rename that to be 'results_') and directly\n> > > run the run2a call into that active entry?\n> > Yes. Will remove the \"results_\".\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 its corresponding\n> > > > + * Ae result, i.e., the Ae result should match the exposure time and\n> > > > + * analog gain with the the effective sensor controls of the statistics.\n> > > > + * Search the required Ae result in the result history and combine it\n> > > > + * with 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();\n> > > > +\n> > > > + ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN);\n> > > > + if (!ctrlValue.isNone())\n> > > > + effectiveGain = ctrlValue.get();\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> > > This is an interesting implementation! (in a good way), though I wonder\n> > > if it's over complicated.\n> > >\n> > > I thought we'd be able to do something simpler, because we know the\n> > > frame number index, so we should be able to associate that with the\n> > > resultsHistory_ and obtain the results from the frame that was queued\n> > > with the same 'frame' number...\n> > >\n> > > Could you check the frame numbers of when the parameters are queued up,\n> > > against the 'frame' that comes in here, and see if it matches up with\n> > > your implementation as a suitable method to tie the results together?\n> > Yes, I struggled with it too :-).\n> > The effective AiqResults may be delayed for an interval which depends\n> > on when the pipeline handler applies controls when cio2 is queued up\n> > and the delay for the sensor controls to be effective, especially\n> > exposure and gain.\n> > In theory, the delay could be the delay on (applying controls delay +\n> > sensor effective delay).\n> > There were three approaches that were considered.\n> >\n> > 1. Let IPA assume the implementation and get the delay property of the\n> > sensor from the pipeline handler and do the calculation.\n> > It makes me a little uncomfortable to let IPA assume the\n> > implementation details of the pipeline handler, so I skipped this.\n> > 2. Associate the results with an ID and return it back to pipeline\n> > handler, and let pipeline send back the effective ID with statistics.\n> > This is how Intel HAL in ChromeOS is implemented, but this requires\n> > changes on the IPA interface and the DelayedControls class.\n> > 3. Current approach which finds the results backwards.\n>\n> As far as I'm aware, 2 is what we also already support. Every operation\n> should have an id/frame number/sequence number which matches the request\n> sequence number.\n>\n> Using your implementation, you should be able to verify (or vilify) the\n> matching of the results you determine against the ones that were\n> generated for the same index/sequence number.\n>\n> Re-reading your text though, am I right to infer that DelayedControls is\n> throwing this option out?\nYes, it may introduce extra delay which makes frame number a little\noff, but we need that since the sensor controls may delay.\nFor the ID in 2, I mean an extra ID field for Aiq (not the frame\nnumber), and let pipeline handlers return the ID back, since\nthe pipeline handler could figure out the exact ID, by hacking into\nDelayedControls. ;-/\n>\n>\n>\n> >\n> > >\n> > >\n> > > > + aiq::AiqResults combinedResults = results_;\n> > > > + combinedResults.setAe(aeMatchedResults.ae());\n> > >\n> > > Oh - that's interesting, so we take the latest calculated results, and\n> > > the ae() from the matched results..\n> > >\n> > > > +\n> > > > + /* Aiq library expects timestamp in microseconds */\n> > >\n> > > Perhaps std::chrono would help us here in the future. Doesn't have to be\n> > > now.\n> > >\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(exposure_));\n> > > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> > > > - op.controls = ctrls;\n> > > > + ControlList sensorCtrls(ctrls_);\n> > > > + sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> > > > + sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> > > > +\n> > > > + op.sensorControls = sensorCtrls;\n> > > >\n> > > > queueFrameAction.emit(frame, op);\n> > > > }\n> > > > --\n> > > > 2.33.1.1089.g2158813163f-goog\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 55081BF415\n\tfor ;\n\tWed, 10 Nov 2021 14:06:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7773260361;\n\tWed, 10 Nov 2021 15:06:44 +0100 (CET)","from mail-oi1-x231.google.com (mail-oi1-x231.google.com\n\t[IPv6:2607:f8b0:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 209AF6033C\n\tfor ;\n\tWed, 10 Nov 2021 15:06:43 +0100 (CET)","by mail-oi1-x231.google.com with SMTP id m6so5440844oim.2\n\tfor ;\n\tWed, 10 Nov 2021 06:06:43 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"maqU3n+u\"; 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=8c8raE3KjUrVL+XfI1ouUxy81DYnSqBjxX160Y3Cvw8=;\n\tb=maqU3n+ugKT+t+AuwOvCjGzu6Le1lDZ5dHV9NJf02zkOzRvuSWpqFH936WmfeAjvk9\n\tOFAUgijVpfdRCAzvrk+tHB2tNdEe56h8ZgSq+u08yQK6jEDUyyJOrm6WWA0/ggRjKfB0\n\tJmAWEzzW8a0Mg5NwFbQEtGJaw/4AwImWCUXcE=","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=8c8raE3KjUrVL+XfI1ouUxy81DYnSqBjxX160Y3Cvw8=;\n\tb=0iCq4yy67BOKbRB5x62QwM3VKnK7t7Evm3wseax/4DUmsJqNFKvYoC3lpdS2qQjxwq\n\tnmgx5YxCjozIYTH3ctRxTpEhH4/I+6rpMPfkV1RLuuGtykb7M63dBEy3SG+PJvS02MZr\n\tSgd/0lnhqsl9rXBZ38TtphMAjBxsweKA5VM13u4QXcdxG6QCsbuEUm3uoCcSBL46GfR7\n\tUq1lX+whVh5qAozvCmGMf5qDZIiQ5xAL4TVdwl+T0a4BCMobxDjXa8NcWwawlM11Km5G\n\tDjs6voS25YseZeUWCjYJVJ+wc6XKTeu+B9a4kNey9YyWFAZpmoloiJQoNa2FqB3vOUvC\n\tmx0g==","X-Gm-Message-State":"AOAM5338EtbjuXMMiowhqdHKl/gbI5q+pmKBGzVCarDWueakuuy0Kbgm\n\tvQqsANP/p8pTH+vzYxA/jXfm876jzc/rqI+dKY5yTviXeoM=","X-Google-Smtp-Source":"ABdhPJxisnq5L1x7CVifn0nnpmFC1jjU2/0kSXfX/XKQnhCRYqqUo8PqcecLQzt8nIEOw32UfQvWLDKLaDZQ9U1yo2A=","X-Received":"by 2002:a54:4401:: with SMTP id\n\tk1mr12959419oiw.143.1636553201459; \n\tWed, 10 Nov 2021 06:06:41 -0800 (PST)","MIME-Version":"1.0","References":"<20211029120001.2469018-1-hanlinchen@chromium.org>\n\t<20211029120001.2469018-3-hanlinchen@chromium.org>\n\t<163586149638.275423.17276391440742370199@Monstersaurus>\n\t\n\t<163655209209.1946932.9341037718959694294@Monstersaurus>","In-Reply-To":"<163655209209.1946932.9341037718959694294@Monstersaurus>","From":"Hanlin Chen ","Date":"Wed, 10 Nov 2021 22:06:30 +0800","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20814,"web_url":"https://patchwork.libcamera.org/comment/20814/","msgid":"<163655368897.1946932.11331992017061456361@Monstersaurus>","date":"2021-11-10T14:14:48","subject":"Re: [libcamera-devel] [PATCH v2 3/6] ipu3: Set statistics with the\n\teffective AE AiqResults","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hanlin Chen (2021-11-10 14:06:30)\n> On Wed, Nov 10, 2021 at 9:48 PM Kieran Bingham\n> wrote:\n> >\n> > Quoting Hanlin Chen (2021-11-10 13:33:13)\n> > > Hi Kieran,\n> > >\n> > > On Tue, Nov 2, 2021 at 9:58 PM Kieran Bingham\n> > > wrote:\n> > > >\n> > > > Quoting Han-Lin Chen (2021-10-29 12:59:58)\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> > > >\n> > > > s/haunting/hunting/\n> > > >\n> > > > Although appropriate for Halloween ;-)\n> > > >\n> > > > > the converging process.\n> > > > >\n> > > > > Signed-off-by: Han-Lin Chen \n> > > > > ---\n> > > > > aiq/aiq_input_parameters.cpp | 2 +-\n> > > > > ipu3.cpp | 66 ++++++++++++++++++++++++++++++------\n> > > > > 2 files changed, 56 insertions(+), 12 deletions(-)\n> > > > >\n> > > > > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp\n> > > > > index bc87b31..46553a6 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> > > > > diff --git a/ipu3.cpp b/ipu3.cpp\n> > > > > index 8126e9d..f463805 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> > > >\n> > > > Is this still needed (results_)? Isn't it just stored in the history ring buffer?\n> > > >\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(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> > > > Very happy to see the artifical run delay removed.\n> > > >\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> > > >\n> > > > Ah, ok so you are still using the results_ ... but is this introducing\n> > > > unnecessary copies? Can't we just decide / create a new entry in the\n> > > > resultsHistory_ (and later rename that to be 'results_') and directly\n> > > > run the run2a call into that active entry?\n> > > Yes. Will remove the \"results_\".\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 its corresponding\n> > > > > + * Ae result, i.e., the Ae result should match the exposure time and\n> > > > > + * analog gain with the the effective sensor controls of the statistics.\n> > > > > + * Search the required Ae result in the result history and combine it\n> > > > > + * with 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();\n> > > > > +\n> > > > > + ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN);\n> > > > > + if (!ctrlValue.isNone())\n> > > > > + effectiveGain = ctrlValue.get();\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> > > > This is an interesting implementation! (in a good way), though I wonder\n> > > > if it's over complicated.\n> > > >\n> > > > I thought we'd be able to do something simpler, because we know the\n> > > > frame number index, so we should be able to associate that with the\n> > > > resultsHistory_ and obtain the results from the frame that was queued\n> > > > with the same 'frame' number...\n> > > >\n> > > > Could you check the frame numbers of when the parameters are queued up,\n> > > > against the 'frame' that comes in here, and see if it matches up with\n> > > > your implementation as a suitable method to tie the results together?\n> > > Yes, I struggled with it too :-).\n> > > The effective AiqResults may be delayed for an interval which depends\n> > > on when the pipeline handler applies controls when cio2 is queued up\n> > > and the delay for the sensor controls to be effective, especially\n> > > exposure and gain.\n> > > In theory, the delay could be the delay on (applying controls delay +\n> > > sensor effective delay).\n> > > There were three approaches that were considered.\n> > >\n> > > 1. Let IPA assume the implementation and get the delay property of the\n> > > sensor from the pipeline handler and do the calculation.\n> > > It makes me a little uncomfortable to let IPA assume the\n> > > implementation details of the pipeline handler, so I skipped this.\n> > > 2. Associate the results with an ID and return it back to pipeline\n> > > handler, and let pipeline send back the effective ID with statistics.\n> > > This is how Intel HAL in ChromeOS is implemented, but this requires\n> > > changes on the IPA interface and the DelayedControls class.\n> > > 3. Current approach which finds the results backwards.\n> >\n> > As far as I'm aware, 2 is what we also already support. Every operation\n> > should have an id/frame number/sequence number which matches the request\n> > sequence number.\n> >\n> > Using your implementation, you should be able to verify (or vilify) the\n> > matching of the results you determine against the ones that were\n> > generated for the same index/sequence number.\n> >\n> > Re-reading your text though, am I right to infer that DelayedControls is\n> > throwing this option out?\n> Yes, it may introduce extra delay which makes frame number a little\n> off, but we need that since the sensor controls may delay.\n> For the ID in 2, I mean an extra ID field for Aiq (not the frame\n> number), and let pipeline handlers return the ID back, since\n> the pipeline handler could figure out the exact ID, by hacking into\n> DelayedControls. ;-/\n\nThis sounds like the same thing that Jean-Michel is looking at too\n(which is probably evident as I think he used a couple of your patches).\n\nDiscussing with him recently, we realised that sending back the\neffective sensor settings from DelayedControls along with the statistics\nbuffer ensured that the IPA had the information it needed.\n\nIs there any other information/context that you need to ensure is kept\nfrom when the parameter buffers are filled in before capturing an image?\n\n--\nKieran\n\n\n> >\n> >\n> >\n> > >\n> > > >\n> > > >\n> > > > > + aiq::AiqResults combinedResults = results_;\n> > > > > + combinedResults.setAe(aeMatchedResults.ae());\n> > > >\n> > > > Oh - that's interesting, so we take the latest calculated results, and\n> > > > the ae() from the matched results..\n> > > >\n> > > > > +\n> > > > > + /* Aiq library expects timestamp in microseconds */\n> > > >\n> > > > Perhaps std::chrono would help us here in the future. Doesn't have to be\n> > > > now.\n> > > >\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(exposure_));\n> > > > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> > > > > - op.controls = ctrls;\n> > > > > + ControlList sensorCtrls(ctrls_);\n> > > > > + sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> > > > > + sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> > > > > +\n> > > > > + op.sensorControls = sensorCtrls;\n> > > > >\n> > > > > queueFrameAction.emit(frame, op);\n> > > > > }\n> > > > > --\n> > > > > 2.33.1.1089.g2158813163f-goog\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 39BBBBDB1C\n\tfor ;\n\tWed, 10 Nov 2021 14:14:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6B9EE60361;\n\tWed, 10 Nov 2021 15:14:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9D8F46033C\n\tfor ;\n\tWed, 10 Nov 2021 15:14:51 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3A7FD501;\n\tWed, 10 Nov 2021 15:14:51 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uJOX5qUM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636553691;\n\tbh=POvPL33R7RnzEjDA/HsHyS6+cjjbarNp/WVSOyJ6EHc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=uJOX5qUMeiXzI+tZ+WJgZhKKVhp4UQHu4DaFDs/+6Vk1OpFS9Wiy7XgXNuk3foIHz\n\tSTxp9fpBy8/lIiF4UqTgmRW2WDvq4c3veNsJcgJWinJkjXlns6O5bcM/xKbD8ivMvq\n\tSjGeTeYuzjx2fyeJvF6AxluBrfCR3APEk+Jf2P1E=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211029120001.2469018-1-hanlinchen@chromium.org>\n\t<20211029120001.2469018-3-hanlinchen@chromium.org>\n\t<163586149638.275423.17276391440742370199@Monstersaurus>\n\t\n\t<163655209209.1946932.9341037718959694294@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Hanlin Chen ","Date":"Wed, 10 Nov 2021 14:14:48 +0000","Message-ID":"<163655368897.1946932.11331992017061456361@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20967,"web_url":"https://patchwork.libcamera.org/comment/20967/","msgid":"","date":"2021-11-16T10:07:06","subject":"Re: [libcamera-devel] [PATCH v2 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 Wed, Nov 10, 2021 at 10:14 PM Kieran Bingham\n wrote:\n>\n> Quoting Hanlin Chen (2021-11-10 14:06:30)\n> > On Wed, Nov 10, 2021 at 9:48 PM Kieran Bingham\n> > wrote:\n> > >\n> > > Quoting Hanlin Chen (2021-11-10 13:33:13)\n> > > > Hi Kieran,\n> > > >\n> > > > On Tue, Nov 2, 2021 at 9:58 PM Kieran Bingham\n> > > > wrote:\n> > > > >\n> > > > > Quoting Han-Lin Chen (2021-10-29 12:59:58)\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> > > > >\n> > > > > s/haunting/hunting/\n> > > > >\n> > > > > Although appropriate for Halloween ;-)\n> > > > >\n> > > > > > the converging process.\n> > > > > >\n> > > > > > Signed-off-by: Han-Lin Chen \n> > > > > > ---\n> > > > > > aiq/aiq_input_parameters.cpp | 2 +-\n> > > > > > ipu3.cpp | 66 ++++++++++++++++++++++++++++++------\n> > > > > > 2 files changed, 56 insertions(+), 12 deletions(-)\n> > > > > >\n> > > > > > diff --git a/aiq/aiq_input_parameters.cpp b/aiq/aiq_input_parameters.cpp\n> > > > > > index bc87b31..46553a6 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> > > > > > diff --git a/ipu3.cpp b/ipu3.cpp\n> > > > > > index 8126e9d..f463805 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> > > > >\n> > > > > Is this still needed (results_)? Isn't it just stored in the history ring buffer?\n> > > > >\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(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> > > > > Very happy to see the artifical run delay removed.\n> > > > >\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> > > > >\n> > > > > Ah, ok so you are still using the results_ ... but is this introducing\n> > > > > unnecessary copies? Can't we just decide / create a new entry in the\n> > > > > resultsHistory_ (and later rename that to be 'results_') and directly\n> > > > > run the run2a call into that active entry?\n> > > > Yes. Will remove the \"results_\".\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 its corresponding\n> > > > > > + * Ae result, i.e., the Ae result should match the exposure time and\n> > > > > > + * analog gain with the the effective sensor controls of the statistics.\n> > > > > > + * Search the required Ae result in the result history and combine it\n> > > > > > + * with 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();\n> > > > > > +\n> > > > > > + ctrlValue = sensorCtrls.get(V4L2_CID_ANALOGUE_GAIN);\n> > > > > > + if (!ctrlValue.isNone())\n> > > > > > + effectiveGain = ctrlValue.get();\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> > > > > This is an interesting implementation! (in a good way), though I wonder\n> > > > > if it's over complicated.\n> > > > >\n> > > > > I thought we'd be able to do something simpler, because we know the\n> > > > > frame number index, so we should be able to associate that with the\n> > > > > resultsHistory_ and obtain the results from the frame that was queued\n> > > > > with the same 'frame' number...\n> > > > >\n> > > > > Could you check the frame numbers of when the parameters are queued up,\n> > > > > against the 'frame' that comes in here, and see if it matches up with\n> > > > > your implementation as a suitable method to tie the results together?\n> > > > Yes, I struggled with it too :-).\n> > > > The effective AiqResults may be delayed for an interval which depends\n> > > > on when the pipeline handler applies controls when cio2 is queued up\n> > > > and the delay for the sensor controls to be effective, especially\n> > > > exposure and gain.\n> > > > In theory, the delay could be the delay on (applying controls delay +\n> > > > sensor effective delay).\n> > > > There were three approaches that were considered.\n> > > >\n> > > > 1. Let IPA assume the implementation and get the delay property of the\n> > > > sensor from the pipeline handler and do the calculation.\n> > > > It makes me a little uncomfortable to let IPA assume the\n> > > > implementation details of the pipeline handler, so I skipped this.\n> > > > 2. Associate the results with an ID and return it back to pipeline\n> > > > handler, and let pipeline send back the effective ID with statistics.\n> > > > This is how Intel HAL in ChromeOS is implemented, but this requires\n> > > > changes on the IPA interface and the DelayedControls class.\n> > > > 3. Current approach which finds the results backwards.\n> > >\n> > > As far as I'm aware, 2 is what we also already support. Every operation\n> > > should have an id/frame number/sequence number which matches the request\n> > > sequence number.\n> > >\n> > > Using your implementation, you should be able to verify (or vilify) the\n> > > matching of the results you determine against the ones that were\n> > > generated for the same index/sequence number.\n> > >\n> > > Re-reading your text though, am I right to infer that DelayedControls is\n> > > throwing this option out?\n> > Yes, it may introduce extra delay which makes frame number a little\n> > off, but we need that since the sensor controls may delay.\n> > For the ID in 2, I mean an extra ID field for Aiq (not the frame\n> > number), and let pipeline handlers return the ID back, since\n> > the pipeline handler could figure out the exact ID, by hacking into\n> > DelayedControls. ;-/\n>\n> This sounds like the same thing that Jean-Michel is looking at too\n> (which is probably evident as I think he used a couple of your patches).\n>\n> Discussing with him recently, we realised that sending back the\n> effective sensor settings from DelayedControls along with the statistics\n> buffer ensured that the IPA had the information it needed.\n>\n> Is there any other information/context that you need to ensure is kept\n> from when the parameter buffers are filled in before capturing an image?\n\nThough I cannot give a full list for now.\nI suppose we will need more Controls exchange through implementing\nper-frame controls,\nand to extend Control definitions to support Android state machine transition.\nAlso an event to pass eeprom content might be needed, which, in my\nunderstanding, contains lens correction per device.\n>\n> --\n> Kieran\n>\n>\n> > >\n> > >\n> > >\n> > > >\n> > > > >\n> > > > >\n> > > > > > + aiq::AiqResults combinedResults = results_;\n> > > > > > + combinedResults.setAe(aeMatchedResults.ae());\n> > > > >\n> > > > > Oh - that's interesting, so we take the latest calculated results, and\n> > > > > the ae() from the matched results..\n> > > > >\n> > > > > > +\n> > > > > > + /* Aiq library expects timestamp in microseconds */\n> > > > >\n> > > > > Perhaps std::chrono would help us here in the future. Doesn't have to be\n> > > > > now.\n> > > > >\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(exposure_));\n> > > > > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> > > > > > - op.controls = ctrls;\n> > > > > > + ControlList sensorCtrls(ctrls_);\n> > > > > > + sensorCtrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_));\n> > > > > > + sensorCtrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_));\n> > > > > > +\n> > > > > > + op.sensorControls = sensorCtrls;\n> > > > > >\n> > > > > > queueFrameAction.emit(frame, op);\n> > > > > > }\n> > > > > > --\n> > > > > > 2.33.1.1089.g2158813163f-goog\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 51382BDB1C\n\tfor ;\n\tTue, 16 Nov 2021 10:07:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 032726038A;\n\tTue, 16 Nov 2021 11:07:20 +0100 (CET)","from mail-ot1-x329.google.com (mail-ot1-x329.google.com\n\t[IPv6:2607:f8b0:4864:20::329])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C744660231\n\tfor ;\n\tTue, 16 Nov 2021 11:07:18 +0100 (CET)","by mail-ot1-x329.google.com with SMTP id\n\tx43-20020a056830246b00b00570d09d34ebso19061769otr.2\n\tfor ;\n\tTue, 16 Nov 2021 02:07:18 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"hHqlPcYb\"; 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=j7edztOpIZO0eL0QK8+CUt3WhlnFaO+Y6jGEGJFAMy8=;\n\tb=hHqlPcYbswCFHyViH1IDOEqKxFNoWp1Nwf93+SJnvwPQq4vx57+gPdxICZk8+Dr1l8\n\tw1t3SDwODfAlnY2rgdttKspZIHOmRxqA6umM8O/DB8kajOsczPkL30oC0tGAdcJ4/E2g\n\tJULmAzh/gxJbH5DYEdfOIRQuXjkthRoOUPRXc=","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=j7edztOpIZO0eL0QK8+CUt3WhlnFaO+Y6jGEGJFAMy8=;\n\tb=AjfUOcOZWhyQDpwQcfRGAEEBBuYWTm5vQm38xMgz+6KMt4e82mcnXcCkGWF+81Gf+G\n\tpeD/DlhkQcSphcOb/Td4QPEEBaCUHAPix1lAjjLuf9rTZm4mp+SFQnT5/d2/Kv13IM/J\n\tdnWZH6FCybXAO6C2V6FG/W9LNqU51kIc7BFtdUV7Oo37i7n12zPBoUVbnUueF2bQ1qNv\n\txIfS6z1EAzwtRO4pZa+BFTTFt88dem1o/z1ksYcd4RgllowEhdOFyeR97/8EaWlYQYJ7\n\tP3VW9Xq8tqXYvxoUsPH+eyfS3qy53tOcdt8Hozj/HUvGkrsT6bPHiJzFl0U4bLZXFfsE\n\tuuUg==","X-Gm-Message-State":"AOAM533Ikf6R0zzJ1RFtgzDn/bXT3rI+AFPhXyl7YsKITf+bggGbhoaH\n\tkb6a2GSY9/puL0sFI4SJu4W+YKv5D3mGPTy9oN+9wvNTbzWqLQ==","X-Google-Smtp-Source":"ABdhPJxCOUMNe5KyKTdsyGNwm2e2r7DKSSVHHAOWCZW63iqcPy1I/k5QA3wQU245cHQg/XSeniWof/aVKvLP921qNmc=","X-Received":"by 2002:a9d:62ce:: with SMTP id z14mr4971615otk.56.1637057237400;\n\tTue, 16 Nov 2021 02:07:17 -0800 (PST)","MIME-Version":"1.0","References":"<20211029120001.2469018-1-hanlinchen@chromium.org>\n\t<20211029120001.2469018-3-hanlinchen@chromium.org>\n\t<163586149638.275423.17276391440742370199@Monstersaurus>\n\t\n\t<163655209209.1946932.9341037718959694294@Monstersaurus>\n\t\n\t<163655368897.1946932.11331992017061456361@Monstersaurus>","In-Reply-To":"<163655368897.1946932.11331992017061456361@Monstersaurus>","From":"Hanlin Chen ","Date":"Tue, 16 Nov 2021 18:07:06 +0800","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]