[{"id":29358,"web_url":"https://patchwork.libcamera.org/comment/29358/","msgid":"<20240428231120.GA4950@pendragon.ideasonboard.com>","date":"2024-04-28T23:11:20","subject":"Re: [PATCH 4/5] libcamera: software_isp: Remove TODO #13","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nThank you for the patch.\n\nOn Tue, Apr 23, 2024 at 08:19:59PM +0200, Milan Zamazal wrote:\n> The current software ISP debayering implementation uses color lookup\n> tables that apply black level subtraction, color gain and gamma\n> correction in a single step, while performing debayering. In theory,\n> black level subtraction and color gains should be applied before\n> debayering and gamma correction after debayering. But because black\n> level subtraction and color gain corrections are currently linear and we\n> average only same-color pixels in debayering, we can apply all the\n> operations after debayering. The only difference is with clipping where\n> out-of-range pixels contribute more than they deserve but this is not\n> generally significant.\n> \n> Combining all the operations at once is conceptually not ideal but it is\n> not incorrect in this case and saves CPU cycles, which is critical for\n> software ISP CPU implementation (it may be less important with future\n> GPU implementation). This means we need the current implementation. It\n> may get replaced or become optional (configurable) in future, for\n> example if the separation is needed due to introducing other image\n> processing operations.\n> \n> Under these circumstances and considering that the lookup tables\n> construction has been moved out of the debayering code in the preceding\n> patch, it makes no sense to keep software ISP TODO #13. Let's remove it\n> in favor of a clarifying code comment.\n\nI still think we need to split those operations. The black level\nsubtraction (coming before debayering and the colour gains) should then\nuse black level values from a tuning file. An additional contrast\nenhancement after debayering can perform automatic histogram stretching\nif desired.\n\nAs this is still being discussed, and experimentation is needed to\nassess the impact, I think the TODO item should stay until we reach a\nconclusion.\n\n> Signed-off-by: Milan Zamazal \n> ---\n> src/ipa/simple/soft_simple.cpp | 10 ++++++++++\n> src/libcamera/software_isp/TODO | 10 ----------\n> 2 files changed, 10 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index c5085f71..b2f4d308 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -247,6 +247,16 @@ void IPASoftSimple::stop()\n> \n> void IPASoftSimple::processStats(const ControlList &sensorControls)\n> {\n> +\t/*\n> +\t * Here black level subtraction, color gains and gamma correction are\n> +\t * combined into a single operation (table lookup) to save CPU cycles.\n> +\t * This works because black level subtraction and color gains are currently\n> +\t * linear operations and we average only same-color pixels in debayering.\n> +\t * If we change anything on this or introduce other operations in between,\n> +\t * it may be needed to split the operations and perform black and gain\n> +\t * corrections before debayering and gamma correction after debayering.\n> +\t */\n> +\n> \tSwIspStats::Histogram histogram = stats_->yHistogram;\n> \tif (ignoreUpdates_ > 0)\n> \t\tblackLevel_.update(histogram);\n> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO\n> index 4fcee39b..fcb02588 100644\n> --- a/src/libcamera/software_isp/TODO\n> +++ b/src/libcamera/software_isp/TODO\n> @@ -267,13 +267,3 @@ This could be handled better with DelayedControls.\n> > \t\t\t\t\t\t V4L2_CID_EXPOSURE }));\n> \n> You should use the DelayedControls class.\n> -\n> ----\n> -\n> -13. Improve black level and colour gains application\n> -\n> -I think the black level should eventually be moved before debayering, and\n> -ideally the colour gains as well. I understand the need for optimizations to\n> -lower the CPU consumption, but at the same time I don't feel comfortable\n> -building up on top of an implementation that may work a bit more by chance than\n> -by correctness, as that's not very maintainable.","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 C05FAC3220\n\tfor ;\n\tSun, 28 Apr 2024 23:11:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2015F63418;\n\tMon, 29 Apr 2024 01:11:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBBD761A90\n\tfor ;\n\tMon, 29 Apr 2024 01:11:27 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [109.130.69.237])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 771F8720;\n\tMon, 29 Apr 2024 01:10:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QGyDB2pt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1714345832;\n\tbh=KUp9+pOrJgvT3lw4dWpY3LH398kbjDcRKvDHywrmKQ8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QGyDB2ptRbYzvpsU+y8KAcDtxKCy5iRo3DS5Nfd5cs1cgA4z8kDcYgTTy/JEF433p\n\tEpscPWZwld4tbM+OiuHoPEvjPHaadJOc19zsf6L2NUsw/X3qi3phsS2PYLA5DfBB0S\n\t758NnTDlTQIS+De39EzkYtaxLya2rcYAS9S9syIU=","Date":"Mon, 29 Apr 2024 02:11:20 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 4/5] libcamera: software_isp: Remove TODO #13","Message-ID":"<20240428231120.GA4950@pendragon.ideasonboard.com>","References":"<20240423182000.1527425-1-mzamazal@redhat.com>\n\t<20240423182000.1527425-5-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240423182000.1527425-5-mzamazal@redhat.com>","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":29362,"web_url":"https://patchwork.libcamera.org/comment/29362/","msgid":"<87bk5szg1i.fsf@redhat.com>","date":"2024-04-29T09:14:49","subject":"Re: [PATCH 4/5] libcamera: software_isp: Remove TODO #13","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n\nHi Laurent,\n\nthank you for your comments.\n\n> On Tue, Apr 23, 2024 at 08:19:59PM +0200, Milan Zamazal wrote:\n>> The current software ISP debayering implementation uses color lookup\n>> tables that apply black level subtraction, color gain and gamma\n>> correction in a single step, while performing debayering. In theory,\n>> black level subtraction and color gains should be applied before\n>> debayering and gamma correction after debayering. But because black\n>> level subtraction and color gain corrections are currently linear and we\n>> average only same-color pixels in debayering, we can apply all the\n>> operations after debayering. The only difference is with clipping where\n>> out-of-range pixels contribute more than they deserve but this is not\n>> generally significant.\n>> \n>> Combining all the operations at once is conceptually not ideal but it is\n>> not incorrect in this case and saves CPU cycles, which is critical for\n>> software ISP CPU implementation (it may be less important with future\n>> GPU implementation). This means we need the current implementation. It\n>> may get replaced or become optional (configurable) in future, for\n>> example if the separation is needed due to introducing other image\n>> processing operations.\n>> \n>> Under these circumstances and considering that the lookup tables\n>> construction has been moved out of the debayering code in the preceding\n>> patch, it makes no sense to keep software ISP TODO #13. Let's remove it\n>> in favor of a clarifying code comment.\n>\n> I still think we need to split those operations. The black level\n> subtraction (coming before debayering and the colour gains) should then\n> use black level values from a tuning file. An additional contrast\n> enhancement after debayering can perform automatic histogram stretching\n> if desired.\n>\n> As this is still being discussed, and experimentation is needed to\n> assess the impact, I think the TODO item should stay until we reach a\n> conclusion.\n\nOK but let's keep it actionable -- IMHO the TODO file should be\ntemporary and the items should be either resolved soon or transformed\ninto something else, such as in-code TODOs.\n\nRegards,\nMilan\n\n>> Signed-off-by: Milan Zamazal \n>> ---\n>> src/ipa/simple/soft_simple.cpp | 10 ++++++++++\n>> src/libcamera/software_isp/TODO | 10 ----------\n>> 2 files changed, 10 insertions(+), 10 deletions(-)\n>> \n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> index c5085f71..b2f4d308 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -247,6 +247,16 @@ void IPASoftSimple::stop()\n>> \n>> void IPASoftSimple::processStats(const ControlList &sensorControls)\n>> {\n>> +\t/*\n>> +\t * Here black level subtraction, color gains and gamma correction are\n>> +\t * combined into a single operation (table lookup) to save CPU cycles.\n>> +\t * This works because black level subtraction and color gains are currently\n>> +\t * linear operations and we average only same-color pixels in debayering.\n>> +\t * If we change anything on this or introduce other operations in between,\n>> +\t * it may be needed to split the operations and perform black and gain\n>> +\t * corrections before debayering and gamma correction after debayering.\n>> +\t */\n>> +\n>> \tSwIspStats::Histogram histogram = stats_->yHistogram;\n>> \tif (ignoreUpdates_ > 0)\n>> \t\tblackLevel_.update(histogram);\n>> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO\n>> index 4fcee39b..fcb02588 100644\n>> --- a/src/libcamera/software_isp/TODO\n>> +++ b/src/libcamera/software_isp/TODO\n>> @@ -267,13 +267,3 @@ This could be handled better with DelayedControls.\n>> > \t\t\t\t\t\t V4L2_CID_EXPOSURE }));\n>> \n>> You should use the DelayedControls class.\n>> -\n>> ----\n>> -\n>> -13. Improve black level and colour gains application\n>> -\n>> -I think the black level should eventually be moved before debayering, and\n>> -ideally the colour gains as well. I understand the need for optimizations to\n>> -lower the CPU consumption, but at the same time I don't feel comfortable\n>> -building up on top of an implementation that may work a bit more by chance than\n>> -by correctness, as that's not very maintainable.","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 AD197BE08B\n\tfor ;\n\tMon, 29 Apr 2024 09:14:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE0F063418;\n\tMon, 29 Apr 2024 11:14:58 +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 49D09633ED\n\tfor ;\n\tMon, 29 Apr 2024 11:14:57 +0200 (CEST)","from mail-wm1-f71.google.com (mail-wm1-f71.google.com\n\t[209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-52-PD-AnhaxPg2xG9FCUqYuHQ-1; Mon, 29 Apr 2024 05:14:52 -0400","by mail-wm1-f71.google.com with SMTP id\n\t5b1f17b1804b1-418f5e7b9b2so18371335e9.3\n\tfor ;\n\tMon, 29 Apr 2024 02:14:52 -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\tl16-20020a05600c4f1000b0041a1fee2854sm29882128wmq.17.2024.04.29.02.14.49\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 29 Apr 2024 02:14: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=\"YnXvFHmB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1714382096;\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=vcJpOl68n5g1WZUQdz0yr9IsONsRds2UONNnx93Vjfg=;\n\tb=YnXvFHmB02X3Zzzdjnl0flFyfr+s+GE8PmgL9Qz8KssjgGJpKSRd3hnKdd0JFL7Nua6XjT\n\tQQ5iHqAYv0C7Sy7LAoBo3DESqlBSTwh0SDBNTiI9JYxHZwfowZ4P6zZ5cGqRDtlKjuS4NF\n\tqTU88bDXW5AF8lIJgb4kUkgmwWRNjys=","X-MC-Unique":"PD-AnhaxPg2xG9FCUqYuHQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1714382091; x=1714986891;\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=vcJpOl68n5g1WZUQdz0yr9IsONsRds2UONNnx93Vjfg=;\n\tb=oJgLOGqdZDPyulB9Hw4lbU3zg5Pmku+942EjGGDpRdGVLZ67O3lRuO/rfAbqv3rM8Q\n\tnKX7ct+oIeAbJW1OLTfq1Doy2OC+pTB0FbGH0PoBtfVKSYa9mX1IqUfpbkKpYZy6wJFT\n\tZcsDGvlaV+nO4HYemF62ZICBDWMJngvrBLulGFFqLpVxc37u5ukeryKnjO+YDAJEnlnx\n\tOu/ABCY8YOD9B1c++RmRErLf+mbNi2tWw9FXrcxAUMt0GU+0WQqQXkCqa24+VS1t1M0v\n\tJ+o3/5MexIuTzRATkxsT6HiANDyfhQYim/PNcjEMVgdAYHPwkmOv/frqxz2laXIuu7JC\n\tsilA==","X-Gm-Message-State":"AOJu0YxRz6zK3Fv/PIjR9zo52vAr5TcFVuvs+6lOgMKT961giUQtaIpQ\n\t5gSCVNoSoi5zVqchN3f1u0eRxqxyKcp+jYv1J1oxCqM7NIVjVnzfUUE/BLKYXIqIF8KEIxcBFJf\n\tfQChEUynhA1hb4XbUNVRMQmQlHXiDQVfTfJKKpm0wS3USMlMDzhVJwv57TQCD7Utf4Ohf8zwS1t\n\tqqvMy0ImyC0BPzeIQCw7WLNdM/y1ei7EzFnDfoZPb5QSSZsfnMUHNKMOU=","X-Received":["by 2002:a05:600c:4ecf:b0:416:bc18:2a00 with SMTP id\n\tg15-20020a05600c4ecf00b00416bc182a00mr7250658wmq.38.1714382090788; \n\tMon, 29 Apr 2024 02:14:50 -0700 (PDT)","by 2002:a05:600c:4ecf:b0:416:bc18:2a00 with SMTP id\n\tg15-20020a05600c4ecf00b00416bc182a00mr7250641wmq.38.1714382090316; \n\tMon, 29 Apr 2024 02:14:50 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEDcH6r27KRBi7OFwLeFlBCccFTI69Bq3AXlDdPxrU8PPsLuVZdk3Bu2IPqLZGLS2EvZJ/0NQ==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 4/5] libcamera: software_isp: Remove TODO #13","In-Reply-To":"<20240428231120.GA4950@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Mon, 29 Apr 2024 02:11:20 +0300\")","References":"<20240423182000.1527425-1-mzamazal@redhat.com>\n\t<20240423182000.1527425-5-mzamazal@redhat.com>\n\t<20240428231120.GA4950@pendragon.ideasonboard.com>","Date":"Mon, 29 Apr 2024 11:14:49 +0200","Message-ID":"<87bk5szg1i.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","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\" "}}]