| Message ID | 20210121115849.682130-3-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi David, Thank you for your review feedback. On Thu, 21 Jan 2021 at 14:53, David Plowman <david.plowman@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@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@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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Nuash, On Thu, Jan 21, 2021 at 11:58:47AM +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> > --- > 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 ++++++++++++++------ Any reason I am missing on why you kept this change private to your IPA instead of making vblank part of the CameraSensorInfo contructed by the CameraSensor class ? Thanks j > 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); > + 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) > +{ > + 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Mon, 25 Jan 2021 at 09:38, Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Nuash, > > On Thu, Jan 21, 2021 at 11:58:47AM +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> > > --- > > 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 ++++++++++++++------ > > Any reason I am missing on why you kept this change private to your > IPA instead of making vblank part of the CameraSensorInfo contructed > by the CameraSensor class ? > Did you mean adding vblank min/max values to the CameraSensorInfo class? If so, sure I can add that and lookup the values in the Raspberry Pi IPA. Note that I would still need the values available in the CameraMode (by copying from the CameraSensorInfo field). This object gets passed into our controllers, and at some point when I have enabled long exposures on imx477, I will need these values in our cam helper class. Regards, Naush > Thanks > j > > > 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); > > + 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) > > +{ > > + 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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Naush, On Mon, Jan 25, 2021 at 12:49:46PM +0000, Naushir Patuck wrote: > Hi Jacopo, > > On Mon, 25 Jan 2021 at 09:38, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > Hi Nuash, > > > > On Thu, Jan 21, 2021 at 11:58:47AM +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> > > > --- > > > 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 ++++++++++++++------ > > > > Any reason I am missing on why you kept this change private to your > > IPA instead of making vblank part of the CameraSensorInfo contructed > > by the CameraSensor class ? > > > > Did you mean adding vblank min/max values to the CameraSensorInfo class? > If so, sure I can add that and lookup the values in the Raspberry Pi IPA. > Note that I would still need the values available in the CameraMode (by > copying from the CameraSensorInfo field). This object gets passed into our > controllers, and at some point when I have enabled long exposures on > imx477, I will need these values in our cam helper class. > I see you need the control limits, not just the value itself.. This might seems philosophical as a distinction, but CameraSensorInfo reports information about the current configuration, so indeed having limits there might not fit that well. Furthermore you have access to the control limits from the pipeline, your solution might be the most correct one at the moment.. My thinking was to find a direction where this information will be made available to all IPA, without them having to replicate this same implementation, but at the current time that's probably the best we can do. I'll review the rest of the series shortly then. Thanks j > > Regards, > Naush > > > > Thanks > > j > > > > > 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); > > > + 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) > > > +{ > > > + 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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > >
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); + 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) +{ + 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);
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> --- 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(-)