<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 9 Jul 2021 at 14:17, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@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>
On 09/07/2021 10:56, Naushir Patuck wrote:<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>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/cam_helper.cpp | 1 +<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>
> 5 files changed, 17 insertions(+), 3 deletions(-)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp<br>
> index 1ec3f03e1aa3..3c6afce7572b 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
> @@ -187,6 +187,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,<br>
> <br>
> deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;<br>
> deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;<br>
> + deviceStatus.frame_length = parsedDeviceStatus.frame_length;<br>
> <br>
> LOG(IPARPI, Debug) << "Metadata updated - " << deviceStatus;<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>
<br>
is the maybe_unused still applicable?<br>
If it's not used - what is the declaration for ?<br></blockquote><div><br></div><div>Yes it is still needed. There is a #define EMABLE_EMBEDDED_DATA switch</div><div>at the top which disables the embedded data parsing path, which is currently set.</div><div>We have seen some problems with the imx219 embedded data values sometimes</div><div>having junk values in them. This needs investigating/fixing before we can turn it</div><div>back on again.</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>
> + = { expHiReg, expLoReg, gainReg, frameLengthHiReg, frameLengthLoReg };<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 efd1a5893db8..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 = { 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>
<br>
It's always better to be doing this in one place isn't it ;-)<br>
<br>
<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>
Mixing C / C++ comment style here ?<br></blockquote><div><br></div><div>Argh. David also pointed this out, and I forgot to add it in this revision.</div><div>Will fix it in the next review :-)</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>
> + 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>
</blockquote></div></div>