<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for the review.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 18 Dec 2020 at 13:04, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@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"><br>
<br>
On 18/12/2020 10:06, Naushir Patuck wrote:<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>
> 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>
> Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</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 | 52 ++++++++++++++++---<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-<br>
> 8 files changed, 127 insertions(+), 14 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h<br>
> index 01fe5abc..1de36039 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(1000, 1 000 000 000) },<br>
<br>
Perhaps we should define some human readable $bignumbers to ensure we<br>
don't mis-represent or mis-calculate large numbers like that.<br>
<br>
Or use some chrono class to be able to express the value in readable<br>
terms, but store it appropriately. Of course then it gets more difficult<br>
to handle that in the Control I think ...<br>
<br>
But anyway, not in this patch.<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 6efa0d7f..018c17b9 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>
<br>
Clip? or Clamp? Doesn't really matter though ;-) both make sense.<br>
<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 0b841cd1..2bd8a754 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> @@ -23,6 +23,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>
> @@ -31,7 +40,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 d087b07e..d309536b 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>
> @@ -150,6 +152,9 @@ private:<br>
> <br>
> /* Distinguish the first camera start from others. */<br>
> bool firstStart_;<br>
> +<br>
> + /* Frame duration (1/fps) given in microseconds. */<br>
> + double minFrameDuration_, maxFrameDuration_;<br>
<br>
Hrm, there's nothing syntactically wrong with this - but I don't think<br>
we've used multiple definitions on a single line anywhere else?<br>
<br>
But this is src/ipa/raspberrypi ... so I think you might get away with<br>
it :-)<br>
<br>
> };<br>
> <br>
> int IPARPi::init(const IPASettings &settings)<br>
> @@ -332,7 +337,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>
> @@ -377,6 +383,9 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> controller_.Initialise();<br>
> controllerInit_ = true;<br>
> <br>
> + minFrameDuration_ = defaultMinFrameDuration;<br>
> + maxFrameDuration_ = defaultMaxFrameDuration;<br>
> +<br>
> /* Supply initial values for gain and exposure. */<br>
> ControlList ctrls(sensorCtrls_);<br>
> AgcStatus agcStatus;<br>
> @@ -524,7 +533,8 @@ bool IPARPi::validateSensorControls()<br>
> {<br>
> static const uint32_t ctrls[] = {<br>
> V4L2_CID_ANALOGUE_GAIN,<br>
> - V4L2_CID_EXPOSURE<br>
> + V4L2_CID_EXPOSURE,<br>
> + V4L2_CID_VBLANK<br>
<br>
Should we keep a comma on the end so additions don't require a previous<br>
line replacement?<br>
<br>
> };<br>
> <br>
> for (auto c : ctrls) {<br>
> @@ -804,6 +814,24 @@ 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>
> +<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>
> + 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>
> @@ -962,15 +990,27 @@ 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>
> - LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time<br>
> - << " (Shutter lines: " << exposureLines << ") Gain: "<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>
> + 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>
<br>
The next time? You mean this only happens on the first call through? or<br>
something else?<br></blockquote><div><br></div><div>By "next time", I mean the next time you go into this function to set exposure, it will be validated against the previous VBLANK ctrl value which is a bit annoything. And the cycle continues on every subsequent call to applyAGC(). Does not matter so much, I do have a workaround for this in my next series.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></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>
<br>
Nothing blocking there that I see though.<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<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 7a5f5881..252cab64 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1233,7 +1233,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>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>