<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 5 Jul 2021 at 12:47, 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 patch!<br>
<br>
On Fri, 2 Jul 2021 at 16:09, 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 used.<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 | 95 +++++++++++++++++++++++<br>
> 1 file changed, 95 insertions(+)<br>
><br>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> index 91d05d9226ff..ddf0863c11b4 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> @@ -6,14 +6,23 @@<br>
> */<br>
><br>
> #include <assert.h><br>
> +#include <cmath><br>
> #include <stddef.h><br>
> #include <stdio.h><br>
> #include <stdlib.h><br>
><br>
> +#include <libcamera/base/log.h><br>
> +<br>
> #include "cam_helper.hpp"<br>
> #include "md_parser.hpp"<br>
><br>
> using namespace RPiController;<br>
> +using namespace libcamera;<br>
> +using libcamera::utils::Duration;<br>
> +<br>
> +namespace libcamera {<br>
> +LOG_DECLARE_CATEGORY(IPARPI)<br>
> +}<br>
><br>
> /*<br>
> * We care about two gain registers and a pair of exposure registers. Their<br>
> @@ -34,6 +43,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>
> @@ -44,6 +56,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>
> void PopulateMetadata(const MdParser::RegisterMap ®isters,<br>
> Metadata &metadata) const override;<br>
> @@ -64,6 +80,85 @@ 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>
> + MdParser::RegisterMap registers;<br>
> + Metadata parsedMetadata;<br>
> +<br>
> + if (buffer.empty())<br>
> + return;<br>
> +<br>
> + if (parser_->Parse(buffer, registers) != MdParser::Status::OK) {<br>
> + LOG(IPARPI, Error) << "Embedded data buffer parsing failed";<br>
> + return;<br>
> + }<br>
> +<br>
> + PopulateMetadata(registers, parsedMetadata);<br>
> + metadata.Merge(parsedMetadata);<br>
> +<br>
> + DeviceStatus deviceStatus, parsedDeviceStatus;<br>
> + if (metadata.Get("device.status", deviceStatus) ||<br>
> + parsedMetadata.Get("device.status", parsedDeviceStatus)) {<br>
> + LOG(IPARPI, Error) << "DeviceStatus not found";<br>
> + return;<br>
> + }<br>
> +<br>
> + /*<br>
> + * The DeviceStatus struct is first populated with values obtained from<br>
> + * DelayedControls. If this reports frame length is > frameLengthMax,<br>
> + * it means we are using a long exposure mode. Since the long exposure<br>
> + * scale factor is not returned back through embedded data, we must rely<br>
> + * on the existing exposure lines and frame length values returned by<br>
> + * DelayedControls.<br>
> + *<br>
> + * Otherwise, all values are updated with what is reported in the<br>
> + * embedded data.<br>
> + */<br>
> + if (deviceStatus.frame_length <= frameLengthMax) {<br>
> + deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;<br>
> + deviceStatus.frame_length = parsedDeviceStatus.frame_length;<br>
> + }<br>
> + deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;<br>
> +<br>
> + LOG(IPARPI, Debug) << "Metadata updated - " << deviceStatus;<br>
> +<br>
> + metadata.Set("device.status", deviceStatus);<br>
> +}<br>
<br>
I guess my only feeling about this is how much it copies from the base<br>
class version (well, parseEmbeddedData, if that still exists?). I<br>
expect you've stared at this yourself for far too long and are<br>
probably quite sick of it by now!!<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>
Here's one last thought, but please feel free to ignore it because<br>
I'll be fine with what you have above!<br>
<br>
The thing is, our default parseEmbeddedData method gives embedded data<br>
a chance to overwrite what came from the delayed controls.</blockquote><div><br></div><div><div>I do agree that CamHelperImx477::Prepare() does look slightly similar to</div><div>the default CamHelper::parseEmbeddedData(). In fact, in the last revision,</div><div>the former would call the latter! I decided to do it the above way to be</div><div>slightly more efficient in how many times we touch the various metadata</div><div>buffers flying around within the general Prepare() operation. This way does</div><div>save 2 accesses, but that does not seem like much to be honest.</div><div><br></div></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"> What if we<br>
derived PopulateMetadata instead. I think we'd have to pass it the<br>
regular metadata too, but then it would get a chance to populate the<br>
parsedMetadata from whichever source it liked better. And everything<br>
else would be untouched. Possibly.<br></blockquote><div><br></div><div>I wouldn't mind doing this, except I don't really like passing in two metadata</div><div>buffers into PopulateMetadata(), it just feels wrong to me.</div><div><br></div><div>Perhaps I should put back the original logic where CamHelperImx477::Prepare()</div><div>would call CamHelper::parseEmbeddedData() but at the expense of a</div><div>few more accesses to the metadata buffers?</div><div><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">
But please don't feel obliged. Maybe that's just more devious than we<br>
want - I'll leave it up to you!<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>
> + if (shift) {<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>
I think I asked once before about qualifying these calls with the base<br>
class name when it isn't necessary. Do we do it elsewhere? Does it<br>
make you worry that these methods might be virtual when they aren't?<br>
But I really don't mind, and am happy either way.<br></blockquote><div><br></div><div>You are right, it should be fine to remove the base class name from this</div><div>call. I will amend that in the next revision.</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>
As I said, I'm fine with this as it stands, so:<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>
> +<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>