[{"id":31721,"web_url":"https://patchwork.libcamera.org/comment/31721/","msgid":"","date":"2024-10-12T19:49:08","subject":"Re: [PATCH v3 1/1] libcamera: software_isp: Get black level from the\n\tcamera helper","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"I gave this a try on a Pixel 3a (IMX 355 and IMX 363). In both cases \nusing 4096 made the resulting image fully black when covering the \nsensor, which IIUC is the desired effect. Not putting any values or \nusing lower ones such as 3200 results in a greenish image - so there's \ndefinitely an improvement here. Thus fell free to add a Tested-by: \nRobert Mader \n\nAt least in the case of the IMX355 the output overall becomes much \ndarker - too dark IMO. Is that expected - and can/will it be compensated \nby improvements of the AWB algorithm?\n\nP.S.: I guess I can submit a follow-up adding the black level for the \nIMX355 - the IMX363 driver hasn't been submitted to upstream yet.\n\nOn 11.10.24 17:39, Milan Zamazal wrote:\n> The black level in software ISP is unconditionally guessed from the\n> obtained frames. CameraSensorHelper optionally provides the black level\n> from camera specifications now. Let's use the value if available.\n>\n> If the black level is not available from the given CameraSensorHelper\n> instance, it's still determined on the fly.\n>\n> Signed-off-by: Milan Zamazal \n> Reviewed-by: Kieran Bingham \n> ---\n> src/ipa/simple/algorithms/blc.cpp | 6 +++++-\n> src/ipa/simple/ipa_context.h | 4 ++++\n> src/ipa/simple/soft_simple.cpp | 9 +++++++++\n> 3 files changed, 18 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n> index b9f2aaa6d..a7af2e12c 100644\n> --- a/src/ipa/simple/algorithms/blc.cpp\n> +++ b/src/ipa/simple/algorithms/blc.cpp\n> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()\n> int BlackLevel::configure(IPAContext &context,\n> \t\t\t [[maybe_unused]] const IPAConfigInfo &configInfo)\n> {\n> -\tcontext.activeState.blc.level = 255;\n> +\tcontext.activeState.blc.level =\n> +\t\tcontext.configuration.black.level.value_or(255);\n> \treturn 0;\n> }\n> \n> @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context,\n> \t\t\t const SwIspStats *stats,\n> \t\t\t [[maybe_unused]] ControlList &metadata)\n> {\n> +\tif (context.configuration.black.level.has_value())\n> +\t\treturn;\n> +\n> \tif (frameContext.sensor.exposure == exposure_ &&\n> \t frameContext.sensor.gain == gain_) {\n> \t\treturn;\n> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> index 3519f20f6..fd121eebe 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -8,6 +8,7 @@\n> #pragma once\n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {\n> \t\tint32_t exposureMin, exposureMax;\n> \t\tdouble againMin, againMax, againMinStep;\n> \t} agc;\n> +\tstruct {\n> +\t\tstd::optional level;\n> +\t} black;\n> };\n> \n> struct IPAActiveState {\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index b28c7039f..079409f6d 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -201,6 +201,15 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n> \t\t\t(context_.configuration.agc.againMax -\n> \t\t\t context_.configuration.agc.againMin) /\n> \t\t\t100.0;\n> +\t\tif (camHelper_->blackLevel().has_value())\n> +\t\t\t/*\n> +\t\t\t * The black level from camHelper_ is a 16 bit value, software ISP\n> +\t\t\t * works with 8 bit pixel values, both regardless of the actual\n> +\t\t\t * sensor pixel width. Hence we obtain the pixel-based black value\n> +\t\t\t * by dividing the value from the helper by 256.\n> +\t\t\t */\n> +\t\t\tcontext_.configuration.black.level =\n> +\t\t\t\tcamHelper_->blackLevel().value() / 256;\n> \t} else {\n> \t\t/*\n> \t\t * The camera sensor gain (g) is usually not equal to the value written","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 EF33DC32F4\n\tfor ;\n\tSat, 12 Oct 2024 19:49:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D4D9B65379;\n\tSat, 12 Oct 2024 21:49:17 +0200 (CEST)","from sender4-op-o12.zoho.com (sender4-op-o12.zoho.com\n\t[136.143.188.12])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1F95618C3\n\tfor ;\n\tSat, 12 Oct 2024 21:49:15 +0200 (CEST)","by mx.zohomail.com with SMTPS id 1728762551297893.8015263158903;\n\tSat, 12 Oct 2024 12:49:11 -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=\"ejgPG0oP\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1728762553; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=DubrCGKQFnEel5M/+zQ1BMRVRxSwSL9uzZ8W6OmAsjNpFZH3lWfxTRp0ul/CM2dBaUoe6pElPYfLXJSpgZHZKInyfgZ2xjTIY9t/phOs55U7uBTeGcnQBZp79Vh4qliCKl7Y6lXpV6zHaHAtBfzaK3FutF74udZpCvOuwD/2Jy8=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1728762553;\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=8dFdltBzSeyRXSufuwom1UqRM90fIr19dbUrucVNUMU=; \n\tb=QHL9KAR0gCT33Qm07GSz1Hj7RkgzTeS+QGPRLbyA+NBDOIUFYeh0vNN/2DBBH/uFwrUmuzLBbptLqpnwV8OizOGoJ7wFVNnUKcD9jEB516hG8FeuA8+30Gn68LOjbv/NTqgIfq0+33VLHh4J0HyqlowGwCq8Nys5jEEBcKojlmo=","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=1728762553;\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=8dFdltBzSeyRXSufuwom1UqRM90fIr19dbUrucVNUMU=;\n\tb=ejgPG0oPloYF0J5wIn4W3rxJ7q4BnQET5Lw5va+vxuL3V+5DvPO0VAaKdVJWZ4X6\n\taOVb+nrioTTh1ARxU1SP99KT9+1wptRM7xJeCGiAYe+N79Fn7/YeVfg+Kzox5dakuNv\n\tE4EJwuPyKghc3PjpcPJ8ixookKXwLCJBRLbCSJ/E=","Message-ID":"","Date":"Sat, 12 Oct 2024 21:49:08 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 1/1] libcamera: software_isp: Get black level from the\n\tcamera helper","To":"libcamera-devel@lists.libcamera.org","References":"<20241011153934.1291362-1-mzamazal@redhat.com>\n\t<20241011153934.1291362-2-mzamazal@redhat.com>","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"<20241011153934.1291362-2-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":31722,"web_url":"https://patchwork.libcamera.org/comment/31722/","msgid":"<5e7a83ab-b917-4c2e-8891-d06fb896dc6c@collabora.com>","date":"2024-10-13T11:23:04","subject":"Re: [PATCH v3 1/1] libcamera: software_isp: Get black level from the\n\tcamera helper","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"Turns out I have to ask for/suggest some more changes:\n\nOn 12.10.24 21:49, Robert Mader wrote:\n> I gave this a try on a Pixel 3a (IMX 355 and IMX 363). In both cases \n> using 4096 made the resulting image fully black when covering the \n> sensor, which IIUC is the desired effect. Not putting any values or \n> using lower ones such as 3200 results in a greenish image - so there's \n> definitely an improvement here. Thus fell free to add a Tested-by: \n> Robert Mader \n>\n> At least in the case of the IMX355 the output overall becomes much \n> darker - too dark IMO. Is that expected - and can/will it be \n> compensated by improvements of the AWB algorithm?\n>\n> P.S.: I guess I can submit a follow-up adding the black level for the \n> IMX355 - the IMX363 driver hasn't been submitted to upstream yet.\n>\n> On 11.10.24 17:39, Milan Zamazal wrote:\n>> The black level in software ISP is unconditionally guessed from the\n>> obtained frames.  CameraSensorHelper optionally provides the black level\n>> from camera specifications now.  Let's use the value if available.\n>>\n>> If the black level is not available from the given CameraSensorHelper\n>> instance, it's still determined on the fly.\n>>\n>> Signed-off-by: Milan Zamazal \n>> Reviewed-by: Kieran Bingham \n>> ---\n>>   src/ipa/simple/algorithms/blc.cpp | 6 +++++-\n>>   src/ipa/simple/ipa_context.h      | 4 ++++\n>>   src/ipa/simple/soft_simple.cpp    | 9 +++++++++\n>>   3 files changed, 18 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/ipa/simple/algorithms/blc.cpp \n>> b/src/ipa/simple/algorithms/blc.cpp\n>> index b9f2aaa6d..a7af2e12c 100644\n>> --- a/src/ipa/simple/algorithms/blc.cpp\n>> +++ b/src/ipa/simple/algorithms/blc.cpp\n>> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()\n>>   int BlackLevel::configure(IPAContext &context,\n>>                 [[maybe_unused]] const IPAConfigInfo &configInfo)\n>>   {\n>> -    context.activeState.blc.level = 255;\n>> +    context.activeState.blc.level =\n>> +        context.configuration.black.level.value_or(255);\n>>       return 0;\n>>   }\n>>   @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context,\n>>                const SwIspStats *stats,\n>>                [[maybe_unused]] ControlList &metadata)\n>>   {\n>> +    if (context.configuration.black.level.has_value())\n>> +        return;\n>> +\n>>       if (frameContext.sensor.exposure == exposure_ &&\n>>           frameContext.sensor.gain == gain_) {\n>>           return;\n>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>> index 3519f20f6..fd121eebe 100644\n>> --- a/src/ipa/simple/ipa_context.h\n>> +++ b/src/ipa/simple/ipa_context.h\n>> @@ -8,6 +8,7 @@\n>>   #pragma once\n>>     #include \n>> +#include \n>>   #include \n>>     #include \n>> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {\n>>           int32_t exposureMin, exposureMax;\n>>           double againMin, againMax, againMinStep;\n>>       } agc;\n>> +    struct {\n>> +        std::optional level;\n>> +    } black;\n>>   };\n>>     struct IPAActiveState {\n>> diff --git a/src/ipa/simple/soft_simple.cpp \n>> b/src/ipa/simple/soft_simple.cpp\n>> index b28c7039f..079409f6d 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -201,6 +201,15 @@ int IPASoftSimple::configure(const IPAConfigInfo \n>> &configInfo)\n>>               (context_.configuration.agc.againMax -\n>>                context_.configuration.agc.againMin) /\n>>               100.0;\n>> +        if (camHelper_->blackLevel().has_value())\n>> +            /*\n>> +             * The black level from camHelper_ is a 16 bit value, \n>> software ISP\n>> +             * works with 8 bit pixel values, both regardless of the \n>> actual\n>> +             * sensor pixel width. Hence we obtain the pixel-based \n>> black value\n>> +             * by dividing the value from the helper by 256.\n>> +             */\n>> +            context_.configuration.black.level =\n>> +                camHelper_->blackLevel().value() / 256;\n\nStyle nit: braces around this block would be good now that there's the \ncomment.\n\nMore importantly though: Assuming setting a value for blackLevel_ \nwithout providing values for gain makes sense, then it would be great to \nadapt the code above to not assume so. It currently only works in builds \nwith asserts disabled - which I used yesterday when trying it. There are \ntwo more similar places in processStats() (camHelper_ ? ...).\n\nForcing the non-cam-helper code for gain seems to fix the regression I \nmentioned in the previous mail.\n\n>>       } else {\n>>           /*\n>>            * The camera sensor gain (g) is usually not equal to the \n>> value written\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 6B77CC326B\n\tfor ;\n\tSun, 13 Oct 2024 11:23:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 46B356537C;\n\tSun, 13 Oct 2024 13:23:13 +0200 (CEST)","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 C603B6536C\n\tfor ;\n\tSun, 13 Oct 2024 13:23:10 +0200 (CEST)","by mx.zohomail.com with SMTPS id 1728818586566554.1521766888253;\n\tSun, 13 Oct 2024 04:23: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=\"bMs9hI5j\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1728818587; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=Kxn/A+yDM+LSyCZQZU8s5NMtSu6yEGTGd1BcU6PqxWoeD39EDwxkOUgrPEmx0Wbpr3PdS6x76qfoUkUlIXvn3otVIdT22L0Oz9hLGsMId1SwCe1HeZSbnGVFwc0dp2oga6mdpjQ6Idyk4ZS72vHdYIppfowaMPW9vnrFpCZJwEI=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1728818587;\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=ReYPu7l/n8i9IS+0kN7h6sNGTsmfk3Pvhk7PcIrd7O8=; \n\tb=MHUMkTULNRFzvLyV0pjn3kYPP5+tiOC7ytO7AQnVRIxFW0uaNySzFLI/8ONr5iORWn1o28/TNiRAW+f9SdcAdBSzP2r8+FmdKghCiBGQQSK2OVFjdyqmJ73OAQb6urVewYy1S5o3AZEE9ME7CZj3qNWSBAkLgR6LHJYiRsD9qr8=","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=1728818587;\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=ReYPu7l/n8i9IS+0kN7h6sNGTsmfk3Pvhk7PcIrd7O8=;\n\tb=bMs9hI5j3nJRG7OaKFG+PTyhLRLMDq59kFuV488FJKmvXK+E2YSCq7JerENNxGZC\n\txDep2F11XsvVbasa3p91NYK78By/UjXTd/xkUc4GfSS88i5RuNbuCeiaxp6GIsV3ivB\n\tTUYYWJPn2a2WaUYo1j2xbHSs9WDD704qOoGl+Rpc=","Message-ID":"<5e7a83ab-b917-4c2e-8891-d06fb896dc6c@collabora.com>","Date":"Sun, 13 Oct 2024 13:23:04 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 1/1] libcamera: software_isp: Get black level from the\n\tcamera helper","To":"libcamera-devel@lists.libcamera.org","References":"<20241011153934.1291362-1-mzamazal@redhat.com>\n\t<20241011153934.1291362-2-mzamazal@redhat.com>\n\t","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":31730,"web_url":"https://patchwork.libcamera.org/comment/31730/","msgid":"<87ed4jgcy1.fsf@redhat.com>","date":"2024-10-14T11:07:34","subject":"Re: [PATCH v3 1/1] libcamera: software_isp: Get black level from\n\tthe camera helper","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Robert,\n\nthank you for testing and review.\n\nRobert Mader writes:\n\n> Turns out I have to ask for/suggest some more changes:\n>\n> On 12.10.24 21:49, Robert Mader wrote:\n>> I gave this a try on a Pixel 3a (IMX 355 and IMX 363). In both cases using 4096 made\n>> the resulting image fully black when covering the sensor, which IIUC is the desired\n>> effect. \n\nYes.\n\n>> Not putting any values or using lower ones such as 3200 results in a\n>> greenish image - so there's definitely an improvement here. Thus fell\n>> free to add a Tested-by: Robert Mader \n>>\n>> At least in the case of the IMX355 the output overall becomes much darker - too dark\n>> IMO. Is that expected - and can/will it be compensated by improvements of the AWB\n>> algorithm?\n\nNo. Blacks and shadows should be darker but overall the image should\nhave about the same brightness. This is handled by auto-exposure, which\nis applied after subtracting the black level.\n\n>> P.S.: I guess I can submit a follow-up adding the black level for the IMX355 - the\n>> IMX363 driver hasn't been submitted to upstream yet.\n>>\n>> On 11.10.24 17:39, Milan Zamazal wrote:\n>>> The black level in software ISP is unconditionally guessed from the\n>>> obtained frames.  CameraSensorHelper optionally provides the black level\n>>> from camera specifications now.  Let's use the value if available.\n>>>\n>>> If the black level is not available from the given CameraSensorHelper\n>>> instance, it's still determined on the fly.\n>>>\n>>> Signed-off-by: Milan Zamazal \n>>> Reviewed-by: Kieran Bingham \n>>> ---\n>>>   src/ipa/simple/algorithms/blc.cpp | 6 +++++-\n>>>   src/ipa/simple/ipa_context.h      | 4 ++++\n>>>   src/ipa/simple/soft_simple.cpp    | 9 +++++++++\n>>>   3 files changed, 18 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n>>> index b9f2aaa6d..a7af2e12c 100644\n>>> --- a/src/ipa/simple/algorithms/blc.cpp\n>>> +++ b/src/ipa/simple/algorithms/blc.cpp\n>>> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()\n>>>   int BlackLevel::configure(IPAContext &context,\n>>>                 [[maybe_unused]] const IPAConfigInfo &configInfo)\n>>>   {\n>>> -    context.activeState.blc.level = 255;\n>>> +    context.activeState.blc.level =\n>>> +        context.configuration.black.level.value_or(255);\n>>>       return 0;\n>>>   }\n>>>   @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context,\n>>>                const SwIspStats *stats,\n>>>                [[maybe_unused]] ControlList &metadata)\n>>>   {\n>>> +    if (context.configuration.black.level.has_value())\n>>> +        return;\n>>> +\n>>>       if (frameContext.sensor.exposure == exposure_ &&\n>>>           frameContext.sensor.gain == gain_) {\n>>>           return;\n>>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>>> index 3519f20f6..fd121eebe 100644\n>>> --- a/src/ipa/simple/ipa_context.h\n>>> +++ b/src/ipa/simple/ipa_context.h\n>>> @@ -8,6 +8,7 @@\n>>>   #pragma once\n>>>     #include \n>>> +#include \n>>>   #include \n>>>     #include \n>>> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {\n>>>           int32_t exposureMin, exposureMax;\n>>>           double againMin, againMax, againMinStep;\n>>>       } agc;\n>>> +    struct {\n>>> +        std::optional level;\n>>> +    } black;\n>>>   };\n>>>     struct IPAActiveState {\n>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>>> index b28c7039f..079409f6d 100644\n>>> --- a/src/ipa/simple/soft_simple.cpp\n>>> +++ b/src/ipa/simple/soft_simple.cpp\n>>> @@ -201,6 +201,15 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n>>>               (context_.configuration.agc.againMax -\n>>>                context_.configuration.agc.againMin) /\n>>>               100.0;\n>>> +        if (camHelper_->blackLevel().has_value())\n>>> +            /*\n>>> +             * The black level from camHelper_ is a 16 bit value, software ISP\n>>> +             * works with 8 bit pixel values, both regardless of the actual\n>>> +             * sensor pixel width. Hence we obtain the pixel-based black value\n>>> +             * by dividing the value from the helper by 256.\n>>> +             */\n>>> +            context_.configuration.black.level =\n>>> +                camHelper_->blackLevel().value() / 256;\n>\n> Style nit: braces around this block would be good now that there's the comment.\n\nOK.\n\n> More importantly though: Assuming setting a value for blackLevel_ without providing\n> values for gain makes sense, then it would be great to adapt the code above to not\n> assume so. It currently only works in builds with asserts disabled - which I used\n> yesterday when trying it. There are two more similar places in processStats()\n> (camHelper_ ? ...).\n>\n> Forcing the non-cam-helper code for gain seems to fix the regression I mentioned in the\n> previous mail.\n\nI'm not sure I understand what you mean exactly. What the patch does is\nthat if a camera sensor helper is available for the given sensor and it\nprovides black level then that black level is used; otherwise black\nlevel auto-detection is used (as before). If there is no helper for the\ngiven sensor, nothing changes. Do you mean there is some trouble in the\ncurrent code in such a case (it seems to work fine for me)? Or do you\ntalk about a situation when the helper provides black level but not\nthe corresponding gains? And what assertion fails?\n\n>>>       } else {\n>>>           /*\n>>>            * The camera sensor gain (g) is usually not equal to the value written\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 8E6E2C32F4\n\tfor ;\n\tMon, 14 Oct 2024 11:07:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4638A65374;\n\tMon, 14 Oct 2024 13:07:43 +0200 (CEST)","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 2727763525\n\tfor ;\n\tMon, 14 Oct 2024 13:07:41 +0200 (CEST)","from mail-wr1-f69.google.com (mail-wr1-f69.google.com\n\t[209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-113-eimYHw2YP62EEBNtGMEofg-1; Mon, 14 Oct 2024 07:07:38 -0400","by mail-wr1-f69.google.com with SMTP id\n\tffacd0b85a97d-37d67f4bf98so875988f8f.1\n\tfor ;\n\tMon, 14 Oct 2024 04:07:38 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-37d4b6a87d3sm11090227f8f.11.2024.10.14.04.07.34\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 14 Oct 2024 04:07:35 -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=\"ETZYUvA0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1728904059;\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\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=mXgakWlfSJduRHXehBrUK1Jz5FOQTfQbRto8VPH7C1A=;\n\tb=ETZYUvA0CPm1OENxp8dJECBgdhIhUp36i1n4MhKZRVZ0fQ/wtuYSzmO45RCgoCplsw/Rik\n\tTeNxikUygpbTBHrW8HSwVFZDUAxpIx5kM/jXoR3jLvPc0nYWyPrYBYqPzloNg9u3Mxpg0C\n\t7vAjPYR92FCVr2c2zmy8CYcoCg4UNTc=","X-MC-Unique":"eimYHw2YP62EEBNtGMEofg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728904057; x=1729508857;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=kGkCvDKaKP452gVdfQaJR+xZNPb3bV0dqreQT+1Eu0E=;\n\tb=M775+20oghdx5osvOZ1dCh/VwIR7pcolgunpE9lNK8ooxEm68oVqOO+c9BK8wMUSOA\n\tlWspUWgPJl6IKWz/JCitfPmzAzSQpZDcPYUY2gla4mL62bBOn1uy2oVFUXRbdJzCSFYR\n\tDx2jCWhxhIT43/KybY98WETOCk4PBVHOWRhC5RsjphXtkqF4FObXd69VJBcuwa0ImW3z\n\tKZyPSY3E4i+X5kLYRZlBLxRrdXh/XhpfLcDy0ivPYeLEvdBMWNlV4Y3CyQZ5P+KeBTZs\n\tW/dsQn6VxPlcTqpeYXvOzvhSsdoHE+9XeaJSl0ZR8unCuF9FM/4YE5bKrr6JNxnA/3wX\n\tNdVg==","X-Gm-Message-State":"AOJu0Yza0eKx5o5kojB5J+NFFsIqKF5a/HUoPjUd4QHO2BfP1NlRJE2z\n\tzJVs9zBuJHi6or6YUPuYON7TWSSbXyAhvjEI3lPiHzChjGhVT7Xdxh4bMcGxOcNpPBekPkxYNQO\n\tsOeucBklo8t0+h02XFx8MMHxe6OSYf6ML6mGWoI7WHMAOGaGish4kWVqNG62RrvGiCCWMUU/lC6\n\tPo5Sypv4tZtwxuj1NBPz8pYxQ7XzHT6qr74Mu0WlFj5VSR1CjhSnQSyss=","X-Received":["by 2002:adf:fe47:0:b0:374:c3e4:d6de with SMTP id\n\tffacd0b85a97d-37d5529b211mr7697299f8f.41.1728904056570; \n\tMon, 14 Oct 2024 04:07:36 -0700 (PDT)","by 2002:adf:fe47:0:b0:374:c3e4:d6de with SMTP id\n\tffacd0b85a97d-37d5529b211mr7697273f8f.41.1728904055891; \n\tMon, 14 Oct 2024 04:07:35 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFcUG2HeTjfyeIpUKmxFEorGpSED37vAuuKfaK7665RMMMnoS1fC7bBRESgbI/02JbHpBGO+Q==","From":"Milan Zamazal ","To":"Robert Mader ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 1/1] libcamera: software_isp: Get black level from\n\tthe camera helper","In-Reply-To":"<5e7a83ab-b917-4c2e-8891-d06fb896dc6c@collabora.com> (Robert\n\tMader's message of \"Sun, 13 Oct 2024 13:23:04 +0200\")","References":"<20241011153934.1291362-1-mzamazal@redhat.com>\n\t<20241011153934.1291362-2-mzamazal@redhat.com>\n\t\n\t<5e7a83ab-b917-4c2e-8891-d06fb896dc6c@collabora.com>","Date":"Mon, 14 Oct 2024 13:07:34 +0200","Message-ID":"<87ed4jgcy1.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":31733,"web_url":"https://patchwork.libcamera.org/comment/31733/","msgid":"","date":"2024-10-14T11:34:24","subject":"Re: [PATCH v3 1/1] libcamera: software_isp: Get black level from the\n\tcamera helper","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"On 14.10.24 13:07, Milan Zamazal wrote:\n> Hi Robert,\n>\n> thank you for testing and review.\n>\n> Robert Mader writes:\n>\n>> Turns out I have to ask for/suggest some more changes:\n>>\n>> On 12.10.24 21:49, Robert Mader wrote:\n>>> I gave this a try on a Pixel 3a (IMX 355 and IMX 363). In both cases using 4096 made\n>>> the resulting image fully black when covering the sensor, which IIUC is the desired\n>>> effect.\n> Yes.\n>\n>>> Not putting any values or using lower ones such as 3200 results in a\n>>> greenish image - so there's definitely an improvement here. Thus fell\n>>> free to add a Tested-by: Robert Mader \n>>>\n>>> At least in the case of the IMX355 the output overall becomes much darker - too dark\n>>> IMO. Is that expected - and can/will it be compensated by improvements of the AWB\n>>> algorithm?\n> No. Blacks and shadows should be darker but overall the image should\n> have about the same brightness. This is handled by auto-exposure, which\n> is applied after subtracting the black level.\nIndeed, and I can confirm that this works correctly once the issue below \nis addressed.\n>>> P.S.: I guess I can submit a follow-up adding the black level for the IMX355 - the\n>>> IMX363 driver hasn't been submitted to upstream yet.\n>>>\n>>> On 11.10.24 17:39, Milan Zamazal wrote:\n>>>> The black level in software ISP is unconditionally guessed from the\n>>>> obtained frames.  CameraSensorHelper optionally provides the black level\n>>>> from camera specifications now.  Let's use the value if available.\n>>>>\n>>>> If the black level is not available from the given CameraSensorHelper\n>>>> instance, it's still determined on the fly.\n>>>>\n>>>> Signed-off-by: Milan Zamazal \n>>>> Reviewed-by: Kieran Bingham \n>>>> ---\n>>>>   src/ipa/simple/algorithms/blc.cpp | 6 +++++-\n>>>>   src/ipa/simple/ipa_context.h      | 4 ++++\n>>>>   src/ipa/simple/soft_simple.cpp    | 9 +++++++++\n>>>>   3 files changed, 18 insertions(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n>>>> index b9f2aaa6d..a7af2e12c 100644\n>>>> --- a/src/ipa/simple/algorithms/blc.cpp\n>>>> +++ b/src/ipa/simple/algorithms/blc.cpp\n>>>> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()\n>>>>   int BlackLevel::configure(IPAContext &context,\n>>>>                 [[maybe_unused]] const IPAConfigInfo &configInfo)\n>>>>   {\n>>>> -    context.activeState.blc.level = 255;\n>>>> +    context.activeState.blc.level =\n>>>> +        context.configuration.black.level.value_or(255);\n>>>>       return 0;\n>>>>   }\n>>>>   @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context,\n>>>>                const SwIspStats *stats,\n>>>>                [[maybe_unused]] ControlList &metadata)\n>>>>   {\n>>>> +    if (context.configuration.black.level.has_value())\n>>>> +        return;\n>>>> +\n>>>>       if (frameContext.sensor.exposure == exposure_ &&\n>>>>           frameContext.sensor.gain == gain_) {\n>>>>           return;\n>>>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>>>> index 3519f20f6..fd121eebe 100644\n>>>> --- a/src/ipa/simple/ipa_context.h\n>>>> +++ b/src/ipa/simple/ipa_context.h\n>>>> @@ -8,6 +8,7 @@\n>>>>   #pragma once\n>>>>     #include \n>>>> +#include \n>>>>   #include \n>>>>     #include \n>>>> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {\n>>>>           int32_t exposureMin, exposureMax;\n>>>>           double againMin, againMax, againMinStep;\n>>>>       } agc;\n>>>> +    struct {\n>>>> +        std::optional level;\n>>>> +    } black;\n>>>>   };\n>>>>     struct IPAActiveState {\n>>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>>>> index b28c7039f..079409f6d 100644\n>>>> --- a/src/ipa/simple/soft_simple.cpp\n>>>> +++ b/src/ipa/simple/soft_simple.cpp\n>>>> @@ -201,6 +201,15 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n>>>>               (context_.configuration.agc.againMax -\n>>>>                context_.configuration.agc.againMin) /\n>>>>               100.0;\n>>>> +        if (camHelper_->blackLevel().has_value())\n>>>> +            /*\n>>>> +             * The black level from camHelper_ is a 16 bit value, software ISP\n>>>> +             * works with 8 bit pixel values, both regardless of the actual\n>>>> +             * sensor pixel width. Hence we obtain the pixel-based black value\n>>>> +             * by dividing the value from the helper by 256.\n>>>> +             */\n>>>> +            context_.configuration.black.level =\n>>>> +                camHelper_->blackLevel().value() / 256;\n>> Style nit: braces around this block would be good now that there's the comment.\n> OK.\n>\n>> More importantly though: Assuming setting a value for blackLevel_ without providing\n>> values for gain makes sense, then it would be great to adapt the code above to not\n>> assume so. It currently only works in builds with asserts disabled - which I used\n>> yesterday when trying it. There are two more similar places in processStats()\n>> (camHelper_ ? ...).\n>>\n>> Forcing the non-cam-helper code for gain seems to fix the regression I mentioned in the\n>> previous mail.\n> I'm not sure I understand what you mean exactly. What the patch does is\n> that if a camera sensor helper is available for the given sensor and it\n> provides black level then that black level is used; otherwise black\n> level auto-detection is used (as before). If there is no helper for the\n> given sensor, nothing changes. Do you mean there is some trouble in the\n> current code in such a case (it seems to work fine for me)? Or do you\n> talk about a situation when the helper provides black level but not\n> the corresponding gains? And what assertion fails?\n\nSorry for being unclear here. There is a problem with the existing code \nthat IMO should get addressed before adding the new black level related \ncode.\n\nThat is the `if (camHelper_)` block here and two `camHelper_ ? ...` \ncases further below where it is assumed that the presence of \n`camHelper_` allows `camHelper_->gain()` and `camHelper_->gainCode()` to \nsucceed.\n\nIn order to test the new black level code added in this series, I added \nthe following camera helpers:\n\n> +class CameraSensorHelperImx355 : public CameraSensorHelper\n> +{\n> +public:\n> +       CameraSensorHelperImx355()\n> +       {\n> +               blackLevel_ = 4096;\n> +       }\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"imx355\", CameraSensorHelperImx355)\n> +\n> +class CameraSensorHelperImx363 : public CameraSensorHelper\n> +{\n> +public:\n> +       CameraSensorHelperImx363()\n> +       {\n> +               blackLevel_ = 4096;\n> +       }\n> +};\n> +REGISTER_CAMERA_SENSOR_HELPER(\"imx363\", CameraSensorHelperImx363)\n\nWith those, `camHelper_` is set and the pre-existing code \nunconditionally calls `camHelper_->gain()`. When assertions are on, one \nis triggered / playback is broken. When assertions are disabled, \nplayback works but the output images are much darker, as described \nabove. That can be fixed by force-disabling the calls to \n`camHelper_->gain()` and using the `camHelper_` code instead.\n\nThis is not strictly related to this series and could be fixed \nindependently, but I guess it makes sense to fix things in an additional \ncommit here.\n\nAFAICS something similar to the new `if \n(camHelper_->blackLevel().has_value())` should be added for gain.\n\n>\n>>>>       } else {\n>>>>           /*\n>>>>            * The camera sensor gain (g) is usually not equal to the value written","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 D8CB6C326C\n\tfor ;\n\tMon, 14 Oct 2024 11:34:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A7FA6537E;\n\tMon, 14 Oct 2024 13:34:37 +0200 (CEST)","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 4E9B265371\n\tfor ;\n\tMon, 14 Oct 2024 13:34:35 +0200 (CEST)","by mx.zohomail.com with SMTPS id 1728905667509248.92720774676025; \n\tMon, 14 Oct 2024 04:34:27 -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=\"OCWkK7Am\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1728905668; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=lQXAhgBhQhbD5n3+pwyq7baQbpKD/ZJ8lr49h6U6BNgkRapukIDRdA5QJXxV9o991FR3UOMzOKnm5H4wcScRywqsT7PcG6et0e9h+M+yPFO5AFKrXFRP0HEEt+g+Y25Cu1P2omkGFhNesEkPhvAfdAnJ7HWJpFyYfidhVSRAkBE=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1728905668;\n\th=Content-Type:Content-Transfer-Encoding:Cc:Cc:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To;\n\tbh=cudO7ZmaVm0ClkNc3BnxvZV8k4nfWaaaxUU0NIkly2A=; \n\tb=HaEKXfeRbuZvVEX1cDvJUbrnRcnRlc9pl4JZ0lFhC34Ten+jDn6qxMaxL96q1a0QTvMrInM+d5iRPyzZoOQWDUXCX/Hxp4k6clu2TtCFoeEOzLbP09yNUVFg/7jlndSBHBJT1tzCKgLfOxRJJntY9dknaNMkxNhNOvn95oDFk4Q=","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=1728905668;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:Cc:Cc:References:From:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Message-Id:Reply-To;\n\tbh=cudO7ZmaVm0ClkNc3BnxvZV8k4nfWaaaxUU0NIkly2A=;\n\tb=OCWkK7Amm2bXW5lQXGIt0xfnGxqo+CwTxtADdMQg7/WlY461BezbtmKHdxhtyPP+\n\tWfD+4Gez6j5Q7FmIP5AtnctMHYojXY5MYN1KIBvTs3vdkUlSYs4pCy0R4ofX6poYUyR\n\tQfxhxkGlTO2SX/dBr4CjkQWd2QI1EeFSURPi8V9c=","Message-ID":"","Date":"Mon, 14 Oct 2024 13:34:24 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 1/1] libcamera: software_isp: Get black level from the\n\tcamera helper","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20241011153934.1291362-1-mzamazal@redhat.com>\n\t<20241011153934.1291362-2-mzamazal@redhat.com>\n\t\n\t<5e7a83ab-b917-4c2e-8891-d06fb896dc6c@collabora.com>\n\t<87ed4jgcy1.fsf@redhat.com>","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"<87ed4jgcy1.fsf@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":31737,"web_url":"https://patchwork.libcamera.org/comment/31737/","msgid":"<87a5f6h5yc.fsf@redhat.com>","date":"2024-10-14T18:53:15","subject":"Re: [PATCH v3 1/1] libcamera: software_isp: Get black level from\n\tthe camera helper","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Robert Mader writes:\n\n> On 14.10.24 13:07, Milan Zamazal wrote:\n>> Hi Robert,\n>>\n>\n>> thank you for testing and review.\n>>\n>> Robert Mader writes:\n>>\n>>> Turns out I have to ask for/suggest some more changes:\n>>>\n>>> On 12.10.24 21:49, Robert Mader wrote:\n>>>> I gave this a try on a Pixel 3a (IMX 355 and IMX 363). In both cases using 4096 made\n>>>> the resulting image fully black when covering the sensor, which IIUC is the desired\n>>>> effect.\n>> Yes.\n>>\n>>>> Not putting any values or using lower ones such as 3200 results in a\n>>>> greenish image - so there's definitely an improvement here. Thus fell\n>>>> free to add a Tested-by: Robert Mader \n>>>>\n>>>> At least in the case of the IMX355 the output overall becomes much darker - too dark\n>>>> IMO. Is that expected - and can/will it be compensated by improvements of the AWB\n>>>> algorithm?\n>> No. Blacks and shadows should be darker but overall the image should\n>> have about the same brightness. This is handled by auto-exposure, which\n>> is applied after subtracting the black level.\n> Indeed, and I can confirm that this works correctly once the issue below is addressed.\n>>>> P.S.: I guess I can submit a follow-up adding the black level for the IMX355 - the\n>>>> IMX363 driver hasn't been submitted to upstream yet.\n>>>>\n>>>> On 11.10.24 17:39, Milan Zamazal wrote:\n>>>>> The black level in software ISP is unconditionally guessed from the\n>>>>> obtained frames.  CameraSensorHelper optionally provides the black level\n>>>>> from camera specifications now.  Let's use the value if available.\n>>>>>\n>>>>> If the black level is not available from the given CameraSensorHelper\n>>>>> instance, it's still determined on the fly.\n>>>>>\n>>>>> Signed-off-by: Milan Zamazal \n>>>>> Reviewed-by: Kieran Bingham \n>>>>> ---\n>>>>>   src/ipa/simple/algorithms/blc.cpp | 6 +++++-\n>>>>>   src/ipa/simple/ipa_context.h      | 4 ++++\n>>>>>   src/ipa/simple/soft_simple.cpp    | 9 +++++++++\n>>>>>   3 files changed, 18 insertions(+), 1 deletion(-)\n>>>>>\n>>>>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n>>>>> index b9f2aaa6d..a7af2e12c 100644\n>>>>> --- a/src/ipa/simple/algorithms/blc.cpp\n>>>>> +++ b/src/ipa/simple/algorithms/blc.cpp\n>>>>> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()\n>>>>>   int BlackLevel::configure(IPAContext &context,\n>>>>>                 [[maybe_unused]] const IPAConfigInfo &configInfo)\n>>>>>   {\n>>>>> -    context.activeState.blc.level = 255;\n>>>>> +    context.activeState.blc.level =\n>>>>> +        context.configuration.black.level.value_or(255);\n>>>>>       return 0;\n>>>>>   }\n>>>>>   @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context,\n>>>>>                const SwIspStats *stats,\n>>>>>                [[maybe_unused]] ControlList &metadata)\n>>>>>   {\n>>>>> +    if (context.configuration.black.level.has_value())\n>>>>> +        return;\n>>>>> +\n>>>>>       if (frameContext.sensor.exposure == exposure_ &&\n>>>>>           frameContext.sensor.gain == gain_) {\n>>>>>           return;\n>>>>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>>>>> index 3519f20f6..fd121eebe 100644\n>>>>> --- a/src/ipa/simple/ipa_context.h\n>>>>> +++ b/src/ipa/simple/ipa_context.h\n>>>>> @@ -8,6 +8,7 @@\n>>>>>   #pragma once\n>>>>>     #include \n>>>>> +#include \n>>>>>   #include \n>>>>>     #include \n>>>>> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {\n>>>>>           int32_t exposureMin, exposureMax;\n>>>>>           double againMin, againMax, againMinStep;\n>>>>>       } agc;\n>>>>> +    struct {\n>>>>> +        std::optional level;\n>>>>> +    } black;\n>>>>>   };\n>>>>>     struct IPAActiveState {\n>>>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>>>>> index b28c7039f..079409f6d 100644\n>>>>> --- a/src/ipa/simple/soft_simple.cpp\n>>>>> +++ b/src/ipa/simple/soft_simple.cpp\n>>>>> @@ -201,6 +201,15 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n>>>>>               (context_.configuration.agc.againMax -\n>>>>>                context_.configuration.agc.againMin) /\n>>>>>               100.0;\n>>>>> +        if (camHelper_->blackLevel().has_value())\n>>>>> +            /*\n>>>>> +             * The black level from camHelper_ is a 16 bit value, software ISP\n>>>>> +             * works with 8 bit pixel values, both regardless of the actual\n>>>>> +             * sensor pixel width. Hence we obtain the pixel-based black value\n>>>>> +             * by dividing the value from the helper by 256.\n>>>>> +             */\n>>>>> +            context_.configuration.black.level =\n>>>>> +                camHelper_->blackLevel().value() / 256;\n>>> Style nit: braces around this block would be good now that there's the comment.\n>> OK.\n>>\n>>> More importantly though: Assuming setting a value for blackLevel_ without providing\n>>> values for gain makes sense, then it would be great to adapt the code above to not\n>>> assume so. It currently only works in builds with asserts disabled - which I used\n>>> yesterday when trying it. There are two more similar places in processStats()\n>>> (camHelper_ ? ...).\n>>>\n>>> Forcing the non-cam-helper code for gain seems to fix the regression I mentioned in the\n>>> previous mail.\n>> I'm not sure I understand what you mean exactly. What the patch does is\n>> that if a camera sensor helper is available for the given sensor and it\n>> provides black level then that black level is used; otherwise black\n>> level auto-detection is used (as before). If there is no helper for the\n>> given sensor, nothing changes. Do you mean there is some trouble in the\n>> current code in such a case (it seems to work fine for me)? Or do you\n>> talk about a situation when the helper provides black level but not\n>> the corresponding gains? And what assertion fails?\n>\n> Sorry for being unclear here. There is a problem with the existing code that IMO should get addressed\n> before adding the new black level related code.\n>\n> That is the `if (camHelper_)` block here and two `camHelper_ ? ...` cases further below where it is\n> assumed that the presence of `camHelper_` allows `camHelper_->gain()` and `camHelper_->gainCode()` to\n> succeed.\n>\n> In order to test the new black level code added in this series, I added the following camera helpers:\n>\n>> +class CameraSensorHelperImx355 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +       CameraSensorHelperImx355()\n>> +       {\n>> +               blackLevel_ = 4096;\n>> +       }\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx355\", CameraSensorHelperImx355)\n>> +\n>> +class CameraSensorHelperImx363 : public CameraSensorHelper\n>> +{\n>> +public:\n>> +       CameraSensorHelperImx363()\n>> +       {\n>> +               blackLevel_ = 4096;\n>> +       }\n>> +};\n>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx363\", CameraSensorHelperImx363)\n>\n> With those, `camHelper_` is set and the pre-existing code unconditionally calls\n> `camHelper_->gain()`. When assertions are on, one is triggered / playback is broken. When assertions are\n> disabled, playback works but the output images are much darker, as described above. That can be fixed by\n> force-disabling the calls to `camHelper_->gain()` and using the `camHelper_` code instead.\n>\n> This is not strictly related to this series and could be fixed independently, but I guess it makes sense\n> to fix things in an additional commit here.\n>\n> AFAICS something similar to the new `if (camHelper_->blackLevel().has_value())` should be added for gain.\n\nI see, thank you for explanation. I can reproduce the problem with dark\noutput. It happens with or without this patch. I don't get any\nassertion error and I wouldn't expect one -- if you mean the assertions\nin CameraSensorHelper::gain*, they pass with the default values. But\n0/0 is returned, which is of course a wrong value.\n\nThis also means that it is not expected that the helper doesn't define\ngains (no surprise considering handling the gain constants was its only\npurpose originally). So there is a question whether a camera sensor\nhelper that doesn't define the gains is a valid helper. I'd say why not\nbut other pipelines would have problems with such a helper too. If we\nwant to support this then it'll require more changes.\n\nAnyway, although it's an interesting issue, it has nothing to do with\nthis patch. If you think there is a valid reason to define camera\nsensor helpers without defining gains, I'd suggest opening a separate\nthread about it or to file a bug.\n\n>>>>>       } else {\n>>>>>           /*\n>>>>>            * The camera sensor gain (g) is usually not equal to the value written","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 93A39C32F4\n\tfor ;\n\tMon, 14 Oct 2024 18:53:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7129A6537E;\n\tMon, 14 Oct 2024 20:53:24 +0200 (CEST)","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 5EC1363525\n\tfor ;\n\tMon, 14 Oct 2024 20:53:22 +0200 (CEST)","from mail-wm1-f70.google.com (mail-wm1-f70.google.com\n\t[209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-84-5ihg7DYdMCi6LeoGcFVqFw-1; Mon, 14 Oct 2024 14:53:20 -0400","by mail-wm1-f70.google.com with SMTP id\n\t5b1f17b1804b1-4313a8e81fdso740075e9.3\n\tfor ;\n\tMon, 14 Oct 2024 11:53:19 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-37d4b79fa21sm12084616f8f.85.2024.10.14.11.53.16\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 14 Oct 2024 11:53:16 -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=\"CbfKMF+n\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1728932001;\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\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=C4Mc43Bd9OeaeFnpWO9M86jbE4GtidvNAd0+f1gfsBo=;\n\tb=CbfKMF+nZ7dkIyeJkoVyb0CTYjrTZIjrjGN8dNoVU/d6ae1CyUl/X/NaCDtY5WfOLnU3WL\n\thImPPGjX+RL2Dyn42/pKO6kchhCO6CvHhoDn4PY6lORc4EO6cmVFpvEHFF/y3uStCkiNns\n\tm0LsWOiydKvBKzh2vKoGj9WsCMbs3Uk=","X-MC-Unique":"5ihg7DYdMCi6LeoGcFVqFw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728931998; x=1729536798;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=Z7ebhgwaDSnbut/Ue3jgQzquwhHHhcr0iPHrOZe4mVY=;\n\tb=CcnecjRg+84Ngi9OP8c7coiLVz2N4e4l3LPuwu1D6/mMWk6k9HBT45AJzcEPaRc4bI\n\tZzi/+eue4AzjhV3zJSi/PKpbGLpnYQ/gXaK6bt5TfGSHpyhMLaDcb4qXCZCreub89pId\n\tr39dloEJ3l3SHfh/9XgmK8DSD50u1p+xaFG1i5bO1YD6xkcnIk6NC5tqj0zwwQpbAtRl\n\t1xZzQ1r9WliC56D1sVCwKlzOWESGNSTRb/A2xTEbl8A8xH96+dwwZe8vgQO1HDcDEjso\n\tu3YGlFVA92DbEwM1d5coGhFGgUR2jpVTr43kaA1oURvV9BptDHLKPJas8sCj3H6sWfIe\n\tsmaw==","X-Gm-Message-State":"AOJu0YyVe9xFwGm4zpcEz8xX4zISwbltbM3JeLUh0C+tSngA5gKfnD5i\n\tSKQOII4UB14J7n8jEjCefMeLCBjNOmRcxSC9EOzZupxxmBO8KmK/gOeH37PrXQPVccCzdaWnVk1\n\tKPDhtAWLOeLU2C4bjCaQZChhP5qK/76J/WCOezr7mlKaNypl4iHIBZyAjodftKl/2JZlZAGK7pL\n\t0cFJtHCG/ZIqN6zI0L/EiDAZ0yfrFssA7/+z4p9NUFkzMOOsk5oyLBe5w=","X-Received":["by 2002:a05:600c:81b:b0:42c:ad30:678 with SMTP id\n\t5b1f17b1804b1-4312560907fmr86396375e9.28.1728931998293; \n\tMon, 14 Oct 2024 11:53:18 -0700 (PDT)","by 2002:a05:600c:81b:b0:42c:ad30:678 with SMTP id\n\t5b1f17b1804b1-4312560907fmr86396165e9.28.1728931997662; \n\tMon, 14 Oct 2024 11:53:17 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IH0QjdNZJVBjZLVJDkarIKKwCojb0Ih9doFF5RqEqEdCH9ImLccjWUFyUalyVbK7xHc0BetCQ==","From":"Milan Zamazal ","To":"Robert Mader ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 1/1] libcamera: software_isp: Get black level from\n\tthe camera helper","In-Reply-To":" (Robert\n\tMader's message of \"Mon, 14 Oct 2024 13:34:24 +0200\")","References":"<20241011153934.1291362-1-mzamazal@redhat.com>\n\t<20241011153934.1291362-2-mzamazal@redhat.com>\n\t\n\t<5e7a83ab-b917-4c2e-8891-d06fb896dc6c@collabora.com>\n\t<87ed4jgcy1.fsf@redhat.com>\n\t","Date":"Mon, 14 Oct 2024 20:53:15 +0200","Message-ID":"<87a5f6h5yc.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":31741,"web_url":"https://patchwork.libcamera.org/comment/31741/","msgid":"<8b8562f5-3251-4a93-a9be-82f5bc787b41@collabora.com>","date":"2024-10-15T10:04:39","subject":"Re: [PATCH v3 1/1] libcamera: software_isp: Get black level from the\n\tcamera helper","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"On 14.10.24 20:53, Milan Zamazal wrote:\n> Robert Mader writes:\n>\n>> On 14.10.24 13:07, Milan Zamazal wrote:\n>>> Hi Robert,\n>>>\n>>> thank you for testing and review.\n>>>\n>>> Robert Mader writes:\n>>>\n>>>> Turns out I have to ask for/suggest some more changes:\n>>>>\n>>>> On 12.10.24 21:49, Robert Mader wrote:\n>>>>> I gave this a try on a Pixel 3a (IMX 355 and IMX 363). In both cases using 4096 made\n>>>>> the resulting image fully black when covering the sensor, which IIUC is the desired\n>>>>> effect.\n>>> Yes.\n>>>\n>>>>> Not putting any values or using lower ones such as 3200 results in a\n>>>>> greenish image - so there's definitely an improvement here. Thus fell\n>>>>> free to add a Tested-by: Robert Mader \n>>>>>\n>>>>> At least in the case of the IMX355 the output overall becomes much darker - too dark\n>>>>> IMO. Is that expected - and can/will it be compensated by improvements of the AWB\n>>>>> algorithm?\n>>> No. Blacks and shadows should be darker but overall the image should\n>>> have about the same brightness. This is handled by auto-exposure, which\n>>> is applied after subtracting the black level.\n>> Indeed, and I can confirm that this works correctly once the issue below is addressed.\n>>>>> P.S.: I guess I can submit a follow-up adding the black level for the IMX355 - the\n>>>>> IMX363 driver hasn't been submitted to upstream yet.\n>>>>>\n>>>>> On 11.10.24 17:39, Milan Zamazal wrote:\n>>>>>> The black level in software ISP is unconditionally guessed from the\n>>>>>> obtained frames.  CameraSensorHelper optionally provides the black level\n>>>>>> from camera specifications now.  Let's use the value if available.\n>>>>>>\n>>>>>> If the black level is not available from the given CameraSensorHelper\n>>>>>> instance, it's still determined on the fly.\n>>>>>>\n>>>>>> Signed-off-by: Milan Zamazal \n>>>>>> Reviewed-by: Kieran Bingham \n>>>>>> ---\n>>>>>>   src/ipa/simple/algorithms/blc.cpp | 6 +++++-\n>>>>>>   src/ipa/simple/ipa_context.h      | 4 ++++\n>>>>>>   src/ipa/simple/soft_simple.cpp    | 9 +++++++++\n>>>>>>   3 files changed, 18 insertions(+), 1 deletion(-)\n>>>>>>\n>>>>>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp\n>>>>>> index b9f2aaa6d..a7af2e12c 100644\n>>>>>> --- a/src/ipa/simple/algorithms/blc.cpp\n>>>>>> +++ b/src/ipa/simple/algorithms/blc.cpp\n>>>>>> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()\n>>>>>>   int BlackLevel::configure(IPAContext &context,\n>>>>>>                 [[maybe_unused]] const IPAConfigInfo &configInfo)\n>>>>>>   {\n>>>>>> -    context.activeState.blc.level = 255;\n>>>>>> +    context.activeState.blc.level =\n>>>>>> +        context.configuration.black.level.value_or(255);\n>>>>>>       return 0;\n>>>>>>   }\n>>>>>>   @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context,\n>>>>>>                const SwIspStats *stats,\n>>>>>>                [[maybe_unused]] ControlList &metadata)\n>>>>>>   {\n>>>>>> +    if (context.configuration.black.level.has_value())\n>>>>>> +        return;\n>>>>>> +\n>>>>>>       if (frameContext.sensor.exposure == exposure_ &&\n>>>>>>           frameContext.sensor.gain == gain_) {\n>>>>>>           return;\n>>>>>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n>>>>>> index 3519f20f6..fd121eebe 100644\n>>>>>> --- a/src/ipa/simple/ipa_context.h\n>>>>>> +++ b/src/ipa/simple/ipa_context.h\n>>>>>> @@ -8,6 +8,7 @@\n>>>>>>   #pragma once\n>>>>>>     #include \n>>>>>> +#include \n>>>>>>   #include \n>>>>>>     #include \n>>>>>> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {\n>>>>>>           int32_t exposureMin, exposureMax;\n>>>>>>           double againMin, againMax, againMinStep;\n>>>>>>       } agc;\n>>>>>> +    struct {\n>>>>>> +        std::optional level;\n>>>>>> +    } black;\n>>>>>>   };\n>>>>>>     struct IPAActiveState {\n>>>>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>>>>>> index b28c7039f..079409f6d 100644\n>>>>>> --- a/src/ipa/simple/soft_simple.cpp\n>>>>>> +++ b/src/ipa/simple/soft_simple.cpp\n>>>>>> @@ -201,6 +201,15 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n>>>>>>               (context_.configuration.agc.againMax -\n>>>>>>                context_.configuration.agc.againMin) /\n>>>>>>               100.0;\n>>>>>> +        if (camHelper_->blackLevel().has_value())\n>>>>>> +            /*\n>>>>>> +             * The black level from camHelper_ is a 16 bit value, software ISP\n>>>>>> +             * works with 8 bit pixel values, both regardless of the actual\n>>>>>> +             * sensor pixel width. Hence we obtain the pixel-based black value\n>>>>>> +             * by dividing the value from the helper by 256.\n>>>>>> +             */\n>>>>>> +            context_.configuration.black.level =\n>>>>>> +                camHelper_->blackLevel().value() / 256;\n>>>> Style nit: braces around this block would be good now that there's the comment.\n>>> OK.\n>>>\n>>>> More importantly though: Assuming setting a value for blackLevel_ without providing\n>>>> values for gain makes sense, then it would be great to adapt the code above to not\n>>>> assume so. It currently only works in builds with asserts disabled - which I used\n>>>> yesterday when trying it. There are two more similar places in processStats()\n>>>> (camHelper_ ? ...).\n>>>>\n>>>> Forcing the non-cam-helper code for gain seems to fix the regression I mentioned in the\n>>>> previous mail.\n>>> I'm not sure I understand what you mean exactly. What the patch does is\n>>> that if a camera sensor helper is available for the given sensor and it\n>>> provides black level then that black level is used; otherwise black\n>>> level auto-detection is used (as before). If there is no helper for the\n>>> given sensor, nothing changes. Do you mean there is some trouble in the\n>>> current code in such a case (it seems to work fine for me)? Or do you\n>>> talk about a situation when the helper provides black level but not\n>>> the corresponding gains? And what assertion fails?\n>> Sorry for being unclear here. There is a problem with the existing code that IMO should get addressed\n>> before adding the new black level related code.\n>>\n>> That is the `if (camHelper_)` block here and two `camHelper_ ? ...` cases further below where it is\n>> assumed that the presence of `camHelper_` allows `camHelper_->gain()` and `camHelper_->gainCode()` to\n>> succeed.\n>>\n>> In order to test the new black level code added in this series, I added the following camera helpers:\n>>\n>>> +class CameraSensorHelperImx355 : public CameraSensorHelper\n>>> +{\n>>> +public:\n>>> +       CameraSensorHelperImx355()\n>>> +       {\n>>> +               blackLevel_ = 4096;\n>>> +       }\n>>> +};\n>>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx355\", CameraSensorHelperImx355)\n>>> +\n>>> +class CameraSensorHelperImx363 : public CameraSensorHelper\n>>> +{\n>>> +public:\n>>> +       CameraSensorHelperImx363()\n>>> +       {\n>>> +               blackLevel_ = 4096;\n>>> +       }\n>>> +};\n>>> +REGISTER_CAMERA_SENSOR_HELPER(\"imx363\", CameraSensorHelperImx363)\n>> With those, `camHelper_` is set and the pre-existing code unconditionally calls\n>> `camHelper_->gain()`. When assertions are on, one is triggered / playback is broken. When assertions are\n>> disabled, playback works but the output images are much darker, as described above. That can be fixed by\n>> force-disabling the calls to `camHelper_->gain()` and using the `camHelper_` code instead.\n>>\n>> This is not strictly related to this series and could be fixed independently, but I guess it makes sense\n>> to fix things in an additional commit here.\n>>\n>> AFAICS something similar to the new `if (camHelper_->blackLevel().has_value())` should be added for gain.\n> I see, thank you for explanation. I can reproduce the problem with dark\n> output. It happens with or without this patch. I don't get any\n> assertion error and I wouldn't expect one -- if you mean the assertions\n> in CameraSensorHelper::gain*, they pass with the default values. But\n> 0/0 is returned, which is of course a wrong value.\n>\n> This also means that it is not expected that the helper doesn't define\n> gains (no surprise considering handling the gain constants was its only\n> purpose originally). So there is a question whether a camera sensor\n> helper that doesn't define the gains is a valid helper. I'd say why not\n> but other pipelines would have problems with such a helper too. If we\n> want to support this then it'll require more changes.\n>\n> Anyway, although it's an interesting issue, it has nothing to do with\n> this patch. If you think there is a valid reason to define camera\n> sensor helpers without defining gains, I'd suggest opening a separate\n> thread about it or to file a bug.\nAgreed, will file an independent issue or mail.\n>\n>>>>>>       } else {\n>>>>>>           /*\n>>>>>>            * The camera sensor gain (g) is usually not equal to the value written","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 77A69C0F2A\n\tfor ;\n\tTue, 15 Oct 2024 10:04:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E69263525;\n\tTue, 15 Oct 2024 12:04:49 +0200 (CEST)","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 5AE1360537\n\tfor ;\n\tTue, 15 Oct 2024 12:04:47 +0200 (CEST)","by mx.zohomail.com with SMTPS id 1728986681850444.07557653740423; \n\tTue, 15 Oct 2024 03:04:41 -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=\"GkW5PK2k\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1728986684; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=e/fyGtEjudaAOUo6I+Wj6UnzY2bkEJcI9q/x2/IVGfh+L4OnRuw7HCJElOx9R17VKnFPTqxzUEuAobBGUmUC16Gl0N3stIMAVg+DIyCZtCOktiqB06mrfsqhr1Odi6K2vmO56+9v6tXA0z1bDHErLWSSSLwMiocQ2ZeE6foR5Bk=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1728986684;\n\th=Content-Type:Content-Transfer-Encoding:Cc:Cc:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To;\n\tbh=XzMrKZWwaBoU8apSg3+Tvor1MkqP0rAUENPLOfA2px0=; \n\tb=kccBtd0wYs45nnjnqjt413vg2F3rwrk5auMJ+dcZTLWWV41K7tYxutf/ZFxBW+GblLeYJbMKtPL0f9wx2rWe0SBBGp+MZhsokGEK9LkbDZXxCsDPpeu7FEUjPtgXcWmL4SbYq5RhLk8JVpP/cpx0li5MgSaHEgJ3wTXXXXJ3/8w=","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=1728986684;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:Cc:Cc:References:From:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Message-Id:Reply-To;\n\tbh=XzMrKZWwaBoU8apSg3+Tvor1MkqP0rAUENPLOfA2px0=;\n\tb=GkW5PK2kY+mGITFzNLHjdrEbX0jLme4Yy+u/EnH5tsyaQwLkfVAzdNIK6tQaRRil\n\tEXxijDBkK6526BocCFcTeIK+OKd0pAo1Aj5Py6nSxTZ6zKpGHZW0B5K+trvscvrXe/T\n\tLaWKGYXf4FU2s8MZj5QSMZmTyBcmSrDM1PoNZj14=","Message-ID":"<8b8562f5-3251-4a93-a9be-82f5bc787b41@collabora.com>","Date":"Tue, 15 Oct 2024 12:04:39 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 1/1] libcamera: software_isp: Get black level from the\n\tcamera helper","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20241011153934.1291362-1-mzamazal@redhat.com>\n\t<20241011153934.1291362-2-mzamazal@redhat.com>\n\t\n\t<5e7a83ab-b917-4c2e-8891-d06fb896dc6c@collabora.com>\n\t<87ed4jgcy1.fsf@redhat.com>\n\t\n\t<87a5f6h5yc.fsf@redhat.com>","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"<87a5f6h5yc.fsf@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]