| Message ID | 20221019090107.19975-8-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:07) > 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> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.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 aed8f68aded9..983745fcad98 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -1029,6 +1029,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.delayCookie]; > + 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. > -- > 2.25.1 >
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 aed8f68aded9..983745fcad98 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -1029,6 +1029,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.delayCookie]; + 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.