[v6,16/18] libcamera: software_isp: Use DelayedControls

21189 diff mbox series

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 <mzamazal@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(-)

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@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)


Reviewed-by: Kieran Bingham <kieran.bingham@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
>

Kieran Bingham <kieran.bingham@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@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@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
>>