| Message ID | 20251024085130.995967-19-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan On Fri, Oct 24, 2025 at 10:50:42AM +0200, Stefan Klug wrote: > In the context of per frame controls I believe 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) I see this being used as void PipelineHandlerRkISP1::frameStart(uint32_t sequence) ... uint32_t sequenceToApply = sequence + data->delayedCtrls_->maxDelay(); data->delayedCtrls_->applyControls(sequenceToApply); and you suggest this as a call pattern. Is this me or this is exactly what was happening before without the trouble of having to query the maxDelay() everytime from the caller side ? Or, in other words, do you expect applyControls to be called with anything but 'sequence + maxDelay()' ? > > Changing these semantics on delayed controls doesn't require much code > change and has the added benefit that we don't run in to clamping for > get() on frames < maxDelay. > > ToDo: This breaks the delayed controls test at the moment. The tests > need to be fixed and other pipelines need to be adjusted accordingly. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/libcamera/delayed_controls.cpp | 15 ++++++++------- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++-- > test/meson.build | 2 +- > 3 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp > index 35a624525b80..8239f3dcf347 100644 > --- a/src/libcamera/delayed_controls.cpp > +++ b/src/libcamera/delayed_controls.cpp > @@ -241,7 +241,7 @@ 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; > > ControlList out(device_->controls()); > for (const auto &ctrl : values_) { > @@ -267,13 +267,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 looks 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/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index d937c94e351f..7a4957d7e535 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1624,10 +1624,11 @@ 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); > > if (isRaw_) { > - data->ipa_->computeParams(sequence + 1, 0); > + data->ipa_->computeParams(sequenceToApply + 1, 0); > } > } > > diff --git a/test/meson.build b/test/meson.build > index 52f04364e4fc..80fb543f2201 100644 > --- a/test/meson.build > +++ b/test/meson.build > @@ -53,7 +53,7 @@ internal_tests = [ > {'name': 'bayer-format', 'sources': ['bayer-format.cpp']}, > {'name': 'byte-stream-buffer', 'sources': ['byte-stream-buffer.cpp']}, > {'name': 'camera-sensor', 'sources': ['camera-sensor.cpp']}, > - {'name': 'delayed_controls', 'sources': ['delayed_controls.cpp']}, > + {'name': 'delayed_controls', 'sources': ['delayed_controls.cpp'], 'should_fail': true}, > {'name': 'event', 'sources': ['event.cpp']}, > {'name': 'event-dispatcher', 'sources': ['event-dispatcher.cpp']}, > {'name': 'event-thread', 'sources': ['event-thread.cpp']}, > -- > 2.48.1 >
Hi Jacopo, Thank you for the review. Quoting Jacopo Mondi (2025-11-06 18:34:07) > Hi Stefan > > On Fri, Oct 24, 2025 at 10:50:42AM +0200, Stefan Klug wrote: > > In the context of per frame controls I believe 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) > > I see this being used as > > void PipelineHandlerRkISP1::frameStart(uint32_t sequence) > > ... > > uint32_t sequenceToApply = sequence + data->delayedCtrls_->maxDelay(); > data->delayedCtrls_->applyControls(sequenceToApply); > > and you suggest this as a call pattern. > > Is this me or this is exactly what was happening before without the > trouble of having to query the maxDelay() everytime from the caller > side ? Or, in other words, do you expect applyControls to be called > with anything but 'sequence + maxDelay()' ? You are right, it is basically the same. But I don't see that as additional trouble. It helps my brain to cope with the sequence of events. If you look at the frameStart() function after applying the whole series, it looks like that: uint32_t sequenceToApply = sequence + data->delayedCtrls_->maxDelay(); data->delayedCtrls_->applyControls(sequenceToApply); computeParamBuffers(sequenceToApply + 1); This code makes it pretty clear, that the lookahead we need to do to prepare the params buffer is the sequenceToApply + 1. So at that point we'd need to fetch the maxDelay anyways. Can you go with that? Best regards, Stefan > > > > > Changing these semantics on delayed controls doesn't require much code > > change and has the added benefit that we don't run in to clamping for > > get() on frames < maxDelay. > > > > ToDo: This breaks the delayed controls test at the moment. The tests > > need to be fixed and other pipelines need to be adjusted accordingly. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/libcamera/delayed_controls.cpp | 15 ++++++++------- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++-- > > test/meson.build | 2 +- > > 3 files changed, 12 insertions(+), 10 deletions(-) > > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp > > index 35a624525b80..8239f3dcf347 100644 > > --- a/src/libcamera/delayed_controls.cpp > > +++ b/src/libcamera/delayed_controls.cpp > > @@ -241,7 +241,7 @@ 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; > > > > ControlList out(device_->controls()); > > for (const auto &ctrl : values_) { > > @@ -267,13 +267,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 looks 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/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index d937c94e351f..7a4957d7e535 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -1624,10 +1624,11 @@ 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); > > > > if (isRaw_) { > > - data->ipa_->computeParams(sequence + 1, 0); > > + data->ipa_->computeParams(sequenceToApply + 1, 0); > > } > > } > > > > diff --git a/test/meson.build b/test/meson.build > > index 52f04364e4fc..80fb543f2201 100644 > > --- a/test/meson.build > > +++ b/test/meson.build > > @@ -53,7 +53,7 @@ internal_tests = [ > > {'name': 'bayer-format', 'sources': ['bayer-format.cpp']}, > > {'name': 'byte-stream-buffer', 'sources': ['byte-stream-buffer.cpp']}, > > {'name': 'camera-sensor', 'sources': ['camera-sensor.cpp']}, > > - {'name': 'delayed_controls', 'sources': ['delayed_controls.cpp']}, > > + {'name': 'delayed_controls', 'sources': ['delayed_controls.cpp'], 'should_fail': true}, > > {'name': 'event', 'sources': ['event.cpp']}, > > {'name': 'event-dispatcher', 'sources': ['event-dispatcher.cpp']}, > > {'name': 'event-thread', 'sources': ['event-thread.cpp']}, > > -- > > 2.48.1 > >
diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp index 35a624525b80..8239f3dcf347 100644 --- a/src/libcamera/delayed_controls.cpp +++ b/src/libcamera/delayed_controls.cpp @@ -241,7 +241,7 @@ 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; ControlList out(device_->controls()); for (const auto &ctrl : values_) { @@ -267,13 +267,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 looks 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/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index d937c94e351f..7a4957d7e535 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -1624,10 +1624,11 @@ 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); if (isRaw_) { - data->ipa_->computeParams(sequence + 1, 0); + data->ipa_->computeParams(sequenceToApply + 1, 0); } } diff --git a/test/meson.build b/test/meson.build index 52f04364e4fc..80fb543f2201 100644 --- a/test/meson.build +++ b/test/meson.build @@ -53,7 +53,7 @@ internal_tests = [ {'name': 'bayer-format', 'sources': ['bayer-format.cpp']}, {'name': 'byte-stream-buffer', 'sources': ['byte-stream-buffer.cpp']}, {'name': 'camera-sensor', 'sources': ['camera-sensor.cpp']}, - {'name': 'delayed_controls', 'sources': ['delayed_controls.cpp']}, + {'name': 'delayed_controls', 'sources': ['delayed_controls.cpp'], 'should_fail': true}, {'name': 'event', 'sources': ['event.cpp']}, {'name': 'event-dispatcher', 'sources': ['event-dispatcher.cpp']}, {'name': 'event-thread', 'sources': ['event-thread.cpp']},
In the context of per frame controls I believe 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 in to clamping for get() on frames < maxDelay. ToDo: This breaks the delayed controls test at the moment. The tests need to be fixed and other pipelines need to be adjusted accordingly. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/libcamera/delayed_controls.cpp | 15 ++++++++------- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++-- test/meson.build | 2 +- 3 files changed, 12 insertions(+), 10 deletions(-)