[{"id":21361,"web_url":"https://patchwork.libcamera.org/comment/21361/","msgid":"<040ec3cc-06c3-2ee6-262b-4370bf1f000c@ideasonboard.com>","date":"2021-11-29T17:13:15","subject":"Re: [libcamera-devel] [PATCH] ipu3: ipa: Report correct exposure in\n\trequest metadata","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Umang,\n\nThanks for the patch !\n\nOn 29/11/2021 18:10, Umang Jain wrote:\n> While populating the ControlList for the request's metadata,\n> the exposure value should be used computed from AGC algorithm\n> instead of sensor's exposure.\n> \n> The issue is caught while debugging a FULL-level CTS test.\n> \n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n\nSounds like a bug indeed :-).\nReviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n\n> ---\n>   src/ipa/ipu3/ipu3.cpp | 2 +-\n>   1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index a8d54a5d..9cd80a02 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -632,7 +632,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>   \n>   \tctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK);\n>   \n> -\tctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration_.get<std::micro>());\n> +\tctrls.set(controls::ExposureTime, context_.frameContext.agc.exposure * lineDuration_.get<std::micro>());\n>   \n>   \t/*\n>   \t * \\todo The Metadata provides a path to getting extended data\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 F28C1BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 29 Nov 2021 17:13:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5AD76605A4;\n\tMon, 29 Nov 2021 18:13:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C797060592\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 29 Nov 2021 18:13:17 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:4f15:3be7:f42c:9dba] (unknown\n\t[IPv6:2a01:e0a:169:7140:4f15:3be7:f42c:9dba])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 732EB2A5;\n\tMon, 29 Nov 2021 18:13:17 +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=\"NW1g8ciX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638205997;\n\tbh=f/KAN3BeCa6a2Fo7DTtp74IhSIj7XOLqu6I/T8epZTY=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=NW1g8ciXvHLdARrB7vtE+7bZWPzIqePSmlrlm9Ff4rSPLbXLTkv5snzAKkxLwDlDV\n\tZnxFYo4JSK0pXh3DusYhFGMPSbiwp3RyMDDIW7kGeFMqzaaOQXU+PC/AQ2AduPPooQ\n\tdwwq62ljEnnozABoKf1eMZXYrtP4RA5QL01Ebk40=","Message-ID":"<040ec3cc-06c3-2ee6-262b-4370bf1f000c@ideasonboard.com>","Date":"Mon, 29 Nov 2021 18:13:15 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.3.1","Content-Language":"en-US","To":"Umang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211129171032.469605-1-umang.jain@ideasonboard.com>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","In-Reply-To":"<20211129171032.469605-1-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] ipu3: ipa: Report correct exposure in\n\trequest metadata","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21362,"web_url":"https://patchwork.libcamera.org/comment/21362/","msgid":"<b0aafeaa-a7ad-cc9b-2487-40218ff7df18@ideasonboard.com>","date":"2021-11-29T17:14:57","subject":"Re: [libcamera-devel] [PATCH] ipu3: ipa: Report correct exposure in\n\trequest metadata","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi JM\n\nOn 11/29/21 10:43 PM, Jean-Michel Hautbois wrote:\n> Hi Umang,\n>\n> Thanks for the patch !\n\n\nThanks for fast review,  I was about to ping you on IRC\n\n>\n> On 29/11/2021 18:10, Umang Jain wrote:\n>> While populating the ControlList for the request's metadata,\n>> the exposure value should be used computed from AGC algorithm\n>> instead of sensor's exposure.\n>>\n>> The issue is caught while debugging a FULL-level CTS test.\n>>\n>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n>\n> Sounds like a bug indeed :-).\n\n\nSpiraled my CTS work by 12ish hours :S\n\n> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n>\n>> ---\n>>   src/ipa/ipu3/ipu3.cpp | 2 +-\n>>   1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index a8d54a5d..9cd80a02 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -632,7 +632,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>>         ctrls.set(controls::ColourTemperature, \n>> context_.frameContext.awb.temperatureK);\n>>   -    ctrls.set(controls::ExposureTime, \n>> context_.frameContext.sensor.exposure * \n>> lineDuration_.get<std::micro>());\n>> +    ctrls.set(controls::ExposureTime, \n>> context_.frameContext.agc.exposure * lineDuration_.get<std::micro>());\n>>         /*\n>>        * \\todo The Metadata provides a path to getting extended data\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 F12C5BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 29 Nov 2021 17:15:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 52140605A4;\n\tMon, 29 Nov 2021 18:15:04 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2024060592\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 29 Nov 2021 18:15:03 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.170])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DE54E2A5;\n\tMon, 29 Nov 2021 18:15:01 +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=\"trBld6No\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638206102;\n\tbh=rzh8bGx6kNHPSqJomj1HLbSCkX/0cKQgimNtoCvScH0=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=trBld6NoqM/1D0aE18yVys/49y+BVofPSldhaPenP8VpQPeJhKMDHtBRP+M7FTbva\n\ts0L2aIydzLh98bZTsIMSDs6SIMkSn8YH8u1KwfCSdoy42AWd80E5ov6S5/B2kLPvJa\n\tgCR5sXtIYRPMlAuLgORQFf/RhmbsAPQIpunTarEs=","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211129171032.469605-1-umang.jain@ideasonboard.com>\n\t<040ec3cc-06c3-2ee6-262b-4370bf1f000c@ideasonboard.com>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<b0aafeaa-a7ad-cc9b-2487-40218ff7df18@ideasonboard.com>","Date":"Mon, 29 Nov 2021 22:44:57 +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":"<040ec3cc-06c3-2ee6-262b-4370bf1f000c@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] ipu3: ipa: Report correct exposure in\n\trequest metadata","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21368,"web_url":"https://patchwork.libcamera.org/comment/21368/","msgid":"<163820691398.3059017.4921315739997984715@Monstersaurus>","date":"2021-11-29T17:28:33","subject":"Re: [libcamera-devel] [PATCH] ipu3: ipa: Report correct exposure in\n\trequest metadata","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2021-11-29 17:14:57)\n> Hi JM\n> \n> On 11/29/21 10:43 PM, Jean-Michel Hautbois wrote:\n> > Hi Umang,\n> >\n> > Thanks for the patch !\n> \n> \n> Thanks for fast review,  I was about to ping you on IRC\n> \n> >\n> > On 29/11/2021 18:10, Umang Jain wrote:\n> >> While populating the ControlList for the request's metadata,\n> >> the exposure value should be used computed from AGC algorithm\n> >> instead of sensor's exposure.\n> >>\n> >> The issue is caught while debugging a FULL-level CTS test.\n> >>\n> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> >\n> > Sounds like a bug indeed :-).\n> \n> \n> Spiraled my CTS work by 12ish hours :S\n\nOuch, and I hate to not reply with the easy answer :-( but... ....\n\n\n\n> \n> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> >\n> >> ---\n> >>   src/ipa/ipu3/ipu3.cpp | 2 +-\n> >>   1 file changed, 1 insertion(+), 1 deletion(-)\n> >>\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index a8d54a5d..9cd80a02 100644\n> >> --- a/src/ipa/ipu3/ipu3.cpp\n> >> +++ b/src/ipa/ipu3/ipu3.cpp\n> >> @@ -632,7 +632,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n\nthe steps of the IPA are:\n\n queue requests\n    processControls ( incoming frame settings #1 )\n    fillParams ( Configure hardware for this frame #1 )\n    parseStatistics ( handle all results of finished frame #1,\n\t              prepare for next one based on completed frame #2+ ).\n\nMy worry - is that the metadata that should be returned when\nparseStatistics completes, is the metadata associated with frame #1.\n\nI.e. it's the frame that was requested first, and has just completed.\n\nThe exposure set on that frame, is the exposure value that was passed in\nfrom DelayedControls during parseStatistics.\n\nWe /could/ also keep all those values in a circular buffer/queue so we\nremember what was asked to be set in #1, when we come round to parsing\nthe statistics and filling in the metadata...\n\n\n> >>         ctrls.set(controls::ColourTemperature, \n> >> context_.frameContext.awb.temperatureK);\n> >>   -    ctrls.set(controls::ExposureTime, \n> >> context_.frameContext.sensor.exposure * \n> >> lineDuration_.get<std::micro>());\n> >> +    ctrls.set(controls::ExposureTime, \n> >> context_.frameContext.agc.exposure * lineDuration_.get<std::micro>());\n\nHowever the change here, is now setting the exposure that we want to\ncalculate, and we'll ask for in an upcoming frame. It's the output of\nthe most recent algorithm calculation, and therefore is not the exposure\nvalue for the frame which has just completed, and therefore it's not the\nright value to be putting in the metadata for the completed request...\n\nCan you provide some more context of what led you to this fix please?\n\n--\nKieran\n\n\n> >>         /*\n> >>        * \\todo The Metadata provides a path to getting extended data\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 21FBABDB13\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 29 Nov 2021 17:28:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 61260605A4;\n\tMon, 29 Nov 2021 18:28:39 +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 2631D60592\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 29 Nov 2021 18:28:37 +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 A76B62A5;\n\tMon, 29 Nov 2021 18:28:36 +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=\"lB3iD4ya\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638206916;\n\tbh=s72vjDIxfuk62s4EvFxTtGQMscs5Kpviz2prHN3qWnU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=lB3iD4yauwzRdz3uD1bMkY49NSpWTr7F36CjOjfwXD+4P/3WWS8VVysco7e5QHm+r\n\teycvRub6JwRivMjkAUHfjXIhLEIJ+sjkeVVmDazkikHR2luZCOT1S94bEwvPv8ndvo\n\t35FQVujlZSXHUXG5upRmgVmuzSzjhZRWvJKfkC+4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<b0aafeaa-a7ad-cc9b-2487-40218ff7df18@ideasonboard.com>","References":"<20211129171032.469605-1-umang.jain@ideasonboard.com>\n\t<040ec3cc-06c3-2ee6-262b-4370bf1f000c@ideasonboard.com>\n\t<b0aafeaa-a7ad-cc9b-2487-40218ff7df18@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tUmang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 29 Nov 2021 17:28:33 +0000","Message-ID":"<163820691398.3059017.4921315739997984715@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] ipu3: ipa: Report correct exposure in\n\trequest metadata","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21374,"web_url":"https://patchwork.libcamera.org/comment/21374/","msgid":"<da05983b-f647-3918-2735-a30c83011f92@ideasonboard.com>","date":"2021-11-29T18:15:52","subject":"Re: [libcamera-devel] [PATCH] ipu3: ipa: Report correct exposure in\n\trequest metadata","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran\n\nOn 11/29/21 10:58 PM, Kieran Bingham wrote:\n> Quoting Umang Jain (2021-11-29 17:14:57)\n>> Hi JM\n>>\n>> On 11/29/21 10:43 PM, Jean-Michel Hautbois wrote:\n>>> Hi Umang,\n>>>\n>>> Thanks for the patch !\n>>\n>> Thanks for fast review,  I was about to ping you on IRC\n>>\n>>> On 29/11/2021 18:10, Umang Jain wrote:\n>>>> While populating the ControlList for the request's metadata,\n>>>> the exposure value should be used computed from AGC algorithm\n>>>> instead of sensor's exposure.\n>>>>\n>>>> The issue is caught while debugging a FULL-level CTS test.\n>>>>\n>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n>>> Sounds like a bug indeed :-).\n>>\n>> Spiraled my CTS work by 12ish hours :S\n> Ouch, and I hate to not reply with the easy answer :-( but... ....\n>\n>\n>\n>>> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n>>>\n>>>> ---\n>>>>    src/ipa/ipu3/ipu3.cpp | 2 +-\n>>>>    1 file changed, 1 insertion(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>> index a8d54a5d..9cd80a02 100644\n>>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>>> @@ -632,7 +632,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> the steps of the IPA are:\n>\n>   queue requests\n>      processControls ( incoming frame settings #1 )\n>      fillParams ( Configure hardware for this frame #1 )\n>      parseStatistics ( handle all results of finished frame #1,\n> \t              prepare for next one based on completed frame #2+ ).\n>\n> My worry - is that the metadata that should be returned when\n> parseStatistics completes, is the metadata associated with frame #1.\n>\n> I.e. it's the frame that was requested first, and has just completed.\n>\n> The exposure set on that frame, is the exposure value that was passed in\n> from DelayedControls during parseStatistics.\n>\n> We /could/ also keep all those values in a circular buffer/queue so we\n> remember what was asked to be set in #1, when we come round to parsing\n> the statistics and filling in the metadata...\n>\n>\n>>>>          ctrls.set(controls::ColourTemperature,\n>>>> context_.frameContext.awb.temperatureK);\n>>>>    -    ctrls.set(controls::ExposureTime,\n>>>> context_.frameContext.sensor.exposure *\n>>>> lineDuration_.get<std::micro>());\n>>>> +    ctrls.set(controls::ExposureTime,\n>>>> context_.frameContext.agc.exposure * lineDuration_.get<std::micro>());\n> However the change here, is now setting the exposure that we want to\n> calculate, and we'll ask for in an upcoming frame. It's the output of\n> the most recent algorithm calculation, and therefore is not the exposure\n> value for the frame which has just completed, and therefore it's not the\n> right value to be putting in the metadata for the completed request...\n>\n> Can you provide some more context of what led you to this fix please?\n\n\nThe context is, a control - ExposureTimeMode (not present on master), if \nset to \"disabled\" - should immediately stop changing exposure. This is \ntested as a CTS test.\n\nwhat happens is :\n\n- Few requests(~1-4th for example) are queued initially with \"auto\" mode \n- exposure can change\n- Subsequent requests (4th - end_of_test) requests are queued with \n\"disabled\" mode - exposure shouldn't change\n- I can control the Algorithms (AGC in this case) to stop setting \nIPAFramecontext.agc.exposure because we are in \"disabled\" mode; as early \nas 4th request\n- Now the last exposure computed will be / should be used to set \nexposure on the sensor for future frames\n- Since delayed controls is involved, I only see sensor exposure getting \nfixed (locked?) to a certain value only /after/ some _more) frames are \npassed i.e. the lag between exposure gets \"fixed\"\n\nSo it fails the test. There are more failures on the test, but those are \nseparate blocks of issues.\n\nSomehow we need to fix the exposure(in real or virtually**) /as soon as/ \nwe get into ExposureTimeMode'Disabled' mode.\n\nI agree with your rationale above, but looks like controlling the \nalgorithms is not enough to get it pass. The delayedControls lag of \nstablising the exposure needs to be taken care of either by introducing \na ring-buffer of frames or using the algorithms value (since it can be \ncontrolled) for now, as part of metadata.\n\n\n>\n> --\n> Kieran\n>\n>\n>>>>          /*\n>>>>         * \\todo The Metadata provides a path to getting extended data\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 1BBFDBF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 29 Nov 2021 18:16:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 549D9605A4;\n\tMon, 29 Nov 2021 19:15:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 124E960592\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 29 Nov 2021 19:15:58 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.170])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CBDB32A5;\n\tMon, 29 Nov 2021 19:15:56 +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=\"cvoIU04t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638209757;\n\tbh=HGPzXW1e84PrQE6rWQAbZScr76ws1tWj8KYq4QFJlJA=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=cvoIU04tBcaY9jMNFj3tLvJf6bpmnvKwUfo9FdzoNdeskoOEfuybbd1n9qezO48lw\n\tPb6XOGelIUH+SpN7sv1NymJi8B9F1+AFy7iBazDz2zTbEWAqoh+i+dG3VDlv8J+4rd\n\tQwB+2+FmegofESCcUeGRTOUrgKGwEb440aou7JNE=","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tJean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211129171032.469605-1-umang.jain@ideasonboard.com>\n\t<040ec3cc-06c3-2ee6-262b-4370bf1f000c@ideasonboard.com>\n\t<b0aafeaa-a7ad-cc9b-2487-40218ff7df18@ideasonboard.com>\n\t<163820691398.3059017.4921315739997984715@Monstersaurus>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<da05983b-f647-3918-2735-a30c83011f92@ideasonboard.com>","Date":"Mon, 29 Nov 2021 23:45:52 +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":"<163820691398.3059017.4921315739997984715@Monstersaurus>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] ipu3: ipa: Report correct exposure in\n\trequest metadata","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21379,"web_url":"https://patchwork.libcamera.org/comment/21379/","msgid":"<163822869006.3059017.2920198212773043617@Monstersaurus>","date":"2021-11-29T23:31:30","subject":"Re: [libcamera-devel] [PATCH] ipu3: ipa: Report correct exposure in\n\trequest metadata","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2021-11-29 18:15:52)\n> Hi Kieran\n> \n> On 11/29/21 10:58 PM, Kieran Bingham wrote:\n> > Quoting Umang Jain (2021-11-29 17:14:57)\n> >> Hi JM\n> >>\n> >> On 11/29/21 10:43 PM, Jean-Michel Hautbois wrote:\n> >>> Hi Umang,\n> >>>\n> >>> Thanks for the patch !\n> >>\n> >> Thanks for fast review,  I was about to ping you on IRC\n> >>\n> >>> On 29/11/2021 18:10, Umang Jain wrote:\n> >>>> While populating the ControlList for the request's metadata,\n> >>>> the exposure value should be used computed from AGC algorithm\n> >>>> instead of sensor's exposure.\n> >>>>\n> >>>> The issue is caught while debugging a FULL-level CTS test.\n> >>>>\n> >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> >>> Sounds like a bug indeed :-).\n> >>\n> >> Spiraled my CTS work by 12ish hours :S\n> > Ouch, and I hate to not reply with the easy answer :-( but... ....\n> >\n> >\n> >\n> >>> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n> >>>\n> >>>> ---\n> >>>>    src/ipa/ipu3/ipu3.cpp | 2 +-\n> >>>>    1 file changed, 1 insertion(+), 1 deletion(-)\n> >>>>\n> >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>>> index a8d54a5d..9cd80a02 100644\n> >>>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>>> @@ -632,7 +632,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> > the steps of the IPA are:\n> >\n> >   queue requests\n> >      processControls ( incoming frame settings #1 )\n> >      fillParams ( Configure hardware for this frame #1 )\n> >      parseStatistics ( handle all results of finished frame #1,\n> >                     prepare for next one based on completed frame #2+ ).\n> >\n> > My worry - is that the metadata that should be returned when\n> > parseStatistics completes, is the metadata associated with frame #1.\n> >\n> > I.e. it's the frame that was requested first, and has just completed.\n> >\n> > The exposure set on that frame, is the exposure value that was passed in\n> > from DelayedControls during parseStatistics.\n> >\n> > We /could/ also keep all those values in a circular buffer/queue so we\n> > remember what was asked to be set in #1, when we come round to parsing\n> > the statistics and filling in the metadata...\n> >\n> >\n> >>>>          ctrls.set(controls::ColourTemperature,\n> >>>> context_.frameContext.awb.temperatureK);\n> >>>>    -    ctrls.set(controls::ExposureTime,\n> >>>> context_.frameContext.sensor.exposure *\n> >>>> lineDuration_.get<std::micro>());\n> >>>> +    ctrls.set(controls::ExposureTime,\n> >>>> context_.frameContext.agc.exposure * lineDuration_.get<std::micro>());\n> > However the change here, is now setting the exposure that we want to\n> > calculate, and we'll ask for in an upcoming frame. It's the output of\n> > the most recent algorithm calculation, and therefore is not the exposure\n> > value for the frame which has just completed, and therefore it's not the\n> > right value to be putting in the metadata for the completed request...\n> >\n> > Can you provide some more context of what led you to this fix please?\n> \n> \n> The context is, a control - ExposureTimeMode (not present on master), if \n> set to \"disabled\" - should immediately stop changing exposure. This is \n> tested as a CTS test.\n> \n> what happens is :\n> \n> - Few requests(~1-4th for example) are queued initially with \"auto\" mode \n> - exposure can change\n> - Subsequent requests (4th - end_of_test) requests are queued with \n> \"disabled\" mode - exposure shouldn't change\n\nOk, this should be handled during processControls().\n\nWhen the mode is set to disabled, we should be locking the exposure and\ngain values, and setting only those, (while still allowing the\nalgorithms to run anyway...).\n\n\n\n> - I can control the Algorithms (AGC in this case) to stop setting \n> IPAFramecontext.agc.exposure because we are in \"disabled\" mode; as early \n> as 4th request\n\nWe should be able to lock them immediately. (Of course what we lock them\nto should be something that makes sense and is useful?)\n\n\n> - Now the last exposure computed will be / should be used to set \n> exposure on the sensor for future frames\n> - Since delayed controls is involved, I only see sensor exposure getting \n> fixed (locked?) to a certain value only /after/ some _more) frames are \n> passed i.e. the lag between exposure gets \"fixed\"\n\nWhen processControls() tells us to lock/disable/whichever - we should be\nmaking sure at that point we store the current state, and set a flag to\nensure that we only refer to the stored state for controls. That can\nmean we never set any more control through delayed controls, and the\nstate will stay where it was.\n\nThis sounds like something that was put on my plate to implement anyway,\nas I muttered something about it being 'easy', but haven't yet got round\nto it.\n\n> So it fails the test. There are more failures on the test, but those are \n> separate blocks of issues.\n> \n> Somehow we need to fix the exposure(in real or virtually**) /as soon as/ \n> we get into ExposureTimeMode'Disabled' mode.\n\nIt's a matter of handling the control when it is set. We simply don't do\nit yet. And I dont' think it's much to implement.\n\nI'll take a look at this tomorrow, and go through it with you if you\nwant.\n\nUnless you can see what needs to be done from the above text and get\nthere before I come online in the morning my time ;-)\n\n\n - Upon receipt of a control disabling the AGC algorithm, we maintain a\n   flag to state if we're enabled or disabled (or locked? I haven't\n   checked the terminology or details yet).\n\n - If we have locked AE/AGC ... we prevent setting any delayed controls\n   through setControls(). That may be either setting only values that we\n   have cached, or it may mean ... simply not calling to reset it.\n\n - Manual settings of the Exposure/Gain then become values that also\n   override through setControls() so that only the manual values can be\n   used if provided. (We will likely track this with something like\n   std::optional, so we know if we have a value, we use it).\n\n> I agree with your rationale above, but looks like controlling the \n> algorithms is not enough to get it pass. The delayedControls lag of \n> stablising the exposure needs to be taken care of either by introducing \n> a ring-buffer of frames or using the algorithms value (since it can be \n> controlled) for now, as part of metadata.\n\nAnyway, those are my notes/thoughts on what I think we need to try to\nimplement to resolve this. We shouldn't set the metadata from the\n'wrong' frame. And I don't think we even need a ring buffer to solve\nthis. It's about handling the request control at the processControls().\n--\nKieran\n\n\n> >\n> > --\n> > Kieran\n> >\n> >\n> >>>>          /*\n> >>>>         * \\todo The Metadata provides a path to getting extended data\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 AE663BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 29 Nov 2021 23:31:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C9CA3605A4;\n\tTue, 30 Nov 2021 00:31:34 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 01622604FC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 30 Nov 2021 00:31:32 +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 843548F0;\n\tTue, 30 Nov 2021 00:31:32 +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=\"UPcQ8+JM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638228692;\n\tbh=DtIOHDa7BMsDkRbtxbdSSe7l65dJRqvXkf+bVOtWOHg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=UPcQ8+JM0I1e+ZSeaVRN8T+m1DDm6OVZuIz1lKPk4PJyFVnwz63r2CCs5IudaiXaV\n\tJE/wB0W/uZrMIhFlVp+kd4cW3fOlNbrsS5HjVyKgo6BGOPKhR5c4uGjnWgEC6BdtFD\n\tNKoHQFny4KubByKP/emEE31bWcUYbx0/dRLxaz/c=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<da05983b-f647-3918-2735-a30c83011f92@ideasonboard.com>","References":"<20211129171032.469605-1-umang.jain@ideasonboard.com>\n\t<040ec3cc-06c3-2ee6-262b-4370bf1f000c@ideasonboard.com>\n\t<b0aafeaa-a7ad-cc9b-2487-40218ff7df18@ideasonboard.com>\n\t<163820691398.3059017.4921315739997984715@Monstersaurus>\n\t<da05983b-f647-3918-2735-a30c83011f92@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tUmang Jain <umang.jain@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 29 Nov 2021 23:31:30 +0000","Message-ID":"<163822869006.3059017.2920198212773043617@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] ipu3: ipa: Report correct exposure in\n\trequest metadata","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21405,"web_url":"https://patchwork.libcamera.org/comment/21405/","msgid":"<53755da1-6cda-0090-2967-f5ff95b4453b@ideasonboard.com>","date":"2021-11-30T04:56:22","subject":"Re: [libcamera-devel] [PATCH] ipu3: ipa: Report correct exposure in\n\trequest metadata","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 11/30/21 5:01 AM, Kieran Bingham wrote:\n> Quoting Umang Jain (2021-11-29 18:15:52)\n>> Hi Kieran\n>>\n>> On 11/29/21 10:58 PM, Kieran Bingham wrote:\n>>> Quoting Umang Jain (2021-11-29 17:14:57)\n>>>> Hi JM\n>>>>\n>>>> On 11/29/21 10:43 PM, Jean-Michel Hautbois wrote:\n>>>>> Hi Umang,\n>>>>>\n>>>>> Thanks for the patch !\n>>>> Thanks for fast review,  I was about to ping you on IRC\n>>>>\n>>>>> On 29/11/2021 18:10, Umang Jain wrote:\n>>>>>> While populating the ControlList for the request's metadata,\n>>>>>> the exposure value should be used computed from AGC algorithm\n>>>>>> instead of sensor's exposure.\n>>>>>>\n>>>>>> The issue is caught while debugging a FULL-level CTS test.\n>>>>>>\n>>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n>>>>> Sounds like a bug indeed :-).\n>>>> Spiraled my CTS work by 12ish hours :S\n>>> Ouch, and I hate to not reply with the easy answer :-( but... ....\n>>>\n>>>\n>>>\n>>>>> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>\n>>>>>\n>>>>>> ---\n>>>>>>     src/ipa/ipu3/ipu3.cpp | 2 +-\n>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)\n>>>>>>\n>>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>>>> index a8d54a5d..9cd80a02 100644\n>>>>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>>>>> @@ -632,7 +632,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n>>> the steps of the IPA are:\n>>>\n>>>    queue requests\n>>>       processControls ( incoming frame settings #1 )\n>>>       fillParams ( Configure hardware for this frame #1 )\n>>>       parseStatistics ( handle all results of finished frame #1,\n>>>                      prepare for next one based on completed frame #2+ ).\n>>>\n>>> My worry - is that the metadata that should be returned when\n>>> parseStatistics completes, is the metadata associated with frame #1.\n>>>\n>>> I.e. it's the frame that was requested first, and has just completed.\n>>>\n>>> The exposure set on that frame, is the exposure value that was passed in\n>>> from DelayedControls during parseStatistics.\n>>>\n>>> We /could/ also keep all those values in a circular buffer/queue so we\n>>> remember what was asked to be set in #1, when we come round to parsing\n>>> the statistics and filling in the metadata...\n>>>\n>>>\n>>>>>>           ctrls.set(controls::ColourTemperature,\n>>>>>> context_.frameContext.awb.temperatureK);\n>>>>>>     -    ctrls.set(controls::ExposureTime,\n>>>>>> context_.frameContext.sensor.exposure *\n>>>>>> lineDuration_.get<std::micro>());\n>>>>>> +    ctrls.set(controls::ExposureTime,\n>>>>>> context_.frameContext.agc.exposure * lineDuration_.get<std::micro>());\n>>> However the change here, is now setting the exposure that we want to\n>>> calculate, and we'll ask for in an upcoming frame. It's the output of\n>>> the most recent algorithm calculation, and therefore is not the exposure\n>>> value for the frame which has just completed, and therefore it's not the\n>>> right value to be putting in the metadata for the completed request...\n>>>\n>>> Can you provide some more context of what led you to this fix please?\n>>\n>> The context is, a control - ExposureTimeMode (not present on master), if\n>> set to \"disabled\" - should immediately stop changing exposure. This is\n>> tested as a CTS test.\n>>\n>> what happens is :\n>>\n>> - Few requests(~1-4th for example) are queued initially with \"auto\" mode\n>> - exposure can change\n>> - Subsequent requests (4th - end_of_test) requests are queued with\n>> \"disabled\" mode - exposure shouldn't change\n> Ok, this should be handled during processControls().\n>\n> When the mode is set to disabled, we should be locking the exposure and\n> gain values, and setting only those, (while still allowing the\n> algorithms to run anyway...).\n\n\nyes, this is already implemented on my side\n\n>\n>\n>\n>> - I can control the Algorithms (AGC in this case) to stop setting\n>> IPAFramecontext.agc.exposure because we are in \"disabled\" mode; as early\n>> as 4th request\n> We should be able to lock them immediately. (Of course what we lock them\n> to should be something that makes sense and is useful?)\n\n\nimplemented already\n\n>\n>\n>> - Now the last exposure computed will be / should be used to set\n>> exposure on the sensor for future frames\n>> - Since delayed controls is involved, I only see sensor exposure getting\n>> fixed (locked?) to a certain value only /after/ some _more) frames are\n>> passed i.e. the lag between exposure gets \"fixed\"\n> When processControls() tells us to lock/disable/whichever - we should be\n> making sure at that point we store the current state, and set a flag to\n> ensure that we only refer to the stored state for controls. That can\n\n\nalready implemented\n\n> mean we never set any more control through delayed controls, and the\n> state will stay where it was.\n\n\nOk, I think I need to place a check on delayed controls settings rather \nthan on metadata.\n\n>\n> This sounds like something that was put on my plate to implement anyway,\n> as I muttered something about it being 'easy', but haven't yet got round\n> to it.\n>\n>> So it fails the test. There are more failures on the test, but those are\n>> separate blocks of issues.\n>>\n>> Somehow we need to fix the exposure(in real or virtually**) /as soon as/\n>> we get into ExposureTimeMode'Disabled' mode.\n> It's a matter of handling the control when it is set. We simply don't do\n> it yet. And I dont' think it's much to implement.\n\n\nit already does, just not where I thought it should be. I might be \nmis-guided with IPAFrameContext::Sensor::Exposure with \nIPAFrameContext::Agc::Exposure and its non-sync use as denoted if you \napply same concept of the patch on gain a few lines above.\n\nI'll send a patch to properly report sensor gain as part of metadata \ninstead of algorithm's\n\n>\n> I'll take a look at this tomorrow, and go through it with you if you\n> want.\n>\n> Unless you can see what needs to be done from the above text and get\n> there before I come online in the morning my time ;-)\n>\n>\n>   - Upon receipt of a control disabling the AGC algorithm, we maintain a\n>     flag to state if we're enabled or disabled (or locked? I haven't\n>     checked the terminology or details yet).\n>\n>   - If we have locked AE/AGC ... we prevent setting any delayed controls\n>     through setControls(). That may be either setting only values that we\n>     have cached, or it may mean ... simply not calling to reset it.\n>\n>   - Manual settings of the Exposure/Gain then become values that also\n>     override through setControls() so that only the manual values can be\n>     used if provided. (We will likely track this with something like\n>     std::optional, so we know if we have a value, we use it).\n>\n>> I agree with your rationale above, but looks like controlling the\n>> algorithms is not enough to get it pass. The delayedControls lag of\n>> stablising the exposure needs to be taken care of either by introducing\n>> a ring-buffer of frames or using the algorithms value (since it can be\n>> controlled) for now, as part of metadata.\n> Anyway, those are my notes/thoughts on what I think we need to try to\n> implement to resolve this. We shouldn't set the metadata from the\n> 'wrong' frame. And I don't think we even need a ring buffer to solve\n> this. It's about handling the request control at the processControls().\n\n\nCool, my problem is resolved for now. Most of the my implementatoin \nalready compliant with what you noted down, so I think it's headed in \nthe right direction.\n\n> --\n> Kieran\n>\n>\n>>> --\n>>> Kieran\n>>>\n>>>\n>>>>>>           /*\n>>>>>>          * \\todo The Metadata provides a path to getting extended data\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 79502BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 30 Nov 2021 04:56:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 31725605B4;\n\tTue, 30 Nov 2021 05:56:28 +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 669C8604FC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 30 Nov 2021 05:56:27 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.170])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 42B768F0;\n\tTue, 30 Nov 2021 05:56:25 +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=\"bKjQ+oCf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638248187;\n\tbh=UrvhaMHHo1rZHnPu6xUfyjWxusCGL/wzFUS5Zlsjvz8=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=bKjQ+oCfbfdIrbla1M+8U0D4oZXSvmGBS9wFgknHKBN4ZOeYOMtRNEp9juWnSeC+q\n\tz7iF9mU2OlfDMpJEiIjZqDJ0Lk3yPymKILdywvWd5GYfSN1a27ABhiwQIk63FCz5QD\n\tJSTZAvAATNDQzfQj82FQkLMRs7qcaRE4xM70t4hA=","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tJean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211129171032.469605-1-umang.jain@ideasonboard.com>\n\t<040ec3cc-06c3-2ee6-262b-4370bf1f000c@ideasonboard.com>\n\t<b0aafeaa-a7ad-cc9b-2487-40218ff7df18@ideasonboard.com>\n\t<163820691398.3059017.4921315739997984715@Monstersaurus>\n\t<da05983b-f647-3918-2735-a30c83011f92@ideasonboard.com>\n\t<163822869006.3059017.2920198212773043617@Monstersaurus>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<53755da1-6cda-0090-2967-f5ff95b4453b@ideasonboard.com>","Date":"Tue, 30 Nov 2021 10:26:22 +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":"<163822869006.3059017.2920198212773043617@Monstersaurus>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] ipu3: ipa: Report correct exposure in\n\trequest metadata","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]