[libcamera-devel] [PATCH] pipeline: raspberrypi: Rework bayer/embedded data buffer matching
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Nov 17 13:21:39 CET 2020
Hi Naush,
On Tue, Nov 17, 2020 at 10:09:27AM +0000, Naushir Patuck wrote:
> On Mon, 16 Nov 2020 at 15:14, Laurent Pinchart wrote:
> > On Mon, Nov 16, 2020 at 12:02:56PM +0000, Naushir Patuck wrote:
> > > There is a condition that would cause the buffers to never be in sync
> > > when we using only a single buffer in the bayer and embedded datastreams.
> > > This occurred because even though both streams would get flushed toresync,
> > > one stream's only buffer was already queued in the device, and would end
> > > up never matching.
> > >
> > > Rework the buffer matching logic by combining updateQueue() and
> > > tryFlushQueue() into a single function findMatchingBuffers(). This would
> > > allow us to flush the queues at the same time as we match buffers,avoiding
> > > the the above condition.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 176 +++++++++---------
> > > 1 file changed, 88 insertions(+), 88 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cppb/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 7ad66f21..26c6731c 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -205,9 +205,7 @@ public:
> > > private:
> > > void checkRequestCompleted();
> > > void tryRunPipeline();
> > > - void tryFlushQueues();
> > > - FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_ttimestamp,
> > > - RPi::Stream *stream);
> > > + bool findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer*&embeddedBuffer);
> > >
> > > unsigned int ispOutputCount_;
> > > };
> > > @@ -1537,7 +1535,6 @@ void RPiCameraData::handleState()
> > >
> > > case State::Idle:
> > > tryRunPipeline();
> > > - tryFlushQueues();
> > > break;
> > > }
> > > }
> > > @@ -1588,41 +1585,8 @@ void RPiCameraData::tryRunPipeline()
> > > bayerQueue_.empty() || embeddedQueue_.empty())
> > > return;
> > >
> > > - /* Start with the front of the bayer buffer queue. */
> > > - bayerBuffer = bayerQueue_.front();
> > > -
> > > - /*
> > > - * Find the embedded data buffer with a matching timestamp to passto
> > > - * the IPA. Any embedded buffers with a timestamp lower than the
> > > - * current bayer buffer will be removed and re-queued to thedriver.
> > > - */
> > > - embeddedBuffer = updateQueue(embeddedQueue_,bayerBuffer->metadata().timestamp,
> > > - &unicam_[Unicam::Embedded]);
> > > -
> > > - if (!embeddedBuffer) {
> > > - LOG(RPI, Debug) << "Could not find matching embeddedbuffer";
> > > -
> > > - /*
> > > - * Look the other way, try to match a bayer buffer with the
> > > - * first embedded buffer in the queue. This will also dosome
> > > - * housekeeping on the bayer image queue - clear out any
> > > - * buffers that are older than the first buffer in theembedded
> > > - * queue.
> > > - *
> > > - * But first check if the embedded queue has emptied out.
> > > - */
> > > - if (embeddedQueue_.empty())
> > > - return;
> > > -
> > > - embeddedBuffer = embeddedQueue_.front();
> > > - bayerBuffer = updateQueue(bayerQueue_,embeddedBuffer->metadata().timestamp,
> > > - &unicam_[Unicam::Image]);
> > > -
> > > - if (!bayerBuffer) {
> > > - LOG(RPI, Debug) << "Could not find matching bayerbuffer - ending.";
> > > - return;
> > > - }
> > > - }
> > > + if (!findMatchingBuffers(bayerBuffer, embeddedBuffer))
> > > + return;
> > >
> > > /* Take the first request from the queue and action the IPA. */
> > > Request *request = requestQueue_.front();
> > > @@ -1665,10 +1629,6 @@ void RPiCameraData::tryRunPipeline()
> > > op.controls = { request->controls() };
> > > ipa_->processEvent(op);
> > >
> > > - /* Ready to use the buffers, pop them off the queue. */
> > > - bayerQueue_.pop();
> > > - embeddedQueue_.pop();
> > > -
> > > /* Set our state to say the pipeline is active. */
> > > state_ = State::Busy;
> > >
> > > @@ -1685,62 +1645,102 @@ void RPiCameraData::tryRunPipeline()
> > > ipa_->processEvent(op);
> > > }
> > >
> > > -void RPiCameraData::tryFlushQueues()
> > > +bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer,FrameBuffer *&embeddedBuffer)
> > > {
> > > - /*
> > > - * It is possible for us to end up in a situation where allavailable
> > > - * Unicam buffers have been dequeued but do not match. This canhappen
> > > - * when the system is heavily loaded and we get out of lock-stepwith
> > > - * the two channels.
> > > - *
> > > - * In such cases, the best thing to do is the re-queue all thebuffers
> > > - * and give a chance for the hardware to return to lock-step. Wedo have
> > > - * to drop all interim frames.
> > > - */
> > > - if (unicam_[Unicam::Image].getBuffers().size() ==bayerQueue_.size() &&
> > > - unicam_[Unicam::Embedded].getBuffers().size() ==embeddedQueue_.size()) {
> > > - /* This cannot happen when Unicam streams are external. */
> > > - assert(!unicam_[Unicam::Image].isExternal());
> > > + unsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0;
> > >
> > > - LOG(RPI, Warning) << "Flushing all buffer queues!";
> > > -
> > > - while (!bayerQueue_.empty()) {
> > > - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> > > - bayerQueue_.pop();
> > > - }
> > > + /* Loop until we find a matching bayer and embedded data buffer. */
> > > + while (!bayerQueue_.empty()) {
> > > + /* Start with the front of the bayer queue. */
> > > + bayerBuffer = bayerQueue_.front();
> > >
> > > + /*
> > > + * Find the embedded data buffer with a matching timestampto pass to
> > > + * the IPA. Any embedded buffers with a timestamp lowerthan the
> > > + * current bayer buffer will be removed and re-queued tothe driver.
> > > + */
> >
> > Missing space before *.
>
> Oops, ack. Surprised the check patch script did not flag this one up.
>
> > > + uint64_t ts = bayerBuffer->metadata().timestamp;
> > > + embeddedBuffer = nullptr;
> > > while (!embeddedQueue_.empty()) {
> > > - unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
> > > - embeddedQueue_.pop();
> > > + FrameBuffer *b = embeddedQueue_.front();
> > > + if (!unicam_[Unicam::Embedded].isExternal() &&b->metadata().timestamp < ts) {
> > > + embeddedQueue_.pop();
> > > + unicam_[Unicam::Embedded].queueBuffer(b);
> > > + embeddedRequeueCount++;
> > > + LOG(RPI, Warning) << "Dropping unmatchedinput frame in stream "
> > > + <<unicam_[Unicam::Embedded].name();
> > > + } else if (unicam_[Unicam::Embedded].isExternal()|| b->metadata().timestamp == ts) {
> >
> > I wonder if we could use the sequence number here, as the timestamp
> > seems a bit fragile. I suppose the current unicam implementation
> > guarantees that the image and embedded data timestamps will be identical
> > if they refer to the same frame, but I'm concerned this could change in
> > the future. Is there a similar guarantee on the sequence number, or can
> > we only rely on the timestamp ?
>
> 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.
Sure, no issue.
> > > + /* The calling function will pop the itemfrom the queue. */
> >
> > Is it the calling function, or the code at the end of this function ?
>
> 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.
>
> > > + embeddedBuffer = b;
> > > + break;
> > > + } else {
> > > + break; /* Only higher timestamps fromhere. */
> > > + }
> > > }
> > > - }
> > > -}
> > >
> > > -FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q,uint64_t timestamp,
> > > - RPi::Stream *stream)
> > > -{
> > > - /*
> > > - * If the unicam streams are external (both have be to the same),then we
> > > - * can only return out the top buffer in the queue, and assumethey have
> > > - * been synced by queuing at the same time. We cannot drop theseframes,
> > > - * as they may have been provided externally.
> > > - */
> > > - while (!q.empty()) {
> > > - FrameBuffer *b = q.front();
> > > - if (!stream->isExternal() && b->metadata().timestamp <timestamp) {
> > > - q.pop();
> > > - stream->queueBuffer(b);
> > > + if (!embeddedBuffer) {
> > > + LOG(RPI, Debug) << "Could not find matchingembedded buffer";
> > > +
> > > + /*
> > > + * If we have requeued all available embedded databuffers in this loop,
> > > + * then we are fully out of sync, so might as wellrequeue all the pending
> > > + * bayer buffers.
> > > + */
> > > + if (embeddedRequeueCount ==unicam_[Unicam::Embedded].getBuffers().size()) {
> > > + /* The embedded queue must be empty atthis point! */
> > > + ASSERT(embeddedQueue_.empty());
> > > +
> > > + LOG(RPI, Warning) << "Flushing bayerstream!";
> > > + while (!bayerQueue_.empty()) {
> > > + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> > > + bayerQueue_.pop();
> > > + }
> > > + return false;
> > > + }
> > > +
> > > + /*
> > > + * Not found a matching embedded buffer for thebayer buffer in
> > > + * the front of the queue. This buffer is noworphaned, so requeue
> > > + * it back to the device.
> > > + */
> > > + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> > > + bayerQueue_.pop();
> > > + bayerRequeueCount++;
> > > LOG(RPI, Warning) << "Dropping unmatched inputframe in stream "
> > > - << stream->name();
> > > - } else if (stream->isExternal() || b->metadata().timestamp== timestamp) {
> > > - /* The calling function will pop the item from thequeue. */
> > > - return b;
> > > + << unicam_[Unicam::Image].name();
> > > +
> > > + /*
> > > + * Similar to the above, if we have requeued allavailable bayer buffers in
> > > + * the loop, then we are fully out of sync, somight as well requeue all the
> > > + * pending embedded data buffers.
> > > + */
> > > + if (bayerRequeueCount ==unicam_[Unicam::Image].getBuffers().size()) {
> > > + /* The embedded queue must be empty atthis point! */
> > > + ASSERT(bayerQueue_.empty());
> > > +
> > > + LOG(RPI, Warning) << "Flushing embeddeddata stream!";
> > > + while (!embeddedQueue_.empty()) {
> > > + unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
> > > + embeddedQueue_.pop();
> > > + }
> > > + return false;
> > > + }
> > > +
> > > + /* If the embedded queue has become empty, wecannot do any more. */
> > > + if (embeddedQueue_.empty())
> > > + return false;
> > > } else {
> > > - break; /* Only higher timestamps from here. */
> > > + /*
> > > + * We have found a matching bayer and embeddeddata buffer, so
> > > + * nothing more to do apart from popping thebuffers from the queue.
> > > + */
> > > + bayerQueue_.pop();
> > > + embeddedQueue_.pop();
> > > + return true;
> > > }
> > > }
> > >
> > > - return nullptr;
> > > + return false;
> >
> > The logic is too complex for my taste, but not worse than it was :-) I'm
> > sure we'll have opportunities to refactor all this in the future when
> > introducing new helpers.
>
> 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 :-)
> Yes, hopefully some of this might become libcamera helpers and simplify
> things!
>
> > With the above issues addressed if applicable,
> >
> > Acked-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > }
> > >
> > > REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list