| Message ID | 20201124175934.238358-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | c286b00aa1eb9331c3bab2b35905dcf2e4ccde66 |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, On 24/11/2020 17:59, Naushir Patuck wrote: > With the recent change in the bayer/embedded buffer matching code, > a condition would make the bayer buffer be requeued back to the device, > even though it could potentially be queued for matching. This would > cause unnecessary frame drops as sync would be lost. > > The fix is to ensure the bayer buffer only gets requeued if the embedded > data buffer queue is not empty, i.e. the buffer truly is orphaned. > Additionally, we do this test before deciding to flush any of the two > queues of all their buffers. > > Fixes: 909882b (pipeline: raspberrypi: Rework bayer/embedded data buffer matching) > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Thanks for the quick turn around on this fix ;-) Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 38 +++++++++++-------- > 1 file changed, 23 insertions(+), 15 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 7271573a..fd306066 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1679,8 +1679,23 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer * > } > > if (!embeddedBuffer) { > + bool flushedBuffers = false; > + > LOG(RPI, Debug) << "Could not find matching embedded buffer"; > > + if (!embeddedQueue_.empty()) { > + /* > + * Not found a matching embedded buffer for the bayer buffer in > + * the front of the queue. This buffer is now orphaned, so requeue > + * it back to the device. > + */ > + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); > + bayerQueue_.pop(); > + bayerRequeueCount++; > + LOG(RPI, Warning) << "Dropping unmatched input frame in stream " > + << unicam_[Unicam::Image].name(); > + } > + > /* > * If we have requeued all available embedded data buffers in this loop, > * then we are fully out of sync, so might as well requeue all the pending > @@ -1695,20 +1710,9 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer * > unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); > bayerQueue_.pop(); > } > - return false; > + flushedBuffers = true; > } > > - /* > - * Not found a matching embedded buffer for the bayer buffer in > - * the front of the queue. This buffer is now orphaned, so requeue > - * it back to the device. > - */ > - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); > - bayerQueue_.pop(); > - bayerRequeueCount++; > - LOG(RPI, Warning) << "Dropping unmatched input frame in stream " > - << unicam_[Unicam::Image].name(); > - > /* > * Similar to the above, if we have requeued all available bayer buffers in Is this statement now outdated? ('similar to "above"') or is it ok? > * the loop, then we are fully out of sync, so might as well requeue all the > @@ -1723,11 +1727,15 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer * > unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front()); > embeddedQueue_.pop(); > } > - return false; > + flushedBuffers = true; > } > > - /* If the embedded queue has become empty, we cannot do any more. */ > - if (embeddedQueue_.empty()) > + /* > + * If the embedded queue has become empty, we cannot do any more. > + * Similarly, if we have flushed any one of our queues, we cannot do > + * any more. Return from here without a buffer pair. > + */ > + if (embeddedQueue_.empty() || flushedBuffers) > return false; > } else { > /* >
Hi Kieran, On Tue, 24 Nov 2020 at 19:16, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Naush, > > On 24/11/2020 17:59, Naushir Patuck wrote: > > With the recent change in the bayer/embedded buffer matching code, > > a condition would make the bayer buffer be requeued back to the device, > > even though it could potentially be queued for matching. This would > > cause unnecessary frame drops as sync would be lost. > > > > The fix is to ensure the bayer buffer only gets requeued if the embedded > > data buffer queue is not empty, i.e. the buffer truly is orphaned. > > Additionally, we do this test before deciding to flush any of the two > > queues of all their buffers. > > > > Fixes: 909882b (pipeline: raspberrypi: Rework bayer/embedded data buffer > matching) > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Thanks for the quick turn around on this fix ;-) > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 38 +++++++++++-------- > > 1 file changed, 23 insertions(+), 15 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 7271573a..fd306066 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1679,8 +1679,23 @@ bool > RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer * > > } > > > > if (!embeddedBuffer) { > > + bool flushedBuffers = false; > > + > > LOG(RPI, Debug) << "Could not find matching > embedded buffer"; > > > > + if (!embeddedQueue_.empty()) { > > + /* > > + * Not found a matching embedded buffer > for the bayer buffer in > > + * the front of the queue. This buffer is > now orphaned, so requeue > > + * it back to the device. > > + */ > > + > unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); > > + bayerQueue_.pop(); > > + bayerRequeueCount++; > > + LOG(RPI, Warning) << "Dropping unmatched > input frame in stream " > > + << > unicam_[Unicam::Image].name(); > > + } > > + > > /* > > * If we have requeued all available embedded data > buffers in this loop, > > * then we are fully out of sync, so might as well > requeue all the pending > > @@ -1695,20 +1710,9 @@ bool > RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer * > > > unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); > > bayerQueue_.pop(); > > } > > - return false; > > + flushedBuffers = true; > > } > > > > - /* > > - * Not found a matching embedded buffer for the > bayer buffer in > > - * the front of the queue. This buffer is now > orphaned, so requeue > > - * it back to the device. > > - */ > > - > unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); > > - bayerQueue_.pop(); > > - bayerRequeueCount++; > > - LOG(RPI, Warning) << "Dropping unmatched input > frame in stream " > > - << unicam_[Unicam::Image].name(); > > - > > /* > > * Similar to the above, if we have requeued all > available bayer buffers in > > Is this statement now outdated? ('similar to "above"') or is it ok? > This statement is still valid, we are flushing the embedded data queue with similar conditions to the bayer queue flush above. Regards, Naush > > > * the loop, then we are fully out of sync, so > might as well requeue all the > > @@ -1723,11 +1727,15 @@ bool > RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer * > > > unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front()); > > embeddedQueue_.pop(); > > } > > - return false; > > + flushedBuffers = true; > > } > > > > - /* If the embedded queue has become empty, we > cannot do any more. */ > > - if (embeddedQueue_.empty()) > > + /* > > + * If the embedded queue has become empty, we > cannot do any more. > > + * Similarly, if we have flushed any one of our > queues, we cannot do > > + * any more. Return from here without a buffer > pair. > > + */ > > + if (embeddedQueue_.empty() || flushedBuffers) > > return false; > > } else { > > /* > > > > -- > Regards > -- > Kieran >
Hi Naush, On 25/11/2020 09:46, Naushir Patuck wrote: > Hi Kieran, > > On Tue, 24 Nov 2020 at 19:16, Kieran Bingham > <kieran.bingham@ideasonboard.com > <mailto:kieran.bingham@ideasonboard.com>> wrote: > > Hi Naush, > > On 24/11/2020 17:59, Naushir Patuck wrote: > > With the recent change in the bayer/embedded buffer matching code, > > a condition would make the bayer buffer be requeued back to the > device, > > even though it could potentially be queued for matching. This would > > cause unnecessary frame drops as sync would be lost. > > > > The fix is to ensure the bayer buffer only gets requeued if the > embedded > > data buffer queue is not empty, i.e. the buffer truly is orphaned. > > Additionally, we do this test before deciding to flush any of the two > > queues of all their buffers. > > > > Fixes: 909882b (pipeline: raspberrypi: Rework bayer/embedded data > buffer matching) > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com > <mailto:naush@raspberrypi.com>> > > Thanks for the quick turn around on this fix ;-) > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com > <mailto:kieran.bingham@ideasonboard.com>> > > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 38 > +++++++++++-------- > > 1 file changed, 23 insertions(+), 15 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 7271573a..fd306066 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1679,8 +1679,23 @@ bool > RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, > FrameBuffer * > > } > > > > if (!embeddedBuffer) { > > + bool flushedBuffers = false; > > + > > LOG(RPI, Debug) << "Could not find matching > embedded buffer"; > > > > + if (!embeddedQueue_.empty()) { > > + /* > > + * Not found a matching embedded > buffer for the bayer buffer in > > + * the front of the queue. This > buffer is now orphaned, so requeue > > + * it back to the device. > > + */ > > + > unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); > > + bayerQueue_.pop(); > > + bayerRequeueCount++; > > + LOG(RPI, Warning) << "Dropping > unmatched input frame in stream " > > + << > unicam_[Unicam::Image].name(); > > + } > > + > > /* > > * If we have requeued all available > embedded data buffers in this loop, > > * then we are fully out of sync, so might > as well requeue all the pending > > @@ -1695,20 +1710,9 @@ bool > RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, > FrameBuffer * > > > unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); > > bayerQueue_.pop(); > > } > > - return false; > > + flushedBuffers = true; > > } > > > > - /* > > - * Not found a matching embedded buffer for > the bayer buffer in > > - * the front of the queue. This buffer is > now orphaned, so requeue > > - * it back to the device. > > - */ > > - > unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); > > - bayerQueue_.pop(); > > - bayerRequeueCount++; > > - LOG(RPI, Warning) << "Dropping unmatched > input frame in stream " > > - << > unicam_[Unicam::Image].name(); > > - > > /* > > * Similar to the above, if we have requeued > all available bayer buffers in > > Is this statement now outdated? ('similar to "above"') or is it ok? > > > This statement is still valid, we are flushing the embedded data queue > with similar conditions to the bayer queue flush above. > Ok - great. I was worried that the code move might have affected it. Well then, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Regards, > Naush > > > > > > * the loop, then we are fully out of sync, > so might as well requeue all the > > @@ -1723,11 +1727,15 @@ bool > RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, > FrameBuffer * > > > unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front()); > > embeddedQueue_.pop(); > > } > > - return false; > > + flushedBuffers = true; > > } > > > > - /* If the embedded queue has become empty, > we cannot do any more. */ > > - if (embeddedQueue_.empty()) > > + /* > > + * If the embedded queue has become empty, > we cannot do any more. > > + * Similarly, if we have flushed any one of > our queues, we cannot do > > + * any more. Return from here without a > buffer pair. > > + */ > > + if (embeddedQueue_.empty() || flushedBuffers) > > return false; > > } else { > > /* > > > > -- > Regards > -- > Kieran >
Hi Kieran, Thank you for the review. David, would you be able to tag this as tested by you? Once that is done, I think it should be good to go. Regards, Naush On Wed, 25 Nov 2020 at 10:03, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Naush, > > On 25/11/2020 09:46, Naushir Patuck wrote: > > Hi Kieran, > > > > On Tue, 24 Nov 2020 at 19:16, Kieran Bingham > > <kieran.bingham@ideasonboard.com > > <mailto:kieran.bingham@ideasonboard.com>> wrote: > > > > Hi Naush, > > > > On 24/11/2020 17:59, Naushir Patuck wrote: > > > With the recent change in the bayer/embedded buffer matching code, > > > a condition would make the bayer buffer be requeued back to the > > device, > > > even though it could potentially be queued for matching. This would > > > cause unnecessary frame drops as sync would be lost. > > > > > > The fix is to ensure the bayer buffer only gets requeued if the > > embedded > > > data buffer queue is not empty, i.e. the buffer truly is orphaned. > > > Additionally, we do this test before deciding to flush any of the > two > > > queues of all their buffers. > > > > > > Fixes: 909882b (pipeline: raspberrypi: Rework bayer/embedded data > > buffer matching) > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com > > <mailto:naush@raspberrypi.com>> > > > > Thanks for the quick turn around on this fix ;-) > > > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com > > <mailto:kieran.bingham@ideasonboard.com>> > > > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 38 > > +++++++++++-------- > > > 1 file changed, 23 insertions(+), 15 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 7271573a..fd306066 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -1679,8 +1679,23 @@ bool > > RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, > > FrameBuffer * > > > } > > > > > > if (!embeddedBuffer) { > > > + bool flushedBuffers = false; > > > + > > > LOG(RPI, Debug) << "Could not find matching > > embedded buffer"; > > > > > > + if (!embeddedQueue_.empty()) { > > > + /* > > > + * Not found a matching embedded > > buffer for the bayer buffer in > > > + * the front of the queue. This > > buffer is now orphaned, so requeue > > > + * it back to the device. > > > + */ > > > + > > unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); > > > + bayerQueue_.pop(); > > > + bayerRequeueCount++; > > > + LOG(RPI, Warning) << "Dropping > > unmatched input frame in stream " > > > + << > > unicam_[Unicam::Image].name(); > > > + } > > > + > > > /* > > > * If we have requeued all available > > embedded data buffers in this loop, > > > * then we are fully out of sync, so might > > as well requeue all the pending > > > @@ -1695,20 +1710,9 @@ bool > > RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, > > FrameBuffer * > > > > > unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); > > > bayerQueue_.pop(); > > > } > > > - return false; > > > + flushedBuffers = true; > > > } > > > > > > - /* > > > - * Not found a matching embedded buffer for > > the bayer buffer in > > > - * the front of the queue. This buffer is > > now orphaned, so requeue > > > - * it back to the device. > > > - */ > > > - > > unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); > > > - bayerQueue_.pop(); > > > - bayerRequeueCount++; > > > - LOG(RPI, Warning) << "Dropping unmatched > > input frame in stream " > > > - << > > unicam_[Unicam::Image].name(); > > > - > > > /* > > > * Similar to the above, if we have requeued > > all available bayer buffers in > > > > Is this statement now outdated? ('similar to "above"') or is it ok? > > > > > > This statement is still valid, we are flushing the embedded data queue > > with similar conditions to the bayer queue flush above. > > > > Ok - great. I was worried that the code move might have affected it. > > Well then, > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Regards, > > Naush > > > > > > > > > > > * the loop, then we are fully out of sync, > > so might as well requeue all the > > > @@ -1723,11 +1727,15 @@ bool > > RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, > > FrameBuffer * > > > > > unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front()); > > > embeddedQueue_.pop(); > > > } > > > - return false; > > > + flushedBuffers = true; > > > } > > > > > > - /* If the embedded queue has become empty, > > we cannot do any more. */ > > > - if (embeddedQueue_.empty()) > > > + /* > > > + * If the embedded queue has become empty, > > we cannot do any more. > > > + * Similarly, if we have flushed any one of > > our queues, we cannot do > > > + * any more. Return from here without a > > buffer pair. > > > + */ > > > + if (embeddedQueue_.empty() || flushedBuffers) > > > return false; > > > } else { > > > /* > > > > > > > -- > > Regards > > -- > > Kieran > > > > -- > Regards > -- > Kieran >
Hi Naush Yes, indeed. It's been working for me. On Thu, 26 Nov 2020 at 09:25, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi Kieran, > > Thank you for the review. David, would you be able to tag this as tested by you? Once that is done, I think it should be good to go. > > Regards, > Naush > > > On Wed, 25 Nov 2020 at 10:03, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: >> >> Hi Naush, >> >> On 25/11/2020 09:46, Naushir Patuck wrote: >> > Hi Kieran, >> > >> > On Tue, 24 Nov 2020 at 19:16, Kieran Bingham >> > <kieran.bingham@ideasonboard.com >> > <mailto:kieran.bingham@ideasonboard.com>> wrote: >> > >> > Hi Naush, >> > >> > On 24/11/2020 17:59, Naushir Patuck wrote: >> > > With the recent change in the bayer/embedded buffer matching code, >> > > a condition would make the bayer buffer be requeued back to the >> > device, >> > > even though it could potentially be queued for matching. This would >> > > cause unnecessary frame drops as sync would be lost. >> > > >> > > The fix is to ensure the bayer buffer only gets requeued if the >> > embedded >> > > data buffer queue is not empty, i.e. the buffer truly is orphaned. >> > > Additionally, we do this test before deciding to flush any of the two >> > > queues of all their buffers. >> > > >> > > Fixes: 909882b (pipeline: raspberrypi: Rework bayer/embedded data >> > buffer matching) >> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com >> > <mailto:naush@raspberrypi.com>> >> > >> > Thanks for the quick turn around on this fix ;-) >> > >> > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com >> > <mailto:kieran.bingham@ideasonboard.com>> >> > >> > > --- >> > > .../pipeline/raspberrypi/raspberrypi.cpp | 38 >> > +++++++++++-------- >> > > 1 file changed, 23 insertions(+), 15 deletions(-) >> > > >> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> > > index 7271573a..fd306066 100644 >> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> > > @@ -1679,8 +1679,23 @@ bool >> > RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, >> > FrameBuffer * >> > > } >> > > >> > > if (!embeddedBuffer) { >> > > + bool flushedBuffers = false; >> > > + >> > > LOG(RPI, Debug) << "Could not find matching >> > embedded buffer"; >> > > >> > > + if (!embeddedQueue_.empty()) { >> > > + /* >> > > + * Not found a matching embedded >> > buffer for the bayer buffer in >> > > + * the front of the queue. This >> > buffer is now orphaned, so requeue >> > > + * it back to the device. >> > > + */ >> > > + >> > unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); >> > > + bayerQueue_.pop(); >> > > + bayerRequeueCount++; >> > > + LOG(RPI, Warning) << "Dropping >> > unmatched input frame in stream " >> > > + << >> > unicam_[Unicam::Image].name(); >> > > + } >> > > + >> > > /* >> > > * If we have requeued all available >> > embedded data buffers in this loop, >> > > * then we are fully out of sync, so might >> > as well requeue all the pending >> > > @@ -1695,20 +1710,9 @@ bool >> > RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, >> > FrameBuffer * >> > > >> > unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); >> > > bayerQueue_.pop(); >> > > } >> > > - return false; >> > > + flushedBuffers = true; >> > > } >> > > >> > > - /* >> > > - * Not found a matching embedded buffer for >> > the bayer buffer in >> > > - * the front of the queue. This buffer is >> > now orphaned, so requeue >> > > - * it back to the device. >> > > - */ >> > > - >> > unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); >> > > - bayerQueue_.pop(); >> > > - bayerRequeueCount++; >> > > - LOG(RPI, Warning) << "Dropping unmatched >> > input frame in stream " >> > > - << >> > unicam_[Unicam::Image].name(); >> > > - >> > > /* >> > > * Similar to the above, if we have requeued >> > all available bayer buffers in >> > >> > Is this statement now outdated? ('similar to "above"') or is it ok? >> > >> > >> > This statement is still valid, we are flushing the embedded data queue >> > with similar conditions to the bayer queue flush above. >> > >> >> Ok - great. I was worried that the code move might have affected it. >> >> Well then, >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> Tested-by: David Plowman <david.plowman@raspberrypi.com> Best regards David >> > Regards, >> > Naush >> > >> > >> > >> > >> > > * the loop, then we are fully out of sync, >> > so might as well requeue all the >> > > @@ -1723,11 +1727,15 @@ bool >> > RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, >> > FrameBuffer * >> > > >> > unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front()); >> > > embeddedQueue_.pop(); >> > > } >> > > - return false; >> > > + flushedBuffers = true; >> > > } >> > > >> > > - /* If the embedded queue has become empty, >> > we cannot do any more. */ >> > > - if (embeddedQueue_.empty()) >> > > + /* >> > > + * If the embedded queue has become empty, >> > we cannot do any more. >> > > + * Similarly, if we have flushed any one of >> > our queues, we cannot do >> > > + * any more. Return from here without a >> > buffer pair. >> > > + */ >> > > + if (embeddedQueue_.empty() || flushedBuffers) >> > > return false; >> > > } else { >> > > /* >> > > >> > >> > -- >> > Regards >> > -- >> > Kieran >> > >> >> -- >> Regards >> -- >> Kieran
Hi David, Naush, On 26/11/2020 09:51, David Plowman wrote: > Hi Naush > > Yes, indeed. It's been working for me. Thanks, I've added: Tested-by: David Plowman <david.plowman@raspberrypi.com> and pushed. -- Kieran > > On Thu, 26 Nov 2020 at 09:25, Naushir Patuck <naush@raspberrypi.com> wrote: >> >> Hi Kieran, >> >> Thank you for the review. David, would you be able to tag this as tested by you? Once that is done, I think it should be good to go. >> >> Regards, >> Naush >> >> >> On Wed, 25 Nov 2020 at 10:03, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: >>> >>> Hi Naush, >>> >>> On 25/11/2020 09:46, Naushir Patuck wrote: >>>> Hi Kieran, >>>> >>>> On Tue, 24 Nov 2020 at 19:16, Kieran Bingham >>>> <kieran.bingham@ideasonboard.com >>>> <mailto:kieran.bingham@ideasonboard.com>> wrote: >>>> >>>> Hi Naush, >>>> >>>> On 24/11/2020 17:59, Naushir Patuck wrote: >>>> > With the recent change in the bayer/embedded buffer matching code, >>>> > a condition would make the bayer buffer be requeued back to the >>>> device, >>>> > even though it could potentially be queued for matching. This would >>>> > cause unnecessary frame drops as sync would be lost. >>>> > >>>> > The fix is to ensure the bayer buffer only gets requeued if the >>>> embedded >>>> > data buffer queue is not empty, i.e. the buffer truly is orphaned. >>>> > Additionally, we do this test before deciding to flush any of the two >>>> > queues of all their buffers. >>>> > >>>> > Fixes: 909882b (pipeline: raspberrypi: Rework bayer/embedded data >>>> buffer matching) >>>> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com >>>> <mailto:naush@raspberrypi.com>> >>>> >>>> Thanks for the quick turn around on this fix ;-) >>>> >>>> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com >>>> <mailto:kieran.bingham@ideasonboard.com>> >>>> >>>> > --- >>>> > .../pipeline/raspberrypi/raspberrypi.cpp | 38 >>>> +++++++++++-------- >>>> > 1 file changed, 23 insertions(+), 15 deletions(-) >>>> > >>>> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>>> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>>> > index 7271573a..fd306066 100644 >>>> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>>> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>>> > @@ -1679,8 +1679,23 @@ bool >>>> RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, >>>> FrameBuffer * >>>> > } >>>> > >>>> > if (!embeddedBuffer) { >>>> > + bool flushedBuffers = false; >>>> > + >>>> > LOG(RPI, Debug) << "Could not find matching >>>> embedded buffer"; >>>> > >>>> > + if (!embeddedQueue_.empty()) { >>>> > + /* >>>> > + * Not found a matching embedded >>>> buffer for the bayer buffer in >>>> > + * the front of the queue. This >>>> buffer is now orphaned, so requeue >>>> > + * it back to the device. >>>> > + */ >>>> > + >>>> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); >>>> > + bayerQueue_.pop(); >>>> > + bayerRequeueCount++; >>>> > + LOG(RPI, Warning) << "Dropping >>>> unmatched input frame in stream " >>>> > + << >>>> unicam_[Unicam::Image].name(); >>>> > + } >>>> > + >>>> > /* >>>> > * If we have requeued all available >>>> embedded data buffers in this loop, >>>> > * then we are fully out of sync, so might >>>> as well requeue all the pending >>>> > @@ -1695,20 +1710,9 @@ bool >>>> RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, >>>> FrameBuffer * >>>> > >>>> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); >>>> > bayerQueue_.pop(); >>>> > } >>>> > - return false; >>>> > + flushedBuffers = true; >>>> > } >>>> > >>>> > - /* >>>> > - * Not found a matching embedded buffer for >>>> the bayer buffer in >>>> > - * the front of the queue. This buffer is >>>> now orphaned, so requeue >>>> > - * it back to the device. >>>> > - */ >>>> > - >>>> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); >>>> > - bayerQueue_.pop(); >>>> > - bayerRequeueCount++; >>>> > - LOG(RPI, Warning) << "Dropping unmatched >>>> input frame in stream " >>>> > - << >>>> unicam_[Unicam::Image].name(); >>>> > - >>>> > /* >>>> > * Similar to the above, if we have requeued >>>> all available bayer buffers in >>>> >>>> Is this statement now outdated? ('similar to "above"') or is it ok? >>>> >>>> >>>> This statement is still valid, we are flushing the embedded data queue >>>> with similar conditions to the bayer queue flush above. >>>> >>> >>> Ok - great. I was worried that the code move might have affected it. >>> >>> Well then, >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>> > > Tested-by: David Plowman <david.plowman@raspberrypi.com> > > Best regards > David > >>>> Regards, >>>> Naush >>>> >>>> >>>> >>>> >>>> > * the loop, then we are fully out of sync, >>>> so might as well requeue all the >>>> > @@ -1723,11 +1727,15 @@ bool >>>> RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, >>>> FrameBuffer * >>>> > >>>> unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front()); >>>> > embeddedQueue_.pop(); >>>> > } >>>> > - return false; >>>> > + flushedBuffers = true; >>>> > } >>>> > >>>> > - /* If the embedded queue has become empty, >>>> we cannot do any more. */ >>>> > - if (embeddedQueue_.empty()) >>>> > + /* >>>> > + * If the embedded queue has become empty, >>>> we cannot do any more. >>>> > + * Similarly, if we have flushed any one of >>>> our queues, we cannot do >>>> > + * any more. Return from here without a >>>> buffer pair. >>>> > + */ >>>> > + if (embeddedQueue_.empty() || flushedBuffers) >>>> > return false; >>>> > } else { >>>> > /* >>>> > >>>> >>>> -- >>>> Regards >>>> -- >>>> Kieran >>>> >>> >>> -- >>> Regards >>> -- >>> Kieran
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 7271573a..fd306066 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1679,8 +1679,23 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer * } if (!embeddedBuffer) { + bool flushedBuffers = false; + LOG(RPI, Debug) << "Could not find matching embedded buffer"; + if (!embeddedQueue_.empty()) { + /* + * Not found a matching embedded buffer for the bayer buffer in + * the front of the queue. This buffer is now orphaned, so requeue + * it back to the device. + */ + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); + bayerQueue_.pop(); + bayerRequeueCount++; + LOG(RPI, Warning) << "Dropping unmatched input frame in stream " + << unicam_[Unicam::Image].name(); + } + /* * If we have requeued all available embedded data buffers in this loop, * then we are fully out of sync, so might as well requeue all the pending @@ -1695,20 +1710,9 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer * unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); bayerQueue_.pop(); } - return false; + flushedBuffers = true; } - /* - * Not found a matching embedded buffer for the bayer buffer in - * the front of the queue. This buffer is now orphaned, so requeue - * it back to the device. - */ - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front()); - bayerQueue_.pop(); - bayerRequeueCount++; - LOG(RPI, Warning) << "Dropping unmatched input frame in stream " - << unicam_[Unicam::Image].name(); - /* * Similar to the above, if we have requeued all available bayer buffers in * the loop, then we are fully out of sync, so might as well requeue all the @@ -1723,11 +1727,15 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer * unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front()); embeddedQueue_.pop(); } - return false; + flushedBuffers = true; } - /* If the embedded queue has become empty, we cannot do any more. */ - if (embeddedQueue_.empty()) + /* + * If the embedded queue has become empty, we cannot do any more. + * Similarly, if we have flushed any one of our queues, we cannot do + * any more. Return from here without a buffer pair. + */ + if (embeddedQueue_.empty() || flushedBuffers) return false; } else { /*
With the recent change in the bayer/embedded buffer matching code, a condition would make the bayer buffer be requeued back to the device, even though it could potentially be queued for matching. This would cause unnecessary frame drops as sync would be lost. The fix is to ensure the bayer buffer only gets requeued if the embedded data buffer queue is not empty, i.e. the buffer truly is orphaned. Additionally, we do this test before deciding to flush any of the two queues of all their buffers. Fixes: 909882b (pipeline: raspberrypi: Rework bayer/embedded data buffer matching) Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 38 +++++++++++-------- 1 file changed, 23 insertions(+), 15 deletions(-)