[{"id":11453,"web_url":"https://patchwork.libcamera.org/comment/11453/","msgid":"","date":"2020-07-21T11:23:41","subject":"Re: [libcamera-devel] [PATCH v4 8/9] libcamera: pipeline:\n\traspberrypi: Add more robust stream buffer logic","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 20/07/2020 10:13, Naushir Patuck wrote:\n> Add further queueing into the RPiStream object to ensure that we always\n> follow the buffer ordering (be it internal or external) given by\n> incoming Requests.\n> \n> This is essential, otherwise we risk dropping frames that are meant to\n> be part of a Request, and can cause the pipeline to stall indefinitely.\n> This also prevents any possibility of mismatched frame buffers going\n> through the pipeline and out to the application.\n> \n\nSounds helpful... some questions below ... but I suspect I've just\nmisinterpreted something...\n\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: Niklas Söderlund \n> ---\n> .../pipeline/raspberrypi/rpi_stream.cpp | 70 ++++++++++++++++---\n> .../pipeline/raspberrypi/rpi_stream.h | 12 +++-\n> 2 files changed, 71 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> index 02f8d3e0..6635a751 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> @@ -112,25 +112,34 @@ int RPiStream::queueBuffer(FrameBuffer *buffer)\n> \t */\n> \tif (!buffer) {\n> \t\tif (availableBuffers_.empty()) {\n> -\t\t\tLOG(RPISTREAM, Warning) << \"No buffers available for \"\n> +\t\t\tLOG(RPISTREAM, Info) << \"No buffers available for \"\n> \t\t\t\t\t\t<< name_;\n> -\t\t\treturn -EINVAL;\n> +\t\t\t/*\n> +\t\t\t * Note that we need to requeue an internal buffer as soon\n> +\t\t\t * as one becomes available.\n> +\t\t\t */\n> +\t\t\trequeueBuffers_.push(nullptr);\n> +\t\t\treturn 0;\n> \t\t}\n> \n> \t\tbuffer = availableBuffers_.front();\n> \t\tavailableBuffers_.pop();\n> \t}\n> \n> -\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> -\t\t\t << \" for \" << name_;\n> +\t/*\n> +\t * If no earlier requests are pending to be queued we can go ahead and\n> +\t * queue the buffer into the device.\n> +\t */\n> +\tif (requeueBuffers_.empty())\n> +\t\treturn queueToDevice(buffer);\n> \n> -\tint ret = dev_->queueBuffer(buffer);\n> -\tif (ret) {\n> -\t\tLOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> -\t\t\t\t << name_;\n> -\t}\n> +\t/*\n> +\t * There are earlier buffers to be queued, so this bufferm ust go on\n\n\ns/bufferm ust/buffer must/\n\nThat could be fixed up while applying. It's minor.\n\n\n\n> +\t * the waiting list.\n> +\t */\n> +\trequeueBuffers_.push(buffer);\n> \n> -\treturn ret;\n> +\treturn 0;\n> }\n> \n> void RPiStream::returnBuffer(FrameBuffer *buffer)\n> @@ -138,7 +147,35 @@ void RPiStream::returnBuffer(FrameBuffer *buffer)\n> \t/* This can only be called for external streams. */\n> \tassert(external_);\n> \n> +\t/* Push this buffer back into the queue to be used again. */\n> \tavailableBuffers_.push(buffer);\n> +\n> +\t/*\n> +\t * Do we have any buffers that are waiting to be queued?\n> +\t * If so, do it now as availableBuffers_ will not be empty.\n> +\t */\n> +\twhile (!requeueBuffers_.empty()) {\n> +\t\tFrameBuffer *buffer = requeueBuffers_.front();\n> +\n> +\t\tif (!buffer) {\n> +\t\t\t/*\n> +\t\t\t * We want to queue an internal buffer, but none\n> +\t\t\t * are available. Can't do anything, quit the loop.\n> +\t\t\t */\n> +\t\t\tif (availableBuffers_.empty())\n> +\t\t\t\tbreak;\n\nCan this happen?\n\nThis looks like:\n\nwhile(requeueBuffer exist)\n\tbuffer = first buffer from requeueBuffers\n\tif (no buffer) {\n\t\t/* unreachable code ? */\n\t}\n\n\n\n> +\n> +\t\t\t/*\n> +\t\t\t * We want to queue an internal buffer, and at least one\n> +\t\t\t * is available.\n> +\t\t\t */\n> +\t\t\tbuffer = availableBuffers_.front();\n> +\t\t\tavailableBuffers_.pop();\n> +\t\t}\n> +\n> +\t\trequeueBuffers_.pop();\n> +\t\tqueueToDevice(buffer);\n> +\t}\n> }\n> \n> bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> @@ -155,10 +192,23 @@ bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> void RPiStream::clearBuffers()\n> {\n> \tavailableBuffers_ = std::queue{};\n> +\trequeueBuffers_ = std::queue{};\n> \tinternalBuffers_.clear();\n> \tbufferList_.clear();\n> }\n> \n> +int RPiStream::queueToDevice(FrameBuffer *buffer)\n> +{\n> +\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> +\t\t\t << \" for \" << name_;\n> +\n> +\tint ret = dev_->queueBuffer(buffer);\n> +\tif (ret)\n> +\t\tLOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> +\t\t\t\t << name_;\n> +\treturn ret;\n> +}\n> +\n> } /* namespace RPi */\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> index af9c2ad2..6222c867 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> @@ -52,6 +52,7 @@ public:\n> \n> private:\n> \tvoid clearBuffers();\n> +\tint queueToDevice(FrameBuffer *buffer);\n> \t/*\n> \t * Indicates that this stream is active externally, i.e. the buffers\n> \t * might be provided by (and returned to) the application.\n> @@ -63,7 +64,7 @@ private:\n> \tstd::string name_;\n> \t/* The actual device stream. */\n> \tstd::unique_ptr dev_;\n> -\t/* All framebuffers associated with this device stream. */\n> +\t/* All frame buffers associated with this device stream. */\n> \tstd::vector bufferList_;\n> \t/*\n> \t * List of frame buffer that we can use if none have been provided by\n> @@ -71,6 +72,15 @@ private:\n> \t * buffers exported internally.\n> \t */\n> \tstd::queue availableBuffers_;\n> +\t/*\n> +\t * List of frame buffer that are to be re-queued into the device.\n\n\"List of frame buffers ...\"\n\n> +\t * A nullptr indicates any internal buffer can be used (from availableBuffers_),\n> +\t * whereas a valid pointer indicates an external buffer to be queued.\n\nOh ... are we pushing nullptrs on to the queues? ... perhaps that\nanswers my unreachable code question above ...\n\n\n> +\t *\n> +\t * Ordering buffers to be queued is important here as it must match the\n> +\t * requests coming from the application.\n> +\t */\n> +\tstd::queue requeueBuffers_;\n> \t/*\n> \t * This is a list of buffers exported internally. Need to keep this around\n> \t * as the stream needs to maintain ownership of these buffers.\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 72EE6C0109\n\tfor ;\n\tTue, 21 Jul 2020 11:23:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CFEC605B2;\n\tTue, 21 Jul 2020 13:23:46 +0200 (CEST)","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 F3D7360496\n\tfor ;\n\tTue, 21 Jul 2020 13:23:44 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 84C5E51A;\n\tTue, 21 Jul 2020 13:23:44 +0200 (CEST)"],"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=\"vA4cozcc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595330624;\n\tbh=ypmrAbRMRsmBqvo4/rRXSgS5DTdVjYsi+pWyG/XtUgY=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=vA4cozccsj6NV7pqmzUtjDaGPVk09GGfvonX13Qd16d1KDM1PVxiWe176XElHQ2f7\n\tGHcyJve1oBXhi44zZAsq6uoFlGYPNBGKwW7ubLvg8MRyWLXF1laOa7twow3BKfwgPg\n\tLs9fOF34Keup1wIQdn1caNvG9UyrAcUVaoTyzAzc=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200720091311.805092-1-naush@raspberrypi.com>\n\t<20200720091311.805092-9-naush@raspberrypi.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Tue, 21 Jul 2020 12:23:41 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200720091311.805092-9-naush@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v4 8/9] libcamera: pipeline:\n\traspberrypi: Add more robust stream buffer logic","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11462,"web_url":"https://patchwork.libcamera.org/comment/11462/","msgid":"","date":"2020-07-21T12:35:51","subject":"Re: [libcamera-devel] [PATCH v4 8/9] libcamera: pipeline:\n\traspberrypi: Add more robust stream buffer logic","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nOn Tue, 21 Jul 2020 at 12:23, Kieran Bingham\n wrote:\n>\n> Hi Naush,\n>\n> On 20/07/2020 10:13, Naushir Patuck wrote:\n> > Add further queueing into the RPiStream object to ensure that we always\n> > follow the buffer ordering (be it internal or external) given by\n> > incoming Requests.\n> >\n> > This is essential, otherwise we risk dropping frames that are meant to\n> > be part of a Request, and can cause the pipeline to stall indefinitely.\n> > This also prevents any possibility of mismatched frame buffers going\n> > through the pipeline and out to the application.\n> >\n>\n> Sounds helpful... some questions below ... but I suspect I've just\n> misinterpreted something...\n>\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: Niklas Söderlund \n> > ---\n> > .../pipeline/raspberrypi/rpi_stream.cpp | 70 ++++++++++++++++---\n> > .../pipeline/raspberrypi/rpi_stream.h | 12 +++-\n> > 2 files changed, 71 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > index 02f8d3e0..6635a751 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > @@ -112,25 +112,34 @@ int RPiStream::queueBuffer(FrameBuffer *buffer)\n> > */\n> > if (!buffer) {\n> > if (availableBuffers_.empty()) {\n> > - LOG(RPISTREAM, Warning) << \"No buffers available for \"\n> > + LOG(RPISTREAM, Info) << \"No buffers available for \"\n> > << name_;\n> > - return -EINVAL;\n> > + /*\n> > + * Note that we need to requeue an internal buffer as soon\n> > + * as one becomes available.\n> > + */\n> > + requeueBuffers_.push(nullptr);\n> > + return 0;\n> > }\n> >\n> > buffer = availableBuffers_.front();\n> > availableBuffers_.pop();\n> > }\n> >\n> > - LOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> > - << \" for \" << name_;\n> > + /*\n> > + * If no earlier requests are pending to be queued we can go ahead and\n> > + * queue the buffer into the device.\n> > + */\n> > + if (requeueBuffers_.empty())\n> > + return queueToDevice(buffer);\n> >\n> > - int ret = dev_->queueBuffer(buffer);\n> > - if (ret) {\n> > - LOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> > - << name_;\n> > - }\n> > + /*\n> > + * There are earlier buffers to be queued, so this bufferm ust go on\n>\n>\n> s/bufferm ust/buffer must/\n>\n> That could be fixed up while applying. It's minor.\n>\n>\n>\n> > + * the waiting list.\n> > + */\n> > + requeueBuffers_.push(buffer);\n> >\n> > - return ret;\n> > + return 0;\n> > }\n> >\n> > void RPiStream::returnBuffer(FrameBuffer *buffer)\n> > @@ -138,7 +147,35 @@ void RPiStream::returnBuffer(FrameBuffer *buffer)\n> > /* This can only be called for external streams. */\n> > assert(external_);\n> >\n> > + /* Push this buffer back into the queue to be used again. */\n> > availableBuffers_.push(buffer);\n> > +\n> > + /*\n> > + * Do we have any buffers that are waiting to be queued?\n> > + * If so, do it now as availableBuffers_ will not be empty.\n> > + */\n> > + while (!requeueBuffers_.empty()) {\n> > + FrameBuffer *buffer = requeueBuffers_.front();\n> > +\n> > + if (!buffer) {\n> > + /*\n> > + * We want to queue an internal buffer, but none\n> > + * are available. Can't do anything, quit the loop.\n> > + */\n> > + if (availableBuffers_.empty())\n> > + break;\n>\n> Can this happen?\n>\n> This looks like:\n>\n> while(requeueBuffer exist)\n> buffer = first buffer from requeueBuffers\n> if (no buffer) {\n> /* unreachable code ? */\n> }\n>\n\nYes it is possible, having a nullptr element in requeueBuffers_ means\na Request did not provide a buffer for that stream, so we have to use\na buffer from availableBuffers_ instead. This buffer will not be\nreturned out to the applications. Hope that makes sense.\n\nRegards,\nNaush\n\n\n>\n>\n> > +\n> > + /*\n> > + * We want to queue an internal buffer, and at least one\n> > + * is available.\n> > + */\n> > + buffer = availableBuffers_.front();\n> > + availableBuffers_.pop();\n> > + }\n> > +\n> > + requeueBuffers_.pop();\n> > + queueToDevice(buffer);\n> > + }\n> > }\n> >\n> > bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> > @@ -155,10 +192,23 @@ bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> > void RPiStream::clearBuffers()\n> > {\n> > availableBuffers_ = std::queue{};\n> > + requeueBuffers_ = std::queue{};\n> > internalBuffers_.clear();\n> > bufferList_.clear();\n> > }\n> >\n> > +int RPiStream::queueToDevice(FrameBuffer *buffer)\n> > +{\n> > + LOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> > + << \" for \" << name_;\n> > +\n> > + int ret = dev_->queueBuffer(buffer);\n> > + if (ret)\n> > + LOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> > + << name_;\n> > + return ret;\n> > +}\n> > +\n> > } /* namespace RPi */\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > index af9c2ad2..6222c867 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > @@ -52,6 +52,7 @@ public:\n> >\n> > private:\n> > void clearBuffers();\n> > + int queueToDevice(FrameBuffer *buffer);\n> > /*\n> > * Indicates that this stream is active externally, i.e. the buffers\n> > * might be provided by (and returned to) the application.\n> > @@ -63,7 +64,7 @@ private:\n> > std::string name_;\n> > /* The actual device stream. */\n> > std::unique_ptr dev_;\n> > - /* All framebuffers associated with this device stream. */\n> > + /* All frame buffers associated with this device stream. */\n> > std::vector bufferList_;\n> > /*\n> > * List of frame buffer that we can use if none have been provided by\n> > @@ -71,6 +72,15 @@ private:\n> > * buffers exported internally.\n> > */\n> > std::queue availableBuffers_;\n> > + /*\n> > + * List of frame buffer that are to be re-queued into the device.\n>\n> \"List of frame buffers ...\"\n>\n> > + * A nullptr indicates any internal buffer can be used (from availableBuffers_),\n> > + * whereas a valid pointer indicates an external buffer to be queued.\n>\n> Oh ... are we pushing nullptrs on to the queues? ... perhaps that\n> answers my unreachable code question above ...\n>\n>\n> > + *\n> > + * Ordering buffers to be queued is important here as it must match the\n> > + * requests coming from the application.\n> > + */\n> > + std::queue requeueBuffers_;\n> > /*\n> > * This is a list of buffers exported internally. Need to keep this around\n> > * as the stream needs to maintain ownership of these buffers.\n> >\n>\n> --\n> Regards\n> --\n> Kieran","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 E47CFC0109\n\tfor ;\n\tTue, 21 Jul 2020 12:36:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4BF7D60868;\n\tTue, 21 Jul 2020 14:36:10 +0200 (CEST)","from mail-lj1-x236.google.com (mail-lj1-x236.google.com\n\t[IPv6:2a00:1450:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 749066053C\n\tfor ;\n\tTue, 21 Jul 2020 14:36:08 +0200 (CEST)","by mail-lj1-x236.google.com with SMTP id s9so23859163ljm.11\n\tfor ;\n\tTue, 21 Jul 2020 05:36:08 -0700 (PDT)"],"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=\"CoNVYHsL\"; 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:content-transfer-encoding;\n\tbh=a+nhtDmlsRe8K7kd+0Vhv9jK3UXYah9pXhjhRqJZTMo=;\n\tb=CoNVYHsLENKiCSqSXTahXHZ25Qna8zr5mqkIXGEowz48pJRAj6PaArAS3fkMxBl5VJ\n\teXfB/eCzzcktRIJyI8tERFoaWO1ZslKOSDMrpN6Oxo9uhYtlAz07LEamfNQPr0wFaR+m\n\tu+VVwwRuXdEcT6vdczGzm0pNtFk82T56M3RVtsBg0tDk0koNB1n38k5TMzrkr54IKisa\n\t5CSActDPmdfU/ItlmKLOS81KgkqtimADPOqvtYAEuq1TLeeKyrfs1cbfLggTeDqEvWvm\n\trruU63g4vaNl8SbcptESxcGCsQlUx7nUivJ4VICEHETPIuFfp8YMSjXJTLy/limSYek2\n\tN2Ug==","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:content-transfer-encoding;\n\tbh=a+nhtDmlsRe8K7kd+0Vhv9jK3UXYah9pXhjhRqJZTMo=;\n\tb=RF7DWkGet7WOc9gCN+++Gpz9KX41OtdWnZ5upEUN5Ioao4mDSInpUE83mwXpUUVK9z\n\tThBH3fOLUnfznCMAWQdI4pWrWrKsMjxrdmEOW2VpmoqfRVyQ+b4M3bZW5g9XaltVVxRF\n\tyu3vbm9H2VTOzW6KH4MEZReruj2B1pkU34EAJCX2MpGRSr4bMvmQg1b5O+/D+xCEvOah\n\tKPEo2mHqSDBqBcUxWtpNJ5G1CKMOOTAcMRGpY8e/oFxgeosQUw+Ghk7vNu8g375/rFVH\n\tsR44ZbBsNN7fyk+UKsabdSi7Lgr6DticU1fdWN3ImnXPyLQRrkWQ25FFrA+c90sXZmdJ\n\tDMXw==","X-Gm-Message-State":"AOAM5314vai8/DF4OyuMHt6jRdAInZSpeBoqJMWmXXSTlfoLIuK+JnkL\n\tTqN4iQaC1MSVoVn8joui/t4NbBQ5jtOc3Y78mVZuKDA5+xZIew==","X-Google-Smtp-Source":"ABdhPJyxIzjIcUiCYImifAB2q7KLgu3lNNY9vGiM62N7JHyth/uwZdhwCSDUYCyvn+BW3bLUTcsVd6i4niSKWM6WQy0=","X-Received":"by 2002:a2e:b042:: with SMTP id\n\td2mr13155904ljl.252.1595334967693; \n\tTue, 21 Jul 2020 05:36:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20200720091311.805092-1-naush@raspberrypi.com>\n\t<20200720091311.805092-9-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 21 Jul 2020 13:35:51 +0100","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v4 8/9] libcamera: pipeline:\n\traspberrypi: Add more robust stream buffer logic","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11509,"web_url":"https://patchwork.libcamera.org/comment/11509/","msgid":"<20200722154248.GJ29813@pendragon.ideasonboard.com>","date":"2020-07-22T15:42:48","subject":"Re: [libcamera-devel] [PATCH v4 8/9] libcamera: pipeline:\n\traspberrypi: Add more robust stream buffer logic","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 Tue, Jul 21, 2020 at 01:35:51PM +0100, Naushir Patuck wrote:\n> On Tue, 21 Jul 2020 at 12:23, Kieran Bingham wrote:\n> > On 20/07/2020 10:13, Naushir Patuck wrote:\n> > > Add further queueing into the RPiStream object to ensure that we always\n> > > follow the buffer ordering (be it internal or external) given by\n> > > incoming Requests.\n> > >\n> > > This is essential, otherwise we risk dropping frames that are meant to\n> > > be part of a Request, and can cause the pipeline to stall indefinitely.\n> > > This also prevents any possibility of mismatched frame buffers going\n> > > through the pipeline and out to the application.\n> >\n> > Sounds helpful... some questions below ... but I suspect I've just\n> > misinterpreted something...\n> >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: Niklas Söderlund \n> > > ---\n> > > .../pipeline/raspberrypi/rpi_stream.cpp | 70 ++++++++++++++++---\n> > > .../pipeline/raspberrypi/rpi_stream.h | 12 +++-\n> > > 2 files changed, 71 insertions(+), 11 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > index 02f8d3e0..6635a751 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > @@ -112,25 +112,34 @@ int RPiStream::queueBuffer(FrameBuffer *buffer)\n> > > */\n> > > if (!buffer) {\n> > > if (availableBuffers_.empty()) {\n> > > - LOG(RPISTREAM, Warning) << \"No buffers available for \"\n> > > + LOG(RPISTREAM, Info) << \"No buffers available for \"\n> > > << name_;\n> > > - return -EINVAL;\n> > > + /*\n> > > + * Note that we need to requeue an internal buffer as soon\n> > > + * as one becomes available.\n> > > + */\n> > > + requeueBuffers_.push(nullptr);\n\nI'm having a hard time parsing this patch. \"requeue\" sounds like the\nqueue contains buffers that are completed and need to be given back to\nthe device, and I think that's not what is happening here. Or is it ?\n\nIt also seems that the buffers are queued to the device in\nreturnBuffers(), which is meant to signal completion. This makes the\ncode flow very opaque to me. I'm afraid I just can't provide meaningful\nreview comments as I don't understand what's going on.\n\n> > > + return 0;\n> > > }\n> > >\n> > > buffer = availableBuffers_.front();\n> > > availableBuffers_.pop();\n> > > }\n> > >\n> > > - LOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> > > - << \" for \" << name_;\n> > > + /*\n> > > + * If no earlier requests are pending to be queued we can go ahead and\n> > > + * queue the buffer into the device.\n> > > + */\n> > > + if (requeueBuffers_.empty())\n> > > + return queueToDevice(buffer);\n> > >\n> > > - int ret = dev_->queueBuffer(buffer);\n> > > - if (ret) {\n> > > - LOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> > > - << name_;\n> > > - }\n> > > + /*\n> > > + * There are earlier buffers to be queued, so this bufferm ust go on\n> >\n> > s/bufferm ust/buffer must/\n> >\n> > That could be fixed up while applying. It's minor.\n> >\n> > > + * the waiting list.\n> > > + */\n> > > + requeueBuffers_.push(buffer);\n> > >\n> > > - return ret;\n> > > + return 0;\n> > > }\n> > >\n> > > void RPiStream::returnBuffer(FrameBuffer *buffer)\n> > > @@ -138,7 +147,35 @@ void RPiStream::returnBuffer(FrameBuffer *buffer)\n> > > /* This can only be called for external streams. */\n> > > assert(external_);\n> > >\n> > > + /* Push this buffer back into the queue to be used again. */\n> > > availableBuffers_.push(buffer);\n> > > +\n> > > + /*\n> > > + * Do we have any buffers that are waiting to be queued?\n> > > + * If so, do it now as availableBuffers_ will not be empty.\n> > > + */\n> > > + while (!requeueBuffers_.empty()) {\n> > > + FrameBuffer *buffer = requeueBuffers_.front();\n> > > +\n> > > + if (!buffer) {\n> > > + /*\n> > > + * We want to queue an internal buffer, but none\n> > > + * are available. Can't do anything, quit the loop.\n> > > + */\n> > > + if (availableBuffers_.empty())\n> > > + break;\n> >\n> > Can this happen?\n> >\n> > This looks like:\n> >\n> > while(requeueBuffer exist)\n> > buffer = first buffer from requeueBuffers\n> > if (no buffer) {\n> > /* unreachable code ? */\n> > }\n> >\n> \n> Yes it is possible, having a nullptr element in requeueBuffers_ means\n> a Request did not provide a buffer for that stream, so we have to use\n> a buffer from availableBuffers_ instead. This buffer will not be\n> returned out to the applications. Hope that makes sense.\n> \n> > > +\n> > > + /*\n> > > + * We want to queue an internal buffer, and at least one\n> > > + * is available.\n> > > + */\n> > > + buffer = availableBuffers_.front();\n> > > + availableBuffers_.pop();\n> > > + }\n> > > +\n> > > + requeueBuffers_.pop();\n> > > + queueToDevice(buffer);\n> > > + }\n> > > }\n> > >\n> > > bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> > > @@ -155,10 +192,23 @@ bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> > > void RPiStream::clearBuffers()\n> > > {\n> > > availableBuffers_ = std::queue{};\n> > > + requeueBuffers_ = std::queue{};\n> > > internalBuffers_.clear();\n> > > bufferList_.clear();\n> > > }\n> > >\n> > > +int RPiStream::queueToDevice(FrameBuffer *buffer)\n> > > +{\n> > > + LOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> > > + << \" for \" << name_;\n> > > +\n> > > + int ret = dev_->queueBuffer(buffer);\n> > > + if (ret)\n> > > + LOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> > > + << name_;\n> > > + return ret;\n> > > +}\n> > > +\n> > > } /* namespace RPi */\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > index af9c2ad2..6222c867 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > @@ -52,6 +52,7 @@ public:\n> > >\n> > > private:\n> > > void clearBuffers();\n> > > + int queueToDevice(FrameBuffer *buffer);\n> > > /*\n> > > * Indicates that this stream is active externally, i.e. the buffers\n> > > * might be provided by (and returned to) the application.\n> > > @@ -63,7 +64,7 @@ private:\n> > > std::string name_;\n> > > /* The actual device stream. */\n> > > std::unique_ptr dev_;\n> > > - /* All framebuffers associated with this device stream. */\n> > > + /* All frame buffers associated with this device stream. */\n> > > std::vector bufferList_;\n> > > /*\n> > > * List of frame buffer that we can use if none have been provided by\n> > > @@ -71,6 +72,15 @@ private:\n> > > * buffers exported internally.\n> > > */\n> > > std::queue availableBuffers_;\n> > > + /*\n> > > + * List of frame buffer that are to be re-queued into the device.\n> >\n> > \"List of frame buffers ...\"\n> >\n> > > + * A nullptr indicates any internal buffer can be used (from availableBuffers_),\n> > > + * whereas a valid pointer indicates an external buffer to be queued.\n> >\n> > Oh ... are we pushing nullptrs on to the queues? ... perhaps that\n> > answers my unreachable code question above ...\n> >\n> > > + *\n> > > + * Ordering buffers to be queued is important here as it must match the\n> > > + * requests coming from the application.\n> > > + */\n> > > + std::queue requeueBuffers_;\n> > > /*\n> > > * This is a list of buffers exported internally. Need to keep this around\n> > > * as the stream needs to maintain ownership of these buffers.","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 9BEDDC2E68\n\tfor ;\n\tWed, 22 Jul 2020 15:43:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 31B956074F;\n\tWed, 22 Jul 2020 17:43:02 +0200 (CEST)","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 7D7D060540\n\tfor ;\n\tWed, 22 Jul 2020 17:43:00 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 06DF4329;\n\tWed, 22 Jul 2020 17:42:53 +0200 (CEST)"],"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=\"BbHOeI7G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595432574;\n\tbh=/D4lR0Gm0QWMpjLiscfD2XvAp+SepsACcHxVFQAdxLI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BbHOeI7GOW9GdiGSgFuWoHyLO7P1i3Xgb7g90G6O2s+97i8FM0VQx7UY3qr+rz5AU\n\tqlsEtdnxt1Lb1FWzsrgY/fo5eaqCtXZY4EsrekdHe3FG3424WWogiUT6emug6S+AFm\n\tXphMxzGxKVjWzRiehkkeXiZJDDE3fyEGL/pLSDlM=","Date":"Wed, 22 Jul 2020 18:42:48 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20200722154248.GJ29813@pendragon.ideasonboard.com>","References":"<20200720091311.805092-1-naush@raspberrypi.com>\n\t<20200720091311.805092-9-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 8/9] libcamera: pipeline:\n\traspberrypi: Add more robust stream buffer logic","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11512,"web_url":"https://patchwork.libcamera.org/comment/11512/","msgid":"","date":"2020-07-22T16:04:54","subject":"Re: [libcamera-devel] [PATCH v4 8/9] libcamera: pipeline:\n\traspberrypi: Add more robust stream buffer logic","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 feedback.\n\nOn Wed, 22 Jul 2020 at 16:43, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Tue, Jul 21, 2020 at 01:35:51PM +0100, Naushir Patuck wrote:\n> > On Tue, 21 Jul 2020 at 12:23, Kieran Bingham wrote:\n> > > On 20/07/2020 10:13, Naushir Patuck wrote:\n> > > > Add further queueing into the RPiStream object to ensure that we always\n> > > > follow the buffer ordering (be it internal or external) given by\n> > > > incoming Requests.\n> > > >\n> > > > This is essential, otherwise we risk dropping frames that are meant to\n> > > > be part of a Request, and can cause the pipeline to stall indefinitely.\n> > > > This also prevents any possibility of mismatched frame buffers going\n> > > > through the pipeline and out to the application.\n> > >\n> > > Sounds helpful... some questions below ... but I suspect I've just\n> > > misinterpreted something...\n> > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > Reviewed-by: Niklas Söderlund \n> > > > ---\n> > > > .../pipeline/raspberrypi/rpi_stream.cpp | 70 ++++++++++++++++---\n> > > > .../pipeline/raspberrypi/rpi_stream.h | 12 +++-\n> > > > 2 files changed, 71 insertions(+), 11 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > index 02f8d3e0..6635a751 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > @@ -112,25 +112,34 @@ int RPiStream::queueBuffer(FrameBuffer *buffer)\n> > > > */\n> > > > if (!buffer) {\n> > > > if (availableBuffers_.empty()) {\n> > > > - LOG(RPISTREAM, Warning) << \"No buffers available for \"\n> > > > + LOG(RPISTREAM, Info) << \"No buffers available for \"\n> > > > << name_;\n> > > > - return -EINVAL;\n> > > > + /*\n> > > > + * Note that we need to requeue an internal buffer as soon\n> > > > + * as one becomes available.\n> > > > + */\n> > > > + requeueBuffers_.push(nullptr);\n>\n> I'm having a hard time parsing this patch. \"requeue\" sounds like the\n> queue contains buffers that are completed and need to be given back to\n> the device, and I think that's not what is happening here. Or is it ?\n>\n> It also seems that the buffers are queued to the device in\n> returnBuffers(), which is meant to signal completion. This makes the\n> code flow very opaque to me. I'm afraid I just can't provide meaningful\n> review comments as I don't understand what's going on.\n>\n\nI must admit, I did hit my head on the wall trying to ensure I have\ncovered all grounds - it all gets very complicated very quickly. And\nmy variable/method names could possibly be chosen better. Here is\nwhat i am trying to achieve with this commit:\n\n1) We must handle drop frames at the start of streaming correctly. By\ncorrectly I mean that we cannot queue Request provided buffers into\nthe device and drop them, i.e. not return them to the application. We\ncannot rely on having as many internal buffers allocated to match the\namount of drop frames requested.\n\n2) Similarly we must account for the fact that not every request will\ncontain a buffer for, say, a RAW image when RAW streams are exported\nto the application. In these cases, we still need to queue an\ninternal buffer. But what happens if we have run out of internal\nbuffers? This needs to be handled correctly.\n\nSo the result is a queue (called, requeueBuffers) that stores requests\nfor buffers needing to be queued to the device in the case we have run\nout of internal buffers, and the application has not provided one in\nthe Request. This happens in queueBuffer(). Ordering is important\nhere, as we must be in lock-step with the order Requests come in.\nWhen a buffer completes (and providing it was an internal buffer in\nthe absence of a RAW buffer in the Request in our example above), we\ncall returnBuffer() which then checks if we had anything to re-queue\n(hence the name requeueBuffers_) to the device now that an internal\nbuffer has been returned to the stream.\n\nHope this explanation makes the code a bit easier to understand. If\nnot, I'm happy to chat or call to provide more clarity :) If you do\nhave any suggestions for naming, please do let me know and I am open\nto changing these.\n\nRegards,\nNaush\n\n\n\n> > > > + return 0;\n> > > > }\n> > > >\n> > > > buffer = availableBuffers_.front();\n> > > > availableBuffers_.pop();\n> > > > }\n> > > >\n> > > > - LOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> > > > - << \" for \" << name_;\n> > > > + /*\n> > > > + * If no earlier requests are pending to be queued we can go ahead and\n> > > > + * queue the buffer into the device.\n> > > > + */\n> > > > + if (requeueBuffers_.empty())\n> > > > + return queueToDevice(buffer);\n> > > >\n> > > > - int ret = dev_->queueBuffer(buffer);\n> > > > - if (ret) {\n> > > > - LOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> > > > - << name_;\n> > > > - }\n> > > > + /*\n> > > > + * There are earlier buffers to be queued, so this bufferm ust go on\n> > >\n> > > s/bufferm ust/buffer must/\n> > >\n> > > That could be fixed up while applying. It's minor.\n> > >\n> > > > + * the waiting list.\n> > > > + */\n> > > > + requeueBuffers_.push(buffer);\n> > > >\n> > > > - return ret;\n> > > > + return 0;\n> > > > }\n> > > >\n> > > > void RPiStream::returnBuffer(FrameBuffer *buffer)\n> > > > @@ -138,7 +147,35 @@ void RPiStream::returnBuffer(FrameBuffer *buffer)\n> > > > /* This can only be called for external streams. */\n> > > > assert(external_);\n> > > >\n> > > > + /* Push this buffer back into the queue to be used again. */\n> > > > availableBuffers_.push(buffer);\n> > > > +\n> > > > + /*\n> > > > + * Do we have any buffers that are waiting to be queued?\n> > > > + * If so, do it now as availableBuffers_ will not be empty.\n> > > > + */\n> > > > + while (!requeueBuffers_.empty()) {\n> > > > + FrameBuffer *buffer = requeueBuffers_.front();\n> > > > +\n> > > > + if (!buffer) {\n> > > > + /*\n> > > > + * We want to queue an internal buffer, but none\n> > > > + * are available. Can't do anything, quit the loop.\n> > > > + */\n> > > > + if (availableBuffers_.empty())\n> > > > + break;\n> > >\n> > > Can this happen?\n> > >\n> > > This looks like:\n> > >\n> > > while(requeueBuffer exist)\n> > > buffer = first buffer from requeueBuffers\n> > > if (no buffer) {\n> > > /* unreachable code ? */\n> > > }\n> > >\n> >\n> > Yes it is possible, having a nullptr element in requeueBuffers_ means\n> > a Request did not provide a buffer for that stream, so we have to use\n> > a buffer from availableBuffers_ instead. This buffer will not be\n> > returned out to the applications. Hope that makes sense.\n> >\n> > > > +\n> > > > + /*\n> > > > + * We want to queue an internal buffer, and at least one\n> > > > + * is available.\n> > > > + */\n> > > > + buffer = availableBuffers_.front();\n> > > > + availableBuffers_.pop();\n> > > > + }\n> > > > +\n> > > > + requeueBuffers_.pop();\n> > > > + queueToDevice(buffer);\n> > > > + }\n> > > > }\n> > > >\n> > > > bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> > > > @@ -155,10 +192,23 @@ bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> > > > void RPiStream::clearBuffers()\n> > > > {\n> > > > availableBuffers_ = std::queue{};\n> > > > + requeueBuffers_ = std::queue{};\n> > > > internalBuffers_.clear();\n> > > > bufferList_.clear();\n> > > > }\n> > > >\n> > > > +int RPiStream::queueToDevice(FrameBuffer *buffer)\n> > > > +{\n> > > > + LOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> > > > + << \" for \" << name_;\n> > > > +\n> > > > + int ret = dev_->queueBuffer(buffer);\n> > > > + if (ret)\n> > > > + LOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> > > > + << name_;\n> > > > + return ret;\n> > > > +}\n> > > > +\n> > > > } /* namespace RPi */\n> > > >\n> > > > } /* namespace libcamera */\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > index af9c2ad2..6222c867 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > > @@ -52,6 +52,7 @@ public:\n> > > >\n> > > > private:\n> > > > void clearBuffers();\n> > > > + int queueToDevice(FrameBuffer *buffer);\n> > > > /*\n> > > > * Indicates that this stream is active externally, i.e. the buffers\n> > > > * might be provided by (and returned to) the application.\n> > > > @@ -63,7 +64,7 @@ private:\n> > > > std::string name_;\n> > > > /* The actual device stream. */\n> > > > std::unique_ptr dev_;\n> > > > - /* All framebuffers associated with this device stream. */\n> > > > + /* All frame buffers associated with this device stream. */\n> > > > std::vector bufferList_;\n> > > > /*\n> > > > * List of frame buffer that we can use if none have been provided by\n> > > > @@ -71,6 +72,15 @@ private:\n> > > > * buffers exported internally.\n> > > > */\n> > > > std::queue availableBuffers_;\n> > > > + /*\n> > > > + * List of frame buffer that are to be re-queued into the device.\n> > >\n> > > \"List of frame buffers ...\"\n> > >\n> > > > + * A nullptr indicates any internal buffer can be used (from availableBuffers_),\n> > > > + * whereas a valid pointer indicates an external buffer to be queued.\n> > >\n> > > Oh ... are we pushing nullptrs on to the queues? ... perhaps that\n> > > answers my unreachable code question above ...\n> > >\n> > > > + *\n> > > > + * Ordering buffers to be queued is important here as it must match the\n> > > > + * requests coming from the application.\n> > > > + */\n> > > > + std::queue requeueBuffers_;\n> > > > /*\n> > > > * This is a list of buffers exported internally. Need to keep this around\n> > > > * as the stream needs to maintain ownership of these buffers.\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 54E29C2E68\n\tfor ;\n\tWed, 22 Jul 2020 16:05:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 225A26099C;\n\tWed, 22 Jul 2020 18:05:14 +0200 (CEST)","from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4704C60540\n\tfor ;\n\tWed, 22 Jul 2020 18:05:12 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id j11so3070808ljo.7\n\tfor ;\n\tWed, 22 Jul 2020 09:05:12 -0700 (PDT)"],"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=\"bXPKKaWS\"; 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:content-transfer-encoding;\n\tbh=2BLNSLTo6nFKppNZ1eKCRlUrlxGS91KKgskY+OM5cb8=;\n\tb=bXPKKaWS1uL6i8LbLO7P5IZHTbVIoA6KFjQY7Yd5IfVISDKX8FEsOTp6MRMaO9Ot1o\n\tJISkmf4YLRhRtVb/mS7t6MMoaG6wY06xAItGYVtcwLlfkh4B1BZmDCwzUfBIpEXRXaWm\n\tO59pqVpKYvXXzT/2lKVWEgRGPpZ2jptB4izBETaDg9Lsmw4EkGlxLsdRZQ2OhZLDNcYa\n\tL2XGTTvOPzUpDhjwiwjS6eLxC1g9UhL/wea4Sz4vr+1Ec9C63Ub1b1YCxF5O8XEmc/OY\n\tNCj57xFaiA2N8bhclAtPuOmzjMeWVkZXIO+ZTkS3GKpwJFW/BS/IqqrftVpZ36d8dQIn\n\tmQiQ==","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:content-transfer-encoding;\n\tbh=2BLNSLTo6nFKppNZ1eKCRlUrlxGS91KKgskY+OM5cb8=;\n\tb=qycyBgIUnozx+1tcaOUbelgRbbPWTJti4BpzCl92ZJYgaBS1tg5M/Y58ADoMaU7m6k\n\tc1Yd3heT5djLBurVYfE7tpbvWW+nbEZ27aYwgH5sY1tujwItIk793w85Dv0q6JZgh0lT\n\tRx0xDgcmwMbZ5pYiX6trtAUXUWuidnfnnJMv4GRVxGvMCJ9sUjVpy+1r0qlXK4whoY65\n\txYrPv2xe+vjI5gWqWIKiQJCIBK5CqU26Nk5MuWghqUlPFpwESQifwlRC8IRdjBcZwKv3\n\tWpCwpy+smUCYhBJpDfpQXWRTeALEOVAgH4t7PdbY1aD15EwE/1MgWiRGOwHbeYGNX99g\n\t+6Dg==","X-Gm-Message-State":"AOAM530KHxPrymqTRmt3EBUoLVbuvu6mLLEL1dobJiEFC3FrhDs1eWG6\n\tnAxb5W3tXTITpWDoddv/FRbOgiLXT3Y2U5DX66+MkNEMudbEHQ==","X-Google-Smtp-Source":"ABdhPJwIOvVUJ9VPD+1Ms1Py4KGF23KE8DdGyZz17TGRjoIdnT98XM0LvYXnwfgAiFxLYtsvTNPqa9JuCEhmEVO+W0U=","X-Received":"by 2002:a2e:2a84:: with SMTP id\n\tq126mr14775096ljq.1.1595433911523; \n\tWed, 22 Jul 2020 09:05:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20200720091311.805092-1-naush@raspberrypi.com>\n\t<20200720091311.805092-9-naush@raspberrypi.com>\n\t\n\t\n\t<20200722154248.GJ29813@pendragon.ideasonboard.com>","In-Reply-To":"<20200722154248.GJ29813@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Wed, 22 Jul 2020 17:04:54 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 8/9] libcamera: pipeline:\n\traspberrypi: Add more robust stream buffer logic","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]