[libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add control of sensor vblanking
Jacopo Mondi
jacopo at jmondi.org
Thu Dec 17 16:17:43 CET 2020
Hi Naush,
a few minors here and there
On Wed, Dec 16, 2020 at 11:22:01AM +0000, Naushir Patuck wrote:
> Add support for setting V4L2_CID_VBLANK appropriately when setting
> V4L2_CID_EXPOSURE. This will allow adaptive framerates during
> viewfinder use cases (e.g. when the exposure time goes above 33ms, we
> can reduce the framerate to lower than 30fps).
>
> The minimum and maximum frame durations are provided via libcamera
> controls, and will prioritise exposure time limits over any AGC request.
>
> V4L2_CID_VBLANK is controlled through the staggered writer, just like
> the exposure and gain controls.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
> ---
> include/libcamera/ipa/raspberrypi.h | 1 +
> src/ipa/raspberrypi/cam_helper.cpp | 35 ++++++++++++++-
> src/ipa/raspberrypi/cam_helper.hpp | 15 ++++++-
> src/ipa/raspberrypi/cam_helper_imx219.cpp | 13 +++++-
> src/ipa/raspberrypi/cam_helper_imx477.cpp | 11 ++++-
> src/ipa/raspberrypi/cam_helper_ov5647.cpp | 11 ++++-
> src/ipa/raspberrypi/raspberrypi.cpp | 45 ++++++++++++++++---
> .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-
> 8 files changed, 120 insertions(+), 14 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 01fe5abc..1de36039 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -65,6 +65,7 @@ static const ControlInfoMap Controls = {
> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> + { &controls::FrameDurations, ControlInfo(1000, 1000000000) },
> };
>
> } /* namespace RPi */
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 6efa0d7f..018c17b9 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -34,8 +34,10 @@ CamHelper *CamHelper::Create(std::string const &cam_name)
> return nullptr;
> }
>
> -CamHelper::CamHelper(MdParser *parser)
> - : parser_(parser), initialized_(false)
> +CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,
> + unsigned int frameIntegrationDiff)
> + : parser_(parser), initialized_(false), maxFrameLength_(maxFrameLength),
> + frameIntegrationDiff_(frameIntegrationDiff)
> {
> }
>
> @@ -56,6 +58,35 @@ double CamHelper::Exposure(uint32_t exposure_lines) const
> return exposure_lines * mode_.line_length / 1000.0;
> }
>
> +uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,
Output parameters should be really passed as pointers, it's very hard
to follow what the caller does otherwise. I see the same usage of
reference for ouput params is widly spread in the IPA, so I won't
bother you asking for a change.
> + double maxFrameDuration) const
> +{
> + uint32_t frameLengthMin, frameLengthMax, vblank;
> + uint32_t exposureLines = ExposureLines(exposure);
> +
> + assert(initialized_);
> +
> + /*
> + * Clip frame length by the frame duration range and the maximum allowable
> + * value in the sensor, given by maxFrameLength_.
> + */
> + frameLengthMin = std::clamp<uint32_t>(1e3 * minFrameDuration / mode_.line_length,
> + mode_.height, maxFrameLength_);
> + frameLengthMax = std::clamp<uint32_t>(1e3 * maxFrameDuration / mode_.line_length,
> + mode_.height, maxFrameLength_);
> + /*
> + * Limit the exposure to the maximum frame duration requested, and
> + * re-calculate if it has been clipped.
> + */
> + exposureLines = std::min(frameLengthMax - frameIntegrationDiff_, exposureLines);
> + exposure = Exposure(exposureLines);
> +
> + /* Limit the vblank to the range allowed by the frame length limits. */
> + vblank = std::clamp(exposureLines + frameIntegrationDiff_,
> + frameLengthMin, frameLengthMax) - mode_.height;
This looks good and matches the description of controls priorities
> + return vblank;
> +}
> +
> void CamHelper::SetCameraMode(const CameraMode &mode)
> {
> mode_ = mode;
> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> index 044c2866..b1739a57 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -62,12 +62,15 @@ class CamHelper
> {
> public:
> static CamHelper *Create(std::string const &cam_name);
> - CamHelper(MdParser *parser);
> + CamHelper(MdParser *parser, unsigned int maxFrameLength,
> + unsigned int frameIntegrationDiff);
> virtual ~CamHelper();
> void SetCameraMode(const CameraMode &mode);
> MdParser &Parser() const { return *parser_; }
> 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;
Is this really virtual ?
> 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) const;
> @@ -76,10 +79,20 @@ public:
> virtual unsigned int HideFramesModeSwitch() const;
> virtual unsigned int MistrustFramesStartup() const;
> virtual unsigned int MistrustFramesModeSwitch() const;
> +
> protected:
> MdParser *parser_;
> CameraMode mode_;
> +
> +private:
> bool initialized_;
> + /* Largest possible frame length, in units of lines. */
> + unsigned int maxFrameLength_;
> + /*
> + * Smallest difference between the frame length and integration time,
> + * in units of lines.
> + */
> + unsigned int frameIntegrationDiff_;
> };
>
> // This is for registering camera helpers with the system, so that the
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> index db8ab879..8688ec09 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -49,13 +49,22 @@ public:
> double Gain(uint32_t gain_code) const override;
> unsigned int MistrustFramesModeSwitch() const override;
> bool SensorEmbeddedDataPresent() const override;
> +
> +private:
> + /*
> + * Smallest difference between the frame length and integration time,
> + * in units of lines.
> + */
> + static constexpr int frameIntegrationDiff = 4;
> + /* Largest possible frame length, in units of lines. */
> + static constexpr int maxFrameLength = 0xffff;
> };
>
> CamHelperImx219::CamHelperImx219()
> #if ENABLE_EMBEDDED_DATA
> - : CamHelper(new MdParserImx219())
> + : CamHelper(new MdParserImx219(), maxFrameLength, frameIntegrationDiff)
> #else
> - : CamHelper(new MdParserRPi())
> + : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)
> #endif
> {
> }
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index 0e896ac7..53961310 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -38,10 +38,19 @@ public:
> uint32_t GainCode(double gain) const override;
> double Gain(uint32_t gain_code) const override;
> bool SensorEmbeddedDataPresent() const override;
> +
> +private:
> + /*
> + * Smallest difference between the frame length and integration time,
> + * in units of lines.
> + */
> + static constexpr int frameIntegrationDiff = 22;
> + /* Largest possible frame length, in units of lines. */
> + static constexpr int maxFrameLength = 0xffdc;
> };
>
> CamHelperImx477::CamHelperImx477()
> - : CamHelper(new MdParserImx477())
> + : CamHelper(new MdParserImx477(), maxFrameLength, frameIntegrationDiff)
> {
> }
>
> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> index 0b841cd1..2bd8a754 100644
> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> @@ -23,6 +23,15 @@ public:
> unsigned int HideFramesModeSwitch() const override;
> unsigned int MistrustFramesStartup() const override;
> unsigned int MistrustFramesModeSwitch() const override;
> +
> +private:
> + /*
> + * Smallest difference between the frame length and integration time,
> + * in units of lines.
> + */
> + static constexpr int frameIntegrationDiff = 4;
> + /* Largest possible frame length, in units of lines. */
> + static constexpr int maxFrameLength = 0xffff;
> };
>
> /*
> @@ -31,7 +40,7 @@ public:
> */
>
> CamHelperOv5647::CamHelperOv5647()
> - : CamHelper(new MdParserRPi())
> + : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)
> {
> }
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index d087b07e..e1fe35f5 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -58,6 +58,8 @@ namespace libcamera {
> /* 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;
Do I count the number of 0s wrong, or the control maximum value is set
to 10^9 and this is 10^8 ? Not much difference though both 100 or 1000
seconds of max exposure should suit all needs :)
These can be made uint_64 already to avoid casts. Up to you.
>
> LOG_DEFINE_CATEGORY(IPARPI)
>
> @@ -150,6 +152,9 @@ private:
>
> /* Distinguish the first camera start from others. */
> bool firstStart_;
> +
> + /* Frame duration (1/fps) given in microseconds. */
> + double minFrameDuration_, maxFrameDuration_;
> };
>
> int IPARPi::init(const IPASettings &settings)
> @@ -332,7 +337,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> sensorMetadata = helper_->SensorEmbeddedDataPresent();
>
> result->data.push_back(gainDelay);
> - result->data.push_back(exposureDelay);
> + result->data.push_back(exposureDelay); /* FOR EXPOSURE ctrl */
s/FOR/for/
> + result->data.push_back(exposureDelay); /* For VBLANK ctrl */
So, VBLANK and EXPOSURE ctrl have the same delay. How do we guarantee
VBLANK is updated first ?
> result->data.push_back(sensorMetadata);
>
> result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;
> @@ -382,6 +388,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> AgcStatus agcStatus;
> agcStatus.shutter_time = DefaultExposureTime;
> agcStatus.analogue_gain = DefaultAnalogueGain;
> + minFrameDuration_ = defaultMinFrameDuration;
> + maxFrameDuration_ = defaultMaxFrameDuration;
I would initialize these before the ctrls declaration. These are not
sent to the sensor, and seeing them in the middle here is confusing.
Or are they needed by the applyAGC() function?
> applyAGC(&agcStatus, ctrls);
>
> result->controls.emplace_back(ctrls);
> @@ -524,7 +532,8 @@ bool IPARPi::validateSensorControls()
> {
> static const uint32_t ctrls[] = {
> V4L2_CID_ANALOGUE_GAIN,
> - V4L2_CID_EXPOSURE
> + V4L2_CID_EXPOSURE,
> + V4L2_CID_VBLANK
> };
>
> for (auto c : ctrls) {
> @@ -804,6 +813,18 @@ void IPARPi::queueRequest(const ControlList &controls)
> break;
> }
Doesn't the SetFixedShutter() function need to be updated to clip the
shutter time in the frameDuration limits too ? Can we address it or at
least record it with a \todo ?
>
> + case controls::FRAME_DURATIONS: {
> + auto frameDurations = ctrl.second.get<Span<const int64_t>>();
> +
> + /* This will be applied once AGC recalculations occur. */
> + minFrameDuration_ = frameDurations[0] ? frameDurations[0] : defaultMinFrameDuration;
> + maxFrameDuration_ = frameDurations[1] ? frameDurations[1] : defaultMaxFrameDuration;
I'm surprised how cast from int64_t to double is automatically
handled..
> + libcameraMetadata_.set(controls::FrameDurations,
> + { static_cast<int64_t>(minFrameDuration_),
> + static_cast<int64_t>(maxFrameDuration_) });
I don't think this matches the control description though:
+ When reported by pipelines, the control expresses the duration of the
+ sensor frame used to produce streams part of the completed Request.
+ The minimum and maximum values shall then be the same, as the sensor
+ frame duration is a fixed parameter.
Should you record the exposure time calculated by GetVBlanking() and
report it here ?
> + break;
> + }
> +
> default:
> LOG(IPARPI, Warning)
> << "Ctrl " << controls::controls.at(ctrl.first)->name()
> @@ -962,15 +983,27 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> {
> int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
> - int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);
>
> - LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time
> - << " (Shutter lines: " << exposureLines << ") Gain: "
> + /* GetVBlanking might clip exposure time to the fps limits. */
> + double exposure = agcStatus->shutter_time;
> + int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_,
> + maxFrameDuration_);
> + int32_t exposureLines = helper_->ExposureLines(exposure);
> +
> + LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure
> + << " (Shutter lines: " << exposureLines << ", AGC requested "
> + << agcStatus->shutter_time << ") Gain: "
> << agcStatus->analogue_gain << " (Gain Code: "
> << gainCode << ")";
>
> - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
> + /*
> + * Due to the behavior of V4L2, the current value of VBLANK could clip the
> + * exposure time without us knowing. The next time though this function should
> + * clip exposure correctly.
Replying to myself: we skip one frame in case VBLANK clips exposure.
I guess setting VBLANK one frame in advance compared to exposure won't
help, as it could clip the previous frame. The only solution is to
prioritize VBLANK when setting controls to the device I guess.
> + */
> + ctrls.set(V4L2_CID_VBLANK, vblanking);
> ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
> + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
> }
>
> void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 7a5f5881..252cab64 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> if (!staggeredCtrl_) {
> staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },
> - { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });
> + { V4L2_CID_EXPOSURE, result.data[resultIdx++] },
> + { V4L2_CID_VBLANK, result.data[resultIdx++] } });
A few issues apart (metadata handling and manual exposure clipping
apart) the rest is all minor stuff. With these fixed/clarified
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> sensorMetadata_ = result.data[resultIdx++];
> }
> }
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list