[PATCH v6 16/18] libcamera: software_isp: Use DelayedControls
Milan Zamazal
mzamazal at redhat.com
Thu Sep 19 20:23:39 CEST 2024
Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> Quoting Milan Zamazal (2024-09-06 13:09:25)
>> 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.
>
> Yeah, I think this is both 'right' and 'wrong'.
>
> But the wrong parts are also wrong on the IPU3/RKISP1/Mali pipelines
> too. Currently - only RPi have this correct.
>
> And we need to fix this - but we should fix it by correctly specifying
> the gains in the libipa camera sensor helpers...
>
> So I think this patch is worthwhile, but I do believe could introduce a
> risk of increasing oscillations ... so we need to tackle this soon!
>
>
>>
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>> 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 f8d923c5..60310a8e 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;
>> - }
>> -
>
> We 'could' keep the ignoreUpdates in the agc module as well until the
> control delays are encoded in the sensor helpers.
>
> That would keep the current behaviour, and move to the common framework
> for handling the delays...
>
> But it's probably not worth it ... unless someone sees uncomfortable
> oscillations ... (before we get to a 'real' fix, and the 'real' fix is
> probably better to spend time on)
OK, thank you for clarification. I agree with focusing on the real fix.
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
>
>> /*
>> * 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<int32_t>(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 7e412e17..40a7b1da 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<Configuration> configs_;
>> std::map<PixelFormat, std::vector<const Configuration *>> formats_;
>>
>> + std::unique_ptr<DelayedControls> delayedCtrls_;
>> +
>> std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;
>> std::queue<std::map<const Stream *, FrameBuffer *>> 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<uint32_t, DelayedControls::ControlParams> params = {
>> + { V4L2_CID_ANALOGUE_GAIN, { 2, false } },
>> + { V4L2_CID_EXPOSURE, { 2, false } },
>> + };
>> + data->delayedCtrls_ =
>> + std::make_unique<DelayedControls>(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
>> --
>> 2.44.1
>>
More information about the libcamera-devel
mailing list