[libcamera-devel,v3,7/7] ipa: raspberrypi: agc: Fix digital gain calculation for manual mode

17404 diff mbox series

The digital gain calculation uses a total exposure value computed with the
current AGC state. However, this is wrong in the case of manual shutter/gain
controls, as the total exposure value used must be the value computed when the
AGC sent the manual shutter/gain controls to the pipeline handler to action.

To fix this, the IPA now adds the historical AgcStatus structure to the metadata
(tagged with "agc.delayed_status"). This historical AgcStatus structure contains
the total exposure value calculated when the AGC sent the manual shutter/gain
controls to the pipeline handler.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Tested-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++++++--
 src/ipa/raspberrypi/raspberrypi.cpp        | 11 +++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

Hi Naush,

Thank you for the patch.

On Mon, Sep 26, 2022 at 10:57:37AM +0100, Naushir Patuck via libcamera-devel wrote:
> The digital gain calculation uses a total exposure value computed with the
> current AGC state. However, this is wrong in the case of manual shutter/gain
> controls, as the total exposure value used must be the value computed when the
> AGC sent the manual shutter/gain controls to the pipeline handler to action.
> 
> To fix this, the IPA now adds the historical AgcStatus structure to the metadata
> (tagged with "agc.delayed_status"). This historical AgcStatus structure contains
> the total exposure value calculated when the AGC sent the manual shutter/gain
> controls to the pipeline handler.

Have you looked at the work we have recently merged that introduced a
frame context queue in libipa ? This has allowed us to solve a very
similar issue in the RkISP1 AWB, which needs to know the colour gains
that have been applied to the frame, and not just the latest gains (the
hardware computes the AWB statistics after applying the colour
gains...).

The issue was fixed in [1], with [2] introducing the frame context queue
in the AWB algorithm, and [3] providing documentation about how the
active state and frame context collaborate. The frame context queue
itself is very similar to the metadata array you introduce in patch 5/7
in this series.

While I have no doubt this series fixes your issue, I'm concerned about
how it relies on passing information through the DelayedControls class,
which affects all pipeline handlers. The more we diverge in the usage of
that class, the more difficult per-frame control will be difficult to
implement. If the differences related to this issue between the
Raspberry Pi on one side and RkISP1 and IPU3 on the other side were
mostly in the IPA module I wouldn't be so concerned, as the IPA modules
differ quite a lot already for historical reasons, but it expands beyond
the IPA side here.

Would you be able to have a look at the frame context queue ? Should we
look together at how we could align the platforms better ?

[1] https://git.libcamera.org/libcamera/libcamera.git/commit/?id=290ebeb59525eb7fdcc6f815d8dee6cbe3d8a314
[2] https://git.libcamera.org/libcamera/libcamera.git/commit/?id=128f22bce55ba2baea082010bceaffdba23f3f0d
[3] https://git.libcamera.org/libcamera/libcamera.git/commit/?id=a2f34f19578e8dae37cb7898710d15b83f176f94

> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Tested-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++++++--
>  src/ipa/raspberrypi/raspberrypi.cpp        | 11 +++++++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index bd54a639d637..b18bd7b5b19e 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -408,6 +408,12 @@ void Agc::switchMode(CameraMode const &cameraMode,
>  
>  void Agc::prepare(Metadata *imageMetadata)
>  {
> +	Duration totalExposureValue = status_.totalExposureValue;
> +	AgcStatus delayedStatus;
> +
> +	if (!imageMetadata->get("agc.delayed_status", delayedStatus))
> +		totalExposureValue = delayedStatus.totalExposureValue;
> +
>  	status_.digitalGain = 1.0;
>  	fetchAwbStatus(imageMetadata); /* always fetch it so that Process knows it's been done */
>  
> @@ -418,8 +424,8 @@ void Agc::prepare(Metadata *imageMetadata)
>  			Duration actualExposure = deviceStatus.shutterSpeed *
>  						  deviceStatus.analogueGain;
>  			if (actualExposure) {
> -				status_.digitalGain = status_.totalExposureValue / actualExposure;
> -				LOG(RPiAgc, Debug) << "Want total exposure " << status_.totalExposureValue;
> +				status_.digitalGain = totalExposureValue / actualExposure;
> +				LOG(RPiAgc, Debug) << "Want total exposure " << totalExposureValue;
>  				/*
>  				 * Never ask for a gain < 1.0, and also impose
>  				 * some upper limit. Make it customisable?
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 0e80ef9c7b95..bc438e99eff6 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -1025,6 +1025,17 @@ void IPARPi::prepareISP(const ISPConfig &data)
>  		embeddedBuffer = it->second.planes()[0];
>  	}
>  
> +	/*
> +	 * AGC wants to know the algorithm status from the time it actioned the
> +	 * sensor exposure/gain changes. So fetch it from the metadata list
> +	 * indexed by the IPA cookie returned, and put it in the current frame
> +	 * metadata.
> +	 */
> +	AgcStatus agcStatus;
> +	RPiController::Metadata &delayedMetadata = rpiMetadata_[data.ipaCookie];
> +	if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus))
> +		rpiMetadata.set("agc.delayed_status", agcStatus);
> +
>  	/*
>  	 * This may overwrite the DeviceStatus using values from the sensor
>  	 * metadata, and may also do additional custom processing.

Hi Laurent,

On Tue, 4 Oct 2022 at 17:21, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, Sep 26, 2022 at 10:57:37AM +0100, Naushir Patuck via
> libcamera-devel wrote:
> > The digital gain calculation uses a total exposure value computed with
> the
> > current AGC state. However, this is wrong in the case of manual
> shutter/gain
> > controls, as the total exposure value used must be the value computed
> when the
> > AGC sent the manual shutter/gain controls to the pipeline handler to
> action.
> >
> > To fix this, the IPA now adds the historical AgcStatus structure to the
> metadata
> > (tagged with "agc.delayed_status"). This historical AgcStatus structure
> contains
> > the total exposure value calculated when the AGC sent the manual
> shutter/gain
> > controls to the pipeline handler
> Have you looked at the work we have recently merged that introduced a
> frame context queue in libipa ? This has allowed us to solve a very
> similar issue in the RkISP1 AWB, which needs to know the colour gains
> that have been applied to the frame, and not just the latest gains (the
> hardware computes the AWB statistics after applying the colour
> gains...).
>

> The issue was fixed in [1], with [2] introducing the frame context queue
> in the AWB algorithm, and [3] providing documentation about how the
> active state and frame context collaborate. The frame context queue
> itself is very similar to the metadata array you introduce in patch 5/7
> in this series.
>

Sorry for the slightly late response. I was aware of the frame context queue
work that was flying around, and I just had a bit of a look through the
implementation.

The frame context queue mechanism does indeed look very similar to my
approach
here.  However, I think there are some fundamental differences:

1) FCQ looks to be integrated more tightly with the IPA algorithms.  This
would
be an enormous change for us.

2) FCQ seems to want to work in a look-ahead manner, from the docs:

"A frame context normally begins its lifetime when the corresponding request
 is queued, way before the frame is captured by the camera sensor."

In fact, one of my first approaches for this series was to do exactly this!
However, we were not comfortable with this for a few reasons.  Most notably,
it breaks our prepare/process pairing sequence of operation.  This would
cause
the prepare to run on stats that may not be very recent, the delay being
variable
based on the number of requests in-flight.  This also effectively breaks the
approach for our per-frame control implementation.  We will go into details
on
that later, but essentially we use a look-back approach instead of doing a
look-ahead as it seems far easier to implement.

3) The FCQ does not directly account for sensor delays and sequencing when
frame drops happen.  The DelayedControls gives us this for free.

We could do a half-way approach and store our controller metadata in the
FCQ to
replace this RPiMetadataArray, but does it gain us much?


>
> While I have no doubt this series fixes your issue, I'm concerned about
> how it relies on passing information through the DelayedControls class,
> which affects all pipeline handlers. The more we diverge in the usage of
> that class, the more difficult per-frame control will be difficult to
> implement. If the differences related to this issue between the
> Raspberry Pi on one side and RkISP1 and IPU3 on the other side were
> mostly in the IPA module I wouldn't be so concerned, as the IPA modules
> differ quite a lot already for historical reasons, but it expands beyond
> the IPA side here.
>

This is a valid concern here.  I certainly don't want to add functionality
that
is specific to RPi in the core, and if this is the case, we need to revert
back and
think of another approach.  However, I do think this generic cookie would
be a
useful addition for all pipeline handlers - fundamentally knowing when a
frame
has returned with the controls applied, accounting for frame drops and
delays.
Not to go too off topic, but our PFC implementation relies on this, and I
can see
other implementations needing the same.  Again, I don't want to jump the gun
too much on the PFC discussion just yet.


> Would you be able to have a look at the frame context queue ? Should we
> look together at how we could align the platforms better ?
>

Yes, it would be ideal to consolidate something that can be commonly used by
all IPAs.  I'll go through the existing implementation in more detail and
provide
any suggestions for expansion.

Regards,
Naush


>
> [1]
> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=290ebeb59525eb7fdcc6f815d8dee6cbe3d8a314
> [2]
> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=128f22bce55ba2baea082010bceaffdba23f3f0d
> [3]
> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=a2f34f19578e8dae37cb7898710d15b83f176f94
>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > Tested-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++++++--
> >  src/ipa/raspberrypi/raspberrypi.cpp        | 11 +++++++++++
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index bd54a639d637..b18bd7b5b19e 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -408,6 +408,12 @@ void Agc::switchMode(CameraMode const &cameraMode,
> >
> >  void Agc::prepare(Metadata *imageMetadata)
> >  {
> > +     Duration totalExposureValue = status_.totalExposureValue;
> > +     AgcStatus delayedStatus;
> > +
> > +     if (!imageMetadata->get("agc.delayed_status", delayedStatus))
> > +             totalExposureValue = delayedStatus.totalExposureValue;
> > +
> >       status_.digitalGain = 1.0;
> >       fetchAwbStatus(imageMetadata); /* always fetch it so that Process
> knows it's been done */
> >
> > @@ -418,8 +424,8 @@ void Agc::prepare(Metadata *imageMetadata)
> >                       Duration actualExposure =
> deviceStatus.shutterSpeed *
> >
>  deviceStatus.analogueGain;
> >                       if (actualExposure) {
> > -                             status_.digitalGain =
> status_.totalExposureValue / actualExposure;
> > -                             LOG(RPiAgc, Debug) << "Want total exposure
> " << status_.totalExposureValue;
> > +                             status_.digitalGain = totalExposureValue /
> actualExposure;
> > +                             LOG(RPiAgc, Debug) << "Want total exposure
> " << totalExposureValue;
> >                               /*
> >                                * Never ask for a gain < 1.0, and also
> impose
> >                                * some upper limit. Make it customisable?
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 0e80ef9c7b95..bc438e99eff6 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -1025,6 +1025,17 @@ void IPARPi::prepareISP(const ISPConfig &data)
> >               embeddedBuffer = it->second.planes()[0];
> >       }
> >
> > +     /*
> > +      * AGC wants to know the algorithm status from the time it
> actioned the
> > +      * sensor exposure/gain changes. So fetch it from the metadata list
> > +      * indexed by the IPA cookie returned, and put it in the current
> frame
> > +      * metadata.
> > +      */
> > +     AgcStatus agcStatus;
> > +     RPiController::Metadata &delayedMetadata =
> rpiMetadata_[data.ipaCookie];
> > +     if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus))
> > +             rpiMetadata.set("agc.delayed_status", agcStatus);
> > +
> >       /*
> >        * This may overwrite the DeviceStatus using values from the sensor
> >        * metadata, and may also do additional custom processing.
>
> --
> Regards,
>
> Laurent Pinchart
>