[{"id":15864,"web_url":"https://patchwork.libcamera.org/comment/15864/","msgid":"<YFtdAbhJZvyCIA/Y@pendragon.ideasonboard.com>","date":"2021-03-24T15:38:41","subject":"Re: [libcamera-devel] [PATCH v3 3/6] libcamera: buffer: Initialise\n\tstatus","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Wed, Mar 24, 2021 at 03:01:22PM +0000, Kieran Bingham wrote:\n> Buffers queued to a pipeline handler may not be yet queued to a device\n> when the request is cancelled.\n> \n> This can lead to the FrameMetadata having never been explicitly set by\n> an underlying V4L2 device.\n> \n> The status field on this is used to check the state of the buffer to\n> determine if it was correctly filled, or if it was cancelled.\n> \n> In the event that the buffer is not used, it must be marked as Cancelled\n> as the metadata associated with that frame will not be valid.\n> \n> Initialise the FrameMetadata to FrameCancelled to prevent uninitialised access.\n> Furthermore, re-initialise the metadata to this state when the buffers\n> are re-used, or added to a Request to ensure that they are in the\n> correct state in the event that they do not get used at all.\n\nI wonder if this scheme could be a bit fragile. We're now essentially\ntelling pipeline handlers that they don't need to take care of marking\nthe buffer status for cancelled requests if the buffers haven't been\nqueued to the device. This means they could possibly overlook some other\nimportant processing needed on buffer. There's no such thing at the\nmoment, time will tell if there will be in the future, in which case we\nmay want to reconsider this.\n\n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> ---\n> v3:\n>  - Initialise as FrameCancelled\n>  - Re-initialise to FrameCancelled when buffers are re-used\n>  - Do not re-order the enum\n> \n>  include/libcamera/buffer.h | 2 +-\n>  src/libcamera/request.cpp  | 9 +++++++--\n>  2 files changed, 8 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 302fe3d3e86b..f6673e2f6eda 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -28,7 +28,7 @@ struct FrameMetadata {\n>  \t\tunsigned int bytesused;\n>  \t};\n>  \n> -\tStatus status;\n> +\tStatus status = FrameCancelled;\n\nThis isn't strictly needed given the code below, but it's nice to be\nconsistent. Maybe we should rename it to FrameInvalid or something along\nthose lines later, to reflect the fact that it's not just about\ncancelled requests now.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n>  \tunsigned int sequence;\n>  \tuint64_t timestamp;\n>  \tstd::vector<Plane> planes;\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 0071667e4a2c..9ea6ed09446b 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -118,8 +118,12 @@ void Request::reuse(ReuseFlag flags)\n>  \tpending_.clear();\n>  \tif (flags & ReuseBuffers) {\n>  \t\tfor (auto pair : bufferMap_) {\n> -\t\t\tpair.second->request_ = this;\n> -\t\t\tpending_.insert(pair.second);\n> +\t\t\tFrameBuffer *buffer = pair.second;\n> +\n> +\t\t\tbuffer->metadata_.status = FrameMetadata::Status::FrameCancelled;\n> +\t\t\tbuffer->request_ = this;\n> +\n> +\t\t\tpending_.insert(buffer);\n>  \t\t}\n>  \t} else {\n>  \t\tbufferMap_.clear();\n> @@ -188,6 +192,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n>  \t}\n>  \n>  \tbuffer->request_ = this;\n> +\tbuffer->metadata_.status = FrameMetadata::Status::FrameCancelled;\n>  \tpending_.insert(buffer);\n>  \tbufferMap_[stream] = buffer;\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 6CBD8C32E7\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Mar 2021 15:39:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9FD06051A;\n\tWed, 24 Mar 2021 16:39:27 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 85B95602E3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Mar 2021 16:39:25 +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 04924580;\n\tWed, 24 Mar 2021 16:39:24 +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=\"AYuuZIGt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616600365;\n\tbh=LITfB+/+gTXlRe2S5PQrDIfsVcbK4JDiCFkHRjpVqq0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AYuuZIGt8a8nG+7odqqaaPBM4bXrs3sbl66A1R6OA7VOitutm5YGS9j+C/QcwLtwK\n\t0YHx/sSftQcR2gjaNhuBLwF4qTl+2QVfUurmfQAMgUkmaZR3MWuQDDZsHHQU4Cv4Ln\n\tOVkaUrpassk5KAyAUVXL3ez7oL1+VUo7vXJ8kiZQ=","Date":"Wed, 24 Mar 2021 17:38:41 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YFtdAbhJZvyCIA/Y@pendragon.ideasonboard.com>","References":"<20210324150125.1318325-1-kieran.bingham@ideasonboard.com>\n\t<20210324150125.1318325-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210324150125.1318325-4-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/6] libcamera: buffer: Initialise\n\tstatus","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15867,"web_url":"https://patchwork.libcamera.org/comment/15867/","msgid":"<ff1a42b6-3713-3dad-261d-33037fb38785@ideasonboard.com>","date":"2021-03-24T15:54:09","subject":"Re: [libcamera-devel] [PATCH v3 3/6] libcamera: buffer: Initialise\n\tstatus","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 24/03/2021 15:38, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Wed, Mar 24, 2021 at 03:01:22PM +0000, Kieran Bingham wrote:\n>> Buffers queued to a pipeline handler may not be yet queued to a device\n>> when the request is cancelled.\n>>\n>> This can lead to the FrameMetadata having never been explicitly set by\n>> an underlying V4L2 device.\n>>\n>> The status field on this is used to check the state of the buffer to\n>> determine if it was correctly filled, or if it was cancelled.\n>>\n>> In the event that the buffer is not used, it must be marked as Cancelled\n>> as the metadata associated with that frame will not be valid.\n>>\n>> Initialise the FrameMetadata to FrameCancelled to prevent uninitialised access.\n>> Furthermore, re-initialise the metadata to this state when the buffers\n>> are re-used, or added to a Request to ensure that they are in the\n>> correct state in the event that they do not get used at all.\n> \n> I wonder if this scheme could be a bit fragile. We're now essentially\n> telling pipeline handlers that they don't need to take care of marking\n> the buffer status for cancelled requests if the buffers haven't been\n> queued to the device. This means they could possibly overlook some other\n> important processing needed on buffer. There's no such thing at the\n> moment, time will tell if there will be in the future, in which case we\n> may want to reconsider this.\n> \n>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>>\n>> ---\n>> v3:\n>>  - Initialise as FrameCancelled\n>>  - Re-initialise to FrameCancelled when buffers are re-used\n>>  - Do not re-order the enum\n>>\n>>  include/libcamera/buffer.h | 2 +-\n>>  src/libcamera/request.cpp  | 9 +++++++--\n>>  2 files changed, 8 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n>> index 302fe3d3e86b..f6673e2f6eda 100644\n>> --- a/include/libcamera/buffer.h\n>> +++ b/include/libcamera/buffer.h\n>> @@ -28,7 +28,7 @@ struct FrameMetadata {\n>>  \t\tunsigned int bytesused;\n>>  \t};\n>>  \n>> -\tStatus status;\n>> +\tStatus status = FrameCancelled;\n> \n> This isn't strictly needed given the code below, but it's nice to be\n> consistent. Maybe we should rename it to FrameInvalid or something along\n> those lines later, to reflect the fact that it's not just about\n> cancelled requests now.\n\nIndeed, now that it's set during addBuffer() this becomes at least a\nlittle redundant.\n\nI agree on the naming doesn't feel right any more either.\nFrameInvalid might be appropriate, and suitable for both an unused, and\ncancelled frame...\n\n\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n>>  \tunsigned int sequence;\n>>  \tuint64_t timestamp;\n>>  \tstd::vector<Plane> planes;\n>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n>> index 0071667e4a2c..9ea6ed09446b 100644\n>> --- a/src/libcamera/request.cpp\n>> +++ b/src/libcamera/request.cpp\n>> @@ -118,8 +118,12 @@ void Request::reuse(ReuseFlag flags)\n>>  \tpending_.clear();\n>>  \tif (flags & ReuseBuffers) {\n>>  \t\tfor (auto pair : bufferMap_) {\n>> -\t\t\tpair.second->request_ = this;\n>> -\t\t\tpending_.insert(pair.second);\n>> +\t\t\tFrameBuffer *buffer = pair.second;\n>> +\n>> +\t\t\tbuffer->metadata_.status = FrameMetadata::Status::FrameCancelled;\n>> +\t\t\tbuffer->request_ = this;\n>> +\n>> +\t\t\tpending_.insert(buffer);\n>>  \t\t}\n>>  \t} else {\n>>  \t\tbufferMap_.clear();\n>> @@ -188,6 +192,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n>>  \t}\n>>  \n>>  \tbuffer->request_ = this;\n>> +\tbuffer->metadata_.status = FrameMetadata::Status::FrameCancelled;\n>>  \tpending_.insert(buffer);\n>>  \tbufferMap_[stream] = buffer;\n>>  \n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 03E84C32E7\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Mar 2021 15:54:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E12368D6B;\n\tWed, 24 Mar 2021 16:54:14 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA3B66051A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Mar 2021 16:54:12 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2DBDC580;\n\tWed, 24 Mar 2021 16:54: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=\"Zoixj5F1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616601252;\n\tbh=wz7BT7yzf+SUS7hSxLmOALPyn2AeEOpfcvM+W22SiPo=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Zoixj5F1I/QCjwY3WAplvmosOt1pqqcN0ZuWLkQA3YAOuvHYIxbXPUbwXejRY8t3w\n\t37esxUL5bTWM6vQ17Tpnw2wXTxjwk4WLytesO2lAqXxK0HNJScJJYIqSFnW53t8Hjq\n\to02E4Wt+dMU+MH9ZWVnWOlctBs7EONhWCyzOZKk0=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20210324150125.1318325-1-kieran.bingham@ideasonboard.com>\n\t<20210324150125.1318325-4-kieran.bingham@ideasonboard.com>\n\t<YFtdAbhJZvyCIA/Y@pendragon.ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","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":"<ff1a42b6-3713-3dad-261d-33037fb38785@ideasonboard.com>","Date":"Wed, 24 Mar 2021 15:54:09 +0000","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":"<YFtdAbhJZvyCIA/Y@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 3/6] libcamera: buffer: Initialise\n\tstatus","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15870,"web_url":"https://patchwork.libcamera.org/comment/15870/","msgid":"<29e5502f-d9d6-0a62-7c9a-b6d5558ad1c4@ideasonboard.com>","date":"2021-03-24T16:25:07","subject":"Re: [libcamera-devel] [PATCH v3 3/6] libcamera: buffer: Initialise\n\tstatus","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 24/03/2021 15:54, Kieran Bingham wrote:\n> Hi Laurent,\n> \n> On 24/03/2021 15:38, Laurent Pinchart wrote:\n>> Hi Kieran,\n>>\n>> Thank you for the patch.\n>>\n>> On Wed, Mar 24, 2021 at 03:01:22PM +0000, Kieran Bingham wrote:\n>>> Buffers queued to a pipeline handler may not be yet queued to a device\n>>> when the request is cancelled.\n>>>\n>>> This can lead to the FrameMetadata having never been explicitly set by\n>>> an underlying V4L2 device.\n>>>\n>>> The status field on this is used to check the state of the buffer to\n>>> determine if it was correctly filled, or if it was cancelled.\n>>>\n>>> In the event that the buffer is not used, it must be marked as Cancelled\n>>> as the metadata associated with that frame will not be valid.\n>>>\n>>> Initialise the FrameMetadata to FrameCancelled to prevent uninitialised access.\n>>> Furthermore, re-initialise the metadata to this state when the buffers\n>>> are re-used, or added to a Request to ensure that they are in the\n>>> correct state in the event that they do not get used at all.\n>>\n>> I wonder if this scheme could be a bit fragile. We're now essentially\n>> telling pipeline handlers that they don't need to take care of marking\n>> the buffer status for cancelled requests if the buffers haven't been\n>> queued to the device. This means they could possibly overlook some other\n>> important processing needed on buffer. There's no such thing at the\n>> moment, time will tell if there will be in the future, in which case we\n>> may want to reconsider this.\n\nIndeed, I was trying to ensure that /all/ unused buffers were\ninitialised to an invalid state so that they weren't incorrectly\nidentified as valid (or that the unintialised status flag may be read by\nan applciation, which could be set to 0 meaning it's a good buffer).\n\nPerhaps we should also do this:\n\n> From d7e9bba52e6395c4669a69bd09450f99c842c078 Mon Sep 17 00:00:00 2001\n> From: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> Date: Wed, 24 Mar 2021 16:21:46 +0000\n> Subject: [PATCH] libcamera: pipeline: ipu3: Cancel unused buffers\n> \n> When the CIO2 returns a cancelled buffer, we will not queue buffers\n> to the IMGU.\n> \n> These buffers should be explicitly marked as cancelled to ensure\n> the application knows there is no valid metadata or frame data\n> provided in the buffer.\n> \n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n>  1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index af05cb2940c8..b495ea0a5b98 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1265,8 +1265,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>  \n>  \t/* If the buffer is cancelled force a complete of the whole request. */\n>  \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> -\t\tfor (auto it : request->buffers())\n> -\t\t\tpipe_->completeBuffer(request, it.second);\n> +\t\tfor (auto it : request->buffers()) {\n> +\t\t\tFrameBufffer *b = it.second;\n> +\t\t\tb->metadata_.status = FrameMetadata::Status::FrameCancelled;\n> +\t\t\tpipe_->completeBuffer(request, b);\n> +\t\t}\n>  \n>  \t\tframeInfos_.remove(info);\n>  \t\tpipe_->completeRequest(request);\n> -- \n> 2.25.1\n\n\nBut I still think the state should be initialised to something to make\nsure we don't find ourselves making choices on an unitialised state\nvariable.\n\n\n\n\n\n>>\n>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>>>\n>>> ---\n>>> v3:\n>>>  - Initialise as FrameCancelled\n>>>  - Re-initialise to FrameCancelled when buffers are re-used\n>>>  - Do not re-order the enum\n>>>\n>>>  include/libcamera/buffer.h | 2 +-\n>>>  src/libcamera/request.cpp  | 9 +++++++--\n>>>  2 files changed, 8 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n>>> index 302fe3d3e86b..f6673e2f6eda 100644\n>>> --- a/include/libcamera/buffer.h\n>>> +++ b/include/libcamera/buffer.h\n>>> @@ -28,7 +28,7 @@ struct FrameMetadata {\n>>>  \t\tunsigned int bytesused;\n>>>  \t};\n>>>  \n>>> -\tStatus status;\n>>> +\tStatus status = FrameCancelled;\n>>\n>> This isn't strictly needed given the code below, but it's nice to be\n>> consistent. Maybe we should rename it to FrameInvalid or something along\n>> those lines later, to reflect the fact that it's not just about\n>> cancelled requests now.\n> \n> Indeed, now that it's set during addBuffer() this becomes at least a\n> little redundant.\n> \n> I agree on the naming doesn't feel right any more either.\n> FrameInvalid might be appropriate, and suitable for both an unused, and\n> cancelled frame...\n> \n> \n>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>>\n>>>  \tunsigned int sequence;\n>>>  \tuint64_t timestamp;\n>>>  \tstd::vector<Plane> planes;\n>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n>>> index 0071667e4a2c..9ea6ed09446b 100644\n>>> --- a/src/libcamera/request.cpp\n>>> +++ b/src/libcamera/request.cpp\n>>> @@ -118,8 +118,12 @@ void Request::reuse(ReuseFlag flags)\n>>>  \tpending_.clear();\n>>>  \tif (flags & ReuseBuffers) {\n>>>  \t\tfor (auto pair : bufferMap_) {\n>>> -\t\t\tpair.second->request_ = this;\n>>> -\t\t\tpending_.insert(pair.second);\n>>> +\t\t\tFrameBuffer *buffer = pair.second;\n>>> +\n>>> +\t\t\tbuffer->metadata_.status = FrameMetadata::Status::FrameCancelled;\n>>> +\t\t\tbuffer->request_ = this;\n>>> +\n>>> +\t\t\tpending_.insert(buffer);\n>>>  \t\t}\n>>>  \t} else {\n>>>  \t\tbufferMap_.clear();\n>>> @@ -188,6 +192,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n>>>  \t}\n>>>  \n>>>  \tbuffer->request_ = this;\n>>> +\tbuffer->metadata_.status = FrameMetadata::Status::FrameCancelled;\n>>>  \tpending_.insert(buffer);\n>>>  \tbufferMap_[stream] = buffer;\n>>>  \n>>\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 EE8FBC32E5\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Mar 2021 16:25:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67B7268D62;\n\tWed, 24 Mar 2021 17:25:11 +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 5B730602E3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Mar 2021 17:25:10 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CE75F580;\n\tWed, 24 Mar 2021 17:25:09 +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=\"vLuLODvO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616603109;\n\tbh=N0Pfw0UYpEfW88Abr15sQZDEWsNqUlrvK2EmwejDMDk=;\n\th=Reply-To:Subject:From:To:Cc:References:Date:In-Reply-To:From;\n\tb=vLuLODvO2KTMpAEmkQZTlNaLterUTTzmJSL/qxp9DZI29dVGZMETeOdoTb09iKyE/\n\tG8jtmOVPEJZJ6EAjAsW93VCyoKX0h90bQKJAKWbMxlkwFjg7PQik9J4AGsF7NMDymp\n\tjWyrycQ1KfoE+mTc8M/dxF+urM88cfCgY9et0l1A=","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20210324150125.1318325-1-kieran.bingham@ideasonboard.com>\n\t<20210324150125.1318325-4-kieran.bingham@ideasonboard.com>\n\t<YFtdAbhJZvyCIA/Y@pendragon.ideasonboard.com>\n\t<ff1a42b6-3713-3dad-261d-33037fb38785@ideasonboard.com>","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":"<29e5502f-d9d6-0a62-7c9a-b6d5558ad1c4@ideasonboard.com>","Date":"Wed, 24 Mar 2021 16:25:07 +0000","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":"<ff1a42b6-3713-3dad-261d-33037fb38785@ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 3/6] libcamera: buffer: Initialise\n\tstatus","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15872,"web_url":"https://patchwork.libcamera.org/comment/15872/","msgid":"<YFtp4tEBzwD9lQa+@pendragon.ideasonboard.com>","date":"2021-03-24T16:33:38","subject":"Re: [libcamera-devel] [PATCH v3 3/6] libcamera: buffer: Initialise\n\tstatus","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Mar 24, 2021 at 04:25:07PM +0000, Kieran Bingham wrote:\n> On 24/03/2021 15:54, Kieran Bingham wrote:\n> > On 24/03/2021 15:38, Laurent Pinchart wrote:\n> >> On Wed, Mar 24, 2021 at 03:01:22PM +0000, Kieran Bingham wrote:\n> >>> Buffers queued to a pipeline handler may not be yet queued to a device\n> >>> when the request is cancelled.\n> >>>\n> >>> This can lead to the FrameMetadata having never been explicitly set by\n> >>> an underlying V4L2 device.\n> >>>\n> >>> The status field on this is used to check the state of the buffer to\n> >>> determine if it was correctly filled, or if it was cancelled.\n> >>>\n> >>> In the event that the buffer is not used, it must be marked as Cancelled\n> >>> as the metadata associated with that frame will not be valid.\n> >>>\n> >>> Initialise the FrameMetadata to FrameCancelled to prevent uninitialised access.\n> >>> Furthermore, re-initialise the metadata to this state when the buffers\n> >>> are re-used, or added to a Request to ensure that they are in the\n> >>> correct state in the event that they do not get used at all.\n> >>\n> >> I wonder if this scheme could be a bit fragile. We're now essentially\n> >> telling pipeline handlers that they don't need to take care of marking\n> >> the buffer status for cancelled requests if the buffers haven't been\n> >> queued to the device. This means they could possibly overlook some other\n> >> important processing needed on buffer. There's no such thing at the\n> >> moment, time will tell if there will be in the future, in which case we\n> >> may want to reconsider this.\n> \n> Indeed, I was trying to ensure that /all/ unused buffers were\n> initialised to an invalid state so that they weren't incorrectly\n> identified as valid (or that the unintialised status flag may be read by\n> an applciation, which could be set to 0 meaning it's a good buffer).\n> \n> Perhaps we should also do this:\n> \n> > From d7e9bba52e6395c4669a69bd09450f99c842c078 Mon Sep 17 00:00:00 2001\n> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > Date: Wed, 24 Mar 2021 16:21:46 +0000\n> > Subject: [PATCH] libcamera: pipeline: ipu3: Cancel unused buffers\n> > \n> > When the CIO2 returns a cancelled buffer, we will not queue buffers\n> > to the IMGU.\n> > \n> > These buffers should be explicitly marked as cancelled to ensure\n> > the application knows there is no valid metadata or frame data\n> > provided in the buffer.\n> > \n> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > ---\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n> >  1 file changed, 5 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index af05cb2940c8..b495ea0a5b98 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1265,8 +1265,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> >  \n> >  \t/* If the buffer is cancelled force a complete of the whole request. */\n> >  \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > -\t\tfor (auto it : request->buffers())\n> > -\t\t\tpipe_->completeBuffer(request, it.second);\n> > +\t\tfor (auto it : request->buffers()) {\n> > +\t\t\tFrameBufffer *b = it.second;\n> > +\t\t\tb->metadata_.status = FrameMetadata::Status::FrameCancelled;\n> > +\t\t\tpipe_->completeBuffer(request, b);\n> > +\t\t}\n> >  \n> >  \t\tframeInfos_.remove(info);\n> >  \t\tpipe_->completeRequest(request);\n\nThis looks good to me too. It has the advantage of not making us feel\nsafe erronously that the libcamera core will always provide a solution\nfor us.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nBut I'm also OK with the original patch for the time being, you can pick\nthe one you like best (or the one selected by your favourite random\nnumber generator :-)).\n\n> But I still think the state should be initialised to something to make\n> sure we don't find ourselves making choices on an unitialised state\n> variable.\n\nFully agreed.\n\n> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >>>\n> >>> ---\n> >>> v3:\n> >>>  - Initialise as FrameCancelled\n> >>>  - Re-initialise to FrameCancelled when buffers are re-used\n> >>>  - Do not re-order the enum\n> >>>\n> >>>  include/libcamera/buffer.h | 2 +-\n> >>>  src/libcamera/request.cpp  | 9 +++++++--\n> >>>  2 files changed, 8 insertions(+), 3 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> >>> index 302fe3d3e86b..f6673e2f6eda 100644\n> >>> --- a/include/libcamera/buffer.h\n> >>> +++ b/include/libcamera/buffer.h\n> >>> @@ -28,7 +28,7 @@ struct FrameMetadata {\n> >>>  \t\tunsigned int bytesused;\n> >>>  \t};\n> >>>  \n> >>> -\tStatus status;\n> >>> +\tStatus status = FrameCancelled;\n> >>\n> >> This isn't strictly needed given the code below, but it's nice to be\n> >> consistent. Maybe we should rename it to FrameInvalid or something along\n> >> those lines later, to reflect the fact that it's not just about\n> >> cancelled requests now.\n> > \n> > Indeed, now that it's set during addBuffer() this becomes at least a\n> > little redundant.\n> > \n> > I agree on the naming doesn't feel right any more either.\n> > FrameInvalid might be appropriate, and suitable for both an unused, and\n> > cancelled frame...\n> > \n> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> >>\n> >>>  \tunsigned int sequence;\n> >>>  \tuint64_t timestamp;\n> >>>  \tstd::vector<Plane> planes;\n> >>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> >>> index 0071667e4a2c..9ea6ed09446b 100644\n> >>> --- a/src/libcamera/request.cpp\n> >>> +++ b/src/libcamera/request.cpp\n> >>> @@ -118,8 +118,12 @@ void Request::reuse(ReuseFlag flags)\n> >>>  \tpending_.clear();\n> >>>  \tif (flags & ReuseBuffers) {\n> >>>  \t\tfor (auto pair : bufferMap_) {\n> >>> -\t\t\tpair.second->request_ = this;\n> >>> -\t\t\tpending_.insert(pair.second);\n> >>> +\t\t\tFrameBuffer *buffer = pair.second;\n> >>> +\n> >>> +\t\t\tbuffer->metadata_.status = FrameMetadata::Status::FrameCancelled;\n> >>> +\t\t\tbuffer->request_ = this;\n> >>> +\n> >>> +\t\t\tpending_.insert(buffer);\n> >>>  \t\t}\n> >>>  \t} else {\n> >>>  \t\tbufferMap_.clear();\n> >>> @@ -188,6 +192,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> >>>  \t}\n> >>>  \n> >>>  \tbuffer->request_ = this;\n> >>> +\tbuffer->metadata_.status = FrameMetadata::Status::FrameCancelled;\n> >>>  \tpending_.insert(buffer);\n> >>>  \tbufferMap_[stream] = buffer;\n> >>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 06BE9C32E5\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Mar 2021 16:34:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 779BD6051A;\n\tWed, 24 Mar 2021 17:34:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 686AC6051A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Mar 2021 17:34:22 +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 C77DF580;\n\tWed, 24 Mar 2021 17:34:21 +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=\"dDQPRd96\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616603662;\n\tbh=/n/8Drg0/Oty77pZRr3gjnMJeVxqUkiyBeoZPPhydvA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dDQPRd96DeB0/xGhVnSUQ92t2uB0xd4V/Yev9+wcGb+nhQLiSI726IjingLfOTQ1y\n\tg+yJ9DhG5xwe4iN4/jONtPMU2/9bua6ec+OLlrmq0d6kB1kskRGjQImNKz05WGXzqu\n\tiN27fPo02eKfavw6qJA0OcytnPz9sJ/CF/2B/5Cg=","Date":"Wed, 24 Mar 2021 18:33:38 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YFtp4tEBzwD9lQa+@pendragon.ideasonboard.com>","References":"<20210324150125.1318325-1-kieran.bingham@ideasonboard.com>\n\t<20210324150125.1318325-4-kieran.bingham@ideasonboard.com>\n\t<YFtdAbhJZvyCIA/Y@pendragon.ideasonboard.com>\n\t<ff1a42b6-3713-3dad-261d-33037fb38785@ideasonboard.com>\n\t<29e5502f-d9d6-0a62-7c9a-b6d5558ad1c4@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<29e5502f-d9d6-0a62-7c9a-b6d5558ad1c4@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/6] libcamera: buffer: Initialise\n\tstatus","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}}]