[libcamera-devel] [PATCH] pipeline: raspberrypi: Fix erroneous bayer buffer requeue on buffer matching
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Nov 26 11:36:51 CET 2020
Hi David, Naush,
On 26/11/2020 09:51, David Plowman wrote:
> Hi Naush
>
> Yes, indeed. It's been working for me.
Thanks, I've added:
Tested-by: David Plowman <david.plowman at raspberrypi.com>
and pushed.
--
Kieran
>
> 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
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list