[libcamera-devel] [PATCH 2/3] pipeline: raspberrypi: Simplify image/embedded buffer matching logic
Naushir Patuck
naush at raspberrypi.com
Tue Feb 8 09:26:17 CET 2022
Hi Laurent,
On Mon, 7 Feb 2022 at 23:54, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, Feb 07, 2022 at 03:12:13PM +0000, Naushir Patuck wrote:
> > Simplify the image and embedded buffer matching logic by removing the
> assumption
> > that we require a buffer match between the two streams. Instead, if an
> image
> > buffer does not match with an embedded data buffer, simply use the
> ControlList
> > provided by DelayedControls for the sensor parameters.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > .../pipeline/raspberrypi/raspberrypi.cpp | 141 ++++--------------
> > 1 file changed, 31 insertions(+), 110 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 0755de84c70c..af234bd18c5b 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -2081,122 +2081,43 @@ void RPiCameraData::tryRunPipeline()
> >
> > bool RPiCameraData::findMatchingBuffers(BayerFrame &bayerFrame,
> FrameBuffer *&embeddedBuffer)
> > {
> > - unsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0;
> > -
> > - /* Loop until we find a matching bayer and embedded data buffer. */
> > - while (!bayerQueue_.empty()) {
> > - /* Start with the front of the bayer queue. */
> > - FrameBuffer *bayerBuffer = bayerQueue_.front().buffer;
> > -
> > - /*
> > - * Find the embedded data buffer with a matching timestamp
> to pass to
> > - * the IPA. Any embedded buffers with a timestamp lower
> than the
> > - * current bayer buffer will be removed and re-queued to
> the driver.
> > - */
> > - uint64_t ts = bayerBuffer->metadata().timestamp;
> > - embeddedBuffer = nullptr;
> > - while (!embeddedQueue_.empty()) {
> > - FrameBuffer *b = embeddedQueue_.front();
> > - if (!unicam_[Unicam::Embedded].isExternal() &&
> b->metadata().timestamp < ts) {
> > - embeddedQueue_.pop();
> > - unicam_[Unicam::Embedded].returnBuffer(b);
> > - embeddedRequeueCount++;
> > - LOG(RPI, Warning) << "Dropping unmatched
> input frame in stream "
> > - <<
> unicam_[Unicam::Embedded].name();
> > - } else if (unicam_[Unicam::Embedded].isExternal()
> || b->metadata().timestamp == ts) {
> > - /* We pop the item from the queue lower
> down. */
> > - embeddedBuffer = b;
> > - break;
> > - } else {
> > - break; /* Only higher timestamps from
> here. */
> > - }
> > - }
> > -
> > - if (!embeddedBuffer) {
> > - bool flushedBuffers = false;
> > -
> > - LOG(RPI, Debug) << "Could not find matching
> embedded buffer";
> > -
> > - if (!sensorMetadata_) {
> > - /*
> > - * If there is no sensor metadata, simply
> return the
> > - * first bayer frame in the queue.
> > - */
> > - LOG(RPI, Debug) << "Returning bayer frame
> without a match";
> > - bayerFrame =
> std::move(bayerQueue_.front());
> > - bayerQueue_.pop();
> > - embeddedBuffer = nullptr;
> > - return true;
> > - }
> > -
> > - 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].returnBuffer(bayerQueue_.front().buffer);
> > - 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
> > - * bayer buffers.
> > - */
> > - if (embeddedRequeueCount ==
> unicam_[Unicam::Embedded].getBuffers().size()) {
> > - /* The embedded queue must be empty at
> this point! */
> > - ASSERT(embeddedQueue_.empty());
> > -
> > - LOG(RPI, Warning) << "Flushing bayer
> stream!";
> > - while (!bayerQueue_.empty()) {
> > -
> unicam_[Unicam::Image].returnBuffer(bayerQueue_.front().buffer);
> > - bayerQueue_.pop();
> > - }
> > - flushedBuffers = true;
> > - }
> > + if (bayerQueue_.empty())
> > + return false;
> >
> > - /*
> > - * Similar to the above, if we have requeued all
> available bayer buffers in
> > - * the loop, then we are fully out of sync, so
> might as well requeue all the
> > - * pending embedded data buffers.
> > - */
> > - if (bayerRequeueCount ==
> unicam_[Unicam::Image].getBuffers().size()) {
> > - /* The bayer queue must be empty at this
> point! */
> > - ASSERT(bayerQueue_.empty());
> > -
> > - LOG(RPI, Warning) << "Flushing embedded
> data stream!";
> > - while (!embeddedQueue_.empty()) {
> > -
> unicam_[Unicam::Embedded].returnBuffer(embeddedQueue_.front());
> > - embeddedQueue_.pop();
> > - }
> > - flushedBuffers = true;
> > - }
> > + /* Start with the front of the bayer queue. */
> > + bayerFrame = std::move(bayerQueue_.front());
> > + bayerQueue_.pop();
> >
> > - /*
> > - * 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 {
> > - /*
> > - * We have found a matching bayer and embedded
> data buffer, so
> > - * nothing more to do apart from assigning the
> bayer frame and
> > - * popping the buffers from the queue.
> > - */
> > - bayerFrame = std::move(bayerQueue_.front());
> > - bayerQueue_.pop();
> > + /*
> > + * Find the embedded data buffer with a matching timestamp to pass
> to
> > + * the IPA. Any embedded buffers with a timestamp lower than the
> > + * current bayer buffer will be removed and re-queued to the
> driver.
> > + */
> > + uint64_t ts = bayerFrame.buffer->metadata().timestamp;
> > + embeddedBuffer = nullptr;
> > + while (!embeddedQueue_.empty()) {
> > + FrameBuffer *b = embeddedQueue_.front();
> > + if (b->metadata().timestamp < ts) {
> > + embeddedQueue_.pop();
> > + unicam_[Unicam::Embedded].returnBuffer(b);
> > + LOG(RPI, Debug) << "Dropping unmatched input frame
> in stream "
> > + <<
> unicam_[Unicam::Embedded].name();
> > + } else if (b->metadata().timestamp == ts) {
>
> It would be very nice if we could switch to using sequence numbers
> instead of timestamps, comparing timestamps for equality always looks
> dangerous to me (even if I know Unicam handles this fine).
>
I see no reason why we cannot switch to sequence numbers here. I'll
test it out when I get a chance, and provide a patch on top of this one
to make the change when I can.
Regards,
Naush
>
> This looks like a nice simplification,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > + /* Found a match! */
> > + embeddedBuffer = b;
> > embeddedQueue_.pop();
> > - return true;
> > + break;
> > + } else {
> > + break; /* Only higher timestamps from here. */
> > }
> > }
> >
> > - return false;
> > + if (!embeddedBuffer && sensorMetadata_) {
> > + /* Log if there is no matching embedded data buffer found.
> */
> > + LOG(RPI, Debug) << "Returning bayer frame without a
> matching embedded buffer.";
> > + }
> > +
> > + return true;
> > }
> >
> > REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi)
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220208/2a8ed5e3/attachment-0001.htm>
More information about the libcamera-devel
mailing list