[libcamera-devel] [PATCH v4 3/5] ipa: raspberrypi: Limit the calculated vblank based on the sensor mode
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Feb 4 23:24:33 CET 2021
Hi Naush,
On Thu, Feb 04, 2021 at 10:19:07PM +0000, Naushir Patuck wrote:
> On Thu, 4 Feb 2021 at 20:19, Laurent Pinchart wrote:
> > On Fri, Jan 29, 2021 at 11:16:14AM +0000, Naushir Patuck 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>
> > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > 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 | 49 +++++++++++++-------
> > > 7 files changed, 47 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> > > index b7b8faf09c69..93d1b7b0296a 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 are clamped by the caller
> > > + * based on the limits for the active sensor 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..256438c931d9 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 fame lengths in units of lines
> > > + uint32_t min_frame_length, max_frame_length;
> > > };
> > >
> > > #ifdef __cplusplus
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 5ccc7a6551f5..e4911b734e20 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 minFrameDuration, double maxFrameDuration);
> > > 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,13 @@ 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 frame length limits for the mode to ensure exposure and
> > > + * framerate calculations are clipped appropriately.
> > > + */
> > > + mode_.min_frame_length = sensorInfo.minFrameLength;
> > > + mode_.max_frame_length = sensorInfo.maxFrameLength;
> > > }
> > >
> > > void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > > @@ -384,8 +392,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 +827,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 +986,28 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> > > static_cast<int32_t>(awbStatus->gain_b * 1000));
> > > }
> > >
> > > +void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuration)
> > > +{
> > > + const double minSensorFrameDuration = 1e-3 * mode_.min_frame_length * mode_.line_length;
> > > + const double maxSensorFrameDuration = 1e-3 * mode_.max_frame_length * 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_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration;
> > > + maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration;
> > > + minFrameDuration_ = std::clamp(minFrameDuration_,
> > > + minSensorFrameDuration, maxSensorFrameDuration);
> > > + maxFrameDuration_ = std::clamp(maxFrameDuration_,
> > > + minSensorFrameDuration, maxSensorFrameDuration);
> >
> > Do we need to keep
> >
> > maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);
> >
> > in case the user passes a minimum higher than the maximum ? Apart from
> > that (which I can fix when applying if needed),
>
> Please correct me if I am wrong, but having the *FrameDuration_ values
> clamped by the *SensorFrameDurations would assure that max >= min, so I
> removed that statement. This is assuming that the sensor driver reports
> max vblank >= min vblank - which it should.
Let's assume
- minSensorFrameDuration = 1000
- maxSensorFrameDuration = 5000
- minFrameDuration = 2000
- maxFrameDuration = 1500
After clamping the minFrameDuration and maxFrameDuration values are not
changed, and minFrameDuration is still > maxFrameDuration. Am I missing
something ?
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > +
> > > + /* 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);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list