<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 Fri, 2 Jul 2021 at 17:09, 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 update - I can see that this change has become more<br>
troublesome than initially expected!<br></blockquote><div><br></div><div>Indeed it has :(</div><div>But frame length is probably important enough that we should record it down.</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>
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 IPA to fill frame length into the DeviceStatus struct from the<br>
> VBLANK Control value passed from DelayedControls.<br>
><br>
> Update imx477 and imx219 CamHelper classes to extract the frame length from the<br>
> embedded data buffer.<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_imx219.cpp | 6 +++++-<br>
> src/ipa/raspberrypi/cam_helper_imx477.cpp | 6 +++++-<br>
> src/ipa/raspberrypi/controller/device_status.h | 5 ++++-<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 2 ++<br>
> 4 files changed, 16 insertions(+), 3 deletions(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> index 4d68e01fce71..a3caab714602 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> @@ -30,7 +30,10 @@ using namespace RPiController;<br>
> constexpr uint32_t gainReg = 0x157;<br>
> constexpr uint32_t expHiReg = 0x15a;<br>
> constexpr uint32_t expLoReg = 0x15b;<br>
> -constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] = { expHiReg, expLoReg, gainReg };<br>
> +constexpr uint32_t frameLengthHiReg = 0x160;<br>
> +constexpr uint32_t frameLengthLoReg = 0x161;<br>
> +constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]]<br>
> + = { expHiReg, expLoReg, gainReg, frameLengthHiReg, frameLengthLoReg };<br>
<br>
Didn't we have a thing about these registers having to be in numerical<br>
order? So gainReg would have to go first. Though I'm assuming that we<br>
still aren't using metadata for the imx219 so it doesn't actually<br>
matter - but we should probably get it the right way round.<br></blockquote><div><br></div><div>With my last set of updates (i.e. C++ifying and generalising the SMIA parser),</div><div>these registers can be listed in any order. </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>
(Should probably have spotted that on a previous review - oh well, sorry!)<br>
<br>
><br>
> class CamHelperImx219 : public CamHelper<br>
> {<br>
> @@ -93,6 +96,7 @@ void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap ®isters,<br>
><br>
> deviceStatus.shutter_speed = Exposure(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expLoReg));<br>
> deviceStatus.analogue_gain = Gain(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(gainReg));<br>
> + deviceStatus.frame_length = <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(frameLengthHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(frameLengthLoReg);<br>
><br>
> metadata.Set("device.status", deviceStatus);<br>
> }<br>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> index 4098fde6f322..91d05d9226ff 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> @@ -23,7 +23,10 @@ constexpr uint32_t expHiReg = 0x0202;<br>
> constexpr uint32_t expLoReg = 0x0203;<br>
> constexpr uint32_t gainHiReg = 0x0204;<br>
> constexpr uint32_t gainLoReg = 0x0205;<br>
> -constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] = { expHiReg, expLoReg, gainHiReg, gainLoReg };<br>
> +constexpr uint32_t frameLengthHiReg = 0x0340;<br>
> +constexpr uint32_t frameLengthLoReg = 0x0341;<br>
> +constexpr std::initializer_list<uint32_t> registerList =<br>
> + { expHiReg, expLoReg, gainHiReg, gainLoReg, frameLengthHiReg, frameLengthLoReg };<br>
><br>
> class CamHelperImx477 : public CamHelper<br>
> {<br>
> @@ -81,6 +84,7 @@ void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap ®isters,<br>
><br>
> deviceStatus.shutter_speed = Exposure(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(expLoReg));<br>
> deviceStatus.analogue_gain = Gain(<a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(gainHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(gainLoReg));<br>
> + deviceStatus.frame_length = <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(frameLengthHiReg) * 256 + <a href="http://registers.at" rel="noreferrer" target="_blank">registers.at</a>(frameLengthLoReg);<br>
><br>
> metadata.Set("device.status", deviceStatus);<br>
> }<br>
> diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h<br>
> index e2511d19b96d..916471ab258b 100644<br>
> --- a/src/ipa/raspberrypi/controller/device_status.h<br>
> +++ b/src/ipa/raspberrypi/controller/device_status.h<br>
> @@ -17,7 +17,7 @@<br>
><br>
> struct DeviceStatus {<br>
> DeviceStatus()<br>
> - : shutter_speed(std::chrono::seconds(0)), analogue_gain(0.0),<br>
> + : shutter_speed(std::chrono::seconds(0)), frame_length(0), analogue_gain(0.0),<br>
> lens_position(0.0), aperture(0.0), flash_intensity(0.0)<br>
> {<br>
> }<br>
> @@ -28,6 +28,7 @@ struct DeviceStatus {<br>
><br>
> out << "Exposure: " << d.shutter_speed<br>
> << " Gain: " << d.analogue_gain<br>
> + << " Frame Length: " << d.frame_length<br>
> << " Aperture: " << d.aperture<br>
> << " Lens: " << d.lens_position<br>
> << " Flash: " << d.flash_intensity;<br>
> @@ -37,6 +38,8 @@ struct DeviceStatus {<br>
><br>
> /* time shutter is open */<br>
> libcamera::utils::Duration shutter_speed;<br>
> + // frame length given in number of lines<br>
<br>
Didn't I see a patch go by where you carefully replaced C++ comments<br>
by C-style ones? :)<br>
Or perhaps that was a later patch... I can't even remember!<br></blockquote><div><br></div><div>Groan, yes, the first patch did this, and I went and used C++ comments</div><div>in this one. Will fix it 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>
With those little things addressed:<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>
> + uint32_t frame_length;<br>
> double analogue_gain;<br>
> /* 1.0/distance-in-metres, or 0 if unknown */<br>
> double lens_position;<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index f51c970befb5..db103a885b7a 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -1015,9 +1015,11 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)<br>
><br>
> int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();<br>
> int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();<br>
> + int32_t vblank = sensorControls.get(V4L2_CID_VBLANK).get<int32_t>();<br>
><br>
> deviceStatus.shutter_speed = helper_->Exposure(exposureLines);<br>
> deviceStatus.analogue_gain = helper_->Gain(gainCode);<br>
> + deviceStatus.frame_length = mode_.height + vblank;<br>
><br>
> LOG(IPARPI, Debug) << "Metadata - " << deviceStatus;<br>
><br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>