<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 11 Dec 2020 at 13:58, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.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>
Thanks for the revised patch!<br>
<br>
On Fri, 11 Dec 2020 at 13:35, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Add support for setting V4L2_CID_VBLANK appropriately when setting<br>
> V4L2_CID_EXPOSURE. This will allow adaptive framerates during<br>
> viewfinder use cases (e.g. when the exposure time goes above 33ms, we<br>
> can reduce the framerate to lower than 30fps).<br>
><br>
> The minimum and maximum frame durations are provided via libcamera<br>
> controls, and will prioritise exposure time limits over any AGC request.<br>
><br>
> V4L2_CID_VBLANK is controlled through the staggered writer, just like<br>
> the exposure and gain controls.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  include/libcamera/ipa/raspberrypi.h           |  1 +<br>
>  src/ipa/raspberrypi/cam_helper.cpp            | 35 +++++++++++++++-<br>
>  src/ipa/raspberrypi/cam_helper.hpp            | 15 ++++++-<br>
>  src/ipa/raspberrypi/cam_helper_imx219.cpp     | 13 +++++-<br>
>  src/ipa/raspberrypi/cam_helper_imx477.cpp     | 11 ++++-<br>
>  src/ipa/raspberrypi/cam_helper_ov5647.cpp     | 11 ++++-<br>
>  src/ipa/raspberrypi/raspberrypi.cpp           | 42 ++++++++++++++++---<br>
>  .../pipeline/raspberrypi/raspberrypi.cpp      |  3 +-<br>
>  8 files changed, 118 insertions(+), 13 deletions(-)<br>
><br>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h<br>
> index 01fe5abc..160ca681 100644<br>
> --- a/include/libcamera/ipa/raspberrypi.h<br>
> +++ b/include/libcamera/ipa/raspberrypi.h<br>
> @@ -65,6 +65,7 @@ static const ControlInfoMap Controls = {<br>
>         { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },<br>
>         { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },<br>
>         { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },<br>
> +       { &controls::FrameDurations, ControlInfo(1.0e3f, 1.0e9f) },<br>
<br>
Just wondering whether it looks a bit strange to initialise explicitly<br>
with floats now that they're int64_t, though I guess it makes no real<br>
difference...<br></blockquote><div><br></div><div>This is wrong!  I forgot to update this bit of code, thanks for spotting that!</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">
<br>
But other than this tiny thing:<br>
<br>
Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Thanks!<br>
David<br>
<br>
>  };<br>
><br>
>  } /* namespace RPi */<br>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp<br>
> index c8ac3232..7fd3e7f6 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
> @@ -34,8 +34,10 @@ CamHelper *CamHelper::Create(std::string const &cam_name)<br>
>         return nullptr;<br>
>  }<br>
><br>
> -CamHelper::CamHelper(MdParser *parser)<br>
> -       : parser_(parser), initialized_(false)<br>
> +CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,<br>
> +                    unsigned int frameIntegrationDiff)<br>
> +       : parser_(parser), initialized_(false), maxFrameLength_(maxFrameLength),<br>
> +         frameIntegrationDiff_(frameIntegrationDiff)<br>
>  {<br>
>  }<br>
><br>
> @@ -56,6 +58,35 @@ double CamHelper::Exposure(uint32_t exposure_lines) const<br>
>         return exposure_lines * mode_.line_length / 1000.0;<br>
>  }<br>
><br>
> +uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,<br>
> +                                double maxFrameDuration) const<br>
> +{<br>
> +       uint32_t frameLengthMin, frameLengthMax, vblank;<br>
> +       uint32_t exposureLines = ExposureLines(exposure);<br>
> +<br>
> +       assert(initialized_);<br>
> +<br>
> +       /*<br>
> +        * Clip frame length by the frame duration range and the maximum allowable<br>
> +        * value in the sensor, given by maxFrameLength_.<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>
> +       /*<br>
> +        * Limit the exposure to the maximum frame duration requested, and<br>
> +        * re-calculate if it has been clipped.<br>
> +        */<br>
> +       exposureLines = std::min(frameLengthMax - frameIntegrationDiff_, exposureLines);<br>
> +       exposure = Exposure(exposureLines);<br>
> +<br>
> +       /* Limit the vblank to the range allowed by the frame length limits. */<br>
> +       vblank = std::clamp(exposureLines + frameIntegrationDiff_,<br>
> +                           frameLengthMin, frameLengthMax) - mode_.height;<br>
> +       return vblank;<br>
> +}<br>
> +<br>
>  void CamHelper::SetCameraMode(const CameraMode &mode)<br>
>  {<br>
>         mode_ = mode;<br>
> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp<br>
> index 044c2866..b1739a57 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper.hpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper.hpp<br>
> @@ -62,12 +62,15 @@ class CamHelper<br>
>  {<br>
>  public:<br>
>         static CamHelper *Create(std::string const &cam_name);<br>
> -       CamHelper(MdParser *parser);<br>
> +       CamHelper(MdParser *parser, unsigned int maxFrameLength,<br>
> +                 unsigned int frameIntegrationDiff);<br>
>         virtual ~CamHelper();<br>
>         void SetCameraMode(const CameraMode &mode);<br>
>         MdParser &Parser() const { return *parser_; }<br>
>         uint32_t ExposureLines(double exposure_us) const;<br>
>         double Exposure(uint32_t exposure_lines) const; // in us<br>
> +       virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,<br>
> +                                     double maxFrameDuration) const;<br>
>         virtual uint32_t GainCode(double gain) const = 0;<br>
>         virtual double Gain(uint32_t gain_code) const = 0;<br>
>         virtual void GetDelays(int &exposure_delay, int &gain_delay) const;<br>
> @@ -76,10 +79,20 @@ public:<br>
>         virtual unsigned int HideFramesModeSwitch() const;<br>
>         virtual unsigned int MistrustFramesStartup() const;<br>
>         virtual unsigned int MistrustFramesModeSwitch() const;<br>
> +<br>
>  protected:<br>
>         MdParser *parser_;<br>
>         CameraMode mode_;<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>
> +        */<br>
> +       unsigned int frameIntegrationDiff_;<br>
>  };<br>
><br>
>  // This is for registering camera helpers with the system, so that the<br>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> index db8ab879..8688ec09 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> @@ -49,13 +49,22 @@ public:<br>
>         double Gain(uint32_t gain_code) const override;<br>
>         unsigned int MistrustFramesModeSwitch() const override;<br>
>         bool SensorEmbeddedDataPresent() const override;<br>
> +<br>
> +private:<br>
> +       /*<br>
> +        * Smallest difference between the frame length and integration time,<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())<br>
> +       : CamHelper(new MdParserImx219(), maxFrameLength, frameIntegrationDiff)<br>
>  #else<br>
> -       : CamHelper(new MdParserRPi())<br>
> +       : CamHelper(new MdParserRPi(), maxFrameLength, 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 0e896ac7..53961310 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> @@ -38,10 +38,19 @@ public:<br>
>         uint32_t GainCode(double gain) const override;<br>
>         double Gain(uint32_t gain_code) const override;<br>
>         bool SensorEmbeddedDataPresent() const override;<br>
> +<br>
> +private:<br>
> +       /*<br>
> +        * Smallest difference between the frame length and integration time,<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())<br>
> +       : CamHelper(new MdParserImx477(), maxFrameLength, 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 dc5d8275..7630c127 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> @@ -22,6 +22,15 @@ public:<br>
>         unsigned int HideFramesModeSwitch() const override;<br>
>         unsigned int MistrustFramesStartup() const override;<br>
>         unsigned int MistrustFramesModeSwitch() const override;<br>
> +<br>
> +private:<br>
> +       /*<br>
> +        * Smallest difference between the frame length and integration time,<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>
> @@ -30,7 +39,7 @@ public:<br>
>   */<br>
><br>
>  CamHelperOv5647::CamHelperOv5647()<br>
> -       : CamHelper(new MdParserRPi())<br>
> +       : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)<br>
>  {<br>
>  }<br>
><br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 60cfdc27..f6d2b543 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -58,6 +58,8 @@ namespace libcamera {<br>
>  /* Configure the sensor with these values initially. */<br>
>  constexpr double DefaultAnalogueGain = 1.0;<br>
>  constexpr unsigned int DefaultExposureTime = 20000;<br>
> +constexpr double defaultMinFrameDuration = 1e6 / 30.0;<br>
> +constexpr double defaultMaxFrameDuration = 1e6 / 0.01;<br>
><br>
>  LOG_DEFINE_CATEGORY(IPARPI)<br>
><br>
> @@ -145,6 +147,9 @@ private:<br>
>         /* LS table allocation passed in from the pipeline handler. */<br>
>         FileDescriptor lsTableHandle_;<br>
>         void *lsTable_;<br>
> +<br>
> +       /* Frame duration (1/fps) given in microseconds. */<br>
> +       double minFrameDuration_, maxFrameDuration_;<br>
>  };<br>
><br>
>  int IPARPi::init(const IPASettings &settings)<br>
> @@ -266,7 +271,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
>                 sensorMetadata = helper_->SensorEmbeddedDataPresent();<br>
><br>
>                 result->data.push_back(gainDelay);<br>
> -               result->data.push_back(exposureDelay);<br>
> +               result->data.push_back(exposureDelay); /* FOR EXPOSURE ctrl */<br>
> +               result->data.push_back(exposureDelay); /* For VBLANK ctrl */<br>
>                 result->data.push_back(sensorMetadata);<br>
><br>
>                 result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;<br>
> @@ -335,6 +341,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
>                 AgcStatus agcStatus;<br>
>                 agcStatus.shutter_time = DefaultExposureTime;<br>
>                 agcStatus.analogue_gain = DefaultAnalogueGain;<br>
> +               minFrameDuration_ = defaultMinFrameDuration;<br>
> +               maxFrameDuration_ = defaultMaxFrameDuration;<br>
>                 applyAGC(&agcStatus, ctrls);<br>
><br>
>                 result->controls.emplace_back(ctrls);<br>
> @@ -712,6 +720,18 @@ void IPARPi::queueRequest(const ControlList &controls)<br>
>                         break;<br>
>                 }<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>
> +                       libcameraMetadata_.set(controls::FrameDurations,<br>
> +                                              { static_cast<int64_t>(minFrameDuration_),<br>
> +                                                static_cast<int64_t>(maxFrameDuration_) });<br>
> +                       break;<br>
> +               }<br>
> +<br>
>                 default:<br>
>                         LOG(IPARPI, Warning)<br>
>                                 << "Ctrl " << controls::<a href="http://controls.at" rel="noreferrer" target="_blank">controls.at</a>(ctrl.first)->name()<br>
> @@ -882,7 +902,12 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)<br>
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
>  {<br>
>         int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);<br>
> -       int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);<br>
> +<br>
> +       /* GetVBlanking might clip exposure time to the fps limits. */<br>
> +       double exposure = agcStatus->shutter_time;<br>
> +       int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_,<br>
> +                                                 maxFrameDuration_);<br>
> +       int32_t exposureLines = helper_->ExposureLines(exposure);<br>
><br>
>         if (unicamCtrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicamCtrls_.end()) {<br>
>                 LOG(IPARPI, Error) << "Can't find analogue gain control";<br>
> @@ -894,13 +919,20 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
>                 return;<br>
>         }<br>
><br>
> -       LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time<br>
> -                          << " (Shutter lines: " << exposureLines << ") Gain: "<br>
> +       LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure<br>
> +                          << " (Shutter lines: " << exposureLines << ", AGC requested "<br>
> +                          << agcStatus->shutter_time << ") Gain: "<br>
>                            << agcStatus->analogue_gain << " (Gain Code: "<br>
>                            << gainCode << ")";<br>
><br>
> -       ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);<br>
> +       /*<br>
> +        * Due to the behavior of V4L2, the current value of VBLANK could clip the<br>
> +        * exposure time without us knowing. The next time though this function should<br>
> +        * clip exposure correctly.<br>
> +        */<br>
> +       ctrls.set(V4L2_CID_VBLANK, vblanking);<br>
>         ctrls.set(V4L2_CID_EXPOSURE, exposureLines);<br>
> +       ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);<br>
>  }<br>
><br>
>  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 2ec1f6e7..13349f31 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1221,7 +1221,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
>                 if (!staggeredCtrl_) {<br>
>                         staggeredCtrl_.init(unicam_[Unicam::Image].dev(),<br>
>                                             { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },<br>
> -                                             { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });<br>
> +                                             { V4L2_CID_EXPOSURE, result.data[resultIdx++] },<br>
> +                                             { V4L2_CID_VBLANK, result.data[resultIdx++] } });<br>
>                         sensorMetadata_ = result.data[resultIdx++];<br>
>                 }<br>
>         }<br>
> --<br>
> 2.25.1<br>
><br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>