[libcamera-devel] [PATCH v1 2/9] ipa: raspberrypi: Add minimum and maximum line length fields to CameraMode

[Naushir Patuck]

Hi Laurent,

Thank you for your feedback.

On Tue, 4 Oct 2022 at 17:43, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, Oct 03, 2022 at 09:39:28AM +0100, Naushir Patuck via
> libcamera-devel wrote:
> > Add fields for minimum and maximum line length (in units of pixels) to
> the
>
> I think the unit is seconds, not pixels.
>
> > CameraMode structure. This replaces the existing lineLength field.
> >
> > Any use of the existing lineLength field is replaced by the new
> minLineLength
> > field, as logically we always want to use the fastest sensor readout by
> default.
>
> I wonder if that's always the case, but I think it's fine here.
>
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/cam_helper.cpp           |  8 ++++----
> >  src/ipa/raspberrypi/controller/camera_mode.h |  2 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp          | 13 +++++++------
> >  3 files changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > index cac8f39ee763..42251ba29682 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -64,13 +64,13 @@ void CamHelper::process([[maybe_unused]]
> StatisticsPtr &stats,
> >  uint32_t CamHelper::exposureLines(const Duration exposure) const
> >  {
> >       assert(initialized_);
> > -     return exposure / mode_.lineLength;
> > +     return exposure / mode_.minLineLength;
> >  }
> >
> >  Duration CamHelper::exposure(uint32_t exposureLines) const
> >  {
> >       assert(initialized_);
> > -     return exposureLines * mode_.lineLength;
> > +     return exposureLines * mode_.minLineLength;
> >  }
> >
> >  uint32_t CamHelper::getVBlanking(Duration &exposure,
> > @@ -86,8 +86,8 @@ uint32_t CamHelper::getVBlanking(Duration &exposure,
> >        * minFrameDuration and maxFrameDuration are clamped by the caller
> >        * based on the limits for the active sensor mode.
> >        */
> > -     frameLengthMin = minFrameDuration / mode_.lineLength;
> > -     frameLengthMax = maxFrameDuration / mode_.lineLength;
> > +     frameLengthMin = minFrameDuration / mode_.minLineLength;
> > +     frameLengthMax = maxFrameDuration / mode_.minLineLength;
> >
> >       /*
> >        * Limit the exposure to the maximum frame duration requested, and
> > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h
> b/src/ipa/raspberrypi/controller/camera_mode.h
> > index a6ccf8c1c600..b7e73aae4698 100644
> > --- a/src/ipa/raspberrypi/controller/camera_mode.h
> > +++ b/src/ipa/raspberrypi/controller/camera_mode.h
> > @@ -32,7 +32,7 @@ struct CameraMode {
> >       /* scaling of the noise compared to the native sensor mode */
> >       double noiseFactor;
> >       /* line time */
> > -     libcamera::utils::Duration lineLength;
> > +     libcamera::utils::Duration minLineLength, maxLineLength;
>
>         libcamera::utils::Duration minLineLength;
>         libcamera::utils::Duration maxLineLength;
>
> would be more readable.
>

I did the single line statement to be consistent with the rest of the
struct definition, but I can amend this change so that all fields
are on a single line.

Regards,
Naush


>
> >       /* 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 358a119da222..67326bcf4a14 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -314,7 +314,7 @@ void IPARPi::start(const ControlList &controls,
> StartConfig *startConfig)
> >       }
> >
> >       startConfig->dropFrameCount = dropFrameCount_;
> > -     const Duration maxSensorFrameDuration = mode_.maxFrameLength *
> mode_.lineLength;
> > +     const Duration maxSensorFrameDuration = mode_.maxFrameLength *
> mode_.minLineLength;
> >       startConfig->maxSensorFrameLengthMs =
> maxSensorFrameDuration.get<std::milli>();
> >
> >       firstStart_ = false;
> > @@ -356,7 +356,8 @@ void IPARPi::setMode(const IPACameraSensorInfo
> &sensorInfo)
> >        * Calculate the line length as the ratio between the line length
> in
> >        * pixels and the pixel rate.
> >        */
> > -     mode_.lineLength = sensorInfo.minLineLength * (1.0s /
> sensorInfo.pixelRate);
> > +     mode_.minLineLength = sensorInfo.minLineLength * (1.0s /
> sensorInfo.pixelRate);
> > +     mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s /
> sensorInfo.pixelRate);
> >
> >       /*
> >        * Set the frame length limits for the mode to ensure exposure and
> > @@ -458,8 +459,8 @@ int IPARPi::configure(const IPACameraSensorInfo
> &sensorInfo,
> >        * based on the current sensor mode.
> >        */
> >       ControlInfoMap::Map ctrlMap = ipaControls;
> > -     const Duration minSensorFrameDuration = mode_.minFrameLength *
> mode_.lineLength;
> > -     const Duration maxSensorFrameDuration = mode_.maxFrameLength *
> mode_.lineLength;
> > +     const Duration minSensorFrameDuration = mode_.minFrameLength *
> mode_.minLineLength;
> > +     const Duration maxSensorFrameDuration = mode_.maxFrameLength *
> mode_.minLineLength;
> >       ctrlMap[&controls::FrameDurationLimits] =
> >
>  ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
> >
>  static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
> > @@ -1150,8 +1151,8 @@ void IPARPi::applyAWB(const struct AwbStatus
> *awbStatus, ControlList &ctrls)
> >
> >  void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration
> maxFrameDuration)
> >  {
> > -     const Duration minSensorFrameDuration = mode_.minFrameLength *
> mode_.lineLength;
> > -     const Duration maxSensorFrameDuration = mode_.maxFrameLength *
> mode_.lineLength;
> > +     const Duration minSensorFrameDuration = mode_.minFrameLength *
> mode_.minLineLength;
> > +     const Duration maxSensorFrameDuration = mode_.maxFrameLength *
> mode_.minLineLength;
> >
> >       /*
> >        * This will only be applied once AGC recalculations occur.
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221005/de2a5403/attachment.htm>