From patchwork Mon Sep 14 14:02:21 2026 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Klug X-Patchwork-Id: 28252 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 1D0C8C3352 for ; Mon, 14 Sep 2026 14:03:50 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id A6E96686BF; Mon, 14 Sep 2026 16:03:49 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="kXTi5Fl7"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id C1DB5686B9 for ; Mon, 14 Sep 2026 16:03:48 +0200 (CEST) Received: from ideasonboard.com (unknown [IPv6:2a00:6020:448c:6c00:a279:75fa:1f6c:7f40]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id C2A9EC3B; Mon, 14 Sep 2026 16:02:08 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1789394528; bh=NFBjekpEEWS3VC084F7dtUFcwKF0GpX7g0vXDqANL+w=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=kXTi5Fl7Rfp+jMYa1k2lVgDaa61iLzdtg7PaiO6HFAANmjq0yK/MRkvBmXFoBoiox XYbeWB7o09DylGsSW8gWuX5iaQ5j4sjFuzHOz7XpYx2M4PPm2FE96kL+LNBgbyKPxT PEwMn2RLHhK/V7phTdlFltkSu98idmYkB4QXRoqU= From: Stefan Klug To: libcamera-devel@lists.libcamera.org Cc: Stefan Klug Subject: [PATCH v3 08/41] libcamera: delayed_controls: Change semantics of sequence numbers Date: Mon, 14 Sep 2026 16:02:21 +0200 Message-ID: <20260914140309.3354666-9-stefan.klug@ideasonboard.com> X-Mailer: git-send-email 2.53.0 In-Reply-To: <20260914140309.3354666-1-stefan.klug@ideasonboard.com> References: <20260914140309.3354666-1-stefan.klug@ideasonboard.com> MIME-Version: 1.0 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" In the context of per frame controls the semantics of DelayedControls::applyControls() are quite difficult to grasp. The function void frameStart(int s) { delayedCtrls->applyControls(s); } seems intuitively wrong as the frame has already started. I think it is easier to think about what needs to be done for a specific sequence number. So (assuming a max sensor delay of 2) the actions would be: - delayedControls.push(n) stores the controls that shall be active on frame n - delayedControls.get(n) returns these controls again - delayedControls.apply(n) applies the slowest control for frame n and does a look back on other controls. So when a frameStart for frame n occurs, it is time to call delayedControls.apply(n + maxDelay) Changing these semantics on delayed controls doesn't require much code change and has the added benefit that we don't run into clamping for get() on frames < maxDelay. Signed-off-by: Stefan Klug --- Changes in v3: - Fixed unit tests - Added error log when get() is called with an illegal sequence - Added fixup for SimplePipeline Changes in v2: - Applied the semantics change to other pipelines as well. --- src/libcamera/delayed_controls.cpp | 21 +++++--- src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- src/libcamera/pipeline/mali-c55/mali-c55.cpp | 6 ++- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +- src/libcamera/pipeline/simple/simple.cpp | 15 ++++-- test/delayed_controls.cpp | 57 ++++++++++---------- 6 files changed, 60 insertions(+), 44 deletions(-) diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp index 1f4fa6974bde..90c7046c66f7 100644 --- a/src/libcamera/delayed_controls.cpp +++ b/src/libcamera/delayed_controls.cpp @@ -239,7 +239,13 @@ bool DelayedControls::push(uint32_t sequence, const ControlList &controls) */ ControlList DelayedControls::get(uint32_t sequence) { - unsigned int index = std::max(0, sequence - maxDelay_); + unsigned int index = sequence; + + if (sequence > writeCount_) + LOG(DelayedControls, Error) + << "Get controls for frame " << sequence + << " but only up to frame " << writeCount_ + << " were sent to the device"; ControlList out(device_->controls()); for (const auto &ctrl : values_) { @@ -265,13 +271,14 @@ ControlList DelayedControls::get(uint32_t sequence) */ /** - * \brief Inform DelayedControls of the start of a new frame - * \param[in] sequence Sequence number of the frame that started + * \brief Apply controls for a frame + * \param[in] sequence Sequence number of the frame to apply * - * Inform the state machine that a new frame has started and of its sequence - * number. Any user of these helpers is responsible to inform the helper about - * the start of any frame. This can be connected with ease to the start of a - * exposure (SOE) V4L2 event. + * Apply controls for the frame \a sequence. This applies the controls with the + * largest delay. For controls with a smaller delay it does a look back and + * applies the controls for the previous sequence. So usually this function is + * called in a start of exposure event as applyControls(startedSequence + + * maxDelay) */ void DelayedControls::applyControls(uint32_t sequence) { diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 14cab9e5559b..1341119f6f66 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -1384,7 +1384,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer) */ void IPU3CameraData::frameStart(uint32_t sequence) { - delayedCtrls_->applyControls(sequence); + delayedCtrls_->applyControls(sequence + delayedCtrls_->maxDelay()); if (processingRequests_.empty()) return; diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp index 73a03373c833..70c751fc7d90 100644 --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp @@ -1849,8 +1849,10 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink) V4L2Subdevice *sensorSubdev = in->sensor_->device(); data->delayedCtrls_ = std::make_unique(sensorSubdev, params); - isp_->frameStart.connect(data->delayedCtrls_.get(), - &DelayedControls::applyControls); + isp_->frameStart.connect(data->delayedCtrls_.get(), [&](uint32_t seq) { + uint32_t lookahead = data->delayedCtrls_->maxDelay(); + data->delayedCtrls_->applyControls(seq + lookahead); + }); /* \todo Init properties. */ diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index a21b921f228c..ffd49157ec67 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -1512,7 +1512,8 @@ void PipelineHandlerRkISP1::frameStart(uint32_t sequence) return; RkISP1CameraData *data = cameraData(activeCamera_); - data->delayedCtrls_->applyControls(sequence); + uint32_t sequenceToApply = sequence + data->delayedCtrls_->maxDelay(); + data->delayedCtrls_->applyControls(sequenceToApply); } bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 35c29ceca1bb..572a73307fdb 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -361,6 +361,8 @@ public: std::unique_ptr swIsp_; SimpleFrames frameInfo_; + void frameStart(uint32_t sequence); + private: void tryPipeline(unsigned int code, const Size &size); static std::vector routedSourcePads(MediaPad *sink); @@ -1058,6 +1060,11 @@ void SimpleCameraData::setSensorControls(const ControlList &sensorControls) } } +void SimpleCameraData::frameStart(uint32_t sequence) +{ + delayedCtrls_->applyControls(sequence + delayedCtrls_->maxDelay()); +} + /* Retrieve all source pads connected to a sink pad through active routes. */ std::vector SimpleCameraData::routedSourcePads(MediaPad *sink) { @@ -1671,8 +1678,8 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL stop(camera); return ret; } - frameStartEmitter->frameStart.connect(data->delayedCtrls_.get(), - &DelayedControls::applyControls); + frameStartEmitter->frameStart.connect(data, + &SimpleCameraData::frameStart); } ret = video->streamOn(); @@ -1711,8 +1718,8 @@ void SimplePipelineHandler::stopDevice(Camera *camera) if (frameStartEmitter) { frameStartEmitter->setFrameStartEnabled(false); - frameStartEmitter->frameStart.disconnect(data->delayedCtrls_.get(), - &DelayedControls::applyControls); + frameStartEmitter->frameStart.disconnect(data, + &SimpleCameraData::frameStart); } if (data->useConversion_) { diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp index 7bd30e7aead8..5bfef285bf2b 100644 --- a/test/delayed_controls.cpp +++ b/test/delayed_controls.cpp @@ -84,23 +84,20 @@ protected: dev_->setControls(&ctrls); delayed->reset(); - /* Trigger the first frame start event */ - delayed->applyControls(0); - /* Test control without delay are set at once. */ for (unsigned int i = 1; i < 100; i++) { int32_t value = 100 + i; ctrls.set(V4L2_CID_BRIGHTNESS, value); - delayed->push(ctrls); + delayed->push(i, ctrls); delayed->applyControls(i); - ControlList result = delayed->get(i); + ControlList result = delayed->get(i - delayed->maxDelay()); int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get(); if (brightness != value) { cerr << "Failed single control without delay" - << " frame " << i + << " frame " << i - delayed->maxDelay() << " expected " << value << " got " << brightness << endl; @@ -126,23 +123,19 @@ protected: dev_->setControls(&ctrls); delayed->reset(); - /* Trigger the first frame start event */ - delayed->applyControls(0); - /* Test single control with delay. */ for (unsigned int i = 1; i < 100; i++) { int32_t value = 10 + i; ctrls.set(V4L2_CID_BRIGHTNESS, value); - delayed->push(ctrls); - + delayed->push(i, ctrls); delayed->applyControls(i); - ControlList result = delayed->get(i); + ControlList result = delayed->get(i - delayed->maxDelay()); int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get(); if (brightness != expected) { cerr << "Failed single control with delay" - << " frame " << i + << " frame " << i - delayed->maxDelay() << " expected " << expected << " got " << brightness << endl; @@ -167,32 +160,38 @@ protected: std::make_unique(dev_.get(), delays); ControlList ctrls; - /* Reset control to value that will be first two frames in test. */ + /* + * Reset control to value that will be first two frames in test. + * We expect the following values: + * Frame 0 1 2 3 4 5 ... + * Brightness 200 11 12 13 14 15 + * Contrast 201 12 13 14 15 16 + */ int32_t expected = 200; ctrls.set(V4L2_CID_BRIGHTNESS, expected); ctrls.set(V4L2_CID_CONTRAST, expected + 1); dev_->setControls(&ctrls); delayed->reset(); - /* Trigger the first frame start event */ - delayed->applyControls(0); - /* Test dual control with delay. */ for (unsigned int i = 1; i < 100; i++) { int32_t value = 10 + i; ctrls.set(V4L2_CID_BRIGHTNESS, value); ctrls.set(V4L2_CID_CONTRAST, value + 1); - delayed->push(ctrls); + delayed->push(i, ctrls); delayed->applyControls(i); - ControlList result = delayed->get(i); + if (i < maxDelay) + continue; + + ControlList result = delayed->get(i - delayed->maxDelay()); int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get(); int32_t contrast = result.get(V4L2_CID_CONTRAST).get(); if (brightness != expected || contrast != expected + 1) { cerr << "Failed dual controls" - << " frame " << i + << " frame " << i - delayed->maxDelay() << " brightness " << brightness << " contrast " << contrast << " expected " << expected @@ -225,35 +224,35 @@ protected: dev_->setControls(&ctrls); delayed->reset(); - /* Trigger the first frame start event */ - delayed->applyControls(0); - /* * Queue all controls before any fake frame start. Note we * can't queue up more then the delayed controls history size * which is 16. Where one spot is used by the reset control. */ - for (unsigned int i = 0; i < 15; i++) { + for (unsigned int i = 1; i < 15; i++) { int32_t value = 10 + i; ctrls.set(V4L2_CID_BRIGHTNESS, value); ctrls.set(V4L2_CID_CONTRAST, value); - delayed->push(ctrls); + delayed->push(i, ctrls); } /* Process all queued controls. */ - for (unsigned int i = 1; i < 16; i++) { - int32_t value = 10 + i - 1; + for (unsigned int i = 1; i < 15; i++) { + int32_t value = 10 + i; delayed->applyControls(i); - ControlList result = delayed->get(i); + if (i < maxDelay) + continue; + + ControlList result = delayed->get(i - maxDelay); int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get(); int32_t contrast = result.get(V4L2_CID_CONTRAST).get(); if (brightness != expected || contrast != expected) { cerr << "Failed multi queue" - << " frame " << i + << " frame " << i - maxDelay << " brightness " << brightness << " contrast " << contrast << " expected " << expected