[libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Limit the calculated vblank based on the sensor mode
David Plowman
david.plowman at raspberrypi.com
Thu Jan 21 15:52:53 CET 2021
Hi Naush
I don't think I can claim to understand every last nuance of this, but
let me try anyway but here goes...!
On Thu, 21 Jan 2021 at 11:58, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> The existing framerate/vblank calculations did not account for the
> sensor mode limits. This commit extracts vblank limits from the sensor
> v4l2 controls, and stores it in the camera modes structure.
>
> Exposure and vblank value calculations now use values clamped to the
> sensor mode limits. The libcamera::controls::FrameDurations metadata
> return values now reflect the minimum and maximum after clamping.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
> src/ipa/raspberrypi/cam_helper.cpp | 16 +++---
> src/ipa/raspberrypi/cam_helper.hpp | 5 +-
> src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +--
> src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +-
> src/ipa/raspberrypi/cam_helper_ov5647.cpp | 4 +-
> src/ipa/raspberrypi/controller/camera_mode.h | 2 +
> src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++------
> 7 files changed, 49 insertions(+), 39 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index b7b8faf09c69..3e17255737b2 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const &cam_name)
> return nullptr;
> }
>
> -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,
> - unsigned int frameIntegrationDiff)
> - : parser_(parser), initialized_(false), maxFrameLength_(maxFrameLength),
> +CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)
> + : parser_(parser), initialized_(false),
> frameIntegrationDiff_(frameIntegrationDiff)
> {
> }
> @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,
> assert(initialized_);
>
> /*
> - * Clamp frame length by the frame duration range and the maximum allowable
> - * value in the sensor, given by maxFrameLength_.
> + * minFrameDuration and maxFrameDuration will have been clamped to the
> + * maximum allowable values in the sensor for this mode.
> */
> - 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_);
> + frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;
> + frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;
> +
> /*
> * Limit the exposure to the maximum frame duration requested, and
> * re-calculate if it has been clipped.
> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> index b1739a57e342..14d70112cb62 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -62,8 +62,7 @@ class CamHelper
> {
> public:
> static CamHelper *Create(std::string const &cam_name);
> - CamHelper(MdParser *parser, unsigned int maxFrameLength,
> - unsigned int frameIntegrationDiff);
> + CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);
> virtual ~CamHelper();
> void SetCameraMode(const CameraMode &mode);
> MdParser &Parser() const { return *parser_; }
> @@ -86,8 +85,6 @@ protected:
>
> 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.
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> index 8688ec091c44..95b8e698fe3b 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -56,15 +56,13 @@ private:
> * 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(), maxFrameLength, frameIntegrationDiff)
> + : CamHelper(new MdParserImx219(), frameIntegrationDiff)
> #else
> - : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)
> + : CamHelper(new MdParserRPi(), frameIntegrationDiff)
> #endif
> {
> }
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index 5396131003f1..eaa7be16d20e 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -45,12 +45,10 @@ private:
> * 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(), maxFrameLength, frameIntegrationDiff)
> + : CamHelper(new MdParserImx477(), frameIntegrationDiff)
> {
> }
>
> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> index 2bd8a754f133..a7f417324048 100644
> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> @@ -30,8 +30,6 @@ private:
> * in units of lines.
> */
> static constexpr int frameIntegrationDiff = 4;
> - /* Largest possible frame length, in units of lines. */
> - static constexpr int maxFrameLength = 0xffff;
> };
>
> /*
> @@ -40,7 +38,7 @@ private:
> */
>
> CamHelperOv5647::CamHelperOv5647()
> - : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)
> + : CamHelper(new MdParserRPi(), frameIntegrationDiff)
> {
> }
>
> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h
> index 920f11beb0b0..3baeadaca076 100644
> --- a/src/ipa/raspberrypi/controller/camera_mode.h
> +++ b/src/ipa/raspberrypi/controller/camera_mode.h
> @@ -37,6 +37,8 @@ struct CameraMode {
> double line_length;
> // any camera transform *not* reflected already in the camera tuning
> libcamera::Transform transform;
> + // minimum and maximum vblanking limits for this mode
> + uint32_t vblank_min, vblank_max;
> };
>
> #ifdef __cplusplus
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 5ccc7a6551f5..9e6e030c4997 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -102,6 +102,7 @@ private:
> void reportMetadata();
> bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);
> void processStats(unsigned int bufferId);
> + void applyFrameDurations(double min, double max);
> void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
> void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);
> void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);
> @@ -279,6 +280,14 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> * the line length in pixels and the pixel rate.
> */
> mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;
> +
> + /*
> + * Set the vertical blanking (frame duration) limits for the mode to
> + * ensure exposure and framerate calculations are clipped appropriately.
> + */
> + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);
Are we happy to assume this exists? (I realise that if it doesn't then
everything is doomed and we probably don't care very much...)
> + mode_.vblank_min = itVblank->second.min().get<int32_t>();
> + mode_.vblank_max = itVblank->second.max().get<int32_t>();
> }
>
> void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> controller_.Initialise();
> controllerInit_ = true;
>
> - minFrameDuration_ = defaultMinFrameDuration;
> - maxFrameDuration_ = defaultMaxFrameDuration;
> + /* Supply initial values for frame durations. */
> + applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);
>
> /* Supply initial values for gain and exposure. */
> ControlList ctrls(sensorCtrls_);
> @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList &controls)
>
> 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_);
> -
> - /*
> - * \todo The values returned in the metadata below must be
> - * correctly clipped by what the sensor mode supports and
> - * what the AGC exposure mode or manual shutter speed limits
> - */
> - libcameraMetadata_.set(controls::FrameDurations,
> - { static_cast<int64_t>(minFrameDuration_),
> - static_cast<int64_t>(maxFrameDuration_) });
> + applyFrameDurations(frameDurations[0], frameDurations[1]);
> break;
> }
>
> @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> static_cast<int32_t>(awbStatus->gain_b * 1000));
> }
>
> +void IPARPi::applyFrameDurations(double min, double max)
I didn't terribly like variables named just "min" and "max", as I
immediately wonder what they are the min and max of... or maybe it's
just a hangover from C programming where there often seemed to be min
and max macros flying around!
But these terribly minor things aside:
Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
Thanks!
David
> +{
> + const double minSensorFrameDuration = 1e-3 * (mode_.vblank_min + mode_.height) *
> + mode_.line_length;
> + const double maxSensorFrameDuration = 1e-3 * (mode_.vblank_max + mode_.height) *
> + mode_.line_length;
> + /*
> + * This will only be applied once AGC recalculations occur.
> + * The values may be clamped based on the sensor mode capabilities as well.
> + */
> + minFrameDuration_ = min ? min : defaultMaxFrameDuration;
> + maxFrameDuration_ = max ? max : defaultMinFrameDuration;
> + minFrameDuration_ = std::clamp(minFrameDuration_,
> + minSensorFrameDuration, maxSensorFrameDuration);
> + maxFrameDuration_ = std::clamp(maxFrameDuration_,
> + minSensorFrameDuration, maxSensorFrameDuration);
> +
> + /* Return the validated limits out though metadata. */
> + libcameraMetadata_.set(controls::FrameDurations,
> + { static_cast<int64_t>(minFrameDuration_),
> + static_cast<int64_t>(maxFrameDuration_) });
> +}
> +
> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> {
> int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
> --
> 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