| Message ID | 20220713120035.15604-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | c49f47589cbafaa94e2baa53424f32a8755185cd |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thanks - that seems to fix it On Wed, 13 Jul 2022 at 13:00, Naushir Patuck <naush@raspberrypi.com> wrote: > The logic used to match asynchronous image and embedded buffers was being > overly > aggressive by possibly allowing an unmatched image buffer to be sent to > the IPA > if the matching embedded buffer had not yet been dequeued. This condition > only > occurs when the system is heavily loaded and dropping frames. > > Fix this by holding image buffer in the queue during these conditions > until the > next embedded buffer dequeue event. > > Reported-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> Tested-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 66a84b1dfb97..ef3c2d11d253 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -2159,16 +2159,12 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame > &bayerFrame, FrameBuffer *&em > if (bayerQueue_.empty()) > return false; > > - /* Start with the front of the bayer queue. */ > - bayerFrame = std::move(bayerQueue_.front()); > - bayerQueue_.pop(); > - > /* > * Find the embedded data buffer with a matching timestamp to pass > to > * the IPA. Any embedded buffers with a timestamp lower than the > * current bayer buffer will be removed and re-queued to the > driver. > */ > - uint64_t ts = bayerFrame.buffer->metadata().timestamp; > + uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp; > embeddedBuffer = nullptr; > while (!embeddedQueue_.empty()) { > FrameBuffer *b = embeddedQueue_.front(); > @@ -2188,10 +2184,23 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame > &bayerFrame, FrameBuffer *&em > } > > if (!embeddedBuffer && sensorMetadata_) { > + if (embeddedQueue_.empty()) { > + /* > + * If the embedded buffer queue is empty, wait for > the next > + * buffer to arrive - dequeue ordering may send > the image > + * buffer first. > + */ > + LOG(RPI, Debug) << "Waiting for next embedded > buffer."; > + return false; > + } > + > /* Log if there is no matching embedded data buffer found. > */ > LOG(RPI, Debug) << "Returning bayer frame without a > matching embedded buffer."; > } > > + bayerFrame = std::move(bayerQueue_.front()); > + bayerQueue_.pop(); > + > return true; > } > > -- > 2.25.1 > >
Hi, Any takers for another review on this change please? Regards, Naush On Wed, 13 Jul 2022 at 13:00, Naushir Patuck <naush@raspberrypi.com> wrote: > The logic used to match asynchronous image and embedded buffers was being > overly > aggressive by possibly allowing an unmatched image buffer to be sent to > the IPA > if the matching embedded buffer had not yet been dequeued. This condition > only > occurs when the system is heavily loaded and dropping frames. > > Fix this by holding image buffer in the queue during these conditions > until the > next embedded buffer dequeue event. > > Reported-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 66a84b1dfb97..ef3c2d11d253 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -2159,16 +2159,12 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame > &bayerFrame, FrameBuffer *&em > if (bayerQueue_.empty()) > return false; > > - /* Start with the front of the bayer queue. */ > - bayerFrame = std::move(bayerQueue_.front()); > - bayerQueue_.pop(); > - > /* > * Find the embedded data buffer with a matching timestamp to pass > to > * the IPA. Any embedded buffers with a timestamp lower than the > * current bayer buffer will be removed and re-queued to the > driver. > */ > - uint64_t ts = bayerFrame.buffer->metadata().timestamp; > + uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp; > embeddedBuffer = nullptr; > while (!embeddedQueue_.empty()) { > FrameBuffer *b = embeddedQueue_.front(); > @@ -2188,10 +2184,23 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame > &bayerFrame, FrameBuffer *&em > } > > if (!embeddedBuffer && sensorMetadata_) { > + if (embeddedQueue_.empty()) { > + /* > + * If the embedded buffer queue is empty, wait for > the next > + * buffer to arrive - dequeue ordering may send > the image > + * buffer first. > + */ > + LOG(RPI, Debug) << "Waiting for next embedded > buffer."; > + return false; > + } > + > /* Log if there is no matching embedded data buffer found. > */ > LOG(RPI, Debug) << "Returning bayer frame without a > matching embedded buffer."; > } > > + bayerFrame = std::move(bayerQueue_.front()); > + bayerQueue_.pop(); > + > return true; > } > > -- > 2.25.1 > >
Hi Naush, Thank you for the patch. On Wed, Jul 13, 2022 at 01:00:35PM +0100, Naushir Patuck via libcamera-devel wrote: > The logic used to match asynchronous image and embedded buffers was being overly > aggressive by possibly allowing an unmatched image buffer to be sent to the IPA > if the matching embedded buffer had not yet been dequeued. This condition only > occurs when the system is heavily loaded and dropping frames. > > Fix this by holding image buffer in the queue during these conditions until the > next embedded buffer dequeue event. > > Reported-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 66a84b1dfb97..ef3c2d11d253 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -2159,16 +2159,12 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em > if (bayerQueue_.empty()) > return false; Not strictly related to this patch, but this can never happen, as it's tested by the caller. > > - /* Start with the front of the bayer queue. */ > - bayerFrame = std::move(bayerQueue_.front()); > - bayerQueue_.pop(); > - > /* > * Find the embedded data buffer with a matching timestamp to pass to > * the IPA. Any embedded buffers with a timestamp lower than the > * current bayer buffer will be removed and re-queued to the driver. > */ > - uint64_t ts = bayerFrame.buffer->metadata().timestamp; > + uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp; > embeddedBuffer = nullptr; > while (!embeddedQueue_.empty()) { > FrameBuffer *b = embeddedQueue_.front(); > @@ -2188,10 +2184,23 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em > } > > if (!embeddedBuffer && sensorMetadata_) { > + if (embeddedQueue_.empty()) { > + /* > + * If the embedded buffer queue is empty, wait for the next > + * buffer to arrive - dequeue ordering may send the image > + * buffer first. > + */ > + LOG(RPI, Debug) << "Waiting for next embedded buffer."; > + return false; > + } > + > /* Log if there is no matching embedded data buffer found. */ > LOG(RPI, Debug) << "Returning bayer frame without a matching embedded buffer."; > } > > + bayerFrame = std::move(bayerQueue_.front()); > + bayerQueue_.pop(); > + This looks fine, but I wonder if it could be simplified by first handling the case where the sensor doesn't produce metadata (possibly in RPiCameraData::tryRunPipeline()). This can be done on top, so I'll apply this patch with Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > return true; > } >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 66a84b1dfb97..ef3c2d11d253 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -2159,16 +2159,12 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em if (bayerQueue_.empty()) return false; - /* Start with the front of the bayer queue. */ - bayerFrame = std::move(bayerQueue_.front()); - bayerQueue_.pop(); - /* * Find the embedded data buffer with a matching timestamp to pass to * the IPA. Any embedded buffers with a timestamp lower than the * current bayer buffer will be removed and re-queued to the driver. */ - uint64_t ts = bayerFrame.buffer->metadata().timestamp; + uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp; embeddedBuffer = nullptr; while (!embeddedQueue_.empty()) { FrameBuffer *b = embeddedQueue_.front(); @@ -2188,10 +2184,23 @@ bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em } if (!embeddedBuffer && sensorMetadata_) { + if (embeddedQueue_.empty()) { + /* + * If the embedded buffer queue is empty, wait for the next + * buffer to arrive - dequeue ordering may send the image + * buffer first. + */ + LOG(RPI, Debug) << "Waiting for next embedded buffer."; + return false; + } + /* Log if there is no matching embedded data buffer found. */ LOG(RPI, Debug) << "Returning bayer frame without a matching embedded buffer."; } + bayerFrame = std::move(bayerQueue_.front()); + bayerQueue_.pop(); + return true; }
The logic used to match asynchronous image and embedded buffers was being overly aggressive by possibly allowing an unmatched image buffer to be sent to the IPA if the matching embedded buffer had not yet been dequeued. This condition only occurs when the system is heavily loaded and dropping frames. Fix this by holding image buffer in the queue during these conditions until the next embedded buffer dequeue event. Reported-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)