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