[libcamera-devel] [PATCH v12 2/3] libcamera: raspberrypi: Add control of sensor vblanking
Jacopo Mondi
jacopo at jmondi.org
Tue Jan 19 18:00:11 CET 2021
Hi Naush,
I know I have my tag here, and I'm possibly going to repeat some
comments. I don't think any of them should block your series though
On Tue, Jan 19, 2021 at 03:30:46PM +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>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> 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 | 56 ++++++++++++++++---
> .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-
> 8 files changed, 130 insertions(+), 15 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 01fe5abce9a6..1de36039cee0 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) },
The static initialization of the control limits worries me a bit.
This should match the sensor limits. I can't ask you to go to fully
implement this, but I fear the arbitrary initialization of the
controls limits the RPi implementation does will be problematic for
application that might what to know in which range they can safely
operate.
I presume your forthcoming applications will probably know those limits
from a per-sensor configuration file ?
> };
>
> } /* namespace RPi */
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 6efa0d7fd030..b7b8faf09c69 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,
> + double maxFrameDuration) const
> +{
> + uint32_t frameLengthMin, frameLengthMax, vblank;
> + uint32_t exposureLines = ExposureLines(exposure);
> +
> + assert(initialized_);
> +
> + /*
> + * Clamp 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_);
min and max frame durations are here adjusted to respect the sensor's
configuration. Shouldn't this be reflected in the metadata ?
> + /*
> + * Limit the exposure to the maximum frame duration requested, and
> + * re-calculate if it has been clipped.
> + */
> + exposureLines = std::min(frameLengthMax - frameIntegrationDiff_, exposureLines);
Can't you here
exposureLines = std::max(frameLengthMin, exposureLines + frameIntegrationDiff_);
To avoid the below clamp, which is partially useless as we're already
sure exposureLines < (frameLengthMax - frameIntegrationDiff_)
(and I'm not sure if frameIntegrationDiff_ has only to be considered
when inspecting the upper limit or also when comparing to the bottom
one)
it will also make sure the below recalculation of 'exposure' is in the limits
> + exposure = Exposure(exposureLines);
> +
> + /* Limit the vblank to the range allowed by the frame length limits. */
> + vblank = std::clamp(exposureLines + frameIntegrationDiff_,
> + frameLengthMin, frameLengthMax) - mode_.height;
Then you can simply
return exposureLines - mode_.height;
> + 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 044c28667f75..b1739a57e342 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,
The first parameter is called just 'exposure' in the function
implementation. Mixing of snake_case and camelCase, but it's in many
places already in the IPA, so...
> + double 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) 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_;
nit: different declaration order than in the derived classes, but
well, minor..
> };
>
> // 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 db8ab879deb7..8688ec091c44 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 0e896ac74f88..5396131003f1 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 0b841cd175fa..2bd8a754f133 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 d087b07e186b..ba9bc398ef87 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;
Quite arbitrary, isn't it ?
I would have expected this to be sent by the pipeline to the IPA by
populating the FrameDuration ControlInfoMap at configure() time. The
'sensorControls' provided at configure might very well contain the
VBLANK control limits, which once combined with the sensor mode
information should give you precise limits
minFrameDuration = (mode.height + min(VBLANK)) * mode.line_lenght
/ (mode.pixel_rate / 1e6)
Or is this not required ?
>
> LOG_DEFINE_CATEGORY(IPARPI)
>
> @@ -150,6 +152,10 @@ private:
>
> /* Distinguish the first camera start from others. */
> bool firstStart_;
> +
> + /* Frame duration (1/fps) limits, given in microseconds. */
> + double minFrameDuration_;
> + double maxFrameDuration_;
> };
>
> int IPARPi::init(const IPASettings &settings)
> @@ -332,7 +338,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 */
> + result->data.push_back(exposureDelay); /* For VBLANK ctrl */
> result->data.push_back(sensorMetadata);
>
> result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;
> @@ -377,6 +384,9 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> controller_.Initialise();
> controllerInit_ = true;
>
> + minFrameDuration_ = defaultMinFrameDuration;
> + maxFrameDuration_ = defaultMaxFrameDuration;
> +
> /* Supply initial values for gain and exposure. */
> ControlList ctrls(sensorCtrls_);
> AgcStatus agcStatus;
> @@ -524,7 +534,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) {
> @@ -551,7 +562,7 @@ bool IPARPi::validateIspControls()
> V4L2_CID_USER_BCM2835_ISP_DENOISE,
> V4L2_CID_USER_BCM2835_ISP_SHARPEN,
> V4L2_CID_USER_BCM2835_ISP_DPC,
> - V4L2_CID_USER_BCM2835_ISP_LENS_SHADING
> + V4L2_CID_USER_BCM2835_ISP_LENS_SHADING,
Unrelated it seems
> };
>
> for (auto c : ctrls) {
> @@ -804,6 +815,25 @@ void IPARPi::queueRequest(const ControlList &controls)
> break;
> }
>
> + 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;
> + maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);
Why this last line ?
Also, it really seems seems arbitrary defaults clips the exposure to
pretty abitrary values. If you don't want to do the calculation of the
defaults, what about skipping the clipping completely ? Or is this
your intetion ? "Pick default values large enough that no clipping
occours" ?
> +
> + /*
> + * \todo The values returned in the metadata below must be
> + * correctly clipped by what the sensor mode suppots and
> + * what the AGC exposure mode or manual shutter speed limits
> + */
Why this metadata can't be set in reportMetadata() which is called
after processStat() which calls GetVBlanking() and updates the limits
?
Sorry for the many questions. The only thing that worries me is the
ControlInfo intialization, but that's not something you should address
at the moment. Other than that with the metadata and default value
assignement clarified, you can retain my tag.
Thanks
j
> + libcameraMetadata_.set(controls::FrameDurations,
> + { static_cast<int64_t>(minFrameDuration_),
> + static_cast<int64_t>(maxFrameDuration_) });
> + break;
> + }
> +
> default:
> LOG(IPARPI, Warning)
> << "Ctrl " << controls::controls.at(ctrl.first)->name()
> @@ -962,15 +992,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.
> + */
> + 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 7a5f5881b9b3..252cab64023e 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++] } });
> sensorMetadata_ = result.data[resultIdx++];
> }
> }
> --
> 2.25.1
>
More information about the libcamera-devel
mailing list