<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 4 Feb 2021 at 22:24, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Thu, Feb 04, 2021 at 10:19:07PM +0000, Naushir Patuck wrote:<br>
> On Thu, 4 Feb 2021 at 20:19, Laurent Pinchart wrote:<br>
> > On Fri, Jan 29, 2021 at 11:16:14AM +0000, Naushir Patuck wrote:<br>
> > > The existing framerate/vblank calculations did not account for the<br>
> > > sensor mode limits. This commit extracts vblank limits from the sensor<br>
> > > v4l2 controls, and stores it in the camera modes structure.<br>
> > ><br>
> > > Exposure and vblank value calculations now use values clamped to the<br>
> > > sensor mode limits. The libcamera::controls::FrameDurations metadata<br>
> > > return values now reflect the minimum and maximum after clamping.<br>
> > ><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> > > ---<br>
> > >  src/ipa/raspberrypi/cam_helper.cpp  Â  Â  Â  Â  Â | 16 +++----<br>
> > >  src/ipa/raspberrypi/cam_helper.hpp  Â  Â  Â  Â  Â |  5 +-<br>
> > >  src/ipa/raspberrypi/cam_helper_imx219.cpp  Â  |  6 +--<br>
> > >  src/ipa/raspberrypi/cam_helper_imx477.cpp  Â  |  4 +-<br>
> > >  src/ipa/raspberrypi/cam_helper_ov5647.cpp  Â  |  4 +-<br>
> > >  src/ipa/raspberrypi/controller/camera_mode.h |  2 +<br>
> > >  src/ipa/raspberrypi/raspberrypi.cpp  Â  Â  Â  Â  | 49 +++++++++++++-------<br>
> > >  7 files changed, 47 insertions(+), 39 deletions(-)<br>
> > ><br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp<br>
> > > index b7b8faf09c69..93d1b7b0296a 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper.cpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
> > > @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const &cam_name)<br>
> > >  Â  Â  Â return nullptr;<br>
> > >  }<br>
> > ><br>
> > > -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  unsigned int frameIntegrationDiff)<br>
> > > -  Â  Â : parser_(parser), initialized_(false), maxFrameLength_(maxFrameLength),<br>
> > > +CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)<br>
> > > +  Â  Â : parser_(parser), initialized_(false),<br>
> > >  Â  Â  Â  Â frameIntegrationDiff_(frameIntegrationDiff)<br>
> > >  {<br>
> > >  }<br>
> > > @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,<br>
> > >  Â  Â  Â assert(initialized_);<br>
> > ><br>
> > >  Â  Â  Â /*<br>
> > > -  Â  Â  * Clamp frame length by the frame duration range and the maximum allowable<br>
> > > -  Â  Â  * value in the sensor, given by maxFrameLength_.<br>
> > > +  Â  Â  * minFrameDuration and maxFrameDuration are clamped by the caller<br>
> > > +  Â  Â  * based on the limits for the active sensor mode.<br>
> > >  Â  Â  Â  */<br>
> > > -  Â  Â frameLengthMin = std::clamp<uint32_t>(1e3 * minFrameDuration / mode_.line_length,<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â mode_.height, maxFrameLength_);<br>
> > > -  Â  Â frameLengthMax = std::clamp<uint32_t>(1e3 * maxFrameDuration / mode_.line_length,<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â mode_.height, maxFrameLength_);<br>
> > > +  Â  Â frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;<br>
> > > +  Â  Â frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;<br>
> > > +<br>
> > >  Â  Â  Â /*<br>
> > >  Â  Â  Â  * Limit the exposure to the maximum frame duration requested, and<br>
> > >  Â  Â  Â  * re-calculate if it has been clipped.<br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp<br>
> > > index b1739a57e342..14d70112cb62 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper.hpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp<br>
> > > @@ -62,8 +62,7 @@ class CamHelper<br>
> > >  {<br>
> > >  public:<br>
> > >  Â  Â  Â static CamHelper *Create(std::string const &cam_name);<br>
> > > -  Â  Â CamHelper(MdParser *parser, unsigned int maxFrameLength,<br>
> > > -  Â  Â  Â  Â  Â  Â  Â unsigned int frameIntegrationDiff);<br>
> > > +  Â  Â CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);<br>
> > >  Â  Â  Â virtual ~CamHelper();<br>
> > >  Â  Â  Â void SetCameraMode(const CameraMode &mode);<br>
> > >  Â  Â  Â MdParser &Parser() const { return *parser_; }<br>
> > > @@ -86,8 +85,6 @@ protected:<br>
> > ><br>
> > >  private:<br>
> > >  Â  Â  Â bool initialized_;<br>
> > > -  Â  Â /* Largest possible frame length, in units of lines. */<br>
> > > -  Â  Â unsigned int maxFrameLength_;<br>
> > >  Â  Â  Â /*<br>
> > >  Â  Â  Â  * Smallest difference between the frame length and integration time,<br>
> > >  Â  Â  Â  * in units of lines.<br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > > index 8688ec091c44..95b8e698fe3b 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > > @@ -56,15 +56,13 @@ private:<br>
> > >  Â  Â  Â  * in units of lines.<br>
> > >  Â  Â  Â  */<br>
> > >  Â  Â  Â static constexpr int frameIntegrationDiff = 4;<br>
> > > -  Â  Â /* Largest possible frame length, in units of lines. */<br>
> > > -  Â  Â static constexpr int maxFrameLength = 0xffff;<br>
> > >  };<br>
> > ><br>
> > >  CamHelperImx219::CamHelperImx219()<br>
> > >  #if ENABLE_EMBEDDED_DATA<br>
> > > -  Â  Â : CamHelper(new MdParserImx219(), maxFrameLength, frameIntegrationDiff)<br>
> > > +  Â  Â : CamHelper(new MdParserImx219(), frameIntegrationDiff)<br>
> > >  #else<br>
> > > -  Â  Â : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)<br>
> > > +  Â  Â : CamHelper(new MdParserRPi(), frameIntegrationDiff)<br>
> > >  #endif<br>
> > >  {<br>
> > >  }<br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > > index 5396131003f1..eaa7be16d20e 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > > @@ -45,12 +45,10 @@ private:<br>
> > >  Â  Â  Â  * in units of lines.<br>
> > >  Â  Â  Â  */<br>
> > >  Â  Â  Â static constexpr int frameIntegrationDiff = 22;<br>
> > > -  Â  Â /* Largest possible frame length, in units of lines. */<br>
> > > -  Â  Â static constexpr int maxFrameLength = 0xffdc;<br>
> > >  };<br>
> > ><br>
> > >  CamHelperImx477::CamHelperImx477()<br>
> > > -  Â  Â : CamHelper(new MdParserImx477(), maxFrameLength, frameIntegrationDiff)<br>
> > > +  Â  Â : CamHelper(new MdParserImx477(), frameIntegrationDiff)<br>
> > >  {<br>
> > >  }<br>
> > ><br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > > index 2bd8a754f133..a7f417324048 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > > @@ -30,8 +30,6 @@ private:<br>
> > >  Â  Â  Â  * in units of lines.<br>
> > >  Â  Â  Â  */<br>
> > >  Â  Â  Â static constexpr int frameIntegrationDiff = 4;<br>
> > > -  Â  Â /* Largest possible frame length, in units of lines. */<br>
> > > -  Â  Â static constexpr int maxFrameLength = 0xffff;<br>
> > >  };<br>
> > ><br>
> > >  /*<br>
> > > @@ -40,7 +38,7 @@ private:<br>
> > >  Â */<br>
> > ><br>
> > >  CamHelperOv5647::CamHelperOv5647()<br>
> > > -  Â  Â : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)<br>
> > > +  Â  Â : CamHelper(new MdParserRPi(), frameIntegrationDiff)<br>
> > >  {<br>
> > >  }<br>
> > ><br>
> > > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h<br>
> > > index 920f11beb0b0..256438c931d9 100644<br>
> > > --- a/src/ipa/raspberrypi/controller/camera_mode.h<br>
> > > +++ b/src/ipa/raspberrypi/controller/camera_mode.h<br>
> > > @@ -37,6 +37,8 @@ struct CameraMode {<br>
> > >  Â  Â  Â double line_length;<br>
> > >  Â  Â  Â // any camera transform *not* reflected already in the camera tuning<br>
> > >  Â  Â  Â libcamera::Transform transform;<br>
> > > +  Â  Â // minimum and maximum fame lengths in units of lines<br>
> > > +  Â  Â uint32_t min_frame_length, max_frame_length;<br>
> > >  };<br>
> > ><br>
> > >  #ifdef __cplusplus<br>
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > index 5ccc7a6551f5..e4911b734e20 100644<br>
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > @@ -102,6 +102,7 @@ private:<br>
> > >  Â  Â  Â void reportMetadata();<br>
> > >  Â  Â  Â bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);<br>
> > >  Â  Â  Â void processStats(unsigned int bufferId);<br>
> > > +  Â  Â void applyFrameDurations(double minFrameDuration, double maxFrameDuration);<br>
> > >  Â  Â  Â void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);<br>
> > >  Â  Â  Â void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);<br>
> > >  Â  Â  Â void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);<br>
> > > @@ -279,6 +280,13 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)<br>
> > >  Â  Â  Â  * the line length in pixels and the pixel rate.<br>
> > >  Â  Â  Â  */<br>
> > >  Â  Â  Â mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;<br>
> > > +<br>
> > > +  Â  Â /*<br>
> > > +  Â  Â  * Set the frame length limits for the mode to ensure exposure and<br>
> > > +  Â  Â  * framerate calculations are clipped appropriately.<br>
> > > +  Â  Â  */<br>
> > > +  Â  Â mode_.min_frame_length = sensorInfo.minFrameLength;<br>
> > > +  Â  Â mode_.max_frame_length = sensorInfo.maxFrameLength;<br>
> > >  }<br>
> > ><br>
> > >  void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> > > @@ -384,8 +392,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> > >  Â  Â  Â  Â  Â  Â  Â controller_.Initialise();<br>
> > >  Â  Â  Â  Â  Â  Â  Â controllerInit_ = true;<br>
> > ><br>
> > > -  Â  Â  Â  Â  Â  Â minFrameDuration_ = defaultMinFrameDuration;<br>
> > > -  Â  Â  Â  Â  Â  Â maxFrameDuration_ = defaultMaxFrameDuration;<br>
> > > +  Â  Â  Â  Â  Â  Â /* Supply initial values for frame durations. */<br>
> > > +  Â  Â  Â  Â  Â  Â applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);<br>
> > ><br>
> > >  Â  Â  Â  Â  Â  Â  Â /* Supply initial values for gain and exposure. */<br>
> > >  Â  Â  Â  Â  Â  Â  Â ControlList ctrls(sensorCtrls_);<br>
> > > @@ -819,20 +827,7 @@ void IPARPi::queueRequest(const ControlList &controls)<br>
> > ><br>
> > >  Â  Â  Â  Â  Â  Â  Â case controls::FRAME_DURATIONS: {<br>
> > >  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â auto frameDurations = ctrl.second.get<Span<const int64_t>>();<br>
> > > -<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â /* This will be applied once AGC recalculations occur. */<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â minFrameDuration_ = frameDurations[0] ? frameDurations[0] : defaultMinFrameDuration;<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â maxFrameDuration_ = frameDurations[1] ? frameDurations[1] : defaultMaxFrameDuration;<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);<br>
> > > -<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â /*<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  * \todo The values returned in the metadata below must be<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  * correctly clipped by what the sensor mode supports and<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  * what the AGC exposure mode or manual shutter speed limits<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  */<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â libcameraMetadata_.set(controls::FrameDurations,<br>
> > > -  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  { static_cast<int64_t>(minFrameDuration_),<br>
> > > - static_cast<int64_t>(maxFrameDuration_) });<br>
> > > +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â applyFrameDurations(frameDurations[0], frameDurations[1]);<br>
> > >  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â break;<br>
> > >  Â  Â  Â  Â  Â  Â  Â }<br>
> > ><br>
> > > @@ -991,6 +986,28 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)<br>
> > >  Â  Â  Â  Â  Â  Â  Â  Â static_cast<int32_t>(awbStatus->gain_b * 1000));<br>
> > >  }<br>
> > ><br>
> > > +void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuration)<br>
> > > +{<br>
> > > +  Â  Â const double minSensorFrameDuration = 1e-3 * mode_.min_frame_length * mode_.line_length;<br>
> > > +  Â  Â const double maxSensorFrameDuration = 1e-3 * mode_.max_frame_length * mode_.line_length;<br>
> > > +<br>
> > > +  Â  Â /*<br>
> > > +  Â  Â  * This will only be applied once AGC recalculations occur.<br>
> > > +  Â  Â  * The values may be clamped based on the sensor mode capabilities as well.<br>
> > > +  Â  Â  */<br>
> > > +  Â  Â minFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration;<br>
> > > +  Â  Â maxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration;<br>
> > > +  Â  Â minFrameDuration_ = std::clamp(minFrameDuration_,<br>
> > > +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  minSensorFrameDuration, maxSensorFrameDuration);<br>
> > > +  Â  Â maxFrameDuration_ = std::clamp(maxFrameDuration_,<br>
> > > +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  minSensorFrameDuration, maxSensorFrameDuration);<br>
> ><br>
> > Do we need to keep<br>
> ><br>
> >  Â  Â  Â  Â maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);<br>
> ><br>
> > in case the user passes a minimum higher than the maximum ? Apart from<br>
> > that (which I can fix when applying if needed),<br>
> <br>
> Please correct me if I am wrong, but having the *FrameDuration_ values<br>
> clamped by the *SensorFrameDurations would assure that max >= min, so I<br>
> removed that statement.  This is assuming that the sensor driver reports<br>
> max vblank >= min vblank - which it should.<br>
<br>
Let's assume<br>
<br>
- minSensorFrameDuration = 1000<br>
- maxSensorFrameDuration = 5000<br>
- minFrameDuration = 2000<br>
- maxFrameDuration = 1500<br>
<br>
After clamping the minFrameDuration and maxFrameDuration values are not<br>
changed, and minFrameDuration is still > maxFrameDuration. Am I missing<br>
something ?<br>
<br></blockquote><div><br></div><div>You are entirely correct, it was me who was missing something :-)</div><div>Please do re-introduce that line when merging.<br></div><div><br></div><div>Thanks!</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> ><br>
> > > +<br>
> > > +  Â  Â /* Return the validated limits out though metadata. */<br>
> > > +  Â  Â libcameraMetadata_.set(controls::FrameDurations,<br>
> > > +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  { static_cast<int64_t>(minFrameDuration_),<br>
> > > +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  static_cast<int64_t>(maxFrameDuration_) });<br>
> > > +}<br>
> > > +<br>
> > >  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
> > >  {<br>
> > >  Â  Â  Â int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>