[libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Limit the calculated vblank based on the sensor mode
Naushir Patuck
naush at raspberrypi.com
Thu Jan 21 16:45:00 CET 2021
Hi David,
Thank you for your review feedback.
On Thu, 21 Jan 2021 at 14:53, David Plowman <david.plowman at raspberrypi.com>
wrote:
> 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...)
>
Yes, we do validate that all controls (sensor and isp) are available on
ipa::configure(). If not we fail hard, so this should be safe.
>
> > + 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!
>
I can change the names to something more appropriate.
Regards,
Naush
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210121/4a59fbad/attachment-0001.htm>
More information about the libcamera-devel
mailing list