[libcamera-devel] [PATCH] pipeline: raspberrypi: Rework bayer/embedded data buffer matching
Naushir Patuck
naush at raspberrypi.com
Tue Nov 17 11:09:27 CET 2020
Hi Laurent,
Thank you for your comments.
On Mon, 16 Nov 2020 at 15:14, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> Thank you for the patch.
>
> 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 data
> streams.
> > This occurred because even though both streams would get flushed to
> resync,
> > 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.cpp
> b/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_t
> timestamp,
> > - 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 pass
> to
> > - * the IPA. Any embedded buffers with a timestamp lower than the
> > - * current bayer buffer will be removed and re-queued to the
> driver.
> > - */
> > - embeddedBuffer = updateQueue(embeddedQueue_,
> bayerBuffer->metadata().timestamp,
> > - &unicam_[Unicam::Embedded]);
> > -
> > - if (!embeddedBuffer) {
> > - LOG(RPI, Debug) << "Could not find matching embedded
> buffer";
> > -
> > - /*
> > - * Look the other way, try to match a bayer buffer with the
> > - * first embedded buffer in the queue. This will also do
> some
> > - * housekeeping on the bayer image queue - clear out any
> > - * buffers that are older than the first buffer in the
> embedded
> > - * 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 bayer
> buffer - 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 all
> available
> > - * Unicam buffers have been dequeued but do not match. This can
> happen
> > - * when the system is heavily loaded and we get out of lock-step
> with
> > - * the two channels.
> > - *
> > - * In such cases, the best thing to do is the re-queue all the
> buffers
> > - * and give a chance for the hardware to return to lock-step. We
> do 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 timestamp
> to pass to
> > + * the IPA. Any embedded buffers with a timestamp lower
> than the
> > + * current bayer buffer will be removed and re-queued to
> the 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 unmatched
> input 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.
> > + /* The calling function will pop the item
> from 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 from
> here. */
> > + }
> > }
> > - }
> > -}
> >
> > -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 assume
> they have
> > - * been synced by queuing at the same time. We cannot drop these
> frames,
> > - * 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 matching
> embedded buffer";
> > +
> > + /*
> > + * If we have requeued all available embedded data
> buffers in this loop,
> > + * then we are fully out of sync, so might as well
> requeue all the pending
> > + * bayer buffers.
> > + */
> > + if (embeddedRequeueCount ==
> unicam_[Unicam::Embedded].getBuffers().size()) {
> > + /* The embedded queue must be empty at
> this point! */
> > + ASSERT(embeddedQueue_.empty());
> > +
> > + LOG(RPI, Warning) << "Flushing bayer
> stream!";
> > + while (!bayerQueue_.empty()) {
> > +
> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> > + bayerQueue_.pop();
> > + }
> > + return false;
> > + }
> > +
> > + /*
> > + * Not found a matching embedded buffer for the
> bayer buffer in
> > + * the front of the queue. This buffer is now
> orphaned, so requeue
> > + * it back to the device.
> > + */
> > +
> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> > + bayerQueue_.pop();
> > + bayerRequeueCount++;
> > LOG(RPI, Warning) << "Dropping unmatched input
> frame in stream "
> > - << stream->name();
> > - } else if (stream->isExternal() || b->metadata().timestamp
> == timestamp) {
> > - /* The calling function will pop the item from the
> queue. */
> > - return b;
> > + << unicam_[Unicam::Image].name();
> > +
> > + /*
> > + * Similar to the above, if we have requeued all
> available bayer buffers in
> > + * the loop, then we are fully out of sync, so
> might as well requeue all the
> > + * pending embedded data buffers.
> > + */
> > + if (bayerRequeueCount ==
> unicam_[Unicam::Image].getBuffers().size()) {
> > + /* The embedded queue must be empty at
> this point! */
> > + ASSERT(bayerQueue_.empty());
> > +
> > + LOG(RPI, Warning) << "Flushing embedded
> data stream!";
> > + while (!embeddedQueue_.empty()) {
> > +
> unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
> > + embeddedQueue_.pop();
> > + }
> > + return false;
> > + }
> > +
> > + /* If the embedded queue has become empty, we
> cannot do any more. */
> > + if (embeddedQueue_.empty())
> > + return false;
> > } else {
> > - break; /* Only higher timestamps from here. */
> > + /*
> > + * We have found a matching bayer and embedded
> data buffer, so
> > + * nothing more to do apart from popping the
> buffers 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!
Regards,
Naush
With the above issues addressed if applicable,
>
> Acked-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > }
> >
> > REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi)
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201117/0ac58bd6/attachment-0001.htm>
More information about the libcamera-devel
mailing list