<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>