<div dir="ltr"><div dir="ltr">Hi Kieran,</div><div dir="ltr"><br></div><div>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.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 25 Nov 2020 at 10:03, 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 25/11/2020 09:46, Naushir Patuck wrote:<br>
> Hi Kieran,<br>
> <br>
> On Tue, 24 Nov 2020 at 19:16, Kieran Bingham<br>
> <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a><br>
> <mailto:<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>>> wrote:<br>
> <br>
> 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<br>
> 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<br>
> 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<br>
> buffer matching)<br>
> > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a><br>
> <mailto:<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>
> <mailto:<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>>><br>
> <br>
> > ---<br>
> > .../pipeline/raspberrypi/raspberrypi.cpp | 38<br>
> +++++++++++--------<br>
> > 1 file changed, 23 insertions(+), 15 deletions(-)<br>
> ><br>
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> 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<br>
> RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer,<br>
> FrameBuffer *<br>
> > }<br>
> > <br>
> > if (!embeddedBuffer) {<br>
> > + bool flushedBuffers = false;<br>
> > +<br>
> > LOG(RPI, Debug) << "Could not find matching<br>
> embedded buffer";<br>
> > <br>
> > + if (!embeddedQueue_.empty()) {<br>
> > + /*<br>
> > + * Not found a matching embedded<br>
> buffer for the bayer buffer in<br>
> > + * the front of the queue. This<br>
> buffer is now orphaned, so requeue<br>
> > + * it back to the device.<br>
> > + */<br>
> > + <br>
> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());<br>
> > + bayerQueue_.pop();<br>
> > + bayerRequeueCount++;<br>
> > + LOG(RPI, Warning) << "Dropping<br>
> unmatched input frame in stream "<br>
> > + <<<br>
> unicam_[Unicam::Image].name();<br>
> > + }<br>
> > +<br>
> > /*<br>
> > * If we have requeued all available<br>
> embedded data buffers in this loop,<br>
> > * then we are fully out of sync, so might<br>
> as well requeue all the pending<br>
> > @@ -1695,20 +1710,9 @@ bool<br>
> RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer,<br>
> FrameBuffer *<br>
> > <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<br>
> the bayer buffer in<br>
> > - * the front of the queue. This buffer is<br>
> now orphaned, so requeue<br>
> > - * it back to the device.<br>
> > - */<br>
> > - <br>
> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());<br>
> > - bayerQueue_.pop();<br>
> > - bayerRequeueCount++;<br>
> > - LOG(RPI, Warning) << "Dropping unmatched<br>
> input frame in stream "<br>
> > - <<<br>
> unicam_[Unicam::Image].name();<br>
> > -<br>
> > /*<br>
> > * Similar to the above, if we have requeued<br>
> all available bayer buffers in<br>
> <br>
> Is this statement now outdated? ('similar to "above"') or is it ok?<br>
> <br>
> <br>
> This statement is still valid, we are flushing the embedded data queue<br>
> with similar conditions to the bayer queue flush above.<br>
> <br>
<br>
Ok - great. I was worried that the code move might have affected it.<br>
<br>
Well then,<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
> Regards,<br>
> Naush<br>
> <br>
> <br>
> <br>
> <br>
> > * the loop, then we are fully out of sync,<br>
> so might as well requeue all the<br>
> > @@ -1723,11 +1727,15 @@ bool<br>
> RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer,<br>
> FrameBuffer *<br>
> > <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,<br>
> we cannot do any more. */<br>
> > - if (embeddedQueue_.empty())<br>
> > + /*<br>
> > + * If the embedded queue has become empty,<br>
> we cannot do any more.<br>
> > + * Similarly, if we have flushed any one of<br>
> our queues, we cannot do<br>
> > + * any more. Return from here without a<br>
> buffer pair.<br>
> > + */<br>
> > + if (embeddedQueue_.empty() || flushedBuffers)<br>
> > return false;<br>
> > } else {<br>
> > /*<br>
> ><br>
> <br>
> -- <br>
> Regards<br>
> --<br>
> Kieran<br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>