[libcamera-devel] [PATCH v5 4/8] ipa: raspberrypi: Add frame_length to DeviceStatus
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Jul 9 15:31:20 CEST 2021
Hi Naush,
On 09/07/2021 14:27, Naushir Patuck wrote:
> Hi Kieran,
>
> Thank you for your review feedback.
>
> On Fri, 9 Jul 2021 at 14:17, Kieran Bingham
> <kieran.bingham at ideasonboard.com
> <mailto:kieran.bingham at ideasonboard.com>> wrote:
>
> Hi Naush,
>
> On 09/07/2021 10:56, Naushir Patuck wrote:
> > Update the IPA to fill frame length into the DeviceStatus struct
> from the
> > VBLANK Control value passed from DelayedControls.
> >
> > Update imx477 and imx219 CamHelper classes to extract the frame
> length from the
> > embedded data buffer.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com
> <mailto:naush at raspberrypi.com>>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com
> <mailto:david.plowman at raspberrypi.com>>
> > ---
> > src/ipa/raspberrypi/cam_helper.cpp | 1 +
> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +++++-
> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 6 +++++-
> > src/ipa/raspberrypi/controller/device_status.h | 5 ++++-
> > src/ipa/raspberrypi/raspberrypi.cpp | 2 ++
> > 5 files changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > index 1ec3f03e1aa3..3c6afce7572b 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -187,6 +187,7 @@ void CamHelper::parseEmbeddedData(Span<const
> uint8_t> buffer,
> >
> > deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
> > deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
> > + deviceStatus.frame_length = parsedDeviceStatus.frame_length;
> >
> > LOG(IPARPI, Debug) << "Metadata updated - " << deviceStatus;
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > index 4d68e01fce71..a3caab714602 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > @@ -30,7 +30,10 @@ using namespace RPiController;
> > constexpr uint32_t gainReg = 0x157;
> > constexpr uint32_t expHiReg = 0x15a;
> > constexpr uint32_t expLoReg = 0x15b;
> > -constexpr std::initializer_list<uint32_t> registerList
> [[maybe_unused]] = { expHiReg, expLoReg, gainReg };
> > +constexpr uint32_t frameLengthHiReg = 0x160;
> > +constexpr uint32_t frameLengthLoReg = 0x161;
> > +constexpr std::initializer_list<uint32_t> registerList
> [[maybe_unused]]
>
> is the maybe_unused still applicable?
> If it's not used - what is the declaration for ?
>
>
> Yes it is still needed. There is a #define EMABLE_EMBEDDED_DATA switch
> at the top which disables the embedded data parsing path, which is
> currently set.
> We have seen some problems with the imx219 embedded data values sometimes
> having junk values in them. This needs investigating/fixing before we
> can turn it
> back on again.
>
>
>
>
> > + = { expHiReg, expLoReg, gainReg, frameLengthHiReg,
> frameLengthLoReg };
> >
> > class CamHelperImx219 : public CamHelper
> > {
> > @@ -93,6 +96,7 @@ void CamHelperImx219::PopulateMetadata(const
> MdParser::RegisterMap ®isters,
> >
> > deviceStatus.shutter_speed = Exposure(registers.at
> <http://registers.at>(expHiReg) * 256 + registers.at
> <http://registers.at>(expLoReg));
> > deviceStatus.analogue_gain = Gain(registers.at
> <http://registers.at>(gainReg));
> > + deviceStatus.frame_length = registers.at
> <http://registers.at>(frameLengthHiReg) * 256 + registers.at
> <http://registers.at>(frameLengthLoReg);
> >
> > metadata.Set("device.status", deviceStatus);
> > }
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > index efd1a5893db8..91d05d9226ff 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > @@ -23,7 +23,10 @@ constexpr uint32_t expHiReg = 0x0202;
> > constexpr uint32_t expLoReg = 0x0203;
> > constexpr uint32_t gainHiReg = 0x0204;
> > constexpr uint32_t gainLoReg = 0x0205;
> > -constexpr std::initializer_list<uint32_t> registerList = {
> expHiReg, expLoReg, gainHiReg, gainLoReg };
> > +constexpr uint32_t frameLengthHiReg = 0x0340;
> > +constexpr uint32_t frameLengthLoReg = 0x0341;
> > +constexpr std::initializer_list<uint32_t> registerList =
> > + { expHiReg, expLoReg, gainHiReg, gainLoReg,
> frameLengthHiReg, frameLengthLoReg };
> >
> > class CamHelperImx477 : public CamHelper
> > {
> > @@ -81,6 +84,7 @@ void CamHelperImx477::PopulateMetadata(const
> MdParser::RegisterMap ®isters,
> >
> > deviceStatus.shutter_speed = Exposure(registers.at
> <http://registers.at>(expHiReg) * 256 + registers.at
> <http://registers.at>(expLoReg));
> > deviceStatus.analogue_gain = Gain(registers.at
> <http://registers.at>(gainHiReg) * 256 + registers.at
> <http://registers.at>(gainLoReg));
> > + deviceStatus.frame_length = registers.at
> <http://registers.at>(frameLengthHiReg) * 256 + registers.at
> <http://registers.at>(frameLengthLoReg);
> >
> > metadata.Set("device.status", deviceStatus);
> > }
> > diff --git a/src/ipa/raspberrypi/controller/device_status.h
> b/src/ipa/raspberrypi/controller/device_status.h
> > index e2511d19b96d..916471ab258b 100644
> > --- a/src/ipa/raspberrypi/controller/device_status.h
> > +++ b/src/ipa/raspberrypi/controller/device_status.h
> > @@ -17,7 +17,7 @@
> >
> > struct DeviceStatus {
> > DeviceStatus()
> > - : shutter_speed(std::chrono::seconds(0)),
> analogue_gain(0.0),
> > + : shutter_speed(std::chrono::seconds(0)),
> frame_length(0), analogue_gain(0.0),
> > lens_position(0.0), aperture(0.0),
> flash_intensity(0.0)
> > {
> > }
> > @@ -28,6 +28,7 @@ struct DeviceStatus {
> >
> > out << "Exposure: " << d.shutter_speed
> > << " Gain: " << d.analogue_gain
> > + << " Frame Length: " << d.frame_length
>
> It's always better to be doing this in one place isn't it ;-)
>
>
> > << " Aperture: " << d.aperture
> > << " Lens: " << d.lens_position
> > << " Flash: " << d.flash_intensity;
> > @@ -37,6 +38,8 @@ struct DeviceStatus {
> >
> > /* time shutter is open */
> > libcamera::utils::Duration shutter_speed;
> > + // frame length given in number of lines
>
> Mixing C / C++ comment style here ?
>
>
> Argh. David also pointed this out, and I forgot to add it in this revision.
> Will fix it in the next review :-)
>
Add
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com> when it's
done too ;-)
> Regards,
> Naush
>
>
>
> > + uint32_t frame_length;
> > double analogue_gain;
> > /* 1.0/distance-in-metres, or 0 if unknown */
> > double lens_position;
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index f51c970befb5..db103a885b7a 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -1015,9 +1015,11 @@ void IPARPi::fillDeviceStatus(const
> ControlList &sensorControls)
> >
> > int32_t exposureLines =
> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > int32_t gainCode =
> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> > + int32_t vblank =
> sensorControls.get(V4L2_CID_VBLANK).get<int32_t>();
> >
> > deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> > deviceStatus.analogue_gain = helper_->Gain(gainCode);
> > + deviceStatus.frame_length = mode_.height + vblank;
> >
> > LOG(IPARPI, Debug) << "Metadata - " << deviceStatus;
> >
> >
>
More information about the libcamera-devel
mailing list