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

[Naushir Patuck]

Hi Kieran,

Thank you for the review.  David, would you be able to tag this as tested
by you?  Once that is done, I think it should be good to go.

Regards,
Naush


On Wed, 25 Nov 2020 at 10:03, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Hi Naush,
>
> On 25/11/2020 09:46, Naushir Patuck wrote:
> > Hi Kieran,
> >
> > On Tue, 24 Nov 2020 at 19:16, Kieran Bingham
> > <kieran.bingham at ideasonboard.com
> > <mailto: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
> >     <mailto:naush at raspberrypi.com>>
> >
> >     Thanks for the quick turn around on this fix ;-)
> >
> >     Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com
> >     <mailto: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.
> >
>
> Ok - great. I was worried that the code move might have affected it.
>
> Well then,
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>

> > 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
> >
>
> --
> Regards
> --
> Kieran
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201126/1d9e84d4/attachment.htm>