[libcamera-devel] [PATCH v12 2/3] libcamera: raspberrypi: Add control of sensor vblanking
Jacopo Mondi
jacopo at jmondi.org
Tue Jan 19 18:11:12 CET 2021
Hi again
On Tue, Jan 19, 2021 at 06:00:11PM +0100, Jacopo Mondi wrote:
> Hi Naush,
>
> I know I have my tag here, and I'm possibly going to repeat some
> comments. I don't think any of them should block your series though
>
> On Tue, Jan 19, 2021 at 03:30:46PM +0000, Naushir Patuck wrote:
> > Add support for setting V4L2_CID_VBLANK appropriately when setting
> > V4L2_CID_EXPOSURE. This will allow adaptive framerates during
> > viewfinder use cases (e.g. when the exposure time goes above 33ms, we
> > can reduce the framerate to lower than 30fps).
> >
> > The minimum and maximum frame durations are provided via libcamera
> > controls, and will prioritise exposure time limits over any AGC request.
> >
> > V4L2_CID_VBLANK is controlled through the staggered writer, just like
> > the exposure and gain controls.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > Tested-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > include/libcamera/ipa/raspberrypi.h | 1 +
> > src/ipa/raspberrypi/cam_helper.cpp | 35 +++++++++++-
> > src/ipa/raspberrypi/cam_helper.hpp | 15 ++++-
> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 13 ++++-
> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 11 +++-
> > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 11 +++-
> > src/ipa/raspberrypi/raspberrypi.cpp | 56 ++++++++++++++++---
> > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-
> > 8 files changed, 130 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index 01fe5abce9a6..1de36039cee0 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -65,6 +65,7 @@ static const ControlInfoMap Controls = {
> > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> > + { &controls::FrameDurations, ControlInfo(1000, 1000000000) },
>
> The static initialization of the control limits worries me a bit.
> This should match the sensor limits. I can't ask you to go to fully
> implement this, but I fear the arbitrary initialization of the
> controls limits the RPi implementation does will be problematic for
> application that might what to know in which range they can safely
> operate.
>
> I presume your forthcoming applications will probably know those limits
> from a per-sensor configuration file ?
>
> > };
> >
> > } /* namespace RPi */
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> > index 6efa0d7fd030..b7b8faf09c69 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -34,8 +34,10 @@ CamHelper *CamHelper::Create(std::string const &cam_name)
> > return nullptr;
> > }
> >
> > -CamHelper::CamHelper(MdParser *parser)
> > - : parser_(parser), initialized_(false)
> > +CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,
> > + unsigned int frameIntegrationDiff)
> > + : parser_(parser), initialized_(false), maxFrameLength_(maxFrameLength),
> > + frameIntegrationDiff_(frameIntegrationDiff)
> > {
> > }
> >
> > @@ -56,6 +58,35 @@ double CamHelper::Exposure(uint32_t exposure_lines) const
> > return exposure_lines * mode_.line_length / 1000.0;
> > }
> >
> > +uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,
> > + double maxFrameDuration) const
> > +{
> > + uint32_t frameLengthMin, frameLengthMax, vblank;
> > + uint32_t exposureLines = ExposureLines(exposure);
> > +
> > + assert(initialized_);
> > +
> > + /*
> > + * Clamp frame length by the frame duration range and the maximum allowable
> > + * value in the sensor, given by maxFrameLength_.
> > + */
> > + 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_);
>
> min and max frame durations are here adjusted to respect the sensor's
> configuration. Shouldn't this be reflected in the metadata ?
>
> > + /*
> > + * Limit the exposure to the maximum frame duration requested, and
> > + * re-calculate if it has been clipped.
> > + */
> > + exposureLines = std::min(frameLengthMax - frameIntegrationDiff_, exposureLines);
>
> Can't you here
> exposureLines = std::max(frameLengthMin, exposureLines + frameIntegrationDiff_);
>
> To avoid the below clamp, which is partially useless as we're already
> sure exposureLines < (frameLengthMax - frameIntegrationDiff_)
>
> (and I'm not sure if frameIntegrationDiff_ has only to be considered
> when inspecting the upper limit or also when comparing to the bottom
> one)
>
> it will also make sure the below recalculation of 'exposure' is in the limits
>
>
>
> > + exposure = Exposure(exposureLines);
> > +
> > + /* Limit the vblank to the range allowed by the frame length limits. */
> > + vblank = std::clamp(exposureLines + frameIntegrationDiff_,
> > + frameLengthMin, frameLengthMax) - mode_.height;
>
> Then you can simply
> return exposureLines - mode_.height;
>
> > + return vblank;
> > +}
> > +
> > void CamHelper::SetCameraMode(const CameraMode &mode)
> > {
> > mode_ = mode;
> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> > index 044c28667f75..b1739a57e342 100644
> > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > @@ -62,12 +62,15 @@ class CamHelper
> > {
> > public:
> > static CamHelper *Create(std::string const &cam_name);
> > - CamHelper(MdParser *parser);
> > + CamHelper(MdParser *parser, unsigned int maxFrameLength,
> > + unsigned int frameIntegrationDiff);
> > virtual ~CamHelper();
> > void SetCameraMode(const CameraMode &mode);
> > MdParser &Parser() const { return *parser_; }
> > uint32_t ExposureLines(double exposure_us) const;
> > double Exposure(uint32_t exposure_lines) const; // in us
> > + virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,
>
> The first parameter is called just 'exposure' in the function
> implementation. Mixing of snake_case and camelCase, but it's in many
> places already in the IPA, so...
>
> > + double maxFrameDuration) const;
> > virtual uint32_t GainCode(double gain) const = 0;
> > virtual double Gain(uint32_t gain_code) const = 0;
> > virtual void GetDelays(int &exposure_delay, int &gain_delay) const;
> > @@ -76,10 +79,20 @@ public:
> > virtual unsigned int HideFramesModeSwitch() const;
> > virtual unsigned int MistrustFramesStartup() const;
> > virtual unsigned int MistrustFramesModeSwitch() const;
> > +
> > protected:
> > MdParser *parser_;
> > CameraMode mode_;
> > +
> > +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.
> > + */
> > + unsigned int frameIntegrationDiff_;
>
> nit: different declaration order than in the derived classes, but
> well, minor..
>
> > };
> >
> > // This is for registering camera helpers with the system, so that the
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > index db8ab879deb7..8688ec091c44 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > @@ -49,13 +49,22 @@ public:
> > double Gain(uint32_t gain_code) const override;
> > unsigned int MistrustFramesModeSwitch() const override;
> > bool SensorEmbeddedDataPresent() const override;
> > +
> > +private:
> > + /*
> > + * Smallest difference between the frame length and integration time,
> > + * 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())
> > + : CamHelper(new MdParserImx219(), maxFrameLength, frameIntegrationDiff)
> > #else
> > - : CamHelper(new MdParserRPi())
> > + : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)
> > #endif
> > {
> > }
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > index 0e896ac74f88..5396131003f1 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > @@ -38,10 +38,19 @@ public:
> > uint32_t GainCode(double gain) const override;
> > double Gain(uint32_t gain_code) const override;
> > bool SensorEmbeddedDataPresent() const override;
> > +
> > +private:
> > + /*
> > + * Smallest difference between the frame length and integration time,
> > + * 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())
> > + : CamHelper(new MdParserImx477(), maxFrameLength, frameIntegrationDiff)
> > {
> > }
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > index 0b841cd175fa..2bd8a754f133 100644
> > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > @@ -23,6 +23,15 @@ public:
> > unsigned int HideFramesModeSwitch() const override;
> > unsigned int MistrustFramesStartup() const override;
> > unsigned int MistrustFramesModeSwitch() const override;
> > +
> > +private:
> > + /*
> > + * Smallest difference between the frame length and integration time,
> > + * in units of lines.
> > + */
> > + static constexpr int frameIntegrationDiff = 4;
> > + /* Largest possible frame length, in units of lines. */
> > + static constexpr int maxFrameLength = 0xffff;
> > };
> >
> > /*
> > @@ -31,7 +40,7 @@ public:
> > */
> >
> > CamHelperOv5647::CamHelperOv5647()
> > - : CamHelper(new MdParserRPi())
> > + : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)
> > {
> > }
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index d087b07e186b..ba9bc398ef87 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -58,6 +58,8 @@ namespace libcamera {
> > /* Configure the sensor with these values initially. */
> > constexpr double DefaultAnalogueGain = 1.0;
> > constexpr unsigned int DefaultExposureTime = 20000;
> > +constexpr double defaultMinFrameDuration = 1e6 / 30.0;
> > +constexpr double defaultMaxFrameDuration = 1e6 / 0.01;
>
> Quite arbitrary, isn't it ?
>
> I would have expected this to be sent by the pipeline to the IPA by
> populating the FrameDuration ControlInfoMap at configure() time. The
Sorry, I meant by "populating the entityControls map at configure()
time". We exchange V4L2 controls at configure() not libcamera
controls, and in fact in my proposed calculation here I use VBLANK.
> 'sensorControls' provided at configure might very well contain the
> VBLANK control limits, which once combined with the sensor mode
> information should give you precise limits
>
> minFrameDuration = (mode.height + min(VBLANK)) * mode.line_lenght
> / (mode.pixel_rate / 1e6)
>
> Or is this not required ?
>
> >
> > LOG_DEFINE_CATEGORY(IPARPI)
> >
> > @@ -150,6 +152,10 @@ private:
> >
> > /* Distinguish the first camera start from others. */
> > bool firstStart_;
> > +
> > + /* Frame duration (1/fps) limits, given in microseconds. */
> > + double minFrameDuration_;
> > + double maxFrameDuration_;
> > };
> >
> > int IPARPi::init(const IPASettings &settings)
> > @@ -332,7 +338,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > sensorMetadata = helper_->SensorEmbeddedDataPresent();
> >
> > result->data.push_back(gainDelay);
> > - result->data.push_back(exposureDelay);
> > + result->data.push_back(exposureDelay); /* For EXPOSURE ctrl */
> > + result->data.push_back(exposureDelay); /* For VBLANK ctrl */
> > result->data.push_back(sensorMetadata);
> >
> > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;
> > @@ -377,6 +384,9 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > controller_.Initialise();
> > controllerInit_ = true;
> >
> > + minFrameDuration_ = defaultMinFrameDuration;
> > + maxFrameDuration_ = defaultMaxFrameDuration;
> > +
> > /* Supply initial values for gain and exposure. */
> > ControlList ctrls(sensorCtrls_);
> > AgcStatus agcStatus;
> > @@ -524,7 +534,8 @@ bool IPARPi::validateSensorControls()
> > {
> > static const uint32_t ctrls[] = {
> > V4L2_CID_ANALOGUE_GAIN,
> > - V4L2_CID_EXPOSURE
> > + V4L2_CID_EXPOSURE,
> > + V4L2_CID_VBLANK,
> > };
> >
> > for (auto c : ctrls) {
> > @@ -551,7 +562,7 @@ bool IPARPi::validateIspControls()
> > V4L2_CID_USER_BCM2835_ISP_DENOISE,
> > V4L2_CID_USER_BCM2835_ISP_SHARPEN,
> > V4L2_CID_USER_BCM2835_ISP_DPC,
> > - V4L2_CID_USER_BCM2835_ISP_LENS_SHADING
> > + V4L2_CID_USER_BCM2835_ISP_LENS_SHADING,
>
> Unrelated it seems
>
> > };
> >
> > for (auto c : ctrls) {
> > @@ -804,6 +815,25 @@ void IPARPi::queueRequest(const ControlList &controls)
> > break;
> > }
> >
> > + 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_);
>
> Why this last line ?
> Also, it really seems seems arbitrary defaults clips the exposure to
> pretty abitrary values. If you don't want to do the calculation of the
> defaults, what about skipping the clipping completely ? Or is this
> your intetion ? "Pick default values large enough that no clipping
> occours" ?
>
> > +
> > + /*
> > + * \todo The values returned in the metadata below must be
> > + * correctly clipped by what the sensor mode suppots and
> > + * what the AGC exposure mode or manual shutter speed limits
> > + */
>
> Why this metadata can't be set in reportMetadata() which is called
> after processStat() which calls GetVBlanking() and updates the limits
> ?
>
>
> Sorry for the many questions. The only thing that worries me is the
> ControlInfo intialization, but that's not something you should address
> at the moment. Other than that with the metadata and default value
> assignement clarified, you can retain my tag.
>
> Thanks
> j
>
> > + libcameraMetadata_.set(controls::FrameDurations,
> > + { static_cast<int64_t>(minFrameDuration_),
> > + static_cast<int64_t>(maxFrameDuration_) });
> > + break;
> > + }
> > +
> > default:
> > LOG(IPARPI, Warning)
> > << "Ctrl " << controls::controls.at(ctrl.first)->name()
> > @@ -962,15 +992,27 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> > {
> > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);
> > - int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);
> >
> > - LOG(IPARPI, Debug) << "Applying AGC Exposure: " << agcStatus->shutter_time
> > - << " (Shutter lines: " << exposureLines << ") Gain: "
> > + /* GetVBlanking might clip exposure time to the fps limits. */
> > + double exposure = agcStatus->shutter_time;
> > + int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_,
> > + maxFrameDuration_);
> > + int32_t exposureLines = helper_->ExposureLines(exposure);
> > +
> > + LOG(IPARPI, Debug) << "Applying AGC Exposure: " << exposure
> > + << " (Shutter lines: " << exposureLines << ", AGC requested "
> > + << agcStatus->shutter_time << ") Gain: "
> > << agcStatus->analogue_gain << " (Gain Code: "
> > << gainCode << ")";
> >
> > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
> > + /*
> > + * Due to the behavior of V4L2, the current value of VBLANK could clip the
> > + * exposure time without us knowing. The next time though this function should
> > + * clip exposure correctly.
> > + */
> > + ctrls.set(V4L2_CID_VBLANK, vblanking);
> > ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
> > + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);
> > }
> >
> > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 7a5f5881b9b3..252cab64023e 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > if (!staggeredCtrl_) {
> > staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> > { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },
> > - { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });
> > + { V4L2_CID_EXPOSURE, result.data[resultIdx++] },
> > + { V4L2_CID_VBLANK, result.data[resultIdx++] } });
> > sensorMetadata_ = result.data[resultIdx++];
> > }
> > }
> > --
> > 2.25.1
> >
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list