[{"id":13725,"web_url":"https://patchwork.libcamera.org/comment/13725/","msgid":"","date":"2020-11-16T13:11:23","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Rework\n\tbayer/embedded data buffer matching","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nI've tried this out both in qcam, in our own libcamera apps, and in\nsimple-cam with StreamRole::StillCapture and bufferCount set to 1 and\n2 (had to rewind my libcamera tree a bit to make simple-cam work\nagain). Anyway, everything worked fine for me.\n\nOn Mon, 16 Nov 2020 at 12:05, Naushir Patuck wrote:\n>\n> There is a condition that would cause the buffers to never be in sync\n> when we using only a single buffer in the bayer and embedded data streams.\n> This occurred because even though both streams would get flushed to resync,\n> one stream's only buffer was already queued in the device, and would end\n> up never matching.\n>\n> Rework the buffer matching logic by combining updateQueue() and\n> tryFlushQueue() into a single function findMatchingBuffers(). This would\n> allow us to flush the queues at the same time as we match buffers, avoiding\n> the the above condition.\n>\n> Signed-off-by: Naushir Patuck \n\nTested-by: David Plowman \n\nBest regards\nDavid\n\n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 176 +++++++++---------\n> 1 file changed, 88 insertions(+), 88 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 7ad66f21..26c6731c 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -205,9 +205,7 @@ public:\n> private:\n> void checkRequestCompleted();\n> void tryRunPipeline();\n> - void tryFlushQueues();\n> - FrameBuffer *updateQueue(std::queue &q, uint64_t timestamp,\n> - RPi::Stream *stream);\n> + bool findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer);\n>\n> unsigned int ispOutputCount_;\n> };\n> @@ -1537,7 +1535,6 @@ void RPiCameraData::handleState()\n>\n> case State::Idle:\n> tryRunPipeline();\n> - tryFlushQueues();\n> break;\n> }\n> }\n> @@ -1588,41 +1585,8 @@ void RPiCameraData::tryRunPipeline()\n> bayerQueue_.empty() || embeddedQueue_.empty())\n> return;\n>\n> - /* Start with the front of the bayer buffer queue. */\n> - bayerBuffer = bayerQueue_.front();\n> -\n> - /*\n> - * Find the embedded data buffer with a matching timestamp to pass to\n> - * the IPA. Any embedded buffers with a timestamp lower than the\n> - * current bayer buffer will be removed and re-queued to the driver.\n> - */\n> - embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,\n> - &unicam_[Unicam::Embedded]);\n> -\n> - if (!embeddedBuffer) {\n> - LOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n> -\n> - /*\n> - * Look the other way, try to match a bayer buffer with the\n> - * first embedded buffer in the queue. This will also do some\n> - * housekeeping on the bayer image queue - clear out any\n> - * buffers that are older than the first buffer in the embedded\n> - * queue.\n> - *\n> - * But first check if the embedded queue has emptied out.\n> - */\n> - if (embeddedQueue_.empty())\n> - return;\n> -\n> - embeddedBuffer = embeddedQueue_.front();\n> - bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,\n> - &unicam_[Unicam::Image]);\n> -\n> - if (!bayerBuffer) {\n> - LOG(RPI, Debug) << \"Could not find matching bayer buffer - ending.\";\n> - return;\n> - }\n> - }\n> + if (!findMatchingBuffers(bayerBuffer, embeddedBuffer))\n> + return;\n>\n> /* Take the first request from the queue and action the IPA. */\n> Request *request = requestQueue_.front();\n> @@ -1665,10 +1629,6 @@ void RPiCameraData::tryRunPipeline()\n> op.controls = { request->controls() };\n> ipa_->processEvent(op);\n>\n> - /* Ready to use the buffers, pop them off the queue. */\n> - bayerQueue_.pop();\n> - embeddedQueue_.pop();\n> -\n> /* Set our state to say the pipeline is active. */\n> state_ = State::Busy;\n>\n> @@ -1685,62 +1645,102 @@ void RPiCameraData::tryRunPipeline()\n> ipa_->processEvent(op);\n> }\n>\n> -void RPiCameraData::tryFlushQueues()\n> +bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer)\n> {\n> - /*\n> - * It is possible for us to end up in a situation where all available\n> - * Unicam buffers have been dequeued but do not match. This can happen\n> - * when the system is heavily loaded and we get out of lock-step with\n> - * the two channels.\n> - *\n> - * In such cases, the best thing to do is the re-queue all the buffers\n> - * and give a chance for the hardware to return to lock-step. We do have\n> - * to drop all interim frames.\n> - */\n> - if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&\n> - unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {\n> - /* This cannot happen when Unicam streams are external. */\n> - assert(!unicam_[Unicam::Image].isExternal());\n> + unsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0;\n>\n> - LOG(RPI, Warning) << \"Flushing all buffer queues!\";\n> -\n> - while (!bayerQueue_.empty()) {\n> - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> - bayerQueue_.pop();\n> - }\n> + /* Loop until we find a matching bayer and embedded data buffer. */\n> + while (!bayerQueue_.empty()) {\n> + /* Start with the front of the bayer queue. */\n> + bayerBuffer = bayerQueue_.front();\n>\n> + /*\n> + * Find the embedded data buffer with a matching timestamp to pass to\n> + * the IPA. Any embedded buffers with a timestamp lower than the\n> + * current bayer buffer will be removed and re-queued to the driver.\n> + */\n> + uint64_t ts = bayerBuffer->metadata().timestamp;\n> + embeddedBuffer = nullptr;\n> while (!embeddedQueue_.empty()) {\n> - unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> - embeddedQueue_.pop();\n> + FrameBuffer *b = embeddedQueue_.front();\n> + if (!unicam_[Unicam::Embedded].isExternal() && b->metadata().timestamp < ts) {\n> + embeddedQueue_.pop();\n> + unicam_[Unicam::Embedded].queueBuffer(b);\n> + embeddedRequeueCount++;\n> + LOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> + << unicam_[Unicam::Embedded].name();\n> + } else if (unicam_[Unicam::Embedded].isExternal() || b->metadata().timestamp == ts) {\n> + /* The calling function will pop the item from the queue. */\n> + embeddedBuffer = b;\n> + break;\n> + } else {\n> + break; /* Only higher timestamps from here. */\n> + }\n> }\n> - }\n> -}\n>\n> -FrameBuffer *RPiCameraData::updateQueue(std::queue &q, uint64_t timestamp,\n> - RPi::Stream *stream)\n> -{\n> - /*\n> - * If the unicam streams are external (both have be to the same), then we\n> - * can only return out the top buffer in the queue, and assume they have\n> - * been synced by queuing at the same time. We cannot drop these frames,\n> - * as they may have been provided externally.\n> - */\n> - while (!q.empty()) {\n> - FrameBuffer *b = q.front();\n> - if (!stream->isExternal() && b->metadata().timestamp < timestamp) {\n> - q.pop();\n> - stream->queueBuffer(b);\n> + if (!embeddedBuffer) {\n> + LOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n> +\n> + /*\n> + * If we have requeued all available embedded data buffers in this loop,\n> + * then we are fully out of sync, so might as well requeue all the pending\n> + * bayer buffers.\n> + */\n> + if (embeddedRequeueCount == unicam_[Unicam::Embedded].getBuffers().size()) {\n> + /* The embedded queue must be empty at this point! */\n> + ASSERT(embeddedQueue_.empty());\n> +\n> + LOG(RPI, Warning) << \"Flushing bayer stream!\";\n> + while (!bayerQueue_.empty()) {\n> + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> + bayerQueue_.pop();\n> + }\n> + return false;\n> + }\n> +\n> + /*\n> + * Not found a matching embedded buffer for the bayer buffer in\n> + * the front of the queue. This buffer is now orphaned, so requeue\n> + * it back to the device.\n> + */\n> + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> + bayerQueue_.pop();\n> + bayerRequeueCount++;\n> LOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> - << stream->name();\n> - } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {\n> - /* The calling function will pop the item from the queue. */\n> - return b;\n> + << unicam_[Unicam::Image].name();\n> +\n> + /*\n> + * Similar to the above, if we have requeued all available bayer buffers in\n> + * the loop, then we are fully out of sync, so might as well requeue all the\n> + * pending embedded data buffers.\n> + */\n> + if (bayerRequeueCount == unicam_[Unicam::Image].getBuffers().size()) {\n> + /* The embedded queue must be empty at this point! */\n> + ASSERT(bayerQueue_.empty());\n> +\n> + LOG(RPI, Warning) << \"Flushing embedded data stream!\";\n> + while (!embeddedQueue_.empty()) {\n> + unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> + embeddedQueue_.pop();\n> + }\n> + return false;\n> + }\n> +\n> + /* If the embedded queue has become empty, we cannot do any more. */\n> + if (embeddedQueue_.empty())\n> + return false;\n> } else {\n> - break; /* Only higher timestamps from here. */\n> + /*\n> + * We have found a matching bayer and embedded data buffer, so\n> + * nothing more to do apart from popping the buffers from the queue.\n> + */\n> + bayerQueue_.pop();\n> + embeddedQueue_.pop();\n> + return true;\n> }\n> }\n>\n> - return nullptr;\n> + return false;\n> }\n>\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi)\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CBEA4BE081\n\tfor ;\n\tMon, 16 Nov 2020 13:11:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 496EC632C3;\n\tMon, 16 Nov 2020 14:11:38 +0100 (CET)","from mail-oi1-x22f.google.com (mail-oi1-x22f.google.com\n\t[IPv6:2607:f8b0:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CDF82631B5\n\tfor ;\n\tMon, 16 Nov 2020 14:11:36 +0100 (CET)","by mail-oi1-x22f.google.com with SMTP id k19so10081516oic.12\n\tfor ;\n\tMon, 16 Nov 2020 05:11:36 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Fa8AwKvK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=b93cR20a9tSkaED2qat11cinGm+3zOvfdZvUheqXw+c=;\n\tb=Fa8AwKvKJH9pSnNOS039qjkzTyqsTv/gtbFkvjMhVIX7FJE3rsd/3KHdTr7XZK7Gef\n\tIN90M5ySr4tgi3BjwRAZ+H7Cyf4fkxyXrVLcHuFmGDrpJFaKEDSs7q3q2H+8xSgf99jA\n\tgWiebXWBCKtE48Y/x4fIaiGsePL0tyotSwb7XBL5mMNRbrQ6pUzWenY9WuDX/y6Hh39d\n\t5if8eNU/DOmkatw9yG3b4AKPaSi7XAwsCAQ+i3NxVBTUpfR/LeQi4w/VQaGiyEjJlbRF\n\tXCUt9qRdMSVX0nthEnPnc6Q+hQwnj0lPqpdVzDqOXfp1UCnF3iyOsXOKcrLM+CIppOsV\n\tefKA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=b93cR20a9tSkaED2qat11cinGm+3zOvfdZvUheqXw+c=;\n\tb=cIeZZtphTlgJFtoZVTz/XRpjfFKoY3EIKCFpmgXNDcMVYgs2kUtoAqtaK/N+Rc9Rbs\n\tMN1Pb9rK9sWA7twiQFjfR/H5DOUGpi0fKb6qQkRkQmEmB2Db5aqkcrLAmYwdIq+gFIlO\n\t5hRwl6PYwz2rQhZJh7s4d9+20sZ4o0oFWfaWcPbgmTX7Bu12VbxEOopJPuFUJB64+LVR\n\tdpD/kd2jPv6jzB837FGtdOd/BDw4RtoiN8QjQS6xajMz4aAf21cZ8JI8DMTSXcbEkY6E\n\taUMR7ADaNfU/M2R5YmUgdv0FHhMS/9zK0Mo+HaHOEI3MkIK6OLyoh/78SUHR+QvD3RBn\n\tlPGA==","X-Gm-Message-State":"AOAM530MitqgfX0AibfpENhnOF873FY34/hk9U8joU+3eg9Dlno+Xr0s\n\twso+btheJB0OJI9t0fpIiMWXM49uZeuX0pbGt+Go+Q==","X-Google-Smtp-Source":"ABdhPJyoiOHuz6IaEvkwmOvdfiH9EmjpgV+enpgus/oUdGW2FAWDpLm8MzBjZIFAGp3lzA1cIsK6pthaGB/foIGSIbE=","X-Received":"by 2002:a05:6808:563:: with SMTP id\n\tj3mr10129854oig.107.1605532295274; \n\tMon, 16 Nov 2020 05:11:35 -0800 (PST)","MIME-Version":"1.0","References":"<20201116120256.149789-1-naush@raspberrypi.com>","In-Reply-To":"<20201116120256.149789-1-naush@raspberrypi.com>","From":"David Plowman ","Date":"Mon, 16 Nov 2020 13:11:23 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Rework\n\tbayer/embedded data buffer matching","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13727,"web_url":"https://patchwork.libcamera.org/comment/13727/","msgid":"<20201116145907.GM6540@pendragon.ideasonboard.com>","date":"2020-11-16T14:59:07","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Rework\n\tbayer/embedded data buffer matching","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Mon, Nov 16, 2020 at 01:11:23PM +0000, David Plowman wrote:\n> Hi Naush\n> \n> I've tried this out both in qcam, in our own libcamera apps, and in\n> simple-cam with StreamRole::StillCapture and bufferCount set to 1 and\n> 2 (had to rewind my libcamera tree a bit to make simple-cam work\n> again).\n\nSorry for breaking that in the first place :-S Kieran is working on a\nfix.\n\n> Anyway, everything worked fine for me.\n> \n> On Mon, 16 Nov 2020 at 12:05, Naushir Patuck wrote:\n> >\n> > There is a condition that would cause the buffers to never be in sync\n> > when we using only a single buffer in the bayer and embedded data streams.\n> > This occurred because even though both streams would get flushed to resync,\n> > one stream's only buffer was already queued in the device, and would end\n> > up never matching.\n> >\n> > Rework the buffer matching logic by combining updateQueue() and\n> > tryFlushQueue() into a single function findMatchingBuffers(). This would\n> > allow us to flush the queues at the same time as we match buffers, avoiding\n> > the the above condition.\n> >\n> > Signed-off-by: Naushir Patuck \n> \n> Tested-by: David Plowman \n> \n> Best regards\n> David\n> \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 176 +++++++++---------\n> > 1 file changed, 88 insertions(+), 88 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 7ad66f21..26c6731c 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -205,9 +205,7 @@ public:\n> > private:\n> > void checkRequestCompleted();\n> > void tryRunPipeline();\n> > - void tryFlushQueues();\n> > - FrameBuffer *updateQueue(std::queue &q, uint64_t timestamp,\n> > - RPi::Stream *stream);\n> > + bool findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer);\n> >\n> > unsigned int ispOutputCount_;\n> > };\n> > @@ -1537,7 +1535,6 @@ void RPiCameraData::handleState()\n> >\n> > case State::Idle:\n> > tryRunPipeline();\n> > - tryFlushQueues();\n> > break;\n> > }\n> > }\n> > @@ -1588,41 +1585,8 @@ void RPiCameraData::tryRunPipeline()\n> > bayerQueue_.empty() || embeddedQueue_.empty())\n> > return;\n> >\n> > - /* Start with the front of the bayer buffer queue. */\n> > - bayerBuffer = bayerQueue_.front();\n> > -\n> > - /*\n> > - * Find the embedded data buffer with a matching timestamp to pass to\n> > - * the IPA. Any embedded buffers with a timestamp lower than the\n> > - * current bayer buffer will be removed and re-queued to the driver.\n> > - */\n> > - embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,\n> > - &unicam_[Unicam::Embedded]);\n> > -\n> > - if (!embeddedBuffer) {\n> > - LOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n> > -\n> > - /*\n> > - * Look the other way, try to match a bayer buffer with the\n> > - * first embedded buffer in the queue. This will also do some\n> > - * housekeeping on the bayer image queue - clear out any\n> > - * buffers that are older than the first buffer in the embedded\n> > - * queue.\n> > - *\n> > - * But first check if the embedded queue has emptied out.\n> > - */\n> > - if (embeddedQueue_.empty())\n> > - return;\n> > -\n> > - embeddedBuffer = embeddedQueue_.front();\n> > - bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,\n> > - &unicam_[Unicam::Image]);\n> > -\n> > - if (!bayerBuffer) {\n> > - LOG(RPI, Debug) << \"Could not find matching bayer buffer - ending.\";\n> > - return;\n> > - }\n> > - }\n> > + if (!findMatchingBuffers(bayerBuffer, embeddedBuffer))\n> > + return;\n> >\n> > /* Take the first request from the queue and action the IPA. */\n> > Request *request = requestQueue_.front();\n> > @@ -1665,10 +1629,6 @@ void RPiCameraData::tryRunPipeline()\n> > op.controls = { request->controls() };\n> > ipa_->processEvent(op);\n> >\n> > - /* Ready to use the buffers, pop them off the queue. */\n> > - bayerQueue_.pop();\n> > - embeddedQueue_.pop();\n> > -\n> > /* Set our state to say the pipeline is active. */\n> > state_ = State::Busy;\n> >\n> > @@ -1685,62 +1645,102 @@ void RPiCameraData::tryRunPipeline()\n> > ipa_->processEvent(op);\n> > }\n> >\n> > -void RPiCameraData::tryFlushQueues()\n> > +bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer)\n> > {\n> > - /*\n> > - * It is possible for us to end up in a situation where all available\n> > - * Unicam buffers have been dequeued but do not match. This can happen\n> > - * when the system is heavily loaded and we get out of lock-step with\n> > - * the two channels.\n> > - *\n> > - * In such cases, the best thing to do is the re-queue all the buffers\n> > - * and give a chance for the hardware to return to lock-step. We do have\n> > - * to drop all interim frames.\n> > - */\n> > - if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&\n> > - unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {\n> > - /* This cannot happen when Unicam streams are external. */\n> > - assert(!unicam_[Unicam::Image].isExternal());\n> > + unsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0;\n> >\n> > - LOG(RPI, Warning) << \"Flushing all buffer queues!\";\n> > -\n> > - while (!bayerQueue_.empty()) {\n> > - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> > - bayerQueue_.pop();\n> > - }\n> > + /* Loop until we find a matching bayer and embedded data buffer. */\n> > + while (!bayerQueue_.empty()) {\n> > + /* Start with the front of the bayer queue. */\n> > + bayerBuffer = bayerQueue_.front();\n> >\n> > + /*\n> > + * Find the embedded data buffer with a matching timestamp to pass to\n> > + * the IPA. Any embedded buffers with a timestamp lower than the\n> > + * current bayer buffer will be removed and re-queued to the driver.\n> > + */\n> > + uint64_t ts = bayerBuffer->metadata().timestamp;\n> > + embeddedBuffer = nullptr;\n> > while (!embeddedQueue_.empty()) {\n> > - unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> > - embeddedQueue_.pop();\n> > + FrameBuffer *b = embeddedQueue_.front();\n> > + if (!unicam_[Unicam::Embedded].isExternal() && b->metadata().timestamp < ts) {\n> > + embeddedQueue_.pop();\n> > + unicam_[Unicam::Embedded].queueBuffer(b);\n> > + embeddedRequeueCount++;\n> > + LOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> > + << unicam_[Unicam::Embedded].name();\n> > + } else if (unicam_[Unicam::Embedded].isExternal() || b->metadata().timestamp == ts) {\n> > + /* The calling function will pop the item from the queue. */\n> > + embeddedBuffer = b;\n> > + break;\n> > + } else {\n> > + break; /* Only higher timestamps from here. */\n> > + }\n> > }\n> > - }\n> > -}\n> >\n> > -FrameBuffer *RPiCameraData::updateQueue(std::queue &q, uint64_t timestamp,\n> > - RPi::Stream *stream)\n> > -{\n> > - /*\n> > - * If the unicam streams are external (both have be to the same), then we\n> > - * can only return out the top buffer in the queue, and assume they have\n> > - * been synced by queuing at the same time. We cannot drop these frames,\n> > - * as they may have been provided externally.\n> > - */\n> > - while (!q.empty()) {\n> > - FrameBuffer *b = q.front();\n> > - if (!stream->isExternal() && b->metadata().timestamp < timestamp) {\n> > - q.pop();\n> > - stream->queueBuffer(b);\n> > + if (!embeddedBuffer) {\n> > + LOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n> > +\n> > + /*\n> > + * If we have requeued all available embedded data buffers in this loop,\n> > + * then we are fully out of sync, so might as well requeue all the pending\n> > + * bayer buffers.\n> > + */\n> > + if (embeddedRequeueCount == unicam_[Unicam::Embedded].getBuffers().size()) {\n> > + /* The embedded queue must be empty at this point! */\n> > + ASSERT(embeddedQueue_.empty());\n> > +\n> > + LOG(RPI, Warning) << \"Flushing bayer stream!\";\n> > + while (!bayerQueue_.empty()) {\n> > + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> > + bayerQueue_.pop();\n> > + }\n> > + return false;\n> > + }\n> > +\n> > + /*\n> > + * Not found a matching embedded buffer for the bayer buffer in\n> > + * the front of the queue. This buffer is now orphaned, so requeue\n> > + * it back to the device.\n> > + */\n> > + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> > + bayerQueue_.pop();\n> > + bayerRequeueCount++;\n> > LOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> > - << stream->name();\n> > - } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {\n> > - /* The calling function will pop the item from the queue. */\n> > - return b;\n> > + << unicam_[Unicam::Image].name();\n> > +\n> > + /*\n> > + * Similar to the above, if we have requeued all available bayer buffers in\n> > + * the loop, then we are fully out of sync, so might as well requeue all the\n> > + * pending embedded data buffers.\n> > + */\n> > + if (bayerRequeueCount == unicam_[Unicam::Image].getBuffers().size()) {\n> > + /* The embedded queue must be empty at this point! */\n> > + ASSERT(bayerQueue_.empty());\n> > +\n> > + LOG(RPI, Warning) << \"Flushing embedded data stream!\";\n> > + while (!embeddedQueue_.empty()) {\n> > + unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> > + embeddedQueue_.pop();\n> > + }\n> > + return false;\n> > + }\n> > +\n> > + /* If the embedded queue has become empty, we cannot do any more. */\n> > + if (embeddedQueue_.empty())\n> > + return false;\n> > } else {\n> > - break; /* Only higher timestamps from here. */\n> > + /*\n> > + * We have found a matching bayer and embedded data buffer, so\n> > + * nothing more to do apart from popping the buffers from the queue.\n> > + */\n> > + bayerQueue_.pop();\n> > + embeddedQueue_.pop();\n> > + return true;\n> > }\n> > }\n> >\n> > - return nullptr;\n> > + return false;\n> > }\n> >\n> > REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 95ADABE082\n\tfor ;\n\tMon, 16 Nov 2020 14:59:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2825B632C3;\n\tMon, 16 Nov 2020 15:59:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E6054631B5\n\tfor ;\n\tMon, 16 Nov 2020 15:59:12 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 60EF7A1B;\n\tMon, 16 Nov 2020 15:59:12 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WvcjzyW0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1605538752;\n\tbh=Adyk49fMoHHxmNYVoYcwPZQVtOXNvADR/yAdZRmm/OY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WvcjzyW0yjwtHj8hn6gsymRxjE5q8OIopKCjeQKthAkxmF+J3TOmtNwiJneY+sMVe\n\tonnPHp+3NWodlYrIYqazwZsHdlQ1okU6MO5mfITbVGjmpzh2iDQ16IRSidSu2I9isQ\n\t+oO063ENC3P3HLM/c+q2ZszKMg9yp762zj31xtUE=","Date":"Mon, 16 Nov 2020 16:59:07 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201116145907.GM6540@pendragon.ideasonboard.com>","References":"<20201116120256.149789-1-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Rework\n\tbayer/embedded data buffer matching","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13728,"web_url":"https://patchwork.libcamera.org/comment/13728/","msgid":"<20201116151423.GN6540@pendragon.ideasonboard.com>","date":"2020-11-16T15:14:23","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Rework\n\tbayer/embedded data buffer matching","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Mon, Nov 16, 2020 at 12:02:56PM +0000, Naushir Patuck wrote:\n> There is a condition that would cause the buffers to never be in sync\n> when we using only a single buffer in the bayer and embedded data streams.\n> This occurred because even though both streams would get flushed to resync,\n> one stream's only buffer was already queued in the device, and would end\n> up never matching.\n> \n> Rework the buffer matching logic by combining updateQueue() and\n> tryFlushQueue() into a single function findMatchingBuffers(). This would\n> allow us to flush the queues at the same time as we match buffers, avoiding\n> the the above condition.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 176 +++++++++---------\n> 1 file changed, 88 insertions(+), 88 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 7ad66f21..26c6731c 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -205,9 +205,7 @@ public:\n> private:\n> \tvoid checkRequestCompleted();\n> \tvoid tryRunPipeline();\n> -\tvoid tryFlushQueues();\n> -\tFrameBuffer *updateQueue(std::queue &q, uint64_t timestamp,\n> -\t\t\t\t RPi::Stream *stream);\n> +\tbool findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer);\n> \n> \tunsigned int ispOutputCount_;\n> };\n> @@ -1537,7 +1535,6 @@ void RPiCameraData::handleState()\n> \n> \tcase State::Idle:\n> \t\ttryRunPipeline();\n> -\t\ttryFlushQueues();\n> \t\tbreak;\n> \t}\n> }\n> @@ -1588,41 +1585,8 @@ void RPiCameraData::tryRunPipeline()\n> \t bayerQueue_.empty() || embeddedQueue_.empty())\n> \t\treturn;\n> \n> -\t/* Start with the front of the bayer buffer queue. */\n> -\tbayerBuffer = bayerQueue_.front();\n> -\n> -\t/*\n> -\t * Find the embedded data buffer with a matching timestamp to pass to\n> -\t * the IPA. Any embedded buffers with a timestamp lower than the\n> -\t * current bayer buffer will be removed and re-queued to the driver.\n> -\t */\n> -\tembeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,\n> -\t\t\t\t &unicam_[Unicam::Embedded]);\n> -\n> -\tif (!embeddedBuffer) {\n> -\t\tLOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n> -\n> -\t\t/*\n> -\t\t * Look the other way, try to match a bayer buffer with the\n> -\t\t * first embedded buffer in the queue. This will also do some\n> -\t\t * housekeeping on the bayer image queue - clear out any\n> -\t\t * buffers that are older than the first buffer in the embedded\n> -\t\t * queue.\n> -\t\t *\n> -\t\t * But first check if the embedded queue has emptied out.\n> -\t\t */\n> -\t\tif (embeddedQueue_.empty())\n> -\t\t\treturn;\n> -\n> -\t\tembeddedBuffer = embeddedQueue_.front();\n> -\t\tbayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,\n> -\t\t\t\t\t &unicam_[Unicam::Image]);\n> -\n> -\t\tif (!bayerBuffer) {\n> -\t\t\tLOG(RPI, Debug) << \"Could not find matching bayer buffer - ending.\";\n> -\t\t\treturn;\n> -\t\t}\n> -\t}\n> +\tif (!findMatchingBuffers(bayerBuffer, embeddedBuffer))\n> +\t\treturn;\n> \n> \t/* Take the first request from the queue and action the IPA. */\n> \tRequest *request = requestQueue_.front();\n> @@ -1665,10 +1629,6 @@ void RPiCameraData::tryRunPipeline()\n> \top.controls = { request->controls() };\n> \tipa_->processEvent(op);\n> \n> -\t/* Ready to use the buffers, pop them off the queue. */\n> -\tbayerQueue_.pop();\n> -\tembeddedQueue_.pop();\n> -\n> \t/* Set our state to say the pipeline is active. */\n> \tstate_ = State::Busy;\n> \n> @@ -1685,62 +1645,102 @@ void RPiCameraData::tryRunPipeline()\n> \tipa_->processEvent(op);\n> }\n> \n> -void RPiCameraData::tryFlushQueues()\n> +bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer)\n> {\n> -\t/*\n> -\t * It is possible for us to end up in a situation where all available\n> -\t * Unicam buffers have been dequeued but do not match. This can happen\n> -\t * when the system is heavily loaded and we get out of lock-step with\n> -\t * the two channels.\n> -\t *\n> -\t * In such cases, the best thing to do is the re-queue all the buffers\n> -\t * and give a chance for the hardware to return to lock-step. We do have\n> -\t * to drop all interim frames.\n> -\t */\n> -\tif (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&\n> -\t unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {\n> -\t\t/* This cannot happen when Unicam streams are external. */\n> -\t\tassert(!unicam_[Unicam::Image].isExternal());\n> +\tunsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0;\n> \n> -\t\tLOG(RPI, Warning) << \"Flushing all buffer queues!\";\n> -\n> -\t\twhile (!bayerQueue_.empty()) {\n> -\t\t\tunicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> -\t\t\tbayerQueue_.pop();\n> -\t\t}\n> +\t/* Loop until we find a matching bayer and embedded data buffer. */\n> +\twhile (!bayerQueue_.empty()) {\n> +\t\t/* Start with the front of the bayer queue. */\n> +\t\tbayerBuffer = bayerQueue_.front();\n> \n> +\t\t/*\n> +\t\t* Find the embedded data buffer with a matching timestamp to pass to\n> +\t\t* the IPA. Any embedded buffers with a timestamp lower than the\n> +\t\t* current bayer buffer will be removed and re-queued to the driver.\n> +\t\t*/\n\nMissing space before *.\n\n> +\t\tuint64_t ts = bayerBuffer->metadata().timestamp;\n> +\t\tembeddedBuffer = nullptr;\n> \t\twhile (!embeddedQueue_.empty()) {\n> -\t\t\tunicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> -\t\t\tembeddedQueue_.pop();\n> +\t\t\tFrameBuffer *b = embeddedQueue_.front();\n> +\t\t\tif (!unicam_[Unicam::Embedded].isExternal() && b->metadata().timestamp < ts) {\n> +\t\t\t\tembeddedQueue_.pop();\n> +\t\t\t\tunicam_[Unicam::Embedded].queueBuffer(b);\n> +\t\t\t\tembeddedRequeueCount++;\n> +\t\t\t\tLOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> +\t\t\t\t\t\t << unicam_[Unicam::Embedded].name();\n> +\t\t\t} else if (unicam_[Unicam::Embedded].isExternal() || b->metadata().timestamp == ts) {\n\nI wonder if we could use the sequence number here, as the timestamp\nseems a bit fragile. I suppose the current unicam implementation\nguarantees that the image and embedded data timestamps will be identical\nif they refer to the same frame, but I'm concerned this could change in\nthe future. Is there a similar guarantee on the sequence number, or can\nwe only rely on the timestamp ?\n\n> +\t\t\t\t/* The calling function will pop the item from the queue. */\n\nIs it the calling function, or the code at the end of this function ?\n\n> +\t\t\t\tembeddedBuffer = b;\n> +\t\t\t\tbreak;\n> +\t\t\t} else {\n> +\t\t\t\tbreak; /* Only higher timestamps from here. */\n> +\t\t\t}\n> \t\t}\n> -\t}\n> -}\n> \n> -FrameBuffer *RPiCameraData::updateQueue(std::queue &q, uint64_t timestamp,\n> -\t\t\t\t\tRPi::Stream *stream)\n> -{\n> -\t/*\n> -\t * If the unicam streams are external (both have be to the same), then we\n> -\t * can only return out the top buffer in the queue, and assume they have\n> -\t * been synced by queuing at the same time. We cannot drop these frames,\n> -\t * as they may have been provided externally.\n> -\t */\n> -\twhile (!q.empty()) {\n> -\t\tFrameBuffer *b = q.front();\n> -\t\tif (!stream->isExternal() && b->metadata().timestamp < timestamp) {\n> -\t\t\tq.pop();\n> -\t\t\tstream->queueBuffer(b);\n> +\t\tif (!embeddedBuffer) {\n> +\t\t\tLOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n> +\n> +\t\t\t/*\n> +\t\t\t * If we have requeued all available embedded data buffers in this loop,\n> +\t\t\t * then we are fully out of sync, so might as well requeue all the pending\n> +\t\t\t * bayer buffers.\n> +\t\t\t */\n> +\t\t\tif (embeddedRequeueCount == unicam_[Unicam::Embedded].getBuffers().size()) {\n> +\t\t\t\t/* The embedded queue must be empty at this point! */\n> +\t\t\t\tASSERT(embeddedQueue_.empty());\n> +\n> +\t\t\t\tLOG(RPI, Warning) << \"Flushing bayer stream!\";\n> +\t\t\t\twhile (!bayerQueue_.empty()) {\n> +\t\t\t\t\tunicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> +\t\t\t\t\tbayerQueue_.pop();\n> +\t\t\t\t}\n> +\t\t\t\treturn false;\n> +\t\t\t}\n> +\n> +\t\t\t/*\n> +\t\t\t * Not found a matching embedded buffer for the bayer buffer in\n> +\t\t\t * the front of the queue. This buffer is now orphaned, so requeue\n> +\t\t\t * it back to the device.\n> +\t\t\t */\n> +\t\t\tunicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> +\t\t\tbayerQueue_.pop();\n> +\t\t\tbayerRequeueCount++;\n> \t\t\tLOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n> -\t\t\t\t\t << stream->name();\n> -\t\t} else if (stream->isExternal() || b->metadata().timestamp == timestamp) {\n> -\t\t\t/* The calling function will pop the item from the queue. */\n> -\t\t\treturn b;\n> +\t\t\t\t\t << unicam_[Unicam::Image].name();\n> +\n> +\t\t\t/*\n> +\t\t\t * Similar to the above, if we have requeued all available bayer buffers in\n> +\t\t\t * the loop, then we are fully out of sync, so might as well requeue all the\n> +\t\t\t * pending embedded data buffers.\n> +\t\t\t */\n> +\t\t\tif (bayerRequeueCount == unicam_[Unicam::Image].getBuffers().size()) {\n> +\t\t\t\t/* The embedded queue must be empty at this point! */\n> +\t\t\t\tASSERT(bayerQueue_.empty());\n> +\n> +\t\t\t\tLOG(RPI, Warning) << \"Flushing embedded data stream!\";\n> +\t\t\t\twhile (!embeddedQueue_.empty()) {\n> +\t\t\t\t\tunicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> +\t\t\t\t\tembeddedQueue_.pop();\n> +\t\t\t\t}\n> +\t\t\t\treturn false;\n> +\t\t\t}\n> +\n> +\t\t\t/* If the embedded queue has become empty, we cannot do any more. */\n> +\t\t\tif (embeddedQueue_.empty())\n> +\t\t\t\treturn false;\n> \t\t} else {\n> -\t\t\tbreak; /* Only higher timestamps from here. */\n> +\t\t\t/*\n> +\t\t\t * We have found a matching bayer and embedded data buffer, so\n> +\t\t\t * nothing more to do apart from popping the buffers from the queue.\n> +\t\t\t */\n> +\t\t\tbayerQueue_.pop();\n> +\t\t\tembeddedQueue_.pop();\n> +\t\t\treturn true;\n> \t\t}\n> \t}\n> \n> -\treturn nullptr;\n> +\treturn false;\n\nThe logic is too complex for my taste, but not worse than it was :-) I'm\nsure we'll have opportunities to refactor all this in the future when\nintroducing new helpers.\n\nWith the above issues addressed if applicable,\n\nAcked-by: Laurent Pinchart \n\n> }\n> \n> REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1C27CBE081\n\tfor ;\n\tMon, 16 Nov 2020 15:14:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A299E632C3;\n\tMon, 16 Nov 2020 16:14:29 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 03730631B5\n\tfor ;\n\tMon, 16 Nov 2020 16:14:29 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 829EBA1B;\n\tMon, 16 Nov 2020 16:14:28 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"a5LHHDHP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1605539668;\n\tbh=jWgTXXOZjIueSQFjV+aEUmhPcQyPMBSxiy6vzRgylRo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=a5LHHDHPV7pHdayvY1Hn81wJfhRrVyy5TrDBwtnSEga6ULXpWvdp7HGBHGSNDM47r\n\tFN8Pt6cLx6tOPo4sfHV1TvRbEPXulF9NlPwxXkC86DGkavOAZHQgUeyWc1AzxcU90q\n\tWjzaH6BB+89vGD58w5t2uLPK2HxtqW2XUhE4knQk=","Date":"Mon, 16 Nov 2020 17:14:23 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20201116151423.GN6540@pendragon.ideasonboard.com>","References":"<20201116120256.149789-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201116120256.149789-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Rework\n\tbayer/embedded data buffer matching","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13730,"web_url":"https://patchwork.libcamera.org/comment/13730/","msgid":"","date":"2020-11-17T10:09:27","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Rework\n\tbayer/embedded data buffer matching","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your comments.\n\nOn Mon, 16 Nov 2020 at 15:14, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Nov 16, 2020 at 12:02:56PM +0000, Naushir Patuck wrote:\n> > There is a condition that would cause the buffers to never be in sync\n> > when we using only a single buffer in the bayer and embedded data\n> streams.\n> > This occurred because even though both streams would get flushed to\n> resync,\n> > one stream's only buffer was already queued in the device, and would end\n> > up never matching.\n> >\n> > Rework the buffer matching logic by combining updateQueue() and\n> > tryFlushQueue() into a single function findMatchingBuffers(). This would\n> > allow us to flush the queues at the same time as we match buffers,\n> avoiding\n> > the the above condition.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 176 +++++++++---------\n> > 1 file changed, 88 insertions(+), 88 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 7ad66f21..26c6731c 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -205,9 +205,7 @@ public:\n> > private:\n> > void checkRequestCompleted();\n> > void tryRunPipeline();\n> > - void tryFlushQueues();\n> > - FrameBuffer *updateQueue(std::queue &q, uint64_t\n> timestamp,\n> > - RPi::Stream *stream);\n> > + bool findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer\n> *&embeddedBuffer);\n> >\n> > unsigned int ispOutputCount_;\n> > };\n> > @@ -1537,7 +1535,6 @@ void RPiCameraData::handleState()\n> >\n> > case State::Idle:\n> > tryRunPipeline();\n> > - tryFlushQueues();\n> > break;\n> > }\n> > }\n> > @@ -1588,41 +1585,8 @@ void RPiCameraData::tryRunPipeline()\n> > bayerQueue_.empty() || embeddedQueue_.empty())\n> > return;\n> >\n> > - /* Start with the front of the bayer buffer queue. */\n> > - bayerBuffer = bayerQueue_.front();\n> > -\n> > - /*\n> > - * Find the embedded data buffer with a matching timestamp to pass\n> to\n> > - * the IPA. Any embedded buffers with a timestamp lower than the\n> > - * current bayer buffer will be removed and re-queued to the\n> driver.\n> > - */\n> > - embeddedBuffer = updateQueue(embeddedQueue_,\n> bayerBuffer->metadata().timestamp,\n> > - &unicam_[Unicam::Embedded]);\n> > -\n> > - if (!embeddedBuffer) {\n> > - LOG(RPI, Debug) << \"Could not find matching embedded\n> buffer\";\n> > -\n> > - /*\n> > - * Look the other way, try to match a bayer buffer with the\n> > - * first embedded buffer in the queue. This will also do\n> some\n> > - * housekeeping on the bayer image queue - clear out any\n> > - * buffers that are older than the first buffer in the\n> embedded\n> > - * queue.\n> > - *\n> > - * But first check if the embedded queue has emptied out.\n> > - */\n> > - if (embeddedQueue_.empty())\n> > - return;\n> > -\n> > - embeddedBuffer = embeddedQueue_.front();\n> > - bayerBuffer = updateQueue(bayerQueue_,\n> embeddedBuffer->metadata().timestamp,\n> > - &unicam_[Unicam::Image]);\n> > -\n> > - if (!bayerBuffer) {\n> > - LOG(RPI, Debug) << \"Could not find matching bayer\n> buffer - ending.\";\n> > - return;\n> > - }\n> > - }\n> > + if (!findMatchingBuffers(bayerBuffer, embeddedBuffer))\n> > + return;\n> >\n> > /* Take the first request from the queue and action the IPA. */\n> > Request *request = requestQueue_.front();\n> > @@ -1665,10 +1629,6 @@ void RPiCameraData::tryRunPipeline()\n> > op.controls = { request->controls() };\n> > ipa_->processEvent(op);\n> >\n> > - /* Ready to use the buffers, pop them off the queue. */\n> > - bayerQueue_.pop();\n> > - embeddedQueue_.pop();\n> > -\n> > /* Set our state to say the pipeline is active. */\n> > state_ = State::Busy;\n> >\n> > @@ -1685,62 +1645,102 @@ void RPiCameraData::tryRunPipeline()\n> > ipa_->processEvent(op);\n> > }\n> >\n> > -void RPiCameraData::tryFlushQueues()\n> > +bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer,\n> FrameBuffer *&embeddedBuffer)\n> > {\n> > - /*\n> > - * It is possible for us to end up in a situation where all\n> available\n> > - * Unicam buffers have been dequeued but do not match. This can\n> happen\n> > - * when the system is heavily loaded and we get out of lock-step\n> with\n> > - * the two channels.\n> > - *\n> > - * In such cases, the best thing to do is the re-queue all the\n> buffers\n> > - * and give a chance for the hardware to return to lock-step. We\n> do have\n> > - * to drop all interim frames.\n> > - */\n> > - if (unicam_[Unicam::Image].getBuffers().size() ==\n> bayerQueue_.size() &&\n> > - unicam_[Unicam::Embedded].getBuffers().size() ==\n> embeddedQueue_.size()) {\n> > - /* This cannot happen when Unicam streams are external. */\n> > - assert(!unicam_[Unicam::Image].isExternal());\n> > + unsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0;\n> >\n> > - LOG(RPI, Warning) << \"Flushing all buffer queues!\";\n> > -\n> > - while (!bayerQueue_.empty()) {\n> > -\n> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> > - bayerQueue_.pop();\n> > - }\n> > + /* Loop until we find a matching bayer and embedded data buffer. */\n> > + while (!bayerQueue_.empty()) {\n> > + /* Start with the front of the bayer queue. */\n> > + bayerBuffer = bayerQueue_.front();\n> >\n> > + /*\n> > + * Find the embedded data buffer with a matching timestamp\n> to pass to\n> > + * the IPA. Any embedded buffers with a timestamp lower\n> than the\n> > + * current bayer buffer will be removed and re-queued to\n> the driver.\n> > + */\n>\n> Missing space before *.\n>\n>\nOops, ack. Surprised the check patch script did not flag this one up.\n\n\n> > + uint64_t ts = bayerBuffer->metadata().timestamp;\n> > + embeddedBuffer = nullptr;\n> > while (!embeddedQueue_.empty()) {\n> > -\n> unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> > - embeddedQueue_.pop();\n> > + FrameBuffer *b = embeddedQueue_.front();\n> > + if (!unicam_[Unicam::Embedded].isExternal() &&\n> b->metadata().timestamp < ts) {\n> > + embeddedQueue_.pop();\n> > + unicam_[Unicam::Embedded].queueBuffer(b);\n> > + embeddedRequeueCount++;\n> > + LOG(RPI, Warning) << \"Dropping unmatched\n> input frame in stream \"\n> > + <<\n> unicam_[Unicam::Embedded].name();\n> > + } else if (unicam_[Unicam::Embedded].isExternal()\n> || b->metadata().timestamp == ts) {\n>\n> I wonder if we could use the sequence number here, as the timestamp\n> seems a bit fragile. I suppose the current unicam implementation\n> guarantees that the image and embedded data timestamps will be identical\n> if they refer to the same frame, but I'm concerned this could change in\n> the future. Is there a similar guarantee on the sequence number, or can\n> we only rely on the timestamp ?\n>\n\nUsing sequence numbers would work equally well. Both sequence number and\ntimestamps are guaranteed to be the same for the embedded + image buffer\npair. However, I would prefer to make that change separate to this patch\nif that is ok? Only because I've done exhaustive testing when using\ntimestamps and not sequence numbers.\n\n\n> > + /* The calling function will pop the item\n> from the queue. */\n>\n> Is it the calling function, or the code at the end of this function ?\n>\n\nSorry, this is an error in the comment. The calling function no longer\npops the item, we do it at the end of the function.\n\n\n>\n> > + embeddedBuffer = b;\n> > + break;\n> > + } else {\n> > + break; /* Only higher timestamps from\n> here. */\n> > + }\n> > }\n> > - }\n> > -}\n> >\n> > -FrameBuffer *RPiCameraData::updateQueue(std::queue &q,\n> uint64_t timestamp,\n> > - RPi::Stream *stream)\n> > -{\n> > - /*\n> > - * If the unicam streams are external (both have be to the same),\n> then we\n> > - * can only return out the top buffer in the queue, and assume\n> they have\n> > - * been synced by queuing at the same time. We cannot drop these\n> frames,\n> > - * as they may have been provided externally.\n> > - */\n> > - while (!q.empty()) {\n> > - FrameBuffer *b = q.front();\n> > - if (!stream->isExternal() && b->metadata().timestamp <\n> timestamp) {\n> > - q.pop();\n> > - stream->queueBuffer(b);\n> > + if (!embeddedBuffer) {\n> > + LOG(RPI, Debug) << \"Could not find matching\n> embedded buffer\";\n> > +\n> > + /*\n> > + * If we have requeued all available embedded data\n> buffers in this loop,\n> > + * then we are fully out of sync, so might as well\n> requeue all the pending\n> > + * bayer buffers.\n> > + */\n> > + if (embeddedRequeueCount ==\n> unicam_[Unicam::Embedded].getBuffers().size()) {\n> > + /* The embedded queue must be empty at\n> this point! */\n> > + ASSERT(embeddedQueue_.empty());\n> > +\n> > + LOG(RPI, Warning) << \"Flushing bayer\n> stream!\";\n> > + while (!bayerQueue_.empty()) {\n> > +\n> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> > + bayerQueue_.pop();\n> > + }\n> > + return false;\n> > + }\n> > +\n> > + /*\n> > + * Not found a matching embedded buffer for the\n> bayer buffer in\n> > + * the front of the queue. This buffer is now\n> orphaned, so requeue\n> > + * it back to the device.\n> > + */\n> > +\n> unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> > + bayerQueue_.pop();\n> > + bayerRequeueCount++;\n> > LOG(RPI, Warning) << \"Dropping unmatched input\n> frame in stream \"\n> > - << stream->name();\n> > - } else if (stream->isExternal() || b->metadata().timestamp\n> == timestamp) {\n> > - /* The calling function will pop the item from the\n> queue. */\n> > - return b;\n> > + << unicam_[Unicam::Image].name();\n> > +\n> > + /*\n> > + * Similar to the above, if we have requeued all\n> available bayer buffers in\n> > + * the loop, then we are fully out of sync, so\n> might as well requeue all the\n> > + * pending embedded data buffers.\n> > + */\n> > + if (bayerRequeueCount ==\n> unicam_[Unicam::Image].getBuffers().size()) {\n> > + /* The embedded queue must be empty at\n> this point! */\n> > + ASSERT(bayerQueue_.empty());\n> > +\n> > + LOG(RPI, Warning) << \"Flushing embedded\n> data stream!\";\n> > + while (!embeddedQueue_.empty()) {\n> > +\n> unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> > + embeddedQueue_.pop();\n> > + }\n> > + return false;\n> > + }\n> > +\n> > + /* If the embedded queue has become empty, we\n> cannot do any more. */\n> > + if (embeddedQueue_.empty())\n> > + return false;\n> > } else {\n> > - break; /* Only higher timestamps from here. */\n> > + /*\n> > + * We have found a matching bayer and embedded\n> data buffer, so\n> > + * nothing more to do apart from popping the\n> buffers from the queue.\n> > + */\n> > + bayerQueue_.pop();\n> > + embeddedQueue_.pop();\n> > + return true;\n> > }\n> > }\n> >\n> > - return nullptr;\n> > + return false;\n>\n> The logic is too complex for my taste, but not worse than it was :-) I'm\n> sure we'll have opportunities to refactor all this in the future when\n> introducing new helpers.\n>\n>\nIndeed it is a bit complicated, and I have tried to simplify it as much as\nI can, but this buffer matching/handling is notoriously difficult to handle\nall edge cases fully :-)\nYes, hopefully some of this might become libcamera helpers and simplify\nthings!\n\nRegards,\nNaush\n\n\nWith the above issues addressed if applicable,\n>\n> Acked-by: Laurent Pinchart \n>\n> > }\n> >\n> > REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7E726BE081\n\tfor ;\n\tTue, 17 Nov 2020 10:09:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 045D563323;\n\tTue, 17 Nov 2020 11:09:46 +0100 (CET)","from mail-lf1-x131.google.com (mail-lf1-x131.google.com\n\t[IPv6:2a00:1450:4864:20::131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9121163282\n\tfor ;\n\tTue, 17 Nov 2020 11:09:44 +0100 (CET)","by mail-lf1-x131.google.com with SMTP id u18so29272482lfd.9\n\tfor ;\n\tTue, 17 Nov 2020 02:09:44 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"kp0Q095B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=enkBu0xBfTk+qSlkp+mZWlt3ITK1WfcLQm+T0jlPa60=;\n\tb=kp0Q095BTOBSssAUGv7HQDvKaJp/DI9UABdWBIA4yKdcGZjVv2mXscht54ECkcxoKZ\n\tfUqrgAUkXGF3jiYQMzSx6uwni3ELNCCJkMvSKTCmlRwMzc2+yVunnT1Bc7NXxbYDBYCw\n\tSbvZBDoFrYxyE7YVhHCnLx1x2TNdiK3E34/99XXdTATCiGq4ARyiTbLE6jkmHhq/i5+M\n\tXcbMUm9u6mYtHeNHSUvLJSPx4x/l2P+cXRdCgn13IgMlRFgiQSRqyXT0mQefryjncgme\n\tg0Jtr7EAlbjJklzwEDz+X+h8KEGh4BwyOrGpiNKfEp0qLP8Pf6BFeuWobKZ05zYc0WQY\n\thvcw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=enkBu0xBfTk+qSlkp+mZWlt3ITK1WfcLQm+T0jlPa60=;\n\tb=rCzQZI/E5+wbNqVZi4gDMoMaljURVSXvcwi+0MTCEP0zyjzeB9+diuixF+CmOLjKi7\n\tcu4er2datLy3qG/nbks8N1vh1K9C1s+k/RocSa6bDvIIYNubWo9Wl249U8EqzIYE4Xx2\n\txl7ihOkY6GSsuRqcOmKlE9YsbPcVJyibUpq2UkqpO74idd4o+MC82AThoIy4M+yzCKrT\n\tyszySSgqBoSQcbtMg69QieFNhQj+Mu3cLgyw39Sl2ZfY0sxB5IhUz8mYhNSo/PK1FGR4\n\t3X5/z0DEt+DvI+1gqMb+FHaYaZPaWo+R0CzQKhCBS6wbs7Dpe/tVKKNZBr1f9GT1iGbD\n\tqk5w==","X-Gm-Message-State":"AOAM531x2v2z55hTaBHFm+ScFvrDS53XMGJzhue4WgAfHBUKivN4Nr82\n\tJezhAMtsjM1VhzA6C6QQnh2uFFSyQOjS3OHgXhG7Ny9qGOU=","X-Google-Smtp-Source":"ABdhPJxvSgH6e9s1tupQJnoRbbRPwOlvxOubtDavXC3XSxvRKsBX+4B5qlMzG0FpLcg5ipzPZrHQsmvvlTBH6POHISA=","X-Received":"by 2002:a05:6512:4c5:: with SMTP id\n\tw5mr1435247lfq.215.1605607783745; \n\tTue, 17 Nov 2020 02:09:43 -0800 (PST)","MIME-Version":"1.0","References":"<20201116120256.149789-1-naush@raspberrypi.com>\n\t<20201116151423.GN6540@pendragon.ideasonboard.com>","In-Reply-To":"<20201116151423.GN6540@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Tue, 17 Nov 2020 10:09:27 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Rework\n\tbayer/embedded data buffer matching","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"multipart/mixed;\n\tboundary=\"===============4562847223727122828==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13746,"web_url":"https://patchwork.libcamera.org/comment/13746/","msgid":"<20201117122139.GD3940@pendragon.ideasonboard.com>","date":"2020-11-17T12:21:39","subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Rework\n\tbayer/embedded data buffer matching","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Nov 17, 2020 at 10:09:27AM +0000, Naushir Patuck wrote:\n> On Mon, 16 Nov 2020 at 15:14, Laurent Pinchart wrote:\n> > On Mon, Nov 16, 2020 at 12:02:56PM +0000, Naushir Patuck wrote:\n> > > There is a condition that would cause the buffers to never be in sync\n> > > when we using only a single buffer in the bayer and embedded datastreams.\n> > > This occurred because even though both streams would get flushed toresync,\n> > > one stream's only buffer was already queued in the device, and would end\n> > > up never matching.\n> > >\n> > > Rework the buffer matching logic by combining updateQueue() and\n> > > tryFlushQueue() into a single function findMatchingBuffers(). This would\n> > > allow us to flush the queues at the same time as we match buffers,avoiding\n> > > the the above condition.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 176 +++++++++---------\n> > > 1 file changed, 88 insertions(+), 88 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cppb/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 7ad66f21..26c6731c 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -205,9 +205,7 @@ public:\n> > > private:\n> > > void checkRequestCompleted();\n> > > void tryRunPipeline();\n> > > - void tryFlushQueues();\n> > > - FrameBuffer *updateQueue(std::queue &q, uint64_ttimestamp,\n> > > - RPi::Stream *stream);\n> > > + bool findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer*&embeddedBuffer);\n> > >\n> > > unsigned int ispOutputCount_;\n> > > };\n> > > @@ -1537,7 +1535,6 @@ void RPiCameraData::handleState()\n> > >\n> > > case State::Idle:\n> > > tryRunPipeline();\n> > > - tryFlushQueues();\n> > > break;\n> > > }\n> > > }\n> > > @@ -1588,41 +1585,8 @@ void RPiCameraData::tryRunPipeline()\n> > > bayerQueue_.empty() || embeddedQueue_.empty())\n> > > return;\n> > >\n> > > - /* Start with the front of the bayer buffer queue. */\n> > > - bayerBuffer = bayerQueue_.front();\n> > > -\n> > > - /*\n> > > - * Find the embedded data buffer with a matching timestamp to passto\n> > > - * the IPA. Any embedded buffers with a timestamp lower than the\n> > > - * current bayer buffer will be removed and re-queued to thedriver.\n> > > - */\n> > > - embeddedBuffer = updateQueue(embeddedQueue_,bayerBuffer->metadata().timestamp,\n> > > - &unicam_[Unicam::Embedded]);\n> > > -\n> > > - if (!embeddedBuffer) {\n> > > - LOG(RPI, Debug) << \"Could not find matching embeddedbuffer\";\n> > > -\n> > > - /*\n> > > - * Look the other way, try to match a bayer buffer with the\n> > > - * first embedded buffer in the queue. This will also dosome\n> > > - * housekeeping on the bayer image queue - clear out any\n> > > - * buffers that are older than the first buffer in theembedded\n> > > - * queue.\n> > > - *\n> > > - * But first check if the embedded queue has emptied out.\n> > > - */\n> > > - if (embeddedQueue_.empty())\n> > > - return;\n> > > -\n> > > - embeddedBuffer = embeddedQueue_.front();\n> > > - bayerBuffer = updateQueue(bayerQueue_,embeddedBuffer->metadata().timestamp,\n> > > - &unicam_[Unicam::Image]);\n> > > -\n> > > - if (!bayerBuffer) {\n> > > - LOG(RPI, Debug) << \"Could not find matching bayerbuffer - ending.\";\n> > > - return;\n> > > - }\n> > > - }\n> > > + if (!findMatchingBuffers(bayerBuffer, embeddedBuffer))\n> > > + return;\n> > >\n> > > /* Take the first request from the queue and action the IPA. */\n> > > Request *request = requestQueue_.front();\n> > > @@ -1665,10 +1629,6 @@ void RPiCameraData::tryRunPipeline()\n> > > op.controls = { request->controls() };\n> > > ipa_->processEvent(op);\n> > >\n> > > - /* Ready to use the buffers, pop them off the queue. */\n> > > - bayerQueue_.pop();\n> > > - embeddedQueue_.pop();\n> > > -\n> > > /* Set our state to say the pipeline is active. */\n> > > state_ = State::Busy;\n> > >\n> > > @@ -1685,62 +1645,102 @@ void RPiCameraData::tryRunPipeline()\n> > > ipa_->processEvent(op);\n> > > }\n> > >\n> > > -void RPiCameraData::tryFlushQueues()\n> > > +bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer,FrameBuffer *&embeddedBuffer)\n> > > {\n> > > - /*\n> > > - * It is possible for us to end up in a situation where allavailable\n> > > - * Unicam buffers have been dequeued but do not match. This canhappen\n> > > - * when the system is heavily loaded and we get out of lock-stepwith\n> > > - * the two channels.\n> > > - *\n> > > - * In such cases, the best thing to do is the re-queue all thebuffers\n> > > - * and give a chance for the hardware to return to lock-step. Wedo have\n> > > - * to drop all interim frames.\n> > > - */\n> > > - if (unicam_[Unicam::Image].getBuffers().size() ==bayerQueue_.size() &&\n> > > - unicam_[Unicam::Embedded].getBuffers().size() ==embeddedQueue_.size()) {\n> > > - /* This cannot happen when Unicam streams are external. */\n> > > - assert(!unicam_[Unicam::Image].isExternal());\n> > > + unsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0;\n> > >\n> > > - LOG(RPI, Warning) << \"Flushing all buffer queues!\";\n> > > -\n> > > - while (!bayerQueue_.empty()) {\n> > > - unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> > > - bayerQueue_.pop();\n> > > - }\n> > > + /* Loop until we find a matching bayer and embedded data buffer. */\n> > > + while (!bayerQueue_.empty()) {\n> > > + /* Start with the front of the bayer queue. */\n> > > + bayerBuffer = bayerQueue_.front();\n> > >\n> > > + /*\n> > > + * Find the embedded data buffer with a matching timestampto pass to\n> > > + * the IPA. Any embedded buffers with a timestamp lowerthan the\n> > > + * current bayer buffer will be removed and re-queued tothe driver.\n> > > + */\n> >\n> > Missing space before *.\n> \n> Oops, ack. Surprised the check patch script did not flag this one up.\n> \n> > > + uint64_t ts = bayerBuffer->metadata().timestamp;\n> > > + embeddedBuffer = nullptr;\n> > > while (!embeddedQueue_.empty()) {\n> > > - unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> > > - embeddedQueue_.pop();\n> > > + FrameBuffer *b = embeddedQueue_.front();\n> > > + if (!unicam_[Unicam::Embedded].isExternal() &&b->metadata().timestamp < ts) {\n> > > + embeddedQueue_.pop();\n> > > + unicam_[Unicam::Embedded].queueBuffer(b);\n> > > + embeddedRequeueCount++;\n> > > + LOG(RPI, Warning) << \"Dropping unmatchedinput frame in stream \"\n> > > + < > > + } else if (unicam_[Unicam::Embedded].isExternal()|| b->metadata().timestamp == ts) {\n> >\n> > I wonder if we could use the sequence number here, as the timestamp\n> > seems a bit fragile. I suppose the current unicam implementation\n> > guarantees that the image and embedded data timestamps will be identical\n> > if they refer to the same frame, but I'm concerned this could change in\n> > the future. Is there a similar guarantee on the sequence number, or can\n> > we only rely on the timestamp ?\n> \n> Using sequence numbers would work equally well. Both sequence number and\n> timestamps are guaranteed to be the same for the embedded + image buffer\n> pair. However, I would prefer to make that change separate to this patch\n> if that is ok? Only because I've done exhaustive testing when using\n> timestamps and not sequence numbers.\n\nSure, no issue.\n\n> > > + /* The calling function will pop the itemfrom the queue. */\n> >\n> > Is it the calling function, or the code at the end of this function ?\n> \n> Sorry, this is an error in the comment. The calling function no longer\n> pops the item, we do it at the end of the function.\n> \n> > > + embeddedBuffer = b;\n> > > + break;\n> > > + } else {\n> > > + break; /* Only higher timestamps fromhere. */\n> > > + }\n> > > }\n> > > - }\n> > > -}\n> > >\n> > > -FrameBuffer *RPiCameraData::updateQueue(std::queue &q,uint64_t timestamp,\n> > > - RPi::Stream *stream)\n> > > -{\n> > > - /*\n> > > - * If the unicam streams are external (both have be to the same),then we\n> > > - * can only return out the top buffer in the queue, and assumethey have\n> > > - * been synced by queuing at the same time. We cannot drop theseframes,\n> > > - * as they may have been provided externally.\n> > > - */\n> > > - while (!q.empty()) {\n> > > - FrameBuffer *b = q.front();\n> > > - if (!stream->isExternal() && b->metadata().timestamp > > - q.pop();\n> > > - stream->queueBuffer(b);\n> > > + if (!embeddedBuffer) {\n> > > + LOG(RPI, Debug) << \"Could not find matchingembedded buffer\";\n> > > +\n> > > + /*\n> > > + * If we have requeued all available embedded databuffers in this loop,\n> > > + * then we are fully out of sync, so might as wellrequeue all the pending\n> > > + * bayer buffers.\n> > > + */\n> > > + if (embeddedRequeueCount ==unicam_[Unicam::Embedded].getBuffers().size()) {\n> > > + /* The embedded queue must be empty atthis point! */\n> > > + ASSERT(embeddedQueue_.empty());\n> > > +\n> > > + LOG(RPI, Warning) << \"Flushing bayerstream!\";\n> > > + while (!bayerQueue_.empty()) {\n> > > + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> > > + bayerQueue_.pop();\n> > > + }\n> > > + return false;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Not found a matching embedded buffer for thebayer buffer in\n> > > + * the front of the queue. This buffer is noworphaned, so requeue\n> > > + * it back to the device.\n> > > + */\n> > > + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n> > > + bayerQueue_.pop();\n> > > + bayerRequeueCount++;\n> > > LOG(RPI, Warning) << \"Dropping unmatched inputframe in stream \"\n> > > - << stream->name();\n> > > - } else if (stream->isExternal() || b->metadata().timestamp== timestamp) {\n> > > - /* The calling function will pop the item from thequeue. */\n> > > - return b;\n> > > + << unicam_[Unicam::Image].name();\n> > > +\n> > > + /*\n> > > + * Similar to the above, if we have requeued allavailable bayer buffers in\n> > > + * the loop, then we are fully out of sync, somight as well requeue all the\n> > > + * pending embedded data buffers.\n> > > + */\n> > > + if (bayerRequeueCount ==unicam_[Unicam::Image].getBuffers().size()) {\n> > > + /* The embedded queue must be empty atthis point! */\n> > > + ASSERT(bayerQueue_.empty());\n> > > +\n> > > + LOG(RPI, Warning) << \"Flushing embeddeddata stream!\";\n> > > + while (!embeddedQueue_.empty()) {\n> > > + unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n> > > + embeddedQueue_.pop();\n> > > + }\n> > > + return false;\n> > > + }\n> > > +\n> > > + /* If the embedded queue has become empty, wecannot do any more. */\n> > > + if (embeddedQueue_.empty())\n> > > + return false;\n> > > } else {\n> > > - break; /* Only higher timestamps from here. */\n> > > + /*\n> > > + * We have found a matching bayer and embeddeddata buffer, so\n> > > + * nothing more to do apart from popping thebuffers from the queue.\n> > > + */\n> > > + bayerQueue_.pop();\n> > > + embeddedQueue_.pop();\n> > > + return true;\n> > > }\n> > > }\n> > >\n> > > - return nullptr;\n> > > + return false;\n> >\n> > The logic is too complex for my taste, but not worse than it was :-) I'm\n> > sure we'll have opportunities to refactor all this in the future when\n> > introducing new helpers.\n>\n> Indeed it is a bit complicated, and I have tried to simplify it as much as\n> I can, but this buffer matching/handling is notoriously difficult to handle\n> all edge cases fully :-)\n> Yes, hopefully some of this might become libcamera helpers and simplify\n> things!\n> \n> > With the above issues addressed if applicable,\n> >\n> > Acked-by: Laurent Pinchart \n> >\n> > > }\n> > >\n> > > REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7E5E0BE081\n\tfor ;\n\tTue, 17 Nov 2020 12:21:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 15FF063321;\n\tTue, 17 Nov 2020 13:21:46 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6AF176331F\n\tfor ;\n\tTue, 17 Nov 2020 13:21:44 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CFA8A2A3;\n\tTue, 17 Nov 2020 13:21:43 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pcu+ASn8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1605615704;\n\tbh=KLRBPThajJBlap0lhdW4tLI9HaOdx13xRH/12s4ZmRs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pcu+ASn8M2cYYIzmterz65hOGa/lJMmuoyc9BfWgAvh1m38DcE51Rw42TDUKojbgO\n\tB0F4CPYgCfWACqaFnryIJafOr9vhj7kc1ZoBPwyQqZrlP3oaZLfgm7rC4M1wSCiGDS\n\t1tR2RdVWhxPMVMfP4stB8e5w5j5VyhUFkB/b7BaI=","Date":"Tue, 17 Nov 2020 14:21:39 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20201117122139.GD3940@pendragon.ideasonboard.com>","References":"<20201116120256.149789-1-naush@raspberrypi.com>\n\t<20201116151423.GN6540@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: raspberrypi: Rework\n\tbayer/embedded data buffer matching","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]