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

[Kieran Bingham]

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