[libcamera-devel] [PATCH v3 4/8] ipa: raspberrypi: Add frame_length to DeviceStatus

Naushir Patuck naush at raspberrypi.com
Fri Jul 2 18:17:01 CEST 2021 

Hi David,

Thank you for your feedback.

On Fri, 2 Jul 2021 at 17:09, David Plowman <david.plowman at raspberrypi.com>
wrote:

> Hi Naush
>
> Thanks for this update - I can see that this change has become more
> troublesome than initially expected!
>

Indeed it has :(
But frame length is probably important enough that we should record it down.


> On Fri, 2 Jul 2021 at 16:09, Naushir Patuck <naush at raspberrypi.com> 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>
> > ---
> >  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 ++
> >  4 files changed, 16 insertions(+), 3 deletions(-)
> >
> > 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]]
> > +       = { expHiReg, expLoReg, gainReg, frameLengthHiReg,
> frameLengthLoReg };
>
> Didn't we have a thing about these registers having to be in numerical
> order? So gainReg would have to go first. Though I'm assuming that we
> still aren't using metadata for the imx219 so it doesn't actually
> matter - but we should probably get it the right way round.
>

With my last set of updates (i.e. C++ifying and generalising the SMIA
parser),
these registers can be listed in any order.


>
> (Should probably have spotted that on a previous review - oh well, sorry!)
>
> >
> >  class CamHelperImx219 : public CamHelper
> >  {
> > @@ -93,6 +96,7 @@ void CamHelperImx219::PopulateMetadata(const
> MdParser::RegisterMap &registers,
> >
> >         deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) *
> 256 + registers.at(expLoReg));
> >         deviceStatus.analogue_gain = Gain(registers.at(gainReg));
> > +       deviceStatus.frame_length = registers.at(frameLengthHiReg) *
> 256 + 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 4098fde6f322..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 [[maybe_unused]]
> = { 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 &registers,
> >
> >         deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) *
> 256 + registers.at(expLoReg));
> >         deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256
> + registers.at(gainLoReg));
> > +       deviceStatus.frame_length = registers.at(frameLengthHiReg) *
> 256 + 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
> >                     << " 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
>
> Didn't I see a patch go by where you carefully replaced C++ comments
> by C-style ones?  :)
> Or perhaps that was a later patch... I can't even remember!
>

Groan, yes, the first patch did this, and I went and used C++ comments
in this one.  Will fix it in the next revision.

Regards,
Naush


>
> With those little things addressed:
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks!
> David
>
> > +       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;
> >
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210702/b9e53585/attachment-0001.htm>

More information about the libcamera-devel mailing list