<div dir="ltr"><div dir="ltr">Hi Kieran,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 24 Nov 2020 at 19:16, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On 24/11/2020 17:59, Naushir Patuck wrote:<br>
> With the recent change in the bayer/embedded buffer matching code,<br>
> a condition would make the bayer buffer be requeued back to the device,<br>
> even though it could potentially be queued for matching. This would<br>
> cause unnecessary frame drops as sync would be lost.<br>
> <br>
> The fix is to ensure the bayer buffer only gets requeued if the embedded<br>
> data buffer queue is not empty, i.e. the buffer truly is orphaned.<br>
> Additionally, we do this test before deciding to flush any of the two<br>
> queues of all their buffers.<br>
> <br>
> Fixes: 909882b (pipeline: raspberrypi: Rework bayer/embedded data buffer matching)<br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
<br>
Thanks for the quick turn around on this fix ;-)<br>
<br>
Tested-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
> ---<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 38 +++++++++++--------<br>
> 1 file changed, 23 insertions(+), 15 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 7271573a..fd306066 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1679,8 +1679,23 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *<br>
> }<br>
> <br>
> if (!embeddedBuffer) {<br>
> + bool flushedBuffers = false;<br>
> +<br>
> LOG(RPI, Debug) << "Could not find matching embedded buffer";<br>
> <br>
> + if (!embeddedQueue_.empty()) {<br>
> + /*<br>
> + * Not found a matching embedded buffer for the bayer buffer in<br>
> + * the front of the queue. This buffer is now orphaned, so requeue<br>
> + * it back to the device.<br>
> + */<br>
> + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());<br>
> + bayerQueue_.pop();<br>
> + bayerRequeueCount++;<br>
> + LOG(RPI, Warning) << "Dropping unmatched input frame in stream "<br>
> + << unicam_[Unicam::Image].name();<br>
> + }<br>
> +<br>
> /*<br>
> * If we have requeued all available embedded data buffers in this loop,<br>
> * then we are fully out of sync, so might as well requeue all the pending<br>
> @@ -1695,20 +1710,9 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *<br>
> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());<br>
> bayerQueue_.pop();<br>
> }<br>
> - return false;<br>
> + flushedBuffers = true;<br>
> }<br>
> <br>
> - /*<br>
> - * Not found a matching embedded buffer for the bayer buffer in<br>
> - * the front of the queue. This buffer is now orphaned, so requeue<br>
> - * it back to the device.<br>
> - */<br>
> - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());<br>
> - bayerQueue_.pop();<br>
> - bayerRequeueCount++;<br>
> - LOG(RPI, Warning) << "Dropping unmatched input frame in stream "<br>
> - << unicam_[Unicam::Image].name();<br>
> -<br>
> /*<br>
> * Similar to the above, if we have requeued all available bayer buffers in<br>
<br>
Is this statement now outdated? ('similar to "above"') or is it ok?<br></blockquote><div><br></div><div>This statement is still valid, we are flushing the embedded data queue with similar conditions to the bayer queue flush above.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> * the loop, then we are fully out of sync, so might as well requeue all the<br>
> @@ -1723,11 +1727,15 @@ bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *<br>
> unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());<br>
> embeddedQueue_.pop();<br>
> }<br>
> - return false;<br>
> + flushedBuffers = true;<br>
> }<br>
> <br>
> - /* If the embedded queue has become empty, we cannot do any more. */<br>
> - if (embeddedQueue_.empty())<br>
> + /*<br>
> + * If the embedded queue has become empty, we cannot do any more.<br>
> + * Similarly, if we have flushed any one of our queues, we cannot do<br>
> + * any more. Return from here without a buffer pair.<br>
> + */<br>
> + if (embeddedQueue_.empty() || flushedBuffers)<br>
> return false;<br>
> } else {<br>
> /*<br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>