<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your feedback!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 15 Jun 2021 at 14:28, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.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>
Thanks for this feature!<br>
<br>
On Mon, 14 Jun 2021 at 11:00, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Update the imx477 CamHelper to use long exposure modes if needed.<br>
> This is done by overloading the CamHelper::GetVBlanking function to return a<br>
> frame length (and vblank value) computed using a scaling factor when the value<br>
> would be larger than what the sensor register could otherwise hold.<br>
><br>
> CamHelperImx477::Prepare is also overloaded to ensure that the "device.status"<br>
> metadata returns the right value if the long exposure scaling factor is use.<br>
> The scaling factor is unfortunately not returned back in metadata.<br>
><br>
> With the current imx477 driver, we can achieve a maximum exposure time of approx<br>
> 127 seconds since the HBLANK control is read-only.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/cam_helper_imx477.cpp | 67 +++++++++++++++++++++++<br>
> 1 file changed, 67 insertions(+)<br>
><br>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> index 7a1100c25afc..549c2620765a 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> @@ -6,6 +6,7 @@<br>
> */<br>
><br>
> #include <assert.h><br>
> +#include <cmath><br>
> #include <stddef.h><br>
> #include <stdio.h><br>
> #include <stdlib.h><br>
> @@ -14,6 +15,7 @@<br>
> #include "md_parser.hpp"<br>
><br>
> using namespace RPiController;<br>
> +using libcamera::utils::Duration;<br>
><br>
> /*<br>
> * We care about two gain registers and a pair of exposure registers. Their<br>
> @@ -30,6 +32,9 @@ public:<br>
> CamHelperImx477();<br>
> uint32_t GainCode(double gain) const override;<br>
> double Gain(uint32_t gain_code) const override;<br>
> + void Prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) override;<br>
> + uint32_t GetVBlanking(Duration &exposure, Duration minFrameDuration,<br>
> + Duration maxFrameDuration) const override;<br>
> void GetDelays(int &exposure_delay, int &gain_delay,<br>
> int &vblank_delay) const override;<br>
> bool SensorEmbeddedDataPresent() const override;<br>
> @@ -40,6 +45,10 @@ private:<br>
> * in units of lines.<br>
> */<br>
> static constexpr int frameIntegrationDiff = 22;<br>
> + /* Maximum frame length allowable for long exposure calculations. */<br>
> + static constexpr int frameLengthMax = 0xffdc;<br>
> + /* Largest long exposure scale factor given as a left shift on the frame length. */<br>
> + static constexpr int longExposureShiftMax = 7;<br>
><br>
> MdParserSmia imx477_parser;<br>
><br>
> @@ -72,6 +81,64 @@ double CamHelperImx477::Gain(uint32_t gain_code) const<br>
> return 1024.0 / (1024 - gain_code);<br>
> }<br>
><br>
> +void CamHelperImx477::Prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata)<br>
> +{<br>
> + DeviceStatus deviceStatus, parsedStatus;<br>
> +<br>
> + /* Get the device status provided by DelayedControls */<br>
> + if (metadata.Get("device.status", deviceStatus) != 0)<br>
> + return;<br>
> +<br>
> + /* Get the device status provided by the embedded data buffer. */<br>
> + CamHelper::parseEmbeddedData(buffer, metadata);<br>
> + metadata.Get("device.status", parsedStatus);<br>
> +<br>
> + /*<br>
> + * If the ratio of DelayedControls to embedded data shutter speed is > 1<br>
> + * and is a factor of 2^N, then we can assume this is a long exposure mode<br>
> + * frame. Since embedded data does not provide any hints of long exposure<br>
> + * modes, make sure we use the DelayedControls values in the metadata.<br>
> + * Otherwise, just go with the embedded data values.<br>
> + */<br>
> + unsigned long ratio = std::lround(deviceStatus.shutter_speed / parsedStatus.shutter_speed);<br>
> + bool replace = (ratio > 1) && ((ratio & (~ratio + 1)) == ratio);<br>
<br>
Is there a case we need to worry about where "ratio" was only rounded<br>
to a power of 2, but wouldn't otherwise have been one? (e.g.<br>
deviceStatus.shutter_speed = 11, parsedStatus.shutter_speed = 5)<br></blockquote><div><br></div><div>The ratio must be a power of 2 or exactly 1 for the imx477. Any other value</div><div>and something has gone wrong :-) In the failure case, I assume that the</div><div>embedded data values are to be trusted and use them over DelatedControl values.</div><div>Perhaps it may be worth adding a log point if this condition ever hits?</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>
> +<br>
> + if (replace)<br>
> + metadata.Set("device.status", deviceStatus);<br>
> +}<br>
> +<br>
> +uint32_t CamHelperImx477::GetVBlanking(Duration &exposure,<br>
> + Duration minFrameDuration,<br>
> + Duration maxFrameDuration) const<br>
> +{<br>
> + uint32_t frameLength, exposureLines;<br>
> + unsigned int shift = 0;<br>
> +<br>
> + frameLength = mode_.height + CamHelper::GetVBlanking(exposure, minFrameDuration,<br>
> + maxFrameDuration);<br>
> + /*<br>
> + * Check if the frame length calculated needs to be setup for long<br>
> + * exposure mode. This will require us to use a long exposure scale<br>
> + * factor provided by a shift operation in the sensor.<br>
> + */<br>
> + while (frameLength > frameLengthMax) {<br>
> + if (++shift > longExposureShiftMax) {<br>
> + shift = longExposureShiftMax;<br>
> + frameLength = frameLengthMax;<br>
> + break;<br>
> + }<br>
> + frameLength >>= 1;<br>
> + }<br>
> +<br>
> + /* Account for any rounding in the scaled frame length value. */<br>
> + frameLength <<= shift;<br>
> + exposureLines = CamHelper::ExposureLines(exposure);<br>
> + exposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);<br>
> + exposure = CamHelper::Exposure(exposureLines);<br>
<br>
Any reason for invoking the base class functions explicitly here? Not<br>
that it bothers me, just curious...<br></blockquote><div><br></div><div>I use CamHelper::GetVBlanking to compute the vblank and exposure values with</div><div>correct clipping and rounding based on the frame length limits. I could possibly move</div><div>that entire calculation into this function, but it would be very similar, so thought it</div><div>would be better to call the base class implementation.</div><div><br></div><div>Regards,</div><div>Naush</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Subject to clarification on my ratio rounding question:<br>
<br>
Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Thanks!<br>
David<br>
<br>
> +<br>
> + return frameLength - mode_.height;<br>
> +}<br>
> +<br>
> void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,<br>
> int &vblank_delay) const<br>
> {<br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>