@@ -239,7 +239,13 @@ bool DelayedControls::push(uint32_t sequence, const ControlList &controls)
*/
ControlList DelayedControls::get(uint32_t sequence)
{
- unsigned int index = std::max<int>(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)
{
@@ -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;
@@ -1849,8 +1849,10 @@ bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink)
V4L2Subdevice *sensorSubdev = in->sensor_->device();
data->delayedCtrls_ = std::make_unique<DelayedControls>(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. */
@@ -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)
@@ -361,6 +361,8 @@ public:
std::unique_ptr<SoftwareIsp> swIsp_;
SimpleFrames frameInfo_;
+ void frameStart(uint32_t sequence);
+
private:
void tryPipeline(unsigned int code, const Size &size);
static std::vector<const MediaPad *> 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<const MediaPad *> 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_) {
@@ -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<int32_t>();
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<int32_t>();
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<DelayedControls>(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>();
int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();
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>();
int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();
if (brightness != expected || contrast != expected) {
cerr << "Failed multi queue"
- << " frame " << i
+ << " frame " << i - maxDelay
<< " brightness " << brightness
<< " contrast " << contrast
<< " expected " << expected
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 <stefan.klug@ideasonboard.com> --- 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(-)