| Message ID | 20250519092245.269048-4-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: > > 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> > --- > .../pipeline/rpi/common/pipeline_base.cpp | 88 +++++++------------ > .../pipeline/rpi/common/pipeline_base.h | 5 +- > 2 files changed, 37 insertions(+), 56 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index 5956485953a2..21f2daf5bab5 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"; > @@ -898,23 +897,6 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera) > 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; > - } > } > } > > @@ -1412,7 +1394,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 (startupFrameCount_) > + md.status = FrameMetadata::Status::FrameStartup; > + if (invalidFrameCount_) > + md.status = FrameMetadata::Status::FrameError; Maybe "if (validFrameCount_) ..." followed by "else if (startupFrameCount_) ..." would be slightly easier to follow? I know, makes no difference. Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks David > + > /* > * Tag the buffer as completed, returning it to the > * application. > @@ -1458,42 +1448,32 @@ 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 (startupFrameCount_) { > + startupFrameCount_--; > + LOG(RPI, Debug) << "Decrementing startup frames to " > + << startupFrameCount_; > + } > + if (invalidFrameCount_) { > + invalidFrameCount_--; > + LOG(RPI, Debug) << "Decrementing invalid frames to " > + << invalidFrameCount_; > } > } > > 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 >
Hi Naush On Mon, May 19, 2025 at 10:20:51AM +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> > --- > .../pipeline/rpi/common/pipeline_base.cpp | 88 +++++++------------ > .../pipeline/rpi/common/pipeline_base.h | 5 +- > 2 files changed, 37 insertions(+), 56 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index 5956485953a2..21f2daf5bab5 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"; > @@ -898,23 +897,6 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera) Does this function now "queue all buffers" ? Or just prepares the internal buffer pools > ret = stream->queueAllBuffers(); > if (ret < 0) > return ret; > - } else { Without the else the above branch could be made easier to read with for (auto const stream : data->streams_) { if (stream->getFlags() & StreamFlag::External) continue; int ret = stream->queueAllBuffers(); if (ret < 0) return ret; } > - /* > - * 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; > - } > } > } > > @@ -1412,7 +1394,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 (startupFrameCount_) > + md.status = FrameMetadata::Status::FrameStartup; > + if (invalidFrameCount_) > + md.status = FrameMetadata::Status::FrameError; > + > /* > * Tag the buffer as completed, returning it to the > * application. > @@ -1458,42 +1448,32 @@ 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 (startupFrameCount_) { > + startupFrameCount_--; > + LOG(RPI, Debug) << "Decrementing startup frames to " > + << startupFrameCount_; > + } > + if (invalidFrameCount_) { > + invalidFrameCount_--; > + LOG(RPI, Debug) << "Decrementing invalid frames to " > + << invalidFrameCount_; Assume you have invalidFrameCount_ = 2; startupFrameCount_ = 3; You want to return Frame# 1 2 3 4 5 6 7 Status Invalid Invalid Startup Startup Startup Success Success right ? If you decrement both invalidFrameCount_ and startupFrameCount_ at the same time, won't you get: Frame# 1 2 3 4 5 6 7 Status Invalid Invalid Startup Success Success Success Success instead ? > } > } > > 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 >
Hi David, Thank you for the review! On Mon, 19 May 2025 at 11:26, 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: > > > > 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> > > --- > > .../pipeline/rpi/common/pipeline_base.cpp | 88 +++++++------------ > > .../pipeline/rpi/common/pipeline_base.h | 5 +- > > 2 files changed, 37 insertions(+), 56 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > index 5956485953a2..21f2daf5bab5 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"; > > @@ -898,23 +897,6 @@ int PipelineHandlerBase::queueAllBuffers(Camera > *camera) > > 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; > > - } > > } > > } > > > > @@ -1412,7 +1394,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 (startupFrameCount_) > > + md.status = FrameMetadata::Status::FrameStartup; > > + if (invalidFrameCount_) > > + md.status = FrameMetadata::Status::FrameError; > > Maybe "if (validFrameCount_) ..." followed by "else if > (startupFrameCount_) ..." would be slightly easier to follow? I know, > makes no difference. > > Sure, I can make the change for the next revision. Regards, Naush > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks > David > > > + > > /* > > * Tag the buffer as completed, returning it to the > > * application. > > @@ -1458,42 +1448,32 @@ 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 (startupFrameCount_) { > > + startupFrameCount_--; > > + LOG(RPI, Debug) << "Decrementing startup frames to " > > + << startupFrameCount_; > > + } > > + if (invalidFrameCount_) { > > + invalidFrameCount_--; > > + LOG(RPI, Debug) << "Decrementing invalid frames to " > > + << invalidFrameCount_; > > } > > } > > > > 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 > > >
Hi Jacopo, Thanks for the feedback! On Tue, 20 May 2025 at 08:02, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > Hi Naush > > On Mon, May 19, 2025 at 10:20:51AM +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> > > --- > > .../pipeline/rpi/common/pipeline_base.cpp | 88 +++++++------------ > > .../pipeline/rpi/common/pipeline_base.h | 5 +- > > 2 files changed, 37 insertions(+), 56 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > index 5956485953a2..21f2daf5bab5 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"; > > @@ -898,23 +897,6 @@ int PipelineHandlerBase::queueAllBuffers(Camera > *camera) > > Does this function now "queue all buffers" ? Or just prepares the > internal buffer pools > It does indeed queue all buffers from the internal pools. prepareBuffers() is a separate call. > > > ret = stream->queueAllBuffers(); > > if (ret < 0) > > return ret; > > - } else { > > Without the else the above branch could be made easier to read with > > for (auto const stream : data->streams_) { > if (stream->getFlags() & StreamFlag::External) > continue; > > int ret = stream->queueAllBuffers(); > if (ret < 0) > return ret; > } > > Ack. I'll update this for the next version. > > > - /* > > - * 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; > > - } > > } > > } > > > > @@ -1412,7 +1394,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 (startupFrameCount_) > > + md.status = FrameMetadata::Status::FrameStartup; > > + if (invalidFrameCount_) > > + md.status = FrameMetadata::Status::FrameError; > > + > > /* > > * Tag the buffer as completed, returning it to the > > * application. > > @@ -1458,42 +1448,32 @@ 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 (startupFrameCount_) { > > + startupFrameCount_--; > > + LOG(RPI, Debug) << "Decrementing startup frames to " > > + << startupFrameCount_; > > + } > > + if (invalidFrameCount_) { > > + invalidFrameCount_--; > > + LOG(RPI, Debug) << "Decrementing invalid frames to " > > + << invalidFrameCount_; > > Assume you have > invalidFrameCount_ = 2; > startupFrameCount_ = 3; > > You want to return > > Frame# 1 2 3 4 5 6 7 > Status Invalid Invalid Startup Startup Startup Success Success > > right ? > > If you decrement both invalidFrameCount_ and startupFrameCount_ at the > same time, won't you get: > > > Frame# 1 2 3 4 5 6 7 > Status Invalid Invalid Startup Success Success Success Success > > instead ? > You are right, this is a bug. I should decrement invalidFrameCount to 0, then decrement startupFrameCount. Will fix this in the next version. Regards, Naush > > > } > > } > > > > 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..21f2daf5bab5 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"; @@ -898,23 +897,6 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera) 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; - } } } @@ -1412,7 +1394,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 (startupFrameCount_) + md.status = FrameMetadata::Status::FrameStartup; + if (invalidFrameCount_) + md.status = FrameMetadata::Status::FrameError; + /* * Tag the buffer as completed, returning it to the * application. @@ -1458,42 +1448,32 @@ 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 (startupFrameCount_) { + startupFrameCount_--; + LOG(RPI, Debug) << "Decrementing startup frames to " + << startupFrameCount_; + } + if (invalidFrameCount_) { + invalidFrameCount_--; + LOG(RPI, Debug) << "Decrementing invalid frames to " + << invalidFrameCount_; } } 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
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> --- .../pipeline/rpi/common/pipeline_base.cpp | 88 +++++++------------ .../pipeline/rpi/common/pipeline_base.h | 5 +- 2 files changed, 37 insertions(+), 56 deletions(-)