[libcamera-devel] [PATCH v1 3/9] ipa: raspberrypi: Pass lineLength into the CamHelper API
Naushir Patuck
naush at raspberrypi.com
Wed Oct 5 15:00:48 CEST 2022
Hi Laurent,
Thank you for your feedback.
On Tue, 4 Oct 2022 at 18:02, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, Oct 03, 2022 at 09:39:29AM +0100, Naushir Patuck via
> libcamera-devel wrote:
> > Update CamHelper::exposureLines() and CamHelper::exposure() to take a
> > line length duration parameter for use in the exposure calculations.
> >
> > For now, only use the minimum line length for all the calculations to
> match
> > the existing IPA behavior.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > src/ipa/raspberrypi/cam_helper.cpp | 12 ++++++------
> > src/ipa/raspberrypi/cam_helper.h | 6 ++++--
> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 3 ++-
> > src/ipa/raspberrypi/cam_helper_imx296.cpp | 10 ++++++----
> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 7 ++++---
> > src/ipa/raspberrypi/cam_helper_imx519.cpp | 7 ++++---
> > src/ipa/raspberrypi/controller/camera_mode.h | 2 +-
> > src/ipa/raspberrypi/raspberrypi.cpp | 8 ++++----
> > 8 files changed, 31 insertions(+), 24 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > index 42251ba29682..fd3527b94501 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -61,16 +61,16 @@ void CamHelper::process([[maybe_unused]]
> StatisticsPtr &stats,
> > {
> > }
> >
> > -uint32_t CamHelper::exposureLines(const Duration exposure) const
> > +uint32_t CamHelper::exposureLines(const Duration exposure, const
> Duration lineLength) const
> > {
> > assert(initialized_);
>
> Is the assert still needed ? Same below.
>
Not strictly needed any more. I can remove it in another patch.
>
> > - return exposure / mode_.minLineLength;
> > + return exposure / lineLength;
> > }
> >
> > -Duration CamHelper::exposure(uint32_t exposureLines) const
> > +Duration CamHelper::exposure(uint32_t exposureLines, const Duration
> lineLength) const
> > {
> > assert(initialized_);
> > - return exposureLines * mode_.minLineLength;
> > + return exposureLines * lineLength;
> > }
> >
> > uint32_t CamHelper::getVBlanking(Duration &exposure,
> > @@ -78,7 +78,7 @@ uint32_t CamHelper::getVBlanking(Duration &exposure,
> > Duration maxFrameDuration) const
> > {
> > uint32_t frameLengthMin, frameLengthMax, vblank;
> > - uint32_t exposureLines = CamHelper::exposureLines(exposure);
> > + uint32_t exposureLines = CamHelper::exposureLines(exposure,
> mode_.minLineLength);
> >
> > assert(initialized_);
> >
> > @@ -94,7 +94,7 @@ uint32_t CamHelper::getVBlanking(Duration &exposure,
> > * re-calculate if it has been clipped.
> > */
> > exposureLines = std::min(frameLengthMax - frameIntegrationDiff_,
> exposureLines);
> > - exposure = CamHelper::exposure(exposureLines);
> > + exposure = CamHelper::exposure(exposureLines, mode_.minLineLength);
> >
> > /* Limit the vblank to the range allowed by the frame length
> limits. */
> > vblank = std::clamp(exposureLines + frameIntegrationDiff_,
> > diff --git a/src/ipa/raspberrypi/cam_helper.h
> b/src/ipa/raspberrypi/cam_helper.h
> > index 70d62719da86..9b5e602689f3 100644
> > --- a/src/ipa/raspberrypi/cam_helper.h
> > +++ b/src/ipa/raspberrypi/cam_helper.h
> > @@ -78,8 +78,10 @@ public:
> > virtual void prepare(libcamera::Span<const uint8_t> buffer,
> > Metadata &metadata);
> > virtual void process(StatisticsPtr &stats, Metadata &metadata);
> > - virtual uint32_t exposureLines(libcamera::utils::Duration
> exposure) const;
> > - virtual libcamera::utils::Duration exposure(uint32_t
> exposureLines) const;
> > + virtual uint32_t exposureLines(const libcamera::utils::Duration
> exposure,
> > + const libcamera::utils::Duration
> lineLength) const;
> > + virtual libcamera::utils::Duration exposure(uint32_t exposureLines,
> > + const
> libcamera::utils::Duration lineLength) const;
> > virtual uint32_t getVBlanking(libcamera::utils::Duration &exposure,
> > libcamera::utils::Duration
> minFrameDuration,
> > libcamera::utils::Duration
> maxFrameDuration) const;
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > index 7ded07a213cd..98a3b31956ec 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > @@ -94,7 +94,8 @@ void CamHelperImx219::populateMetadata(const
> MdParser::RegisterMap ®isters,
> > {
> > DeviceStatus deviceStatus;
> >
> > - deviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256
> + registers.at(expLoReg));
> > + deviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256
> + registers.at(expLoReg),
> > + mode_.minLineLength);
> > deviceStatus.analogueGain = gain(registers.at(gainReg));
> > deviceStatus.frameLength = registers.at(frameLengthHiReg) * 256 +
> registers.at(frameLengthLoReg);
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx296.cpp
> b/src/ipa/raspberrypi/cam_helper_imx296.cpp
> > index ab1d157aaf45..74361ffe7cc2 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx296.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx296.cpp
> > @@ -21,8 +21,8 @@ public:
> > CamHelperImx296();
> > uint32_t gainCode(double gain) const override;
> > double gain(uint32_t gainCode) const override;
> > - uint32_t exposureLines(Duration exposure) const override;
> > - Duration exposure(uint32_t exposureLines) const override;
> > + uint32_t exposureLines(const Duration exposure, const Duration
> lineLength) const override;
> > + Duration exposure(uint32_t exposureLines, const Duration
> lineLength) const override;
> >
> > private:
> > static constexpr uint32_t maxGainCode = 239;
> > @@ -51,12 +51,14 @@ double CamHelperImx296::gain(uint32_t gainCode) const
> > return std::pow(10.0, gainCode / 200.0);
> > }
> >
> > -uint32_t CamHelperImx296::exposureLines(Duration exposure) const
> > +uint32_t CamHelperImx296::exposureLines(const Duration exposure,
> > + [[maybe_unused]] const Duration
> lineLength) const
>
> Does this mean that the IMX296 has no configurable horizontal blanking ?
>
I've not looked into it in detail yet. Hence leaving this as-is. Once I
verify this,
I will update this code.
Regards,
Naush
>
> > {
> > return (exposure - 14.26us) / timePerLine;
> > }
> >
> > -Duration CamHelperImx296::exposure(uint32_t exposureLines) const
> > +Duration CamHelperImx296::exposure(uint32_t exposureLines,
> > + [[maybe_unused]] const Duration
> lineLength) const
> > {
> > return exposureLines * timePerLine + 14.26us;
> > }
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > index aa306d6661c6..71529bdd3034 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > @@ -144,9 +144,9 @@ uint32_t CamHelperImx477::getVBlanking(Duration
> &exposure,
> > if (shift) {
> > /* Account for any rounding in the scaled frame length
> value. */
> > frameLength <<= shift;
> > - exposureLines = CamHelperImx477::exposureLines(exposure);
> > + exposureLines = CamHelperImx477::exposureLines(exposure,
> mode_.minLineLength);
> > exposureLines = std::min(exposureLines, frameLength -
> frameIntegrationDiff);
> > - exposure = CamHelperImx477::exposure(exposureLines);
> > + exposure = CamHelperImx477::exposure(exposureLines,
> mode_.minLineLength);
> > }
> >
> > return frameLength - mode_.height;
> > @@ -170,7 +170,8 @@ void CamHelperImx477::populateMetadata(const
> MdParser::RegisterMap ®isters,
> > {
> > DeviceStatus deviceStatus;
> >
> > - deviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256
> + registers.at(expLoReg));
> > + deviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256
> + registers.at(expLoReg),
> > + mode_.minLineLength);
> > deviceStatus.analogueGain = gain(registers.at(gainHiReg) * 256 +
> registers.at(gainLoReg));
> > deviceStatus.frameLength = registers.at(frameLengthHiReg) * 256 +
> registers.at(frameLengthLoReg);
> > deviceStatus.sensorTemperature = std::clamp<int8_t>(registers.at(temperatureReg),
> -20, 80);
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx519.cpp
> b/src/ipa/raspberrypi/cam_helper_imx519.cpp
> > index 54e104e7659a..2c120dad1680 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx519.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx519.cpp
> > @@ -144,9 +144,9 @@ uint32_t CamHelperImx519::getVBlanking(Duration
> &exposure,
> > if (shift) {
> > /* Account for any rounding in the scaled frame length
> value. */
> > frameLength <<= shift;
> > - exposureLines = CamHelperImx519::exposureLines(exposure);
> > + exposureLines = CamHelperImx519::exposureLines(exposure,
> mode_.minLineLength);
> > exposureLines = std::min(exposureLines, frameLength -
> frameIntegrationDiff);
> > - exposure = CamHelperImx519::exposure(exposureLines);
> > + exposure = CamHelperImx519::exposure(exposureLines,
> mode_.minLineLength);
> > }
> >
> > return frameLength - mode_.height;
> > @@ -170,7 +170,8 @@ void CamHelperImx519::populateMetadata(const
> MdParser::RegisterMap ®isters,
> > {
> > DeviceStatus deviceStatus;
> >
> > - deviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256
> + registers.at(expLoReg));
> > + deviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256
> + registers.at(expLoReg),
> > + mode_.minLineLength);
> > deviceStatus.analogueGain = gain(registers.at(gainHiReg) * 256 +
> registers.at(gainLoReg));
> > deviceStatus.frameLength = registers.at(frameLengthHiReg) * 256 +
> registers.at(frameLengthLoReg);
> >
> > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h
> b/src/ipa/raspberrypi/controller/camera_mode.h
> > index b7e73aae4698..cda6ac4e200a 100644
> > --- a/src/ipa/raspberrypi/controller/camera_mode.h
> > +++ b/src/ipa/raspberrypi/controller/camera_mode.h
> > @@ -31,7 +31,7 @@ struct CameraMode {
> > double scaleX, scaleY;
> > /* scaling of the noise compared to the native sensor mode */
> > double noiseFactor;
> > - /* line time */
> > + /* minimum and maximum line times */
>
> This belongs to the previous patch.
>
> > libcamera::utils::Duration minLineLength, maxLineLength;
> > /* any camera transform *not* reflected already in the camera
> tuning */
> > libcamera::Transform transform;
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 67326bcf4a14..13807f4d47f7 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -477,7 +477,7 @@ int IPARPi::configure(const IPACameraSensorInfo
> &sensorInfo,
> > const uint32_t exposureMin =
> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();
> >
> > ctrlMap[&controls::ExposureTime] =
> > -
> ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin).get<std::micro>()),
> > +
> ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin,
> mode_.minLineLength).get<std::micro>()),
> >
> static_cast<int32_t>(maxShutter.get<std::micro>()));
> >
> > result->controlInfo = ControlInfoMap(std::move(ctrlMap),
> controls::controls);
> > @@ -550,7 +550,7 @@ void IPARPi::reportMetadata()
> >
> deviceStatus->shutterSpeed.get<std::micro>());
> > libcameraMetadata_.set(controls::AnalogueGain,
> deviceStatus->analogueGain);
> > libcameraMetadata_.set(controls::FrameDuration,
> > -
> helper_->exposure(deviceStatus->frameLength).get<std::micro>());
> > +
> helper_->exposure(deviceStatus->frameLength,
> mode_.minLineLength).get<std::micro>());
> > if (deviceStatus->sensorTemperature)
> >
> libcameraMetadata_.set(controls::SensorTemperature,
> *deviceStatus->sensorTemperature);
> > }
> > @@ -1106,7 +1106,7 @@ void IPARPi::fillDeviceStatus(const ControlList
> &sensorControls)
> > int32_t gainCode =
> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> > int32_t vblank =
> sensorControls.get(V4L2_CID_VBLANK).get<int32_t>();
> >
> > - deviceStatus.shutterSpeed = helper_->exposure(exposureLines);
> > + deviceStatus.shutterSpeed = helper_->exposure(exposureLines,
> mode_.minLineLength);
> > deviceStatus.analogueGain = helper_->gain(gainCode);
> > deviceStatus.frameLength = mode_.height + vblank;
> >
> > @@ -1198,7 +1198,7 @@ void IPARPi::applyAGC(const struct AgcStatus
> *agcStatus, ControlList &ctrls)
> > /* getVBlanking might clip exposure time to the fps limits. */
> > Duration exposure = agcStatus->shutterTime;
> > int32_t vblanking = helper_->getVBlanking(exposure,
> minFrameDuration_, maxFrameDuration_);
> > - int32_t exposureLines = helper_->exposureLines(exposure);
> > + int32_t exposureLines = helper_->exposureLines(exposure,
> mode_.minLineLength);
> >
> > LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure
> > << " (Shutter lines: " << exposureLines << ",
> AGC requested "
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221005/aabe4481/attachment.htm>
More information about the libcamera-devel
mailing list