<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your comments.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 16 Nov 2020 at 15:14, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@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>
Thank you for the patch.<br>
<br>
On Mon, Nov 16, 2020 at 12:02:56PM +0000, Naushir Patuck wrote:<br>
> There is a condition that would cause the buffers to never be in sync<br>
> when we using only a single buffer in the bayer and embedded data streams.<br>
> This occurred because even though both streams would get flushed to resync,<br>
> one stream's only buffer was already queued in the device, and would end<br>
> up never matching.<br>
> <br>
> Rework the buffer matching logic by combining updateQueue() and<br>
> tryFlushQueue() into a single function findMatchingBuffers(). This would<br>
> allow us to flush the queues at the same time as we match buffers, avoiding<br>
> the the above condition.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 176 +++++++++---------<br>
> 1 file changed, 88 insertions(+), 88 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 7ad66f21..26c6731c 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -205,9 +205,7 @@ public:<br>
> private:<br>
> void checkRequestCompleted();<br>
> void tryRunPipeline();<br>
> - void tryFlushQueues();<br>
> - FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,<br>
> - RPi::Stream *stream);<br>
> + bool findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer);<br>
> <br>
> unsigned int ispOutputCount_;<br>
> };<br>
> @@ -1537,7 +1535,6 @@ void RPiCameraData::handleState()<br>
> <br>
> case State::Idle:<br>
> tryRunPipeline();<br>
> - tryFlushQueues();<br>
> break;<br>
> }<br>
> }<br>
> @@ -1588,41 +1585,8 @@ void RPiCameraData::tryRunPipeline()<br>
> bayerQueue_.empty() || embeddedQueue_.empty())<br>
> return;<br>
> <br>
> - /* Start with the front of the bayer buffer queue. */<br>
> - bayerBuffer = bayerQueue_.front();<br>
> -<br>
> - /*<br>
> - * Find the embedded data buffer with a matching timestamp to pass to<br>
> - * the IPA. Any embedded buffers with a timestamp lower than the<br>
> - * current bayer buffer will be removed and re-queued to the driver.<br>
> - */<br>
> - embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,<br>
> - &unicam_[Unicam::Embedded]);<br>
> -<br>
> - if (!embeddedBuffer) {<br>
> - LOG(RPI, Debug) << "Could not find matching embedded buffer";<br>
> -<br>
> - /*<br>
> - * Look the other way, try to match a bayer buffer with the<br>
> - * first embedded buffer in the queue. This will also do some<br>
> - * housekeeping on the bayer image queue - clear out any<br>
> - * buffers that are older than the first buffer in the embedded<br>
> - * queue.<br>
> - *<br>
> - * But first check if the embedded queue has emptied out.<br>
> - */<br>
> - if (embeddedQueue_.empty())<br>
> - return;<br>
> -<br>
> - embeddedBuffer = embeddedQueue_.front();<br>
> - bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,<br>
> - &unicam_[Unicam::Image]);<br>
> -<br>
> - if (!bayerBuffer) {<br>
> - LOG(RPI, Debug) << "Could not find matching bayer buffer - ending.";<br>
> - return;<br>
> - }<br>
> - }<br>
> + if (!findMatchingBuffers(bayerBuffer, embeddedBuffer))<br>
> + return;<br>
> <br>
> /* Take the first request from the queue and action the IPA. */<br>
> Request *request = requestQueue_.front();<br>
> @@ -1665,10 +1629,6 @@ void RPiCameraData::tryRunPipeline()<br>
> op.controls = { request->controls() };<br>
> ipa_->processEvent(op);<br>
> <br>
> - /* Ready to use the buffers, pop them off the queue. */<br>
> - bayerQueue_.pop();<br>
> - embeddedQueue_.pop();<br>
> -<br>
> /* Set our state to say the pipeline is active. */<br>
> state_ = State::Busy;<br>
> <br>
> @@ -1685,62 +1645,102 @@ void RPiCameraData::tryRunPipeline()<br>
> ipa_->processEvent(op);<br>
> }<br>
> <br>
> -void RPiCameraData::tryFlushQueues()<br>
> +bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer)<br>
> {<br>
> - /*<br>
> - * It is possible for us to end up in a situation where all available<br>
> - * Unicam buffers have been dequeued but do not match. This can happen<br>
> - * when the system is heavily loaded and we get out of lock-step with<br>
> - * the two channels.<br>
> - *<br>
> - * In such cases, the best thing to do is the re-queue all the buffers<br>
> - * and give a chance for the hardware to return to lock-step. We do have<br>
> - * to drop all interim frames.<br>
> - */<br>
> - if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&<br>
> - unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {<br>
> - /* This cannot happen when Unicam streams are external. */<br>
> - assert(!unicam_[Unicam::Image].isExternal());<br>
> + unsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0;<br>
> <br>
> - LOG(RPI, Warning) << "Flushing all buffer queues!";<br>
> -<br>
> - while (!bayerQueue_.empty()) {<br>
> - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());<br>
> - bayerQueue_.pop();<br>
> - }<br>
> + /* Loop until we find a matching bayer and embedded data buffer. */<br>
> + while (!bayerQueue_.empty()) {<br>
> + /* Start with the front of the bayer queue. */<br>
> + bayerBuffer = bayerQueue_.front();<br>
> <br>
> + /*<br>
> + * Find the embedded data buffer with a matching timestamp to pass to<br>
> + * the IPA. Any embedded buffers with a timestamp lower than the<br>
> + * current bayer buffer will be removed and re-queued to the driver.<br>
> + */<br>
<br>
Missing space before *.<br>
<br></blockquote><div><br></div><div>Oops, ack. Surprised the check patch script did not flag this one up.</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">
> + uint64_t ts = bayerBuffer->metadata().timestamp;<br>
> + embeddedBuffer = nullptr;<br>
> while (!embeddedQueue_.empty()) {<br>
> - unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());<br>
> - embeddedQueue_.pop();<br>
> + FrameBuffer *b = embeddedQueue_.front();<br>
> + if (!unicam_[Unicam::Embedded].isExternal() && b->metadata().timestamp < ts) {<br>
> + embeddedQueue_.pop();<br>
> + unicam_[Unicam::Embedded].queueBuffer(b);<br>
> + embeddedRequeueCount++;<br>
> + LOG(RPI, Warning) << "Dropping unmatched input frame in stream "<br>
> + << unicam_[Unicam::Embedded].name();<br>
> + } else if (unicam_[Unicam::Embedded].isExternal() || b->metadata().timestamp == ts) {<br>
<br>
I wonder if we could use the sequence number here, as the timestamp<br>
seems a bit fragile. I suppose the current unicam implementation<br>
guarantees that the image and embedded data timestamps will be identical<br>
if they refer to the same frame, but I'm concerned this could change in<br>
the future. Is there a similar guarantee on the sequence number, or can<br>
we only rely on the timestamp ?<br></blockquote><div><br></div><div>Using sequence numbers would work equally well. Both sequence number and timestamps are guaranteed to be the same for the embedded + image buffer pair. However, I would prefer to make that change separate to this patch if that is ok? Only because I've done exhaustive testing when using timestamps and not sequence numbers.<br></div><div><br></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 calling function will pop the item from the queue. */<br>
<br>
Is it the calling function, or the code at the end of this function ?<br></blockquote><div><br></div><div>Sorry, this is an error in the comment. The calling function no longer pops the item, we do it at the end of the function.</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>
> + embeddedBuffer = b;<br>
> + break;<br>
> + } else {<br>
> + break; /* Only higher timestamps from here. */<br>
> + }<br>
> }<br>
> - }<br>
> -}<br>
> <br>
> -FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,<br>
> - RPi::Stream *stream)<br>
> -{<br>
> - /*<br>
> - * If the unicam streams are external (both have be to the same), then we<br>
> - * can only return out the top buffer in the queue, and assume they have<br>
> - * been synced by queuing at the same time. We cannot drop these frames,<br>
> - * as they may have been provided externally.<br>
> - */<br>
> - while (!q.empty()) {<br>
> - FrameBuffer *b = q.front();<br>
> - if (!stream->isExternal() && b->metadata().timestamp < timestamp) {<br>
> - q.pop();<br>
> - stream->queueBuffer(b);<br>
> + if (!embeddedBuffer) {<br>
> + LOG(RPI, Debug) << "Could not find matching embedded buffer";<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>
> + * bayer buffers.<br>
> + */<br>
> + if (embeddedRequeueCount == unicam_[Unicam::Embedded].getBuffers().size()) {<br>
> + /* The embedded queue must be empty at this point! */<br>
> + ASSERT(embeddedQueue_.empty());<br>
> +<br>
> + LOG(RPI, Warning) << "Flushing bayer stream!";<br>
> + while (!bayerQueue_.empty()) {<br>
> + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());<br>
> + bayerQueue_.pop();<br>
> + }<br>
> + return false;<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>
> - << stream->name();<br>
> - } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {<br>
> - /* The calling function will pop the item from the queue. */<br>
> - return b;<br>
> + << unicam_[Unicam::Image].name();<br>
> +<br>
> + /*<br>
> + * Similar to the above, if we have requeued all available bayer buffers in<br>
> + * the loop, then we are fully out of sync, so might as well requeue all the<br>
> + * pending embedded data buffers.<br>
> + */<br>
> + if (bayerRequeueCount == unicam_[Unicam::Image].getBuffers().size()) {<br>
> + /* The embedded queue must be empty at this point! */<br>
> + ASSERT(bayerQueue_.empty());<br>
> +<br>
> + LOG(RPI, Warning) << "Flushing embedded data stream!";<br>
> + while (!embeddedQueue_.empty()) {<br>
> + unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());<br>
> + embeddedQueue_.pop();<br>
> + }<br>
> + return false;<br>
> + }<br>
> +<br>
> + /* If the embedded queue has become empty, we cannot do any more. */<br>
> + if (embeddedQueue_.empty())<br>
> + return false;<br>
> } else {<br>
> - break; /* Only higher timestamps from here. */<br>
> + /*<br>
> + * We have found a matching bayer and embedded data buffer, so<br>
> + * nothing more to do apart from popping the buffers from the queue.<br>
> + */<br>
> + bayerQueue_.pop();<br>
> + embeddedQueue_.pop();<br>
> + return true;<br>
> }<br>
> }<br>
> <br>
> - return nullptr;<br>
> + return false;<br>
<br>
The logic is too complex for my taste, but not worse than it was :-) I'm<br>
sure we'll have opportunities to refactor all this in the future when<br>
introducing new helpers.<br>
<br></blockquote><div> </div><div>Indeed it is a bit complicated, and I have tried to simplify it as much as I can, but this buffer matching/handling is notoriously difficult to handle all edge cases fully :-)</div><div>Yes, hopefully some of this might become libcamera helpers and simplify things!</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
With the above issues addressed if applicable,<br>
<br>
Acked-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> }<br>
> <br>
> REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>