<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 4 Oct 2022 at 17:21, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Mon, Sep 26, 2022 at 10:57:37AM +0100, Naushir Patuck via libcamera-devel wrote:<br>
> The digital gain calculation uses a total exposure value computed with the<br>
> current AGC state. However, this is wrong in the case of manual shutter/gain<br>
> controls, as the total exposure value used must be the value computed when the<br>
> AGC sent the manual shutter/gain controls to the pipeline handler to action.<br>
> <br>
> To fix this, the IPA now adds the historical AgcStatus structure to the metadata<br>
> (tagged with "agc.delayed_status"). This historical AgcStatus structure contains<br>
> the total exposure value calculated when the AGC sent the manual shutter/gain<br>
> controls to the pipeline handler<br>
Have you looked at the work we have recently merged that introduced a<br>
frame context queue in libipa ? This has allowed us to solve a very<br>
similar issue in the RkISP1 AWB, which needs to know the colour gains<br>
that have been applied to the frame, and not just the latest gains (the<br>
hardware computes the AWB statistics after applying the colour<br>
gains...).<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
The issue was fixed in [1], with [2] introducing the frame context queue<br>
in the AWB algorithm, and [3] providing documentation about how the<br>
active state and frame context collaborate. The frame context queue<br>
itself is very similar to the metadata array you introduce in patch 5/7<br>
in this series.<br></blockquote><div><br></div><div>Sorry for the slightly late response. I was aware of the frame context queue<br>work that was flying around, and I just had a bit of a look through the<br>implementation.<br><br>The frame context queue mechanism does indeed look very similar to my approach<br>here. However, I think there are some fundamental differences:<br><br>1) FCQ looks to be integrated more tightly with the IPA algorithms. This would<br>be an enormous change for us.<br><br>2) FCQ seems to want to work in a look-ahead manner, from the docs:<br><br>"A frame context normally begins its lifetime when the corresponding request<br> is queued, way before the frame is captured by the camera sensor."<br><br>In fact, one of my first approaches for this series was to do exactly this!<br>However, we were not comfortable with this for a few reasons. Most notably,<br>it breaks our prepare/process pairing sequence of operation. This would cause<br>the prepare to run on stats that may not be very recent, the delay being variable<br>based on the number of requests in-flight. This also effectively breaks the<br>approach for our per-frame control implementation. We will go into details on<br>that later, but essentially we use a look-back approach instead of doing a<br>look-ahead as it seems far easier to implement.</div><div><br></div><div>3) The FCQ does not directly account for sensor delays and sequencing when</div><div>frame drops happen. The DelayedControls gives us this for free.</div><div><br>We could do a half-way approach and store our controller metadata in the FCQ to<br>replace this RPiMetadataArray, but does it gain us much?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
While I have no doubt this series fixes your issue, I'm concerned about<br>
how it relies on passing information through the DelayedControls class,<br>
which affects all pipeline handlers. The more we diverge in the usage of<br>
that class, the more difficult per-frame control will be difficult to<br>
implement. If the differences related to this issue between the<br>
Raspberry Pi on one side and RkISP1 and IPU3 on the other side were<br>
mostly in the IPA module I wouldn't be so concerned, as the IPA modules<br>
differ quite a lot already for historical reasons, but it expands beyond<br>
the IPA side here.<br></blockquote><div><br></div><div>This is a valid concern here. I certainly don't want to add functionality that<br>is specific to RPi in the core, and if this is the case, we need to revert back and<br>think of another approach. However, I do think this generic cookie would be a<br>useful addition for all pipeline handlers - fundamentally knowing when a frame<br>has returned with the controls applied, accounting for frame drops and delays.</div><div>Not to go too off topic, but our PFC implementation relies on this, and I can see</div><div>other implementations needing the same. Again, I don't want to jump the gun</div><div>too much on the PFC discussion just yet.</div></div><div class="gmail_quote"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Would you be able to have a look at the frame context queue ? Should we<br>
look together at how we could align the platforms better ?<br></blockquote><div><br></div><div>Yes, it would be ideal to consolidate something that can be commonly used by</div><div>all IPAs. I'll go through the existing implementation in more detail and provide</div><div>any suggestions for expansion.</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[1] <a href="https://git.libcamera.org/libcamera/libcamera.git/commit/?id=290ebeb59525eb7fdcc6f815d8dee6cbe3d8a314" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/commit/?id=290ebeb59525eb7fdcc6f815d8dee6cbe3d8a314</a><br>
[2] <a href="https://git.libcamera.org/libcamera/libcamera.git/commit/?id=128f22bce55ba2baea082010bceaffdba23f3f0d" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/commit/?id=128f22bce55ba2baea082010bceaffdba23f3f0d</a><br>
[3] <a href="https://git.libcamera.org/libcamera/libcamera.git/commit/?id=a2f34f19578e8dae37cb7898710d15b83f176f94" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/commit/?id=a2f34f19578e8dae37cb7898710d15b83f176f94</a><br>
<br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++++++--<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 11 +++++++++++<br>
> 2 files changed, 19 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> index bd54a639d637..b18bd7b5b19e 100644<br>
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp<br>
> @@ -408,6 +408,12 @@ void Agc::switchMode(CameraMode const &cameraMode,<br>
> <br>
> void Agc::prepare(Metadata *imageMetadata)<br>
> {<br>
> + Duration totalExposureValue = status_.totalExposureValue;<br>
> + AgcStatus delayedStatus;<br>
> +<br>
> + if (!imageMetadata->get("agc.delayed_status", delayedStatus))<br>
> + totalExposureValue = delayedStatus.totalExposureValue;<br>
> +<br>
> status_.digitalGain = 1.0;<br>
> fetchAwbStatus(imageMetadata); /* always fetch it so that Process knows it's been done */<br>
> <br>
> @@ -418,8 +424,8 @@ void Agc::prepare(Metadata *imageMetadata)<br>
> Duration actualExposure = deviceStatus.shutterSpeed *<br>
> deviceStatus.analogueGain;<br>
> if (actualExposure) {<br>
> - status_.digitalGain = status_.totalExposureValue / actualExposure;<br>
> - LOG(RPiAgc, Debug) << "Want total exposure " << status_.totalExposureValue;<br>
> + status_.digitalGain = totalExposureValue / actualExposure;<br>
> + LOG(RPiAgc, Debug) << "Want total exposure " << totalExposureValue;<br>
> /*<br>
> * Never ask for a gain < 1.0, and also impose<br>
> * some upper limit. Make it customisable?<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 0e80ef9c7b95..bc438e99eff6 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -1025,6 +1025,17 @@ void IPARPi::prepareISP(const ISPConfig &data)<br>
> embeddedBuffer = it->second.planes()[0];<br>
> }<br>
> <br>
> + /*<br>
> + * AGC wants to know the algorithm status from the time it actioned the<br>
> + * sensor exposure/gain changes. So fetch it from the metadata list<br>
> + * indexed by the IPA cookie returned, and put it in the current frame<br>
> + * metadata.<br>
> + */<br>
> + AgcStatus agcStatus;<br>
> + RPiController::Metadata &delayedMetadata = rpiMetadata_[data.ipaCookie];<br>
> + if (!delayedMetadata.get<AgcStatus>("agc.status", agcStatus))<br>
> + rpiMetadata.set("agc.delayed_status", agcStatus);<br>
> +<br>
> /*<br>
> * This may overwrite the DeviceStatus using values from the sensor<br>
> * metadata, and may also do additional custom processing.<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>