<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 7 Feb 2022 at 23:54, 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, Feb 07, 2022 at 03:12:13PM +0000, Naushir Patuck wrote:<br>
> Simplify the image and embedded buffer matching logic by removing the assumption<br>
> that we require a buffer match between the two streams. Instead, if an image<br>
> buffer does not match with an embedded data buffer, simply use the ControlList<br>
> provided by DelayedControls for the sensor parameters.<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 | 141 ++++--------------<br>
> 1 file changed, 31 insertions(+), 110 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 0755de84c70c..af234bd18c5b 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -2081,122 +2081,43 @@ void RPiCameraData::tryRunPipeline()<br>
> <br>
> bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer)<br>
> {<br>
> - unsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0;<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>
> - FrameBuffer *bayerBuffer = bayerQueue_.front().buffer;<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>
> - uint64_t ts = bayerBuffer->metadata().timestamp;<br>
> - embeddedBuffer = nullptr;<br>
> - while (!embeddedQueue_.empty()) {<br>
> - FrameBuffer *b = embeddedQueue_.front();<br>
> - if (!unicam_[Unicam::Embedded].isExternal() && b->metadata().timestamp < ts) {<br>
> - embeddedQueue_.pop();<br>
> - unicam_[Unicam::Embedded].returnBuffer(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>
> - /* We pop the item from the queue lower down. */<br>
> - embeddedBuffer = b;<br>
> - break;<br>
> - } else {<br>
> - break; /* Only higher timestamps from here. */<br>
> - }<br>
> - }<br>
> -<br>
> - if (!embeddedBuffer) {<br>
> - bool flushedBuffers = false;<br>
> -<br>
> - LOG(RPI, Debug) << "Could not find matching embedded buffer";<br>
> -<br>
> - if (!sensorMetadata_) {<br>
> - /*<br>
> - * If there is no sensor metadata, simply return the<br>
> - * first bayer frame in the queue.<br>
> - */<br>
> - LOG(RPI, Debug) << "Returning bayer frame without a match";<br>
> - bayerFrame = std::move(bayerQueue_.front());<br>
> - bayerQueue_.pop();<br>
> - embeddedBuffer = nullptr;<br>
> - return true;<br>
> - }<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].returnBuffer(bayerQueue_.front().buffer);<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>
> - * 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].returnBuffer(bayerQueue_.front().buffer);<br>
> - bayerQueue_.pop();<br>
> - }<br>
> - flushedBuffers = true;<br>
> - }<br>
> + if (bayerQueue_.empty())<br>
> + return false;<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 bayer 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].returnBuffer(embeddedQueue_.front());<br>
> - embeddedQueue_.pop();<br>
> - }<br>
> - flushedBuffers = true;<br>
> - }<br>
> + /* Start with the front of the bayer queue. */<br>
> + bayerFrame = std::move(bayerQueue_.front());<br>
> + bayerQueue_.pop();<br>
> <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>
> - * We have found a matching bayer and embedded data buffer, so<br>
> - * nothing more to do apart from assigning the bayer frame and<br>
> - * popping the buffers from the queue.<br>
> - */<br>
> - bayerFrame = std::move(bayerQueue_.front());<br>
> - bayerQueue_.pop();<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>
> + uint64_t ts = bayerFrame.buffer->metadata().timestamp;<br>
> + embeddedBuffer = nullptr;<br>
> + while (!embeddedQueue_.empty()) {<br>
> + FrameBuffer *b = embeddedQueue_.front();<br>
> + if (b->metadata().timestamp < ts) {<br>
> + embeddedQueue_.pop();<br>
> + unicam_[Unicam::Embedded].returnBuffer(b);<br>
> + LOG(RPI, Debug) << "Dropping unmatched input frame in stream "<br>
> + << unicam_[Unicam::Embedded].name();<br>
> + } else if (b->metadata().timestamp == ts) {<br>
<br>
It would be very nice if we could switch to using sequence numbers<br>
instead of timestamps, comparing timestamps for equality always looks<br>
dangerous to me (even if I know Unicam handles this fine).<br></blockquote><div><br></div><div>I see no reason why we cannot switch to sequence numbers here. I'll</div><div>test it out when I get a chance, and provide a patch on top of this one</div><div>to make the change when I can.</div><div><br></div><div>Regards,</div><div>Naush</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>
This looks like a nice simplification,<br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> + /* Found a match! */<br>
> + embeddedBuffer = b;<br>
> embeddedQueue_.pop();<br>
> - return true;<br>
> + break;<br>
> + } else {<br>
> + break; /* Only higher timestamps from here. */<br>
> }<br>
> }<br>
> <br>
> - return false;<br>
> + if (!embeddedBuffer && sensorMetadata_) {<br>
> + /* Log if there is no matching embedded data buffer found. */<br>
> + LOG(RPI, Debug) << "Returning bayer frame without a matching embedded buffer.";<br>
> + }<br>
> +<br>
> + return true;<br>
> }<br>
> <br>
> REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>