[libcamera-devel] [PATCH 2/2] libcamera: raspberrypi: Limit the maximum framerate to 30.0fps
Naushir Patuck
naush at raspberrypi.com
Tue May 12 12:25:28 CEST 2020
Hi Laurent,
On Tue, 12 May 2020 at 01:38, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, May 11, 2020 at 11:01:50AM +0100, Naushir Patuck wrote:
> > With the previous change to adapt framerate using VBLANK, the maximum
> > framerate was limited by the sensor mode. This may not be entirely
> > appropriate for what an application expects in the case where a mode
> > can produce > 30.0fps.
>
> This answers a question in my review of the previous patch :-) I
> wouldn't be opposed to squashing the two patches together.
Sure, I have no problem with that.
> > Provide a mechanism to limit the maximum framerate. This is currently
> > a const set to 30.0fps. In future, this value could be passed in via
> > a Request or stream configuration.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > src/ipa/raspberrypi/cam_helper.hpp | 2 +-
> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 18 ++++++++++++++----
> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 18 ++++++++++++++----
> > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 18 ++++++++++++++----
> > src/ipa/raspberrypi/raspberrypi.cpp | 8 +++++++-
> > 5 files changed, 50 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> > index 37281c78..4f7a3897 100644
> > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > @@ -74,7 +74,7 @@ public:
> > 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) const = 0;
> > + virtual uint32_t GetVBlanking(double exposure_us, double maxFps) const = 0;
> > 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;
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > index d34a1f1f..51a18aca 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > @@ -45,7 +45,7 @@ class CamHelperImx219 : public CamHelper
> > {
> > public:
> > CamHelperImx219();
> > - uint32_t GetVBlanking(double exposure) const override;
> > + uint32_t GetVBlanking(double exposure, double maxFps) const override;
> > uint32_t GainCode(double gain) const override;
> > double Gain(uint32_t gain_code) const override;
> > unsigned int MistrustFramesModeSwitch() const override;
> > @@ -55,6 +55,8 @@ public:
> > private:
> > /* Smallest difference between the frame length and integration time. */
> > static constexpr int frameIntegrationDiff = 4;
> > + /* Largest possible frame length. */
> > + static constexpr int maxFrameLength = 0xffff;
> > };
> >
> > CamHelperImx219::CamHelperImx219()
> > @@ -66,12 +68,20 @@ CamHelperImx219::CamHelperImx219()
> > {
> > }
> >
> > -uint32_t CamHelperImx219::GetVBlanking(double exposure) const
> > +uint32_t CamHelperImx219::GetVBlanking(double exposure, double maxFps) const
> > {
> > + uint32_t frameLengthMin, vblank;
> > uint32_t exposureLines = ExposureLines(exposure);
> >
> > - return std::max<uint32_t>(mode_.height, exposureLines) -
> > - mode_.height + frameIntegrationDiff;
> > + vblank = std::max<uint32_t>(mode_.height, exposureLines) -
> > + mode_.height + frameIntegrationDiff;
> > +
> > + /* Limit the vblank to the maximum framerate requested. */
> > + frameLengthMin = std::min<uint32_t>(1e9 / (mode_.line_length * maxFps),
> > + maxFrameLength);
> > + vblank = std::max(vblank, frameLengthMin - mode_.height);
> > +
> > + return vblank;
> > }
> >
> > uint32_t CamHelperImx219::GainCode(double gain) const
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > index 242d9689..ff815f9c 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > @@ -35,7 +35,7 @@ class CamHelperImx477 : public CamHelper
> > {
> > public:
> > CamHelperImx477();
> > - uint32_t GetVBlanking(double exposure) const override;
> > + uint32_t GetVBlanking(double exposure, double maxFps) const override;
> > uint32_t GainCode(double gain) const override;
> > double Gain(uint32_t gain_code) const override;
> > bool SensorEmbeddedDataPresent() const override;
> > @@ -44,6 +44,8 @@ public:
> > private:
> > /* Smallest difference between the frame length and integration time. */
> > static constexpr int frameIntegrationDiff = 22;
> > + /* Largest possible frame length. */
> > + static constexpr int maxFrameLength = 0xffdc;
> > };
> >
> > CamHelperImx477::CamHelperImx477()
> > @@ -51,12 +53,20 @@ CamHelperImx477::CamHelperImx477()
> > {
> > }
> >
> > -uint32_t CamHelperImx477::GetVBlanking(double exposure) const
> > +uint32_t CamHelperImx477::GetVBlanking(double exposure, double maxFps) const
> > {
> > + uint32_t frameLengthMin, vblank;
> > uint32_t exposureLines = ExposureLines(exposure);
> >
> > - return std::max<uint32_t>(mode_.height, exposureLines) -
> > - mode_.height + frameIntegrationDiff;
> > + vblank = std::max<uint32_t>(mode_.height, exposureLines) -
> > + mode_.height + frameIntegrationDiff;
> > +
> > + /* Limit the vblank to the maximum framerate requested. */
> > + frameLengthMin = std::min<uint32_t>(1e9 / (mode_.line_length * maxFps),
> > + maxFrameLength);
> > + vblank = std::max(vblank, frameLengthMin - mode_.height);
> > +
> > + return vblank;
> > }
> >
> > uint32_t CamHelperImx477::GainCode(double gain) const
> > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > index ff37779c..53ac3c3b 100644
> > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > @@ -16,7 +16,7 @@ class CamHelperOv5647 : public CamHelper
> > {
> > public:
> > CamHelperOv5647();
> > - uint32_t GetVBlanking(double exposure) const override;
> > + uint32_t GetVBlanking(double exposure, double maxFps) const override;
> > uint32_t GainCode(double gain) const override;
> > double Gain(uint32_t gain_code) const override;
> > void GetDelays(int &exposure_delay, int &gain_delay) const override;
> > @@ -27,6 +27,8 @@ public:
> > private:
> > /* Smallest difference between the frame length and integration time. */
> > static constexpr int frameIntegrationDiff = 4;
> > + /* Largest possible frame length. */
> > + static constexpr int maxFrameLength = 0xffff;
> > };
> >
> > /*
> > @@ -39,12 +41,20 @@ CamHelperOv5647::CamHelperOv5647()
> > {
> > }
> >
> > -uint32_t CamHelperOv5647::GetVBlanking(double exposure) const
> > +uint32_t CamHelperOv5647::GetVBlanking(double exposure, double maxFps) const
> > {
> > + uint32_t frameLengthMin, vblank;
> > uint32_t exposureLines = ExposureLines(exposure);
> >
> > - return std::max<uint32_t>(mode_.height, exposureLines) -
> > - mode_.height + frameIntegrationDiff;
> > + vblank = std::max<uint32_t>(mode_.height, exposureLines) -
> > + mode_.height + frameIntegrationDiff;
> > +
> > + /* Limit the vblank to the maximum framerate requested. */
> > + frameLengthMin = std::min<uint32_t>(1e9 / (mode_.line_length * maxFps),
> > + maxFrameLength);
> > + vblank = std::max(vblank, frameLengthMin - mode_.height);
>
> The implementations of this functions are growing and are still nearly
> identical. Unless there's a good reason to keep them separate, I would
> try to only expose frameIntegrationDiff from the derived classes and
> GetVBlanking() in the base class.
It was a coin toss between putting it in the base class vs derived
class. I'll move it to the derived class as a virtual function so it
can be overridden if needed.
>
> > +
> > + return vblank;
> > }
> >
> > uint32_t CamHelperOv5647::GainCode(double gain) const
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 104727ea..3a2cc16e 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -136,6 +136,12 @@ private:
> > /* LS table allocation passed in from the pipeline handler. */
> > uint32_t lsTableHandle_;
> > void *lsTable_;
> > +
> > + /*
> > + * Have some defaults here, but eventually it should come from the
> > + * application via a Request, or perhaps stream configuration.
> > + */
> > + static constexpr double fpsMax = 30.0;
> > };
In patch v2, I will add a control for fps so that this can be removed as well.
Regards,
Naush
> >
> > int IPARPi::init(const IPASettings &settings)
> > @@ -795,7 +801,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
> >
> > int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
> > int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
> > - int32_t vblanking = helper_->GetVBlanking(agcStatus->shutter_time);
> > + int32_t vblanking = helper_->GetVBlanking(agcStatus->shutter_time, fpsMax);
> >
> > if (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {
> > LOG(IPARPI, Error) << "Can't find analogue gain control";
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list