From patchwork Tue Apr 23 18:19:59 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Milan Zamazal X-Patchwork-Id: 19933 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 0BE60C328D for ; Tue, 23 Apr 2024 18:20:29 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 51107633FE; Tue, 23 Apr 2024 20:20:28 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="QlJtMV4m"; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id BB5F2633EC for ; Tue, 23 Apr 2024 20:20:25 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1713896424; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=9iPHvaerPCvm1J8fb11HBL7RWIiLE6dX/qwpyR82Kfw=; b=QlJtMV4mBAF2/z98RZ1JeV6bW4DAhm7mZFwrN/UQjNcqHbnG384W4c6ytirG6p5W5Ynwau 3YhehECfyqHtfN4CfoEAmdHyZeI/n09U6eJvV8quGYyB59P8wpNOGdFfb3Lz06Topfx6JW +ZQP4yzl6ZMYDO+lLlPeTBo5oLe/1gU= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-18-UUBtCbdGM8eGlgW3UzAiBA-1; Tue, 23 Apr 2024 14:20:22 -0400 X-MC-Unique: UUBtCbdGM8eGlgW3UzAiBA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 87FBA29AC00A for ; Tue, 23 Apr 2024 18:20:22 +0000 (UTC) Received: from nuthatch.redhat.com (unknown [10.45.225.63]) by smtp.corp.redhat.com (Postfix) with ESMTP id AE81649102; Tue, 23 Apr 2024 18:20:21 +0000 (UTC) From: Milan Zamazal To: libcamera-devel@lists.libcamera.org Cc: Milan Zamazal Subject: [PATCH 4/5] libcamera: software_isp: Remove TODO #13 Date: Tue, 23 Apr 2024 20:19:59 +0200 Message-ID: <20240423182000.1527425-5-mzamazal@redhat.com> In-Reply-To: <20240423182000.1527425-1-mzamazal@redhat.com> References: <20240423182000.1527425-1-mzamazal@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" The current software ISP debayering implementation uses color lookup tables that apply black level subtraction, color gain and gamma correction in a single step, while performing debayering. In theory, black level subtraction and color gains should be applied before debayering and gamma correction after debayering. But because black level subtraction and color gain corrections are currently linear and we average only same-color pixels in debayering, we can apply all the operations after debayering. The only difference is with clipping where out-of-range pixels contribute more than they deserve but this is not generally significant. Combining all the operations at once is conceptually not ideal but it is not incorrect in this case and saves CPU cycles, which is critical for software ISP CPU implementation (it may be less important with future GPU implementation). This means we need the current implementation. It may get replaced or become optional (configurable) in future, for example if the separation is needed due to introducing other image processing operations. Under these circumstances and considering that the lookup tables construction has been moved out of the debayering code in the preceding patch, it makes no sense to keep software ISP TODO #13. Let's remove it in favor of a clarifying code comment. Signed-off-by: Milan Zamazal --- src/ipa/simple/soft_simple.cpp | 10 ++++++++++ src/libcamera/software_isp/TODO | 10 ---------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index c5085f71..b2f4d308 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -247,6 +247,16 @@ void IPASoftSimple::stop() void IPASoftSimple::processStats(const ControlList &sensorControls) { + /* + * Here black level subtraction, color gains and gamma correction are + * combined into a single operation (table lookup) to save CPU cycles. + * This works because black level subtraction and color gains are currently + * linear operations and we average only same-color pixels in debayering. + * If we change anything on this or introduce other operations in between, + * it may be needed to split the operations and perform black and gain + * corrections before debayering and gamma correction after debayering. + */ + SwIspStats::Histogram histogram = stats_->yHistogram; if (ignoreUpdates_ > 0) blackLevel_.update(histogram); diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO index 4fcee39b..fcb02588 100644 --- a/src/libcamera/software_isp/TODO +++ b/src/libcamera/software_isp/TODO @@ -267,13 +267,3 @@ This could be handled better with DelayedControls. > V4L2_CID_EXPOSURE })); You should use the DelayedControls class. - ---- - -13. Improve black level and colour gains application - -I think the black level should eventually be moved before debayering, and -ideally the colour gains as well. I understand the need for optimizations to -lower the CPU consumption, but at the same time I don't feel comfortable -building up on top of an implementation that may work a bit more by chance than -by correctness, as that's not very maintainable.