From patchwork Thu Aug 15 08:16:20 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Milan Zamazal X-Patchwork-Id: 20929 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 1287FC32D6 for ; Thu, 15 Aug 2024 08:17:37 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 8010E633D9; Thu, 15 Aug 2024 10:17:37 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="RgTQpAXb"; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id C6439633D6 for ; Thu, 15 Aug 2024 10:17:32 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1723709851; 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=zqi62KONqOYAkH9I2+EWHCyIrENNhFJ60R7QakIJ3pA=; b=RgTQpAXbF6AINcOdDoN3/BlU/7i/WDD1qkHDUq6/Bn0jSMYmJZqvohLMZao5wMFQ26+suC QJb80b4+64KlF5Diz3H2IV8GfA9KyFrm3H32UmeoAZFZzpe1OKcKmxf3s2aSMbP6iL/Zjj QCxsHMAhd7tV6lYPG+TLWKF6sbBs+lg= Received: from mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-637-0ORnjQvNOEeeNJEy6uPSXw-1; Thu, 15 Aug 2024 04:17:30 -0400 X-MC-Unique: 0ORnjQvNOEeeNJEy6uPSXw-1 Received: from mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.40]) (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 mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 29B1D1955D48; Thu, 15 Aug 2024 08:17:29 +0000 (UTC) Received: from nuthatch.redhat.com (unknown [10.45.225.128]) by mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id F179819560A3; Thu, 15 Aug 2024 08:17:26 +0000 (UTC) From: Milan Zamazal To: libcamera-devel@lists.libcamera.org Cc: Milan Zamazal , Umang Jain , Laurent Pinchart , Daniel Scally Subject: [PATCH v4 16/18] libcamera: software_isp: Use DelayedControls Date: Thu, 15 Aug 2024 10:16:20 +0200 Message-ID: <20240815081622.174210-17-mzamazal@redhat.com> In-Reply-To: <20240815081622.174210-1-mzamazal@redhat.com> References: <20240815081622.174210-1-mzamazal@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.0 on 10.30.177.40 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" Use the standard libcamera mechanism to report the "current" controls rather than delaying updates by counting from the last update. MY SPECULATION -- valid or not?: A problem is that with software ISP we cannot be sure about the sensor delay. The original implementation simply skips exposure updates for 2 frames, which should be enough in most cases. After this change, we assume the delay being exactly 2 frames, which may or may not be correct and may work with outdated values if the delay is shorter. This patch also prepares moving exposure+gain to an algorithm module where the original delay mechanism would be a (possibly unnecessary) complication. Resolves software ISP TODO #11 + #12. Signed-off-by: Milan Zamazal --- src/ipa/simple/soft_simple.cpp | 16 +------------ src/libcamera/pipeline/simple/simple.cpp | 18 ++++++++++++--- src/libcamera/software_isp/TODO | 29 ------------------------ 3 files changed, 16 insertions(+), 47 deletions(-) diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index 8f01bf7d..0e47b9c5 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -59,8 +59,7 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module public: IPASoftSimple() : params_(nullptr), stats_(nullptr), - context_({ {}, {}, { kMaxFrameContexts } }), - ignoreUpdates_(0) + context_({ {}, {}, { kMaxFrameContexts } }) { } @@ -98,7 +97,6 @@ private: int32_t exposure_; double againMin_, againMax_, againMinStep_; double again_; - unsigned int ignoreUpdates_; }; IPASoftSimple::~IPASoftSimple() @@ -298,16 +296,6 @@ void IPASoftSimple::processStats(const uint32_t frame, /* \todo Switch to the libipa/algorithm.h API someday. */ - /* - * AE / AGC, use 2 frames delay to make sure that the exposure and - * the gain set have applied to the camera sensor. - * \todo This could be handled better with DelayedControls. - */ - if (ignoreUpdates_ > 0) { - --ignoreUpdates_; - return; - } - /* * Calculate Mean Sample Value (MSV) according to formula from: * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf @@ -356,8 +344,6 @@ void IPASoftSimple::processStats(const uint32_t frame, ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(camHelper_ ? camHelper_->gainCode(again_) : again_)); - ignoreUpdates_ = 2; - setSensorControls.emit(ctrls); LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index ed50ab52..9a778994 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -32,6 +32,7 @@ #include "libcamera/internal/camera.h" #include "libcamera/internal/camera_sensor.h" #include "libcamera/internal/converter.h" +#include "libcamera/internal/delayed_controls.h" #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/pipeline_handler.h" @@ -278,6 +279,8 @@ public: std::vector configs_; std::map> formats_; + std::unique_ptr delayedCtrls_; + std::vector> conversionBuffers_; std::queue> conversionQueue_; bool useConversion_; @@ -900,14 +903,13 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId) { - /* \todo Use the DelayedControls class */ swIsp_->processStats(frame, bufferId, - sensor_->getControls({ V4L2_CID_ANALOGUE_GAIN, - V4L2_CID_EXPOSURE })); + delayedCtrls_->get(frame)); } void SimpleCameraData::setSensorControls(const ControlList &sensorControls) { + delayedCtrls_->push(sensorControls); ControlList ctrls(sensorControls); sensor_->setControls(&ctrls); } @@ -1288,6 +1290,16 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c) if (outputCfgs.empty()) return 0; + std::unordered_map params = { + { V4L2_CID_ANALOGUE_GAIN, { 2, false } }, + { V4L2_CID_EXPOSURE, { 2, false } }, + }; + data->delayedCtrls_ = + std::make_unique(data->sensor_->device(), + params); + data->video_->frameStart.connect(data->delayedCtrls_.get(), + &DelayedControls::applyControls); + StreamConfiguration inputCfg; inputCfg.pixelFormat = pipeConfig->captureFormat; inputCfg.size = pipeConfig->captureSize; diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO index 9978afc0..8eeede46 100644 --- a/src/libcamera/software_isp/TODO +++ b/src/libcamera/software_isp/TODO @@ -209,35 +209,6 @@ At some point, yes. --- -11. Improve handling the sensor controls which take effect with a delay - -> void IPASoftSimple::processStats(const ControlList &sensorControls) -> { -> ... -> /* -> * AE / AGC, use 2 frames delay to make sure that the exposure and -> * the gain set have applied to the camera sensor. -> */ -> if (ignore_updates_ > 0) { -> --ignore_updates_; -> return; -> } - -This could be handled better with DelayedControls. - ---- - -12. Use DelayedControls class in ispStatsReady() - -> void SimpleCameraData::ispStatsReady() -> { -> swIsp_->processStats(sensor_->getControls({ V4L2_CID_ANALOGUE_GAIN, -> 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