<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 21 Jan 2021 at 14:53, 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>
I don't think I can claim to understand every last nuance of this, but<br>
let me try anyway but here goes...!<br>
<br>
On Thu, 21 Jan 2021 at 11:58, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> The existing framerate/vblank calculations did not account for the<br>
> sensor mode limits. This commit extracts vblank limits from the sensor<br>
> v4l2 controls, and stores it in the camera modes structure.<br>
><br>
> Exposure and vblank value calculations now use values clamped to the<br>
> sensor mode limits. The libcamera::controls::FrameDurations metadata<br>
> return values now reflect the minimum and maximum after clamping.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  src/ipa/raspberrypi/cam_helper.cpp           | 16 +++---<br>
>  src/ipa/raspberrypi/cam_helper.hpp           |  5 +-<br>
>  src/ipa/raspberrypi/cam_helper_imx219.cpp    |  6 +--<br>
>  src/ipa/raspberrypi/cam_helper_imx477.cpp    |  4 +-<br>
>  src/ipa/raspberrypi/cam_helper_ov5647.cpp    |  4 +-<br>
>  src/ipa/raspberrypi/controller/camera_mode.h |  2 +<br>
>  src/ipa/raspberrypi/raspberrypi.cpp          | 51 ++++++++++++++------<br>
>  7 files changed, 49 insertions(+), 39 deletions(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp<br>
> index b7b8faf09c69..3e17255737b2 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
> @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const &cam_name)<br>
>         return nullptr;<br>
>  }<br>
><br>
> -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,<br>
> -                    unsigned int frameIntegrationDiff)<br>
> -       : parser_(parser), initialized_(false), maxFrameLength_(maxFrameLength),<br>
> +CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)<br>
> +       : parser_(parser), initialized_(false),<br>
>           frameIntegrationDiff_(frameIntegrationDiff)<br>
>  {<br>
>  }<br>
> @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,<br>
>         assert(initialized_);<br>
><br>
>         /*<br>
> -        * Clamp frame length by the frame duration range and the maximum allowable<br>
> -        * value in the sensor, given by maxFrameLength_.<br>
> +        * minFrameDuration and maxFrameDuration will have been clamped to the<br>
> +        * maximum allowable values in the sensor for this mode.<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>
> +       frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;<br>
> +       frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;<br>
> +<br>
>         /*<br>
>          * Limit the exposure to the maximum frame duration requested, and<br>
>          * re-calculate if it has been clipped.<br>
> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp<br>
> index b1739a57e342..14d70112cb62 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper.hpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper.hpp<br>
> @@ -62,8 +62,7 @@ class CamHelper<br>
>  {<br>
>  public:<br>
>         static CamHelper *Create(std::string const &cam_name);<br>
> -       CamHelper(MdParser *parser, unsigned int maxFrameLength,<br>
> -                 unsigned int frameIntegrationDiff);<br>
> +       CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);<br>
>         virtual ~CamHelper();<br>
>         void SetCameraMode(const CameraMode &mode);<br>
>         MdParser &Parser() const { return *parser_; }<br>
> @@ -86,8 +85,6 @@ protected:<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>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> index 8688ec091c44..95b8e698fe3b 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> @@ -56,15 +56,13 @@ private:<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(), maxFrameLength, frameIntegrationDiff)<br>
> +       : CamHelper(new MdParserImx219(), frameIntegrationDiff)<br>
>  #else<br>
> -       : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)<br>
> +       : CamHelper(new MdParserRPi(), 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 5396131003f1..eaa7be16d20e 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> @@ -45,12 +45,10 @@ private:<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(), maxFrameLength, frameIntegrationDiff)<br>
> +       : CamHelper(new MdParserImx477(), 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 2bd8a754f133..a7f417324048 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> @@ -30,8 +30,6 @@ private:<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>
> @@ -40,7 +38,7 @@ private:<br>
>   */<br>
><br>
>  CamHelperOv5647::CamHelperOv5647()<br>
> -       : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)<br>
> +       : CamHelper(new MdParserRPi(), frameIntegrationDiff)<br>
>  {<br>
>  }<br>
><br>
> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h<br>
> index 920f11beb0b0..3baeadaca076 100644<br>
> --- a/src/ipa/raspberrypi/controller/camera_mode.h<br>
> +++ b/src/ipa/raspberrypi/controller/camera_mode.h<br>
> @@ -37,6 +37,8 @@ struct CameraMode {<br>
>         double line_length;<br>
>         // any camera transform *not* reflected already in the camera tuning<br>
>         libcamera::Transform transform;<br>
> +       // minimum and maximum vblanking limits for this mode<br>
> +       uint32_t vblank_min, vblank_max;<br>
>  };<br>
><br>
>  #ifdef __cplusplus<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 5ccc7a6551f5..9e6e030c4997 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -102,6 +102,7 @@ private:<br>
>         void reportMetadata();<br>
>         bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);<br>
>         void processStats(unsigned int bufferId);<br>
> +       void applyFrameDurations(double min, double max);<br>
>         void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);<br>
>         void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);<br>
>         void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);<br>
> @@ -279,6 +280,14 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)<br>
>          * the line length in pixels and the pixel rate.<br>
>          */<br>
>         mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;<br>
> +<br>
> +       /*<br>
> +        * Set the vertical blanking (frame duration) limits for the mode to<br>
> +        * ensure exposure and framerate calculations are clipped appropriately.<br>
> +        */<br>
> +       const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);<br>
<br>
Are we happy to assume this exists? (I realise that if it doesn't then<br>
everything is doomed and we probably don't care very much...)<br></blockquote><div><br></div><div>Yes, we do validate that all controls (sensor and isp) are available on ipa::configure().  If not we fail hard, so this should be safe.</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>
> +       mode_.vblank_min = itVblank->second.min().get<int32_t>();<br>
> +       mode_.vblank_max = itVblank->second.max().get<int32_t>();<br>
>  }<br>
><br>
>  void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
>                 controller_.Initialise();<br>
>                 controllerInit_ = true;<br>
><br>
> -               minFrameDuration_ = defaultMinFrameDuration;<br>
> -               maxFrameDuration_ = defaultMaxFrameDuration;<br>
> +               /* Supply initial values for frame durations. */<br>
> +               applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);<br>
><br>
>                 /* Supply initial values for gain and exposure. */<br>
>                 ControlList ctrls(sensorCtrls_);<br>
> @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList &controls)<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>
> -                       maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);<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>
> +                       applyFrameDurations(frameDurations[0], frameDurations[1]);<br>
>                         break;<br>
>                 }<br>
><br>
> @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)<br>
>                   static_cast<int32_t>(awbStatus->gain_b * 1000));<br>
>  }<br>
><br>
> +void IPARPi::applyFrameDurations(double min, double max)<br>
<br>
I didn't terribly like variables named just "min" and "max", as I<br>
immediately wonder what they are the min and max of... or maybe it's<br>
just a hangover from C programming where there often seemed to be min<br>
and max macros flying around!<br></blockquote><div><br></div><div>I can change the names to something more appropriate.</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>
But these terribly minor things aside:<br>
<br>
Reviewed-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>
> +       const double minSensorFrameDuration = 1e-3 * (mode_.vblank_min + mode_.height) *<br>
> +                                             mode_.line_length;<br>
> +       const double maxSensorFrameDuration = 1e-3 * (mode_.vblank_max + mode_.height) *<br>
> +                                             mode_.line_length;<br>
> +       /*<br>
> +        * This will only be applied once AGC recalculations occur.<br>
> +        * The values may be clamped based on the sensor mode capabilities as well.<br>
> +        */<br>
> +       minFrameDuration_ = min ? min : defaultMaxFrameDuration;<br>
> +       maxFrameDuration_ = max ? max : defaultMinFrameDuration;<br>
> +       minFrameDuration_ = std::clamp(minFrameDuration_,<br>
> +                                      minSensorFrameDuration, maxSensorFrameDuration);<br>
> +       maxFrameDuration_ = std::clamp(maxFrameDuration_,<br>
> +                                      minSensorFrameDuration, maxSensorFrameDuration);<br>
> +<br>
> +       /* Return the validated limits out though metadata. */<br>
> +       libcameraMetadata_.set(controls::FrameDurations,<br>
> +                              { static_cast<int64_t>(minFrameDuration_),<br>
> +                                static_cast<int64_t>(maxFrameDuration_) });<br>
> +}<br>
> +<br>
>  void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
>  {<br>
>         int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);<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>