<div dir="ltr"><div dir="ltr"><div dir="ltr"><div>Hi David,</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 19 May 2025 at 11:17, David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">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>
Thanks for the patch.<br>
<br>
On Mon, 19 May 2025 at 10:23, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Replace the dropFrameCount parameter returned from ipa::start() to the<br>
> pipeline handler by startupFrameCount and invalidFrameCount. The former<br>
> counts the number of frames required for AWB/AGC to converge, and the<br>
> latter counts the number of invalid frames produced by the sensor when<br>
> starting up.<br>
><br>
> In the pipeline handler, use the sum of these 2 values to replicate the<br>
> existing dropFrameCount behaviour.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> include/libcamera/ipa/raspberrypi.mojom | 3 ++-<br>
> src/ipa/rpi/common/ipa_base.cpp | 14 ++++++++------<br>
> .../pipeline/rpi/common/pipeline_base.cpp | 4 ++--<br>
> 3 files changed, 12 insertions(+), 9 deletions(-)<br>
><br>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom<br>
> index e30c70bde08b..12b083e9d04b 100644<br>
> --- a/include/libcamera/ipa/raspberrypi.mojom<br>
> +++ b/include/libcamera/ipa/raspberrypi.mojom<br>
> @@ -52,7 +52,8 @@ struct ConfigResult {<br>
><br>
> struct StartResult {<br>
> libcamera.ControlList controls;<br>
> - int32 dropFrameCount;<br>
> + int32 startupFrameCount;<br>
> + int32 invalidFrameCount;<br>
> };<br>
><br>
> struct PrepareParams {<br>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp<br>
> index e0a93daa9db2..c15f8a7bf71e 100644<br>
> --- a/src/ipa/rpi/common/ipa_base.cpp<br>
> +++ b/src/ipa/rpi/common/ipa_base.cpp<br>
> @@ -324,6 +324,7 @@ void IpaBase::start(const ControlList &controls, StartResult *result)<br>
> * "mistrusted", which depends on whether this is a startup from cold,<br>
> * or merely a mode switch in a running system.<br>
> */<br>
> + unsigned int agcConvergenceFrames = 0, awbConvergenceFrames = 0;<br>
> frameCount_ = 0;<br>
> if (firstStart_) {<br>
> dropFrameCount_ = helper_->hideFramesStartup();<br>
> @@ -336,7 +337,6 @@ void IpaBase::start(const ControlList &controls, StartResult *result)<br>
> * (mistrustCount_) that they won't see. But if zero (i.e.<br>
> * no convergence necessary), no frames need to be dropped.<br>
> */<br>
> - unsigned int agcConvergenceFrames = 0;<br>
> RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(<br>
> controller_.getAlgorithm("agc"));<br>
> if (agc) {<br>
> @@ -345,7 +345,6 @@ void IpaBase::start(const ControlList &controls, StartResult *result)<br>
> agcConvergenceFrames += mistrustCount_;<br>
> }<br>
><br>
> - unsigned int awbConvergenceFrames = 0;<br>
> RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(<br>
> controller_.getAlgorithm("awb"));<br>
> if (awb) {<br>
> @@ -353,15 +352,18 @@ void IpaBase::start(const ControlList &controls, StartResult *result)<br>
> if (awbConvergenceFrames)<br>
> awbConvergenceFrames += mistrustCount_;<br>
> }<br>
> -<br>
> - dropFrameCount_ = std::max({ dropFrameCount_, agcConvergenceFrames, awbConvergenceFrames });<br>
> - LOG(IPARPI, Debug) << "Drop " << dropFrameCount_ << " frames on startup";<br>
> } else {<br>
> dropFrameCount_ = helper_->hideFramesModeSwitch();<br>
> mistrustCount_ = helper_->mistrustFramesModeSwitch();<br>
> }<br>
><br>
> - result->dropFrameCount = dropFrameCount_;<br>
> + result->startupFrameCount = std::max({ agcConvergenceFrames, awbConvergenceFrames });<br>
> + result->invalidFrameCount = dropFrameCount_;<br>
> +<br>
> + dropFrameCount_ = std::max({ dropFrameCount_, agcConvergenceFrames, awbConvergenceFrames });<br>
<br>
TBH, I'm not 100% clear why we do that max operation (I realise the<br>
old version does it too), do you remember?<br></blockquote><div><br></div><div>I think it's just accounting for the worst case where we run the prepare() / process() cycle on every frame during startup.</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>
> + LOG(IPARPI, Debug) << "Startup frames: " << result->startupFrameCount<br>
> + << " Invalid frames: " << result->invalidFrameCount;<br>
<br>
I just wondered very slightly whether it might be slightly easier to<br>
follow if we don't use "dropFrameCount" or "dropFrameCount_" here,<br>
because in the PH it means something slightly different (is that<br>
right?). So maybe replace it by "invalidFrameCount"? I know, very<br>
minor!<br>
<br>
><br>
> firstStart_ = false;<br>
> lastRunTimestamp_ = 0;<br>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp<br>
> index 1f13e5230fae..5956485953a2 100644<br>
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp<br>
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp<br>
> @@ -660,8 +660,8 @@ int PipelineHandlerBase::start(Camera *camera, const ControlList *controls)<br>
> data->setSensorControls(result.controls);<br>
><br>
> /* Configure the number of dropped frames required on startup. */<br>
> - data->dropFrameCount_ = data->config_.disableStartupFrameDrops<br>
> - ? 0 : result.dropFrameCount;<br>
<br>
Can you say why we return "result.dropFrameCount" here and not<br>
"result.invalidFrameCount"? I've obviously forgotten what's going on<br>
here!<br></blockquote><div><br></div><div>This "models" the existing behavior, but changes in subsequent patches to account for
result.invalidFrameCount correctly.</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>
Assuming this is all just recollection failure on my part:<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>
> + data->dropFrameCount_ = data->config_.disableStartupFrameDrops ?<br>
> + 0 : result.startupFrameCount + result.invalidFrameCount;<br>
><br>
> for (auto const stream : data->streams_)<br>
> stream->resetBuffers();<br>
> --<br>
> 2.43.0<br>
><br>
</blockquote></div></div>
</div>