[{"id":33598,"web_url":"https://patchwork.libcamera.org/comment/33598/","msgid":"","date":"2025-03-17T11:40:19","subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"Thanks!\n\nTested-by: Robert Mader \n\nOn 17.03.25 12:26, Milan Zamazal wrote:\n> Automatic black level setting in software ISP updates the determined\n> black level value when exposure or gain change. It stores the last\n> exposure and gain values to detect the change.\n>\n> BlackLevel::configure() resets the stored black level value but not the\n> exposure and gain values. This can prevent updating the black value and\n> cause bad image output e.g. after suspending and resuming a camera, if\n> exposure and gain remain unchanged.\n>\n> Let's reset the stored exposure and gain values in\n> BlackLevel::configure() to fix the problem.\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259\n> Signed-off-by: Milan Zamazal \n> ---\n> src/ipa/simple/algorithms/blc.cpp | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> index 1d7d370b..14cf31bf 100644\n> --- a/src/ipa/simple/algorithms/blc.cpp\n> +++ b/src/ipa/simple/algorithms/blc.cpp\n> @@ -1,6 +1,6 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> - * Copyright (C) 2024, Red Hat Inc.\n> + * Copyright (C) 2024-2025, Red Hat Inc.\n> *\n> * Black level handling\n> */\n> @@ -38,6 +38,9 @@ int BlackLevel::init([[maybe_unused]] IPAContext &context,\n> int BlackLevel::configure(IPAContext &context,\n> \t\t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n> {\n> +\texposure_ = 0;\n> +\tgain_ = 0;\n> +\n> \tif (definedLevel_.has_value())\n> \t\tcontext.configuration.black.level = definedLevel_;\n> \tcontext.activeState.blc.level =","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 D39F1C32FA\n\tfor ;\n\tMon, 17 Mar 2025 11:40:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D3AA168826;\n\tMon, 17 Mar 2025 12:40:32 +0100 (CET)","from sender3-pp-f112.zoho.com (sender3-pp-f112.zoho.com\n\t[136.143.184.112])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 961A7617F8\n\tfor ;\n\tMon, 17 Mar 2025 12:40:30 +0100 (CET)","by mx.zohomail.com with SMTPS id 1742211622006443.0141348537197;\n\tMon, 17 Mar 2025 04:40:22 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=collabora.com\n\theader.i=robert.mader@collabora.com header.b=\"i9emPuwz\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1742211626; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=OHxnnSw3wFY21bFx/FAC3EgQ1/y//vpFboB9Xcpcko+dFXonIh9LeMFSJeSUV9E9c+3LCmlCvlTEPmckxBtTOIkcYknRw+Y/ukEEO7idNHTeXqQE1+FY2tJqVU5pZBG+/HtEXictyuY0RoBtWywbUH8qzttG8mEzq9ueuvPIpmM=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1742211626;\n\th=Content-Type:Content-Transfer-Encoding:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To:Cc;\n\tbh=CcDlJKNCySl63SP4JQxRr+g1pxtQxSRVTtun5v6Lsw8=; \n\tb=F1qi6+6LK717WzJR1nI3gzz8GNA3c5wC2ugw8vch+zt5GBcJamhF6sxzgYY55iVz4jTREwPGTAyieyWKbghTwIezQ8hhK3ONZjyXcmBU84OqTfjPaUQiIa0E+/nJbhoAjlDR2vaiwXMzLFQ8+0QyK87NofzFzjpwwM8KO6OShIQ=","ARC-Authentication-Results":"i=1; mx.zohomail.com;\n\tdkim=pass header.i=collabora.com;\n\tspf=pass smtp.mailfrom=robert.mader@collabora.com;\n\tdmarc=pass header.from=","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1742211626;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:References:From:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Message-Id:Reply-To:Cc;\n\tbh=CcDlJKNCySl63SP4JQxRr+g1pxtQxSRVTtun5v6Lsw8=;\n\tb=i9emPuwzjmQq7RVkvYtwC+wuRiE7aPMRgF9/ruWy1s7UaQU0Tc3Ii1amK+xta19F\n\tgOJn5suWlJZ9dVBznI4+gPEzpQFCSW2zPftG+cABFCT6b1TnJi+gk8Y/YGhnBk0Q8vE\n\tAz/m4j/UTIJ9E04l7dFlTgZQFUEO6p0fQI3FeCXE=","Message-ID":"","Date":"Mon, 17 Mar 2025 12:40:19 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","References":"<20250317112658.15084-1-mzamazal@redhat.com>","Content-Language":"en-US, de-DE, en-GB","From":"Robert Mader ","In-Reply-To":"<20250317112658.15084-1-mzamazal@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":33635,"web_url":"https://patchwork.libcamera.org/comment/33635/","msgid":"<174229797929.1827314.12167064880939749362@ping.linuxembedded.co.uk>","date":"2025-03-18T11:39:39","subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2025-03-17 11:26:58)\n> Automatic black level setting in software ISP updates the determined\n> black level value when exposure or gain change. It stores the last\n> exposure and gain values to detect the change.\n> \n> BlackLevel::configure() resets the stored black level value but not the\n> exposure and gain values. This can prevent updating the black value and\n> cause bad image output e.g. after suspending and resuming a camera, if\n> exposure and gain remain unchanged.\n> \n> Let's reset the stored exposure and gain values in\n> BlackLevel::configure() to fix the problem.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259\n> Signed-off-by: Milan Zamazal \n> ---\n> src/ipa/simple/algorithms/blc.cpp | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> index 1d7d370b..14cf31bf 100644\n> --- a/src/ipa/simple/algorithms/blc.cpp\n> +++ b/src/ipa/simple/algorithms/blc.cpp\n> @@ -1,6 +1,6 @@\n> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> /*\n> - * Copyright (C) 2024, Red Hat Inc.\n> + * Copyright (C) 2024-2025, Red Hat Inc.\n> *\n> * Black level handling\n> */\n> @@ -38,6 +38,9 @@ int BlackLevel::init([[maybe_unused]] IPAContext &context,\n> int BlackLevel::configure(IPAContext &context,\n> [[maybe_unused]] const IPAConfigInfo &configInfo)\n> {\n> + exposure_ = 0;\n> + gain_ = 0;\n> +\n\nI wonder if we should try to make sure algorithms do not store private\nstate, and only use storage in the IPAContext so we can ensure we can\nzero/reset the context on configure for all algos?\n\nFor now this seems reasonable to re-initialise.\n\nJust to check though - is this sufficient for stop/start cycles?\n\nIf someone does:\n\n camera->configure()\n camera->start()\n ....\n camera->stop()\n camera->start()\n ... Will there be the same bug here? Or will it be ok ?\n\nOr should these be re-initialised at start() ?\n\nPerhaps it's ok - as the issue is that the black level was reset here in\nconfigure, so the flag that was also checking when to recalibrate also\nneeds to be reset here.\n\nA comment to state that clearly might be helpful here somewhere that the\nexposure and gain are used to determine when to recalibrate the black\nlevels ...\n\n\n> if (definedLevel_.has_value())\n> context.configuration.black.level = definedLevel_;\n> context.activeState.blc.level =\n> -- \n> 2.48.1\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 6FB48C32F5\n\tfor ;\n\tTue, 18 Mar 2025 11:39:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9B4668942;\n\tTue, 18 Mar 2025 12:39:43 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0A2CB68940\n\tfor ;\n\tTue, 18 Mar 2025 12:39:42 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C18C6778;\n\tTue, 18 Mar 2025 12:37:59 +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=\"VmIWSSML\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742297879;\n\tbh=WMV+Jr+CNzzcQMkXPwegjZvkvhsFyqTkvlqlvMTtb2k=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=VmIWSSMLTnHXh3gbKhuOpAxxV2m/FaxHsT0a2Qz2vmsHtQMLiZV4V3g6ldlqwWPxA\n\tPucycfcspcA1g14u2swdEtMKFcYdZgqfSj5dHeHaZ+JMuc5Nf/2bQfwePzVuISSdKj\n\tIz3T5Vl2C2jeK+pr4SPg58hl7scnlvvYA0PjFfBo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250317112658.15084-1-mzamazal@redhat.com>","References":"<20250317112658.15084-1-mzamazal@redhat.com>","Subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","From":"Kieran Bingham ","Cc":"Milan Zamazal ,\n\tRobert Mader ","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Tue, 18 Mar 2025 11:39:39 +0000","Message-ID":"<174229797929.1827314.12167064880939749362@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":33636,"web_url":"https://patchwork.libcamera.org/comment/33636/","msgid":"<855xk64fsk.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-03-18T13:41:31","subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi,\n\nRobert Mader writes:\n\n> Thanks!\n>\n> Tested-by: Robert Mader \n\nThank you for testing.\n\nKieran Bingham writes:\n\n> Quoting Milan Zamazal (2025-03-17 11:26:58)\n>> Automatic black level setting in software ISP updates the determined\n>> black level value when exposure or gain change. It stores the last\n>\n>> exposure and gain values to detect the change.\n>> \n>> BlackLevel::configure() resets the stored black level value but not the\n>> exposure and gain values. This can prevent updating the black value and\n>> cause bad image output e.g. after suspending and resuming a camera, if\n>> exposure and gain remain unchanged.\n>> \n>> Let's reset the stored exposure and gain values in\n>> BlackLevel::configure() to fix the problem.\n>> \n>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259\n>> Signed-off-by: Milan Zamazal \n>> ---\n>> src/ipa/simple/algorithms/blc.cpp | 5 ++++-\n>> 1 file changed, 4 insertions(+), 1 deletion(-)\n>> \n>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n>> index 1d7d370b..14cf31bf 100644\n>> --- a/src/ipa/simple/algorithms/blc.cpp\n>> +++ b/src/ipa/simple/algorithms/blc.cpp\n>> @@ -1,6 +1,6 @@\n>> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> /*\n>> - * Copyright (C) 2024, Red Hat Inc.\n>> + * Copyright (C) 2024-2025, Red Hat Inc.\n>> *\n>> * Black level handling\n>> */\n>> @@ -38,6 +38,9 @@ int BlackLevel::init([[maybe_unused]] IPAContext &context,\n>> int BlackLevel::configure(IPAContext &context,\n>> [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> {\n>> + exposure_ = 0;\n>> + gain_ = 0;\n>> +\n>\n> I wonder if we should try to make sure algorithms do not store private\n> state, and only use storage in the IPAContext so we can ensure we can\n> zero/reset the context on configure for all algos?\n\nIf it is desirable to put these two variables to IPAContext although\nthey are basically private to BlackLevel class and are not supposed to\nbe used anywhere else, I can post v2 with such a change.\n\n> For now this seems reasonable to re-initialise.\n>\n> Just to check though - is this sufficient for stop/start cycles?\n>\n> If someone does:\n>\n> camera->configure()\n> camera->start()\n> ....\n> camera->stop()\n> camera->start()\n> ... Will there be the same bug here? Or will it be ok ?\n>\n> Or should these be re-initialised at start() ?\n\nAFAICS the active state gets reset only in configure, so it should be\nfine as it is.\n\n> Perhaps it's ok - as the issue is that the black level was reset here in\n> configure, so the flag that was also checking when to recalibrate also\n> needs to be reset here.\n\nYes.\n\n> A comment to state that clearly might be helpful here somewhere that the\n> exposure and gain are used to determine when to recalibrate the black\n> levels ...\n\nOK.\n\n>> if (definedLevel_.has_value())\n>> context.configuration.black.level = definedLevel_;\n>> context.activeState.blc.level =\n>> -- \n>> 2.48.1\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 D7D50C326C\n\tfor ;\n\tTue, 18 Mar 2025 13:41:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 37EFD68947;\n\tTue, 18 Mar 2025 14:41:40 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1D01268940\n\tfor ;\n\tTue, 18 Mar 2025 14:41:38 +0100 (CET)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-222-ePkHXxWDPyOPP7WzbRfVLA-1; Tue, 18 Mar 2025 09:41:35 -0400","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-3978ef9a284so1561179f8f.3\n\tfor ;\n\tTue, 18 Mar 2025 06:41:35 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb ([85.93.96.130])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-395c888117csm17917700f8f.44.2025.03.18.06.41.32\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 18 Mar 2025 06:41:32 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"iNs0vcxz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1742305297;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=2RMOiANP4QZdmM7Q4MQl2Q4Bg9u0I6GtOyJVhh7W31U=;\n\tb=iNs0vcxzWxckanS8317A42LLhpYdfYYTW51IPqVyNgV+bWW7+Ce6h63mQ3MCPCb+9vugpT\n\t9mS31LgRaEGqbmM8OgnigRM5dhGBCZyi6l6LjVph3N9REuCmK344w/8ZGBZ1O/ygOuBluu\n\t8a1kb1onUppG/3ZCILXaWhipRiEmO10=","X-MC-Unique":"ePkHXxWDPyOPP7WzbRfVLA-1","X-Mimecast-MFC-AGG-ID":"ePkHXxWDPyOPP7WzbRfVLA_1742305294","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1742305293; x=1742910093;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=2RMOiANP4QZdmM7Q4MQl2Q4Bg9u0I6GtOyJVhh7W31U=;\n\tb=NsI+UGujbgt+ggz0Py/n1e1OoNOwyaNPYFCdJY/NtIFZu35QyYhoPr4Y5Z0VRNYBzb\n\tMttsF3Mg+CRWHe/pLo+MXAZ1Tv/nd6/RGrODoLxjMSHXvW2S6yvGIdXKYbdB1cmFgbTS\n\taaJvJ1y3YPqM1UpMs+Fsh/a0Uhx9qR241OLBhaFn18lrhRtWho1cnbXN8bFKjbEZmCVy\n\t4fWHC6mGFvbwI267YnWzeltu424fnRXYMHkINhFx6oR4QZx0R1ZC0HGWGgxq/wO7zLON\n\tW+FF3SDFPk0oH/0e5yv/l2iLLhCpm7nlNN/dEr6pxjLvwEkb9HcQvpb2GJCSyL9zq8iU\n\tnmPw==","X-Gm-Message-State":"AOJu0YwIOWHh/WBzqLKnZFDrXRHlrCdWVvjauqoGs8pnrHroitw+hWHZ\n\tUCcAIcMZaV1Xp5mK8b/SA7VxGsncXBDwkezgdVv9FUyEP+cdpobo/g6g4uiaLIrjQZlXpOHy0sv\n\tfdlEQXi4hZdBYIyNpn2vtZDwyzyKsbJHoWfwKfOZsBzv5RM911bdmZuEzAqqiK5HB539laIRt7U\n\tg+kjiKSw==","X-Gm-Gg":"ASbGnctSpYyPr91TIfOEnZOo7W7kiAFPH1T80086bgoZFR0pCw50nxRvgmfoBFf/fVe\n\ts/9GzeE37mJdM8t8RLb8QRX8RLoHD4nVCRZXSUdE+rYlzmQgxxZ144mNEcJxZwkn6fuSbBLY+JK\n\tTqsmrf+ZVU7TVcvv56JA1n81JvLRtfyaV1QgHO4QFhKsUP7tGUcx03aXTC0YV6CszXYctVN7o66\n\tqLUhzhVblu/3lV0Q/hXHooSAHDrDrYv3JvNZXX5VtKXhXgpO/ZRIGmbVCMnvje5GHt2lbxUYJkS\n\tUYIdWS+tef09uel/ZzYCLTedQ7Sp/gxezGQ7V+O2","X-Received":["by 2002:a5d:6c6f:0:b0:38f:3b9b:6f91 with SMTP id\n\tffacd0b85a97d-3971d33349fmr16451450f8f.12.1742305293617; \n\tTue, 18 Mar 2025 06:41:33 -0700 (PDT)","by 2002:a5d:6c6f:0:b0:38f:3b9b:6f91 with SMTP id\n\tffacd0b85a97d-3971d33349fmr16451426f8f.12.1742305293166; \n\tTue, 18 Mar 2025 06:41:33 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFrwsoDHS1GoiXzkLRSBuEMwhlKtzIzz9xvSsBojG1BQXW9v1+LlRbOVrRac4f/r8AkAEZS6A==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, Robert Mader\n\t","Subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","In-Reply-To":" (Robert\n\tMader's message of \"Mon, 17 Mar 2025 12:40:19 +0100\")","References":"<20250317112658.15084-1-mzamazal@redhat.com>\n\t","Date":"Tue, 18 Mar 2025 14:41:31 +0100","Message-ID":"<855xk64fsk.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"ErCu97yipXD7ZmJX9USZvvkreuoPx1tcIva7sPLg2cQ_1742305294","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":33637,"web_url":"https://patchwork.libcamera.org/comment/33637/","msgid":"<6d3500fc-c742-4314-9168-6bc6d9cba0b4@collabora.com>","date":"2025-03-18T13:52:02","subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"FTR., I quickly tested whether \nhttps://patchwork.libcamera.org/patch/21703/ fixes the issue as well and \napparently it does.\n\nSo maybe we should just land that instead.\n\nOn 18.03.25 12:39, Kieran Bingham wrote:\n> Quoting Milan Zamazal (2025-03-17 11:26:58)\n>> Automatic black level setting in software ISP updates the determined\n>> black level value when exposure or gain change. It stores the last\n>> exposure and gain values to detect the change.\n>>\n>> BlackLevel::configure() resets the stored black level value but not the\n>> exposure and gain values. This can prevent updating the black value and\n>> cause bad image output e.g. after suspending and resuming a camera, if\n>> exposure and gain remain unchanged.\n>>\n>> Let's reset the stored exposure and gain values in\n>> BlackLevel::configure() to fix the problem.\n>>\n>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259\n>> Signed-off-by: Milan Zamazal \n>> ---\n>> src/ipa/simple/algorithms/blc.cpp | 5 ++++-\n>> 1 file changed, 4 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n>> index 1d7d370b..14cf31bf 100644\n>> --- a/src/ipa/simple/algorithms/blc.cpp\n>> +++ b/src/ipa/simple/algorithms/blc.cpp\n>> @@ -1,6 +1,6 @@\n>> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> /*\n>> - * Copyright (C) 2024, Red Hat Inc.\n>> + * Copyright (C) 2024-2025, Red Hat Inc.\n>> *\n>> * Black level handling\n>> */\n>> @@ -38,6 +38,9 @@ int BlackLevel::init([[maybe_unused]] IPAContext &context,\n>> int BlackLevel::configure(IPAContext &context,\n>> [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> {\n>> + exposure_ = 0;\n>> + gain_ = 0;\n>> +\n> I wonder if we should try to make sure algorithms do not store private\n> state, and only use storage in the IPAContext so we can ensure we can\n> zero/reset the context on configure for all algos?\n>\n> For now this seems reasonable to re-initialise.\n>\n> Just to check though - is this sufficient for stop/start cycles?\n>\n> If someone does:\n>\n> camera->configure()\n> camera->start()\n> ....\n> camera->stop()\n> camera->start()\n> ... Will there be the same bug here? Or will it be ok ?\n>\n> Or should these be re-initialised at start() ?\n>\n> Perhaps it's ok - as the issue is that the black level was reset here in\n> configure, so the flag that was also checking when to recalibrate also\n> needs to be reset here.\n>\n> A comment to state that clearly might be helpful here somewhere that the\n> exposure and gain are used to determine when to recalibrate the black\n> levels ...\n>\n>\n>> if (definedLevel_.has_value())\n>> context.configuration.black.level = definedLevel_;\n>> context.activeState.blc.level =\n>> -- \n>> 2.48.1\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 7E31EC32F5\n\tfor ;\n\tTue, 18 Mar 2025 13:52:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8803068947;\n\tTue, 18 Mar 2025 14:52:13 +0100 (CET)","from sender4-pp-f112.zoho.com (sender4-pp-f112.zoho.com\n\t[136.143.188.112])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2219068940\n\tfor ;\n\tTue, 18 Mar 2025 14:52:10 +0100 (CET)","by mx.zohomail.com with SMTPS id 17423059264921014.31246517849;\n\tTue, 18 Mar 2025 06:52:06 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=collabora.com\n\theader.i=robert.mader@collabora.com header.b=\"BMaVwXeU\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1742305927; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=N5M6ABiyhccpBYs01y8XhALy3AaR8oz32UPsYKgbvrKjdEZw7wg7dPKCXoiEIZqtMTuuTrqK1bXy+IArq/1UbXOpoZaJxaxsOACyP+j/k5EvyzaNawOYZ83hSL4B2IaAht17V/HrbEFL+Gi8OOE6OXgzowu8LJM00w/8NwZQn4s=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1742305927;\n\th=Content-Type:Content-Transfer-Encoding:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To:Cc;\n\tbh=37h5kAyaUN3h3KggcSUbplFrnsqwKdufdt6bBTqnXUY=; \n\tb=I3x8ueetUrry+1aGHT+EkOC8OIoGW+8dYXUWmkoDHiqO6yOtij6zGDctZxAL0QIrbb1s7cFXZjGDf2TO/GSfWTRvSc5Dws4Xe5gQUmNhHzUKdQVO5OAkvxNX2hWh24/iSBc0NFkrre2SIqLCKPX4aFoRBj+Nu8gcdFGHgRYXss8=","ARC-Authentication-Results":"i=1; mx.zohomail.com;\n\tdkim=pass header.i=collabora.com;\n\tspf=pass smtp.mailfrom=robert.mader@collabora.com;\n\tdmarc=pass header.from=","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1742305927;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:References:From:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Message-Id:Reply-To:Cc;\n\tbh=37h5kAyaUN3h3KggcSUbplFrnsqwKdufdt6bBTqnXUY=;\n\tb=BMaVwXeUi3epG7brr+1RrWluHixtbf+/VYkYPgrUPKNp/X4cv+34e+Pgo4BvEMr0\n\ts11q+KvkINTG1XtP/OOVakkOWweDXTFDYTELdWjmQb7YmN2Qmm9Tq39ZubDn6fGIFU2\n\tEnPXJnwdbLVmAkq82nY3Rn23zXDhiYKjUbxAYkyY=","Message-ID":"<6d3500fc-c742-4314-9168-6bc6d9cba0b4@collabora.com>","Date":"Tue, 18 Mar 2025 14:52:02 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","To":"Kieran Bingham ,\n\tMilan Zamazal , libcamera-devel@lists.libcamera.org","References":"<20250317112658.15084-1-mzamazal@redhat.com>\n\t<174229797929.1827314.12167064880939749362@ping.linuxembedded.co.uk>","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"<174229797929.1827314.12167064880939749362@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":33638,"web_url":"https://patchwork.libcamera.org/comment/33638/","msgid":"<174230685622.3394313.13836057328054380205@ping.linuxembedded.co.uk>","date":"2025-03-18T14:07:36","subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Robert Mader (2025-03-18 13:52:02)\n> FTR., I quickly tested whether \n> https://patchwork.libcamera.org/patch/21703/ fixes the issue as well and \n> apparently it does.\n> \n> So maybe we should just land that instead.\n\nI would actually think 'this' one is the right approach. I think active\nstate and the configuration should be reset before we recall configure\non each component, to ensure that each time we configure a camera it's\nfrom a clean state, and that nothing is left dangling (err, apparently\nexcept these algo-private data structures...).\n\nEssentially, that's what I was querying with 'should the private\nvariables be part of those structures so they are also cleared at the\nsame time' ... but I agree this is just an internal detail to the blc\n'algo here'.\n\n> On 18.03.25 12:39, Kieran Bingham wrote:\n> > Quoting Milan Zamazal (2025-03-17 11:26:58)\n> >> Automatic black level setting in software ISP updates the determined\n> >> black level value when exposure or gain change. It stores the last\n> >> exposure and gain values to detect the change.\n> >>\n> >> BlackLevel::configure() resets the stored black level value but not the\n> >> exposure and gain values. This can prevent updating the black value and\n> >> cause bad image output e.g. after suspending and resuming a camera, if\n> >> exposure and gain remain unchanged.\n> >>\n> >> Let's reset the stored exposure and gain values in\n> >> BlackLevel::configure() to fix the problem.\n> >>\n> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259\n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> src/ipa/simple/algorithms/blc.cpp | 5 ++++-\n> >> 1 file changed, 4 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> >> index 1d7d370b..14cf31bf 100644\n> >> --- a/src/ipa/simple/algorithms/blc.cpp\n> >> +++ b/src/ipa/simple/algorithms/blc.cpp\n> >> @@ -1,6 +1,6 @@\n> >> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> /*\n> >> - * Copyright (C) 2024, Red Hat Inc.\n> >> + * Copyright (C) 2024-2025, Red Hat Inc.\n> >> *\n> >> * Black level handling\n> >> */\n> >> @@ -38,6 +38,9 @@ int BlackLevel::init([[maybe_unused]] IPAContext &context,\n> >> int BlackLevel::configure(IPAContext &context,\n> >> [[maybe_unused]] const IPAConfigInfo &configInfo)\n> >> {\n> >> + exposure_ = 0;\n> >> + gain_ = 0;\n> >> +\n> > I wonder if we should try to make sure algorithms do not store private\n> > state, and only use storage in the IPAContext so we can ensure we can\n> > zero/reset the context on configure for all algos?\n> >\n> > For now this seems reasonable to re-initialise.\n> >\n> > Just to check though - is this sufficient for stop/start cycles?\n> >\n> > If someone does:\n> >\n> > camera->configure()\n> > camera->start()\n> > ....\n> > camera->stop()\n> > camera->start()\n> > ... Will there be the same bug here? Or will it be ok ?\n> >\n> > Or should these be re-initialised at start() ?\n> >\n> > Perhaps it's ok - as the issue is that the black level was reset here in\n> > configure, so the flag that was also checking when to recalibrate also\n> > needs to be reset here.\n> >\n> > A comment to state that clearly might be helpful here somewhere that the\n> > exposure and gain are used to determine when to recalibrate the black\n> > levels ...\n> >\n> >\n> >> if (definedLevel_.has_value())\n> >> context.configuration.black.level = definedLevel_;\n> >> context.activeState.blc.level =\n> >> -- \n> >> 2.48.1\n> >>\n> -- \n> Robert Mader\n> Consultant Software Developer\n> \n> Collabora Ltd.\n> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK\n> Registered in England & Wales, no. 5513718\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 2E86FC326C\n\tfor ;\n\tTue, 18 Mar 2025 14:07:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3804468947;\n\tTue, 18 Mar 2025 15:07:41 +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 723F768940\n\tfor ;\n\tTue, 18 Mar 2025 15:07:39 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1E23BA2F;\n\tTue, 18 Mar 2025 15:05:57 +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=\"Y5s57HQZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742306757;\n\tbh=hDdrPKc30w++BoPi/VTyFI7Pn+t8+SLFEuyCkQqz6ho=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Y5s57HQZ6wH9+btsXGBj0U/FjJXgxk39O3Vy4m5ZrN+Z+mOnrB/O6ypO/vBlLsAGH\n\tZ4+U9gWlLPaWn9bwvNthiwygOGcPhpH+yIAHDj/5YeJV1nL5nW51r7xfEQOl07SW7o\n\tiOISrPEgX1wfF6zLypdSL7yo45iS2oEGpr2mO+3A=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<6d3500fc-c742-4314-9168-6bc6d9cba0b4@collabora.com>","References":"<20250317112658.15084-1-mzamazal@redhat.com>\n\t<174229797929.1827314.12167064880939749362@ping.linuxembedded.co.uk>\n\t<6d3500fc-c742-4314-9168-6bc6d9cba0b4@collabora.com>","Subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","From":"Kieran Bingham ","To":"Milan Zamazal ,\n\tRobert Mader ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 18 Mar 2025 14:07:36 +0000","Message-ID":"<174230685622.3394313.13836057328054380205@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":33639,"web_url":"https://patchwork.libcamera.org/comment/33639/","msgid":"<174230723690.3394313.483400942534184084@ping.linuxembedded.co.uk>","date":"2025-03-18T14:13:56","subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2025-03-18 13:41:31)\n> Hi,\n> \n> Robert Mader writes:\n> \n> > Thanks!\n> >\n> > Tested-by: Robert Mader \n> \n> Thank you for testing.\n> \n> Kieran Bingham writes:\n> \n> > Quoting Milan Zamazal (2025-03-17 11:26:58)\n> >> Automatic black level setting in software ISP updates the determined\n> >> black level value when exposure or gain change. It stores the last\n> >\n> >> exposure and gain values to detect the change.\n> >> \n> >> BlackLevel::configure() resets the stored black level value but not the\n> >> exposure and gain values. This can prevent updating the black value and\n> >> cause bad image output e.g. after suspending and resuming a camera, if\n> >> exposure and gain remain unchanged.\n> >> \n> >> Let's reset the stored exposure and gain values in\n> >> BlackLevel::configure() to fix the problem.\n> >> \n> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259\n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> src/ipa/simple/algorithms/blc.cpp | 5 ++++-\n> >> 1 file changed, 4 insertions(+), 1 deletion(-)\n> >> \n> >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> >> index 1d7d370b..14cf31bf 100644\n> >> --- a/src/ipa/simple/algorithms/blc.cpp\n> >> +++ b/src/ipa/simple/algorithms/blc.cpp\n> >> @@ -1,6 +1,6 @@\n> >> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> /*\n> >> - * Copyright (C) 2024, Red Hat Inc.\n> >> + * Copyright (C) 2024-2025, Red Hat Inc.\n> >> *\n> >> * Black level handling\n> >> */\n> >> @@ -38,6 +38,9 @@ int BlackLevel::init([[maybe_unused]] IPAContext &context,\n> >> int BlackLevel::configure(IPAContext &context,\n> >> [[maybe_unused]] const IPAConfigInfo &configInfo)\n> >> {\n> >> + exposure_ = 0;\n> >> + gain_ = 0;\n> >> +\n> >\n> > I wonder if we should try to make sure algorithms do not store private\n> > state, and only use storage in the IPAContext so we can ensure we can\n> > zero/reset the context on configure for all algos?\n> \n> If it is desirable to put these two variables to IPAContext although\n> they are basically private to BlackLevel class and are not supposed to\n> be used anywhere else, I can post v2 with such a change.\n\nAs this is your IPA - that's mostly for you to decide - but my query is\nthat I consider that the top level \n\nIPASoftSimple::configure(const IPAConfigInfo &configInfo)\n{\n...\n \t/* Clear the IPA context before the streaming session. */\n\tcontext_.configuration = {};\n\tcontext_.activeState = {};\n \tcontext_.frameContexts.clear();\n...\n}\n\nbeing called before each algo->configure() is the 'right thing to do'\n... but that misses clearing any private state in an algorithm.\n\nResetting the whole structure is convenient and easy that's all...\n\n\nKnowing that there is no equivalent bug between start()->stop()->start()\ncalls, I'm fine with this patch.\n\n\nReviewed-by: Kieran Bingham \n\n\n> \n> > For now this seems reasonable to re-initialise.\n> >\n> > Just to check though - is this sufficient for stop/start cycles?\n> >\n> > If someone does:\n> >\n> > camera->configure()\n> > camera->start()\n> > ....\n> > camera->stop()\n> > camera->start()\n> > ... Will there be the same bug here? Or will it be ok ?\n> >\n> > Or should these be re-initialised at start() ?\n> \n> AFAICS the active state gets reset only in configure, so it should be\n> fine as it is.\n> \n> > Perhaps it's ok - as the issue is that the black level was reset here in\n> > configure, so the flag that was also checking when to recalibrate also\n> > needs to be reset here.\n> \n> Yes.\n> \n> > A comment to state that clearly might be helpful here somewhere that the\n> > exposure and gain are used to determine when to recalibrate the black\n> > levels ...\n> \n> OK.\n\nIt's not clear 'here in configure()' that exposure_ and gain_ are tied to\nthe black level. That's the only reason I think a comment would be\nbeneficial ...\n\nUltimately they are used to identify that the auto-black-level should\nrecalibrate black level if the 'scene' is changed.\n\nKnowing that black level is in fact an intrinsically 'static' property\nof cameras, I do wonder if this should be run on every change in\nexposure - but I think the point is it's a fall back when we don't have\na definition of what the real black level is - so it 'shouldn't' be used\nmuch in real terms.\n\n--\n\n\n\n> \n> >> if (definedLevel_.has_value())\n> >> context.configuration.black.level = definedLevel_;\n> >> context.activeState.blc.level =\n> >> -- \n> >> 2.48.1\n> >>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1E369C32F5\n\tfor ;\n\tTue, 18 Mar 2025 14:14:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 61CD568947;\n\tTue, 18 Mar 2025 15:14:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B2CA868940\n\tfor ;\n\tTue, 18 Mar 2025 15:13:59 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 667C8A2F;\n\tTue, 18 Mar 2025 15:12:17 +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=\"sqF9V0YG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742307137;\n\tbh=5ygDzGrzRzeRo4cV++TV/2T1naEmpWm5SpY89yM9ScE=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=sqF9V0YG+2xdaApxd4kJVm1VWJmN+MKlTLGwCLu1XMkE+eV27rlAX/sw/fgc+HRQ5\n\te0qvkpieNM+bL+fWhgOJs3wWm919gYYhx9XlyYpuqRaeuctc71M8qGqpSzbbKoTlXP\n\tvASB6hdIoXDFiY8jfArvOXCXPGoHQtJy812MVxuM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<855xk64fsk.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","References":"<20250317112658.15084-1-mzamazal@redhat.com>\n\t\n\t<855xk64fsk.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","Subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tRobert Mader ","To":"Milan Zamazal ","Date":"Tue, 18 Mar 2025 14:13:56 +0000","Message-ID":"<174230723690.3394313.483400942534184084@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":33648,"web_url":"https://patchwork.libcamera.org/comment/33648/","msgid":"<85wmcltk9v.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-03-19T09:57:48","subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Kieran Bingham writes:\n\n> Quoting Milan Zamazal (2025-03-18 13:41:31)\n>> Hi,\n>> \n>\n>> Robert Mader writes:\n>> \n>> > Thanks!\n>> >\n>> > Tested-by: Robert Mader \n>> \n>> Thank you for testing.\n>> \n>> Kieran Bingham writes:\n>> \n>> > Quoting Milan Zamazal (2025-03-17 11:26:58)\n>> >> Automatic black level setting in software ISP updates the determined\n>> >> black level value when exposure or gain change. It stores the last\n>> >\n>> >> exposure and gain values to detect the change.\n>> >> \n>> >> BlackLevel::configure() resets the stored black level value but not the\n>> >> exposure and gain values. This can prevent updating the black value and\n>> >> cause bad image output e.g. after suspending and resuming a camera, if\n>> >> exposure and gain remain unchanged.\n>> >> \n>> >> Let's reset the stored exposure and gain values in\n>> >> BlackLevel::configure() to fix the problem.\n>> >> \n>> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259\n>> >> Signed-off-by: Milan Zamazal \n>> >> ---\n>> >> src/ipa/simple/algorithms/blc.cpp | 5 ++++-\n>> >> 1 file changed, 4 insertions(+), 1 deletion(-)\n>> >> \n>> >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n>> >> index 1d7d370b..14cf31bf 100644\n>> >> --- a/src/ipa/simple/algorithms/blc.cpp\n>> >> +++ b/src/ipa/simple/algorithms/blc.cpp\n>> >> @@ -1,6 +1,6 @@\n>> >> /* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> >> /*\n>> >> - * Copyright (C) 2024, Red Hat Inc.\n>> >> + * Copyright (C) 2024-2025, Red Hat Inc.\n>> >> *\n>> >> * Black level handling\n>> >> */\n>> >> @@ -38,6 +38,9 @@ int BlackLevel::init([[maybe_unused]] IPAContext &context,\n>> >> int BlackLevel::configure(IPAContext &context,\n>> >> [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> >> {\n>> >> + exposure_ = 0;\n>> >> + gain_ = 0;\n>> >> +\n>> >\n>> > I wonder if we should try to make sure algorithms do not store private\n>> > state, and only use storage in the IPAContext so we can ensure we can\n>> > zero/reset the context on configure for all algos?\n>> \n>> If it is desirable to put these two variables to IPAContext although\n>> they are basically private to BlackLevel class and are not supposed to\n>> be used anywhere else, I can post v2 with such a change.\n>\n> As this is your IPA - that's mostly for you to decide - but my query is\n> that I consider that the top level \n>\n> IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n> {\n> ...\n> \t/* Clear the IPA context before the streaming session. */\n> \tcontext_.configuration = {};\n> \tcontext_.activeState = {};\n> \tcontext_.frameContexts.clear();\n> ...\n> }\n>\n> being called before each algo->configure() is the 'right thing to do'\n> ... but that misses clearing any private state in an algorithm.\n>\n> Resetting the whole structure is convenient and easy that's all...\n\nI think clarity outweighs encapsulation here so I changed it in v2.\nPosted, let's see whether it feels better.\n\n> Knowing that there is no equivalent bug between start()->stop()->start()\n> calls, I'm fine with this patch.\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n>> \n>> > For now this seems reasonable to re-initialise.\n>> >\n>> > Just to check though - is this sufficient for stop/start cycles?\n>> >\n>> > If someone does:\n>> >\n>> > camera->configure()\n>> > camera->start()\n>> > ....\n>> > camera->stop()\n>> > camera->start()\n>> > ... Will there be the same bug here? Or will it be ok ?\n>> >\n>> > Or should these be re-initialised at start() ?\n>> \n>> AFAICS the active state gets reset only in configure, so it should be\n>> fine as it is.\n>> \n>> > Perhaps it's ok - as the issue is that the black level was reset here in\n>> > configure, so the flag that was also checking when to recalibrate also\n>> > needs to be reset here.\n>> \n>> Yes.\n>> \n>> > A comment to state that clearly might be helpful here somewhere that the\n>> > exposure and gain are used to determine when to recalibrate the black\n>> > levels ...\n>> \n>> OK.\n>\n> It's not clear 'here in configure()' that exposure_ and gain_ are tied to\n> the black level. That's the only reason I think a comment would be\n> beneficial ...\n>\n> Ultimately they are used to identify that the auto-black-level should\n> recalibrate black level if the 'scene' is changed.\n>\n> Knowing that black level is in fact an intrinsically 'static' property\n> of cameras, I do wonder if this should be run on every change in\n> exposure - but I think the point is it's a fall back when we don't have\n> a definition of what the real black level is - so it 'shouldn't' be used\n> much in real terms.\n>\n> --\n>\n>\n>\n>> \n>> >> if (definedLevel_.has_value())\n>> >> context.configuration.black.level = definedLevel_;\n>> >> context.activeState.blc.level =\n>> >> -- \n>> >> 2.48.1\n>> >>\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 497EBC3301\n\tfor ;\n\tWed, 19 Mar 2025 09:57:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 930CC6894D;\n\tWed, 19 Mar 2025 10:57:56 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0543468942\n\tfor ;\n\tWed, 19 Mar 2025 10:57:54 +0100 (CET)","from mail-wm1-f72.google.com (mail-wm1-f72.google.com\n\t[209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-611-Re6HSqadMt6ewzTUOzxG4A-1; Wed, 19 Mar 2025 05:57:52 -0400","by mail-wm1-f72.google.com with SMTP id\n\t5b1f17b1804b1-43cf172ffe1so36638515e9.3\n\tfor ;\n\tWed, 19 Mar 2025 02:57:52 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-43d440eda26sm13691315e9.36.2025.03.19.02.57.49\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 19 Mar 2025 02:57:49 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"CPRrjYXC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1742378274;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=lMSwPvoRrX4y59skaXhEOkEONl3eG9FU9O9FptEqA9s=;\n\tb=CPRrjYXCLPkBQ8xozgyzyGIFEKB6aRoBrMxJBjN7uvyyZ8iZkZMtvOi0xR5BvEi3aq5lwQ\n\tlx2jGivRDwm0qXcgnhd+YvROmMG5wad5+zPzFA6p/R4GOzeygAn3yG7y4wo0Ui3tXbBQjW\n\tVxG9hGHCwy7DTiP5JKvLtxhIJIzzbZ4=","X-MC-Unique":"Re6HSqadMt6ewzTUOzxG4A-1","X-Mimecast-MFC-AGG-ID":"Re6HSqadMt6ewzTUOzxG4A_1742378271","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1742378271; x=1742983071;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=lMSwPvoRrX4y59skaXhEOkEONl3eG9FU9O9FptEqA9s=;\n\tb=J8oU8Nijd7FTWaRSbTtV6lLFYHOOAxmI4Yw0tJYArVjZvdPfowYs+Yv7pMabA8MqiP\n\tKXvTG6TMmbgiNq6xu64Qh7j7hkBDaJG4289HcxULhOc9RRdHV2s3P5J0ZEDjNdjHuCX5\n\ttPcoGTyHVa6PxsDdJ4hGTGRMUaGTcRuKS2pXERaXDusfGzuQeemeiGrmOSbi9gfpPi1q\n\tjAyRNa6F9pOxDp6p6wbivu8XcvZMkENBmA43wuS9PnBotWQ82tjsKqTo/x3UOsf4XE4i\n\tS4mc0hp6dtROI3XCRHyMBDuATuZYVQFsK5ikYJMBpFCF2TZQHTmLuBfQKZyGsQjO2h+A\n\ttEmQ==","X-Gm-Message-State":"AOJu0YzM7S2Fvgfnu0ggoP8hKYzmFtss4n8k/ydIhyGOzX0UcpA7srcz\n\tK8pIk+VKYajJ1+F8KkH7Mr7kUWSU1eDcT/eh0T1K6aMb6RVE0tY2hwllLBc9C99PT8NVUDF3wDd\n\tG9aPRZyJrdNn93JuND1OIrajyFPJMSCf+kWXZxEpI4e6yIyAIS0po78kUsjbAxSBKoRcxfA43nT\n\tM2JAYK6Q==","X-Gm-Gg":"ASbGncuLBCgnQm3pKVQxdj1JT982Kei7f32u9o42LJDk8nLP7/JLes00B50uwZwSmU5\n\tXvmuUfO89zQ5RI4LtPsFwfz0gJZDpV+YorYydy4t/ychsghsXbYiD3jwcBrlRb00+w+9Ehb/m10\n\tJNGeBwtUrlqvMkEznb8Eh/g7FQpTLp2XuU84iii2fezwrVSEg43d6FHqXVArr/ug6c4CFyWcJTL\n\tpqabQIm2xIct/rmmNJt2gFc4+uWf2CIBudBTlW2X0aqGe+Ar/9Y3X1InvnhzLhMRNDpPYmSUzxo\n\tMG6w+wDFsNf8503p9g2aTysUg512WQyIE9qcFnHkLP5nG+LZOKjpRXkVZPFcylh+CQ/3","X-Received":["by 2002:a05:6000:1acc:b0:399:6dc0:f134 with SMTP id\n\tffacd0b85a97d-39973b23a58mr1810765f8f.51.1742378270626; \n\tWed, 19 Mar 2025 02:57:50 -0700 (PDT)","by 2002:a05:6000:1acc:b0:399:6dc0:f134 with SMTP id\n\tffacd0b85a97d-39973b23a58mr1810718f8f.51.1742378270094; \n\tWed, 19 Mar 2025 02:57:50 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGZXr3QzIN7X/x+iKxd2IrZWmqFL0bTJkDBc4zggNKUWhs3oIvKPygzz+mjGWxrP7GsAzehDw==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, Robert Mader\n\t","Subject":"Re: [PATCH] libcamera: software_isp: Reset stored exposure in black\n\tlevel","In-Reply-To":"<174230723690.3394313.483400942534184084@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Tue, 18 Mar 2025 14:13:56 +0000\")","References":"<20250317112658.15084-1-mzamazal@redhat.com>\n\t\n\t<855xk64fsk.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<174230723690.3394313.483400942534184084@ping.linuxembedded.co.uk>","Date":"Wed, 19 Mar 2025 10:57:48 +0100","Message-ID":"<85wmcltk9v.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"jg5LwQ4_z42gr4zf2kfC36DZo3-A-fvzjCc6NMLEoUI_1742378271","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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\" "}}]