| Message ID | 20250522075244.1198110-4-naush@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Thu, May 22, 2025 at 08:48:19AM +0100, Naushir Patuck wrote: > Split the pipeline handler drop frame tracking into startup frames and > invalid frames, as reported by the IPA. > > Remove the drop buffer handling logic in the pipeline handler. Now all > image buffers are returned out with the appropriate FrameStatus set > for startup or invalid frames. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/rpi/common/pipeline_base.cpp | 98 ++++++++----------- > .../pipeline/rpi/common/pipeline_base.h | 5 +- > 2 files changed, 42 insertions(+), 61 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index 5956485953a2..3f0b7abdc59a 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -659,9 +659,9 @@ int PipelineHandlerBase::start(Camera *camera, const ControlList *controls) > if (!result.controls.empty()) > data->setSensorControls(result.controls); > > - /* Configure the number of dropped frames required on startup. */ > - data->dropFrameCount_ = data->config_.disableStartupFrameDrops ? > - 0 : result.startupFrameCount + result.invalidFrameCount; > + /* Configure the number of startup and invalid frames reported by the IPA. */ > + data->startupFrameCount_ = result.startupFrameCount; > + data->invalidFrameCount_ = result.invalidFrameCount; > This makes my comment on the previous patch moot > for (auto const stream : data->streams_) > stream->resetBuffers(); > @@ -678,7 +678,6 @@ int PipelineHandlerBase::start(Camera *camera, const ControlList *controls) > data->buffersAllocated_ = true; > } > > - /* We need to set the dropFrameCount_ before queueing buffers. */ > ret = queueAllBuffers(camera); > if (ret) { > LOG(RPI, Error) << "Failed to queue buffers"; > @@ -894,28 +893,12 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera) > int ret; > > for (auto const stream : data->streams_) { > - if (!(stream->getFlags() & StreamFlag::External)) { > - ret = stream->queueAllBuffers(); > - if (ret < 0) > - return ret; > - } else { > - /* > - * For external streams, we must queue up a set of internal > - * buffers to handle the number of drop frames requested by > - * the IPA. This is done by passing nullptr in queueBuffer(). > - * > - * The below queueBuffer() call will do nothing if there > - * are not enough internal buffers allocated, but this will > - * be handled by queuing the request for buffers in the > - * RPiStream object. > - */ > - unsigned int i; > - for (i = 0; i < data->dropFrameCount_; i++) { > - ret = stream->queueBuffer(nullptr); > - if (ret) > - return ret; > - } > - } > + if (stream->getFlags() & StreamFlag::External) > + continue; > + > + ret = stream->queueAllBuffers(); > + if (ret < 0) > + return ret; > } > > return 0; > @@ -1412,7 +1395,15 @@ void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) > * buffer back to the stream. > */ > Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front(); > - if (!dropFrameCount_ && request && request->findBuffer(stream) == buffer) { > + if (request && request->findBuffer(stream) == buffer) { > + FrameMetadata &md = buffer->_d()->metadata(); > + > + /* Mark the non-converged and invalid frames in the metadata. */ > + if (invalidFrameCount_) > + md.status = FrameMetadata::Status::FrameError; > + else if (startupFrameCount_) > + md.status = FrameMetadata::Status::FrameStartup; > + > /* > * Tag the buffer as completed, returning it to the > * application. > @@ -1458,42 +1449,31 @@ void CameraData::handleState() > > void CameraData::checkRequestCompleted() > { > - bool requestCompleted = false; > - /* > - * If we are dropping this frame, do not touch the request, simply > - * change the state to IDLE when ready. > - */ > - if (!dropFrameCount_) { > - Request *request = requestQueue_.front(); > - if (request->hasPendingBuffers()) > - return; > + Request *request = requestQueue_.front(); > + if (request->hasPendingBuffers()) > + return; > > - /* Must wait for metadata to be filled in before completing. */ > - if (state_ != State::IpaComplete) > - return; > + /* Must wait for metadata to be filled in before completing. */ > + if (state_ != State::IpaComplete) > + return; > > - LOG(RPI, Debug) << "Completing request sequence: " > - << request->sequence(); > + LOG(RPI, Debug) << "Completing request sequence: " > + << request->sequence(); > > - pipe()->completeRequest(request); > - requestQueue_.pop(); > - requestCompleted = true; > - } > + pipe()->completeRequest(request); > + requestQueue_.pop(); > > - /* > - * Make sure we have three outputs completed in the case of a dropped > - * frame. > - */ > - if (state_ == State::IpaComplete && > - ((ispOutputCount_ == ispOutputTotal_ && dropFrameCount_) || > - requestCompleted)) { > - LOG(RPI, Debug) << "Going into Idle state"; > - state_ = State::Idle; > - if (dropFrameCount_) { > - dropFrameCount_--; > - LOG(RPI, Debug) << "Dropping frame at the request of the IPA (" > - << dropFrameCount_ << " left)"; > - } > + LOG(RPI, Debug) << "Going into Idle state"; > + state_ = State::Idle; > + > + if (invalidFrameCount_) { > + invalidFrameCount_--; > + LOG(RPI, Debug) << "Decrementing invalid frames to " > + << invalidFrameCount_; > + } else if (startupFrameCount_) { > + startupFrameCount_--; > + LOG(RPI, Debug) << "Decrementing startup frames to " > + << startupFrameCount_; > } Thanks, this fixes my comment on the previous version. Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > } > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h > index aae0c2f35888..6023f9f9d6b3 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h > @@ -48,7 +48,7 @@ class CameraData : public Camera::Private > public: > CameraData(PipelineHandler *pipe) > : Camera::Private(pipe), state_(State::Stopped), > - dropFrameCount_(0), buffersAllocated_(false), > + startupFrameCount_(0), invalidFrameCount_(0), buffersAllocated_(false), > ispOutputCount_(0), ispOutputTotal_(0) > { > } > @@ -151,7 +151,8 @@ public: > /* Mapping of CropParams keyed by the output stream order in CameraConfiguration */ > std::map<unsigned int, CropParams> cropParams_; > > - unsigned int dropFrameCount_; > + unsigned int startupFrameCount_; > + unsigned int invalidFrameCount_; > > /* > * If set, this stores the value that represets a gain of one for > -- > 2.43.0 >
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp index 5956485953a2..3f0b7abdc59a 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp @@ -659,9 +659,9 @@ int PipelineHandlerBase::start(Camera *camera, const ControlList *controls) if (!result.controls.empty()) data->setSensorControls(result.controls); - /* Configure the number of dropped frames required on startup. */ - data->dropFrameCount_ = data->config_.disableStartupFrameDrops ? - 0 : result.startupFrameCount + result.invalidFrameCount; + /* Configure the number of startup and invalid frames reported by the IPA. */ + data->startupFrameCount_ = result.startupFrameCount; + data->invalidFrameCount_ = result.invalidFrameCount; for (auto const stream : data->streams_) stream->resetBuffers(); @@ -678,7 +678,6 @@ int PipelineHandlerBase::start(Camera *camera, const ControlList *controls) data->buffersAllocated_ = true; } - /* We need to set the dropFrameCount_ before queueing buffers. */ ret = queueAllBuffers(camera); if (ret) { LOG(RPI, Error) << "Failed to queue buffers"; @@ -894,28 +893,12 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera) int ret; for (auto const stream : data->streams_) { - if (!(stream->getFlags() & StreamFlag::External)) { - ret = stream->queueAllBuffers(); - if (ret < 0) - return ret; - } else { - /* - * For external streams, we must queue up a set of internal - * buffers to handle the number of drop frames requested by - * the IPA. This is done by passing nullptr in queueBuffer(). - * - * The below queueBuffer() call will do nothing if there - * are not enough internal buffers allocated, but this will - * be handled by queuing the request for buffers in the - * RPiStream object. - */ - unsigned int i; - for (i = 0; i < data->dropFrameCount_; i++) { - ret = stream->queueBuffer(nullptr); - if (ret) - return ret; - } - } + if (stream->getFlags() & StreamFlag::External) + continue; + + ret = stream->queueAllBuffers(); + if (ret < 0) + return ret; } return 0; @@ -1412,7 +1395,15 @@ void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) * buffer back to the stream. */ Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front(); - if (!dropFrameCount_ && request && request->findBuffer(stream) == buffer) { + if (request && request->findBuffer(stream) == buffer) { + FrameMetadata &md = buffer->_d()->metadata(); + + /* Mark the non-converged and invalid frames in the metadata. */ + if (invalidFrameCount_) + md.status = FrameMetadata::Status::FrameError; + else if (startupFrameCount_) + md.status = FrameMetadata::Status::FrameStartup; + /* * Tag the buffer as completed, returning it to the * application. @@ -1458,42 +1449,31 @@ void CameraData::handleState() void CameraData::checkRequestCompleted() { - bool requestCompleted = false; - /* - * If we are dropping this frame, do not touch the request, simply - * change the state to IDLE when ready. - */ - if (!dropFrameCount_) { - Request *request = requestQueue_.front(); - if (request->hasPendingBuffers()) - return; + Request *request = requestQueue_.front(); + if (request->hasPendingBuffers()) + return; - /* Must wait for metadata to be filled in before completing. */ - if (state_ != State::IpaComplete) - return; + /* Must wait for metadata to be filled in before completing. */ + if (state_ != State::IpaComplete) + return; - LOG(RPI, Debug) << "Completing request sequence: " - << request->sequence(); + LOG(RPI, Debug) << "Completing request sequence: " + << request->sequence(); - pipe()->completeRequest(request); - requestQueue_.pop(); - requestCompleted = true; - } + pipe()->completeRequest(request); + requestQueue_.pop(); - /* - * Make sure we have three outputs completed in the case of a dropped - * frame. - */ - if (state_ == State::IpaComplete && - ((ispOutputCount_ == ispOutputTotal_ && dropFrameCount_) || - requestCompleted)) { - LOG(RPI, Debug) << "Going into Idle state"; - state_ = State::Idle; - if (dropFrameCount_) { - dropFrameCount_--; - LOG(RPI, Debug) << "Dropping frame at the request of the IPA (" - << dropFrameCount_ << " left)"; - } + LOG(RPI, Debug) << "Going into Idle state"; + state_ = State::Idle; + + if (invalidFrameCount_) { + invalidFrameCount_--; + LOG(RPI, Debug) << "Decrementing invalid frames to " + << invalidFrameCount_; + } else if (startupFrameCount_) { + startupFrameCount_--; + LOG(RPI, Debug) << "Decrementing startup frames to " + << startupFrameCount_; } } diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h index aae0c2f35888..6023f9f9d6b3 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h @@ -48,7 +48,7 @@ class CameraData : public Camera::Private public: CameraData(PipelineHandler *pipe) : Camera::Private(pipe), state_(State::Stopped), - dropFrameCount_(0), buffersAllocated_(false), + startupFrameCount_(0), invalidFrameCount_(0), buffersAllocated_(false), ispOutputCount_(0), ispOutputTotal_(0) { } @@ -151,7 +151,8 @@ public: /* Mapping of CropParams keyed by the output stream order in CameraConfiguration */ std::map<unsigned int, CropParams> cropParams_; - unsigned int dropFrameCount_; + unsigned int startupFrameCount_; + unsigned int invalidFrameCount_; /* * If set, this stores the value that represets a gain of one for