[libcamera-devel] [PATCH v5 2/4] ipa: raspberrypi: Switch ipa and cam_helper to use utils::Duration
Naushir Patuck
naush at raspberrypi.com
Tue Jun 8 11:58:00 CEST 2021
On Tue, 8 Jun 2021 at 10:53, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> On Tue, Jun 08, 2021 at 10:47:47AM +0100, Naushir Patuck wrote:
> > On Tue, 8 Jun 2021 at 02:17, Laurent Pinchart wrote:
> > > On Tue, May 25, 2021 at 12:42:39PM +0100, Naushir Patuck wrote:
> > > > Switch the ipa and cam_helper code to use libcamera::utils::Duration
> for
> > > > all time based variables. This improves code readability and avoids
> > > > possible errors when converting between time bases.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > > ---
> > > > src/ipa/raspberrypi/cam_helper.cpp | 20 +++---
> > > > src/ipa/raspberrypi/cam_helper.hpp | 10 +--
> > > > src/ipa/raspberrypi/controller/camera_mode.h | 6 +-
> > > > src/ipa/raspberrypi/raspberrypi.cpp | 64
> +++++++++++---------
> > > > 4 files changed, 55 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > > > index 09917f3cc079..7b4331a87a42 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > > > @@ -18,6 +18,7 @@
> > > >
> > > > using namespace RPiController;
> > > > using namespace libcamera;
> > > > +using libcamera::utils::Duration;
> > >
> > > I would have used utils::Duration below, but I don't mind.
> > >
> > > >
> > > > namespace libcamera {
> > > > LOG_DECLARE_CATEGORY(IPARPI)
> > > > @@ -61,20 +62,21 @@ void CamHelper::Process([[maybe_unused]]
> StatisticsPtr &stats,
> > > > {
> > > > }
> > > >
> > > > -uint32_t CamHelper::ExposureLines(double exposure_us) const
> > > > +uint32_t CamHelper::ExposureLines(const Duration &exposure) const
> > >
> > > Given that the duration only stores a double value, you could pass it
> by
> > > value instead of by reference. The comment applies to the whole series.
> > > That would be my preference, but it's up to you.
> >
> > Agreed, I will change all functions to pass by value in the next
> revision.
> >
> > > Overall the code is really much nicer !
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > Thanks. Yes, this change does make time based variables so much easier
> > to work with! At some point, we should change libcamera controls to use
> > this :-)
>
> That's on my todo list :-)
>
> By the way, would it be possible to add a test case for the Duration
> class in the next version ?
>
Sure, I will add some unit tests in the next revision.
Naush
>
> > > > {
> > > > assert(initialized_);
> > > > - return exposure_us * 1000.0 / mode_.line_length;
> > > > + return exposure / mode_.line_length;
> > > > }
> > > >
> > > > -double CamHelper::Exposure(uint32_t exposure_lines) const
> > > > +Duration CamHelper::Exposure(uint32_t exposure_lines) const
> > > > {
> > > > assert(initialized_);
> > > > - return exposure_lines * mode_.line_length / 1000.0;
> > > > + return exposure_lines * mode_.line_length;
> > > > }
> > > >
> > > > -uint32_t CamHelper::GetVBlanking(double &exposure, double
> minFrameDuration,
> > > > - double maxFrameDuration) const
> > > > +uint32_t CamHelper::GetVBlanking(Duration &exposure,
> > > > + const Duration &minFrameDuration,
> > > > + const Duration &maxFrameDuration)
> const
> > > > {
> > > > uint32_t frameLengthMin, frameLengthMax, vblank;
> > > > uint32_t exposureLines = ExposureLines(exposure);
> > > > @@ -85,8 +87,8 @@ uint32_t CamHelper::GetVBlanking(double &exposure,
> double minFrameDuration,
> > > > * minFrameDuration and maxFrameDuration are clamped by the
> caller
> > > > * based on the limits for the active sensor mode.
> > > > */
> > > > - frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;
> > > > - frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;
> > > > + frameLengthMin = minFrameDuration / mode_.line_length;
> > > > + frameLengthMax = maxFrameDuration / mode_.line_length;
> > > >
> > > > /*
> > > > * Limit the exposure to the maximum frame duration requested,
> and
> > > > @@ -182,7 +184,7 @@ void CamHelper::parseEmbeddedData(Span<const
> uint8_t> buffer,
> > > > return;
> > > > }
> > > >
> > > > - deviceStatus.shutter_speed = Exposure(exposureLines);
> > > > + deviceStatus.shutter_speed =
> Exposure(exposureLines).get<std::micro>();
> > > > deviceStatus.analogue_gain = Gain(gainCode);
> > > >
> > > > LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp
> b/src/ipa/raspberrypi/cam_helper.hpp
> > > > index a52f3f0b583c..07481e4174a7 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > > > @@ -15,6 +15,7 @@
> > > > #include "controller/metadata.hpp"
> > > > #include "md_parser.hpp"
> > > >
> > > > +#include "libcamera/internal/utils.h"
> > > > #include "libcamera/internal/v4l2_videodevice.h"
> > > >
> > > > namespace RPiController {
> > > > @@ -72,10 +73,11 @@ public:
> > > > virtual void Prepare(libcamera::Span<const uint8_t> buffer,
> > > > Metadata &metadata);
> > > > virtual void Process(StatisticsPtr &stats, Metadata &metadata);
> > > > - uint32_t ExposureLines(double exposure_us) const;
> > > > - double Exposure(uint32_t exposure_lines) const; // in us
> > > > - virtual uint32_t GetVBlanking(double &exposure_us, double
> minFrameDuration,
> > > > - double maxFrameDuration) const;
> > > > + uint32_t ExposureLines(const libcamera::utils::Duration
> &exposure) const;
> > > > + libcamera::utils::Duration Exposure(uint32_t exposure_lines)
> const;
> > > > + virtual uint32_t GetVBlanking(libcamera::utils::Duration
> &exposure,
> > > > + const libcamera::utils::Duration
> &minFrameDuration,
> > > > + const libcamera::utils::Duration
> &maxFrameDuration) const;
> > > > virtual uint32_t GainCode(double gain) const = 0;
> > > > virtual double Gain(uint32_t gain_code) const = 0;
> > > > virtual void GetDelays(int &exposure_delay, int &gain_delay,
> > > > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h
> b/src/ipa/raspberrypi/controller/camera_mode.h
> > > > index 256438c931d9..2aa2335dcf90 100644
> > > > --- a/src/ipa/raspberrypi/controller/camera_mode.h
> > > > +++ b/src/ipa/raspberrypi/controller/camera_mode.h
> > > > @@ -8,6 +8,8 @@
> > > >
> > > > #include <libcamera/transform.h>
> > > >
> > > > +#include "libcamera/internal/utils.h"
> > > > +
> > > > // Description of a "camera mode", holding enough information for
> control
> > > > // algorithms to adapt their behaviour to the different modes of
> the camera,
> > > > // including binning, scaling, cropping etc.
> > > > @@ -33,8 +35,8 @@ struct CameraMode {
> > > > double scale_x, scale_y;
> > > > // scaling of the noise compared to the native sensor mode
> > > > double noise_factor;
> > > > - // line time in nanoseconds
> > > > - double line_length;
> > > > + // line time
> > > > + libcamera::utils::Duration line_length;
> > > > // any camera transform *not* reflected already in the camera
> tuning
> > > > libcamera::Transform transform;
> > > > // minimum and maximum fame lengths in units of lines
> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > index 52d91db282ea..4e02e94084f7 100644
> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > @@ -55,19 +55,22 @@
> > > >
> > > > namespace libcamera {
> > > >
> > > > +using namespace std::literals::chrono_literals;
> > > > +using utils::Duration;
> > > > +
> > > > /* Configure the sensor with these values initially. */
> > > > constexpr double DefaultAnalogueGain = 1.0;
> > > > -constexpr unsigned int DefaultExposureTime = 20000;
> > > > -constexpr double defaultMinFrameDuration = 1e6 / 30.0;
> > > > -constexpr double defaultMaxFrameDuration = 1e6 / 0.01;
> > > > +constexpr Duration DefaultExposureTime = 20.0ms;
> > > > +constexpr Duration defaultMinFrameDuration = 1.0s / 30.0;
> > > > +constexpr Duration defaultMaxFrameDuration = 100.0s;
> > > >
> > > > /*
> > > > - * Determine the minimum allowable inter-frame duration (in us) to
> run the
> > > > - * controller algorithms. If the pipeline handler provider frames
> at a rate
> > > > - * higher than this, we rate-limit the controller Prepare() and
> Process() calls
> > > > - * to lower than or equal to this rate.
> > > > + * Determine the minimum allowable inter-frame duration to run the
> controller
> > > > + * algorithms. If the pipeline handler provider frames at a rate
> higher than this,
> > > > + * we rate-limit the controller Prepare() and Process() calls to
> lower than or
> > > > + * equal to this rate.
> > > > */
> > > > -constexpr double controllerMinFrameDuration = 1e6 / 60.0;
> > > > +constexpr Duration controllerMinFrameDuration = 1.0s / 60.0;
> > > >
> > > > LOG_DEFINE_CATEGORY(IPARPI)
> > > >
> > > > @@ -111,7 +114,8 @@ private:
> > > > void reportMetadata();
> > > > void fillDeviceStatus(const ControlList &sensorControls);
> > > > void processStats(unsigned int bufferId);
> > > > - void applyFrameDurations(double minFrameDuration, double
> maxFrameDuration);
> > > > + void applyFrameDurations(const Duration &minFrameDuration,
> > > > + const Duration &maxFrameDuration);
> > > > void applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls);
> > > > void applyAWB(const struct AwbStatus *awbStatus, ControlList
> &ctrls);
> > > > void applyDG(const struct AgcStatus *dgStatus, ControlList
> &ctrls);
> > > > @@ -167,9 +171,9 @@ private:
> > > > /* Distinguish the first camera start from others. */
> > > > bool firstStart_;
> > > >
> > > > - /* Frame duration (1/fps) limits, given in microseconds. */
> > > > - double minFrameDuration_;
> > > > - double maxFrameDuration_;
> > > > + /* Frame duration (1/fps) limits. */
> > > > + Duration minFrameDuration_;
> > > > + Duration maxFrameDuration_;
> > > > };
> > > >
> > > > int IPARPi::init(const IPASettings &settings,
> ipa::RPi::SensorConfig *sensorConfig)
> > > > @@ -311,10 +315,10 @@ void IPARPi::setMode(const CameraSensorInfo
> &sensorInfo)
> > > > mode_.noise_factor = sqrt(mode_.bin_x * mode_.bin_y);
> > > >
> > > > /*
> > > > - * Calculate the line length in nanoseconds as the ratio
> between
> > > > - * the line length in pixels and the pixel rate.
> > > > + * Calculate the line length as the ratio between the line
> length in
> > > > + * pixels and the pixel rate.
> > > > */
> > > > - mode_.line_length = 1e9 * sensorInfo.lineLength /
> sensorInfo.pixelRate;
> > > > + mode_.line_length = sensorInfo.lineLength * (1.0s /
> sensorInfo.pixelRate);
> > > >
> > > > /*
> > > > * Set the frame length limits for the mode to ensure exposure
> and
> > > > @@ -387,7 +391,7 @@ int IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> > > > /* Supply initial values for gain and exposure. */
> > > > ControlList ctrls(sensorCtrls_);
> > > > AgcStatus agcStatus;
> > > > - agcStatus.shutter_time = DefaultExposureTime;
> > > > + agcStatus.shutter_time =
> DefaultExposureTime.get<std::micro>();
> > > > agcStatus.analogue_gain = DefaultAnalogueGain;
> > > > applyAGC(&agcStatus, ctrls);
> > > >
> > > > @@ -862,7 +866,7 @@ void IPARPi::queueRequest(const ControlList
> &controls)
> > > >
> > > > case controls::FRAME_DURATIONS: {
> > > > auto frameDurations =
> ctrl.second.get<Span<const int64_t>>();
> > > > - applyFrameDurations(frameDurations[0],
> frameDurations[1]);
> > > > + applyFrameDurations(frameDurations[0] * 1.0us,
> frameDurations[1] * 1.0us);
> > > > break;
> > > > }
> > > >
> > > > @@ -937,9 +941,9 @@ void IPARPi::prepareISP(const
> ipa::RPi::ISPConfig &data)
> > > > returnEmbeddedBuffer(data.embeddedBufferId);
> > > >
> > > > /* Allow a 10% margin on the comparison below. */
> > > > - constexpr double eps = controllerMinFrameDuration * 1e3 * 0.1;
> > > > + Duration delta = (frameTimestamp - lastRunTimestamp_) * 1.0ns;
> > > > if (lastRunTimestamp_ && frameCount_ > dropFrameCount_ &&
> > > > - frameTimestamp - lastRunTimestamp_ + eps <
> controllerMinFrameDuration * 1e3) {
> > > > + delta < controllerMinFrameDuration * 0.9) {
> > > > /*
> > > > * Ensure we merge the previous frame's metadata with
> the current
> > > > * frame. This will not overwrite exposure/gain values
> for the
> > > > @@ -1012,7 +1016,7 @@ 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>();
> > > >
> > > > - deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> > > > + deviceStatus.shutter_speed =
> helper_->Exposure(exposureLines).get<std::micro>();
> > > > deviceStatus.analogue_gain = helper_->Gain(gainCode);
> > > >
> > > > LOG(IPARPI, Debug) << "Metadata - Exposure : "
> > > > @@ -1057,10 +1061,11 @@ void IPARPi::applyAWB(const struct AwbStatus
> *awbStatus, ControlList &ctrls)
> > > > static_cast<int32_t>(awbStatus->gain_b * 1000));
> > > > }
> > > >
> > > > -void IPARPi::applyFrameDurations(double minFrameDuration, double
> maxFrameDuration)
> > > > +void IPARPi::applyFrameDurations(const Duration &minFrameDuration,
> > > > + const Duration &maxFrameDuration)
> > > > {
> > > > - const double minSensorFrameDuration = 1e-3 *
> mode_.min_frame_length * mode_.line_length;
> > > > - const double maxSensorFrameDuration = 1e-3 *
> mode_.max_frame_length * mode_.line_length;
> > > > + const Duration minSensorFrameDuration = mode_.min_frame_length
> * mode_.line_length;
> > > > + const Duration maxSensorFrameDuration = mode_.max_frame_length
> * mode_.line_length;
> > > >
> > > > /*
> > > > * This will only be applied once AGC recalculations occur.
> > > > @@ -1076,20 +1081,20 @@ void IPARPi::applyFrameDurations(double
> minFrameDuration, double maxFrameDuratio
> > > >
> > > > /* Return the validated limits via metadata. */
> > > > libcameraMetadata_.set(controls::FrameDurations,
> > > > - {
> static_cast<int64_t>(minFrameDuration_),
> > > > -
> static_cast<int64_t>(maxFrameDuration_) });
> > > > + {
> static_cast<int64_t>(minFrameDuration_.get<std::micro>()),
> > > > + static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) });
> > > >
> > > > /*
> > > > * Calculate the maximum exposure time possible for the AGC to
> use.
> > > > * GetVBlanking() will update maxShutter with the largest
> exposure
> > > > * value possible.
> > > > */
> > > > - double maxShutter = std::numeric_limits<double>::max();
> > > > + Duration maxShutter = Duration::max();
> > > > helper_->GetVBlanking(maxShutter, minFrameDuration_,
> maxFrameDuration_);
> > > >
> > > > RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
> > > > controller_.GetAlgorithm("agc"));
> > > > - agc->SetMaxShutter(maxShutter);
> > > > + agc->SetMaxShutter(maxShutter.get<std::micro>());
> > > > }
> > > >
> > > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus,
> ControlList &ctrls)
> > > > @@ -1097,9 +1102,8 @@ void IPARPi::applyAGC(const struct AgcStatus
> *agcStatus, ControlList &ctrls)
> > > > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
> > > >
> > > > /* GetVBlanking might clip exposure time to the fps limits. */
> > > > - double exposure = agcStatus->shutter_time;
> > > > - int32_t vblanking = helper_->GetVBlanking(exposure,
> minFrameDuration_,
> > > > - maxFrameDuration_);
> > > > + Duration exposure = agcStatus->shutter_time * 1.0us;
> > > > + int32_t vblanking = helper_->GetVBlanking(exposure,
> minFrameDuration_, maxFrameDuration_);
> > > > int32_t exposureLines = helper_->ExposureLines(exposure);
> > > >
> > > > LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210608/4f25cd68/attachment-0001.htm>
More information about the libcamera-devel
mailing list