[libcamera-devel] [PATCH] pipeline: raspberrypi: Fix erroneous bayer buffer requeue on buffer matching

[Naushir Patuck]

Hi Kieran,

On Tue, 24 Nov 2020 at 19:16, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Hi Naush,
>
> On 24/11/2020 17:59, Naushir Patuck wrote:
> > With the recent change in the bayer/embedded buffer matching code,
> > a condition would make the bayer buffer be requeued back to the device,
> > even though it could potentially be queued for matching. This would
> > cause unnecessary frame drops as sync would be lost.
> >
> > The fix is to ensure the bayer buffer only gets requeued if the embedded
> > data buffer queue is not empty, i.e. the buffer truly is orphaned.
> > Additionally, we do this test before deciding to flush any of the two
> > queues of all their buffers.
> >
> > Fixes: 909882b (pipeline: raspberrypi: Rework bayer/embedded data buffer
> matching)
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
>
> Thanks for the quick turn around on this fix ;-)
>
> Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 38 +++++++++++--------
> >  1 file changed, 23 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 7271573a..fd306066 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1679,8 +1679,23 @@ bool
> RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *
> >               }
> >
> >               if (!embeddedBuffer) {
> > +                     bool flushedBuffers = false;
> > +
> >                       LOG(RPI, Debug) << "Could not find matching
> embedded buffer";
> >
> > +                     if (!embeddedQueue_.empty()) {
> > +                             /*
> > +                              * 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 "
> > +                                               <<
> unicam_[Unicam::Image].name();
> > +                     }
> > +
> >                       /*
> >                        * 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
> > @@ -1695,20 +1710,9 @@ bool
> RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *
> >
>  unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> >                                       bayerQueue_.pop();
> >                               }
> > -                             return false;
> > +                             flushedBuffers = true;
> >                       }
> >
> > -                     /*
> > -                      * 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 "
> > -                                       << unicam_[Unicam::Image].name();
> > -
> >                       /*
> >                        * Similar to the above, if we have requeued all
> available bayer buffers in
>
> Is this statement now outdated? ('similar to "above"') or is it ok?
>

This statement is still valid, we are flushing the embedded data queue with
similar conditions to the bayer queue flush above.

Regards,
Naush



>
> >                        * the loop, then we are fully out of sync, so
> might as well requeue all the
> > @@ -1723,11 +1727,15 @@ bool
> RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *
> >
>  unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
> >                                       embeddedQueue_.pop();
> >                               }
> > -                             return false;
> > +                             flushedBuffers = true;
> >                       }
> >
> > -                     /* If the embedded queue has become empty, we
> cannot do any more. */
> > -                     if (embeddedQueue_.empty())
> > +                     /*
> > +                      * If the embedded queue has become empty, we
> cannot do any more.
> > +                      * Similarly, if we have flushed any one of our
> queues, we cannot do
> > +                      * any more. Return from here without a buffer
> pair.
> > +                      */
> > +                     if (embeddedQueue_.empty() || flushedBuffers)
> >                               return false;
> >               } else {
> >                       /*
> >
>
> --
> Regards
> --
> Kieran
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201125/aa082f79/attachment.htm>