<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 4 Oct 2022 at 17:43, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@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">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Mon, Oct 03, 2022 at 09:39:28AM +0100, Naushir Patuck via libcamera-devel wrote:<br>
> Add fields for minimum and maximum line length (in units of pixels) to the<br>
<br>
I think the unit is seconds, not pixels.<br>
<br>
> CameraMode structure. This replaces the existing lineLength field.<br>
> <br>
> Any use of the existing lineLength field is replaced by the new minLineLength<br>
> field, as logically we always want to use the fastest sensor readout by default.<br>
<br>
I wonder if that's always the case, but I think it's fine here.<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           |  8 ++++----<br>
>  src/ipa/raspberrypi/controller/camera_mode.h |  2 +-<br>
>  src/ipa/raspberrypi/raspberrypi.cpp          | 13 +++++++------<br>
>  3 files changed, 12 insertions(+), 11 deletions(-)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp<br>
> index cac8f39ee763..42251ba29682 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
> @@ -64,13 +64,13 @@ void CamHelper::process([[maybe_unused]] StatisticsPtr &stats,<br>
>  uint32_t CamHelper::exposureLines(const Duration exposure) const<br>
>  {<br>
>       assert(initialized_);<br>
> -     return exposure / mode_.lineLength;<br>
> +     return exposure / mode_.minLineLength;<br>
>  }<br>
>  <br>
>  Duration CamHelper::exposure(uint32_t exposureLines) const<br>
>  {<br>
>       assert(initialized_);<br>
> -     return exposureLines * mode_.lineLength;<br>
> +     return exposureLines * mode_.minLineLength;<br>
>  }<br>
>  <br>
>  uint32_t CamHelper::getVBlanking(Duration &exposure,<br>
> @@ -86,8 +86,8 @@ uint32_t CamHelper::getVBlanking(Duration &exposure,<br>
>        * minFrameDuration and maxFrameDuration are clamped by the caller<br>
>        * based on the limits for the active sensor mode.<br>
>        */<br>
> -     frameLengthMin = minFrameDuration / mode_.lineLength;<br>
> -     frameLengthMax = maxFrameDuration / mode_.lineLength;<br>
> +     frameLengthMin = minFrameDuration / mode_.minLineLength;<br>
> +     frameLengthMax = maxFrameDuration / mode_.minLineLength;<br>
>  <br>
>       /*<br>
>        * Limit the exposure to the maximum frame duration requested, and<br>
> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h<br>
> index a6ccf8c1c600..b7e73aae4698 100644<br>
> --- a/src/ipa/raspberrypi/controller/camera_mode.h<br>
> +++ b/src/ipa/raspberrypi/controller/camera_mode.h<br>
> @@ -32,7 +32,7 @@ struct CameraMode {<br>
>       /* scaling of the noise compared to the native sensor mode */<br>
>       double noiseFactor;<br>
>       /* line time */<br>
> -     libcamera::utils::Duration lineLength;<br>
> +     libcamera::utils::Duration minLineLength, maxLineLength;<br>
<br>
        libcamera::utils::Duration minLineLength;<br>
        libcamera::utils::Duration maxLineLength;<br>
<br>
would be more readable.<br></blockquote><div><br></div><div>I did the single line statement to be consistent with the rest of the</div><div>struct definition, but I can amend this change so that all fields</div><div>are on a single line.</div><div><br></div><div>Regards,</div><div>Naush</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>
>       /* any camera transform *not* reflected already in the camera tuning */<br>
>       libcamera::Transform transform;<br>
>       /* minimum and maximum fame lengths in units of lines */<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 358a119da222..67326bcf4a14 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -314,7 +314,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)<br>
>       }<br>
>  <br>
>       startConfig->dropFrameCount = dropFrameCount_;<br>
> -     const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;<br>
> +     const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength;<br>
>       startConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get<std::milli>();<br>
>  <br>
>       firstStart_ = false;<br>
> @@ -356,7 +356,8 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)<br>
>        * Calculate the line length as the ratio between the line length in<br>
>        * pixels and the pixel rate.<br>
>        */<br>
> -     mode_.lineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);<br>
> +     mode_.minLineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);<br>
> +     mode_.maxLineLength = sensorInfo.maxLineLength * (1.0s / sensorInfo.pixelRate);<br>
>  <br>
>       /*<br>
>        * Set the frame length limits for the mode to ensure exposure and<br>
> @@ -458,8 +459,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,<br>
>        * based on the current sensor mode.<br>
>        */<br>
>       ControlInfoMap::Map ctrlMap = ipaControls;<br>
> -     const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.lineLength;<br>
> -     const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;<br>
> +     const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;<br>
> +     const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength;<br>
>       ctrlMap[&controls::FrameDurationLimits] =<br>
>               ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),<br>
>                           static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));<br>
> @@ -1150,8 +1151,8 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)<br>
>  <br>
>  void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration)<br>
>  {<br>
> -     const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.lineLength;<br>
> -     const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.lineLength;<br>
> +     const Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;<br>
> +     const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength;<br>
>  <br>
>       /*<br>
>        * This will only be applied once AGC recalculations occur.<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>