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

[David Plowman]

Hi Naush

Yes, indeed. It's been working for me.

On Thu, 26 Nov 2020 at 09:25, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> 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>
>>
>>

Tested-by: David Plowman <david.plowman at raspberrypi.com>

Best regards
David

>> > 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