| Message ID | 20210129111616.1047483-4-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. 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@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Jacopo Mondi <jacopo@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), Reviewed-by: Laurent Pinchart <laurent.pinchart@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);
Hi Laurent, Thank you for your review feedback. On Thu, 4 Feb 2021 at 20:19, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > 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@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Reviewed-by: Jacopo Mondi <jacopo@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. Regards, Naush > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 >
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@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > Reviewed-by: Jacopo Mondi <jacopo@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@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);
Hi Laurent, On Thu, 4 Feb 2021 at 22:24, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > 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@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > Reviewed-by: Jacopo Mondi <jacopo@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 ? > > You are entirely correct, it was me who was missing something :-) Please do re-introduce that line when merging. Thanks! Naush > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 >
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); + + /* 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);