[{"id":23231,"web_url":"https://patchwork.libcamera.org/comment/23231/","msgid":"","date":"2022-05-30T09:21:13","subject":"Re: [libcamera-devel] [PATCH v3 23/30] py: Implement\n\tFrameBufferPlane","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nThank you for the patch.\n\nOn Fri, May 27, 2022 at 05:44:40PM +0300, Tomi Valkeinen wrote:\n> Implement FrameBufferPlane class and adjust the methods and uses\n> accordingly.\n> \n> Note that we don't expose the fd as a SharedFD, but as an int.\n\nThis may cause use-after-free (or rather use-after-close) issues and/or\nfd leaks, but I think that's OK for now. We can improve it later.\n\n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/cam/cam_kms.py | 6 +--\n> src/py/cam/cam_qtgl.py | 2 +-\n> src/py/libcamera/py_main.cpp | 45 +++++++++++----------\n> src/py/libcamera/utils/MappedFrameBuffer.py | 18 ++++-----\n> 4 files changed, 36 insertions(+), 35 deletions(-)\n> \n> diff --git a/src/py/cam/cam_kms.py b/src/py/cam/cam_kms.py\n> index 49b00211..3d1e8c27 100644\n> --- a/src/py/cam/cam_kms.py\n> +++ b/src/py/cam/cam_kms.py\n> @@ -131,10 +131,10 @@ class KMSRenderer:\n> fds = []\n> strides = []\n> offsets = []\n> - for i in range(fb.num_planes):\n> - fds.append(fb.fd(i))\n> + for p in fb.planes:\n> + fds.append(p.fd)\n> strides.append(cfg.stride)\n> - offsets.append(fb.offset(i))\n> + offsets.append(p.offset)\n> \n> drmfb = pykms.DmabufFramebuffer(self.card, w, h, fmt,\n> fds, strides, offsets)\n> diff --git a/src/py/cam/cam_qtgl.py b/src/py/cam/cam_qtgl.py\n> index 4b43f51d..6cfbd347 100644\n> --- a/src/py/cam/cam_qtgl.py\n> +++ b/src/py/cam/cam_qtgl.py\n> @@ -269,7 +269,7 @@ class MainWindow(QtWidgets.QWidget):\n> EGL_WIDTH, w,\n> EGL_HEIGHT, h,\n> EGL_LINUX_DRM_FOURCC_EXT, fmt,\n> - EGL_DMA_BUF_PLANE0_FD_EXT, fb.fd(0),\n> + EGL_DMA_BUF_PLANE0_FD_EXT, fb.planes[0].fd,\n> EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0,\n> EGL_DMA_BUF_PLANE0_PITCH_EXT, cfg.stride,\n> EGL_NONE,\n> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> index 52f70811..fcf009f0 100644\n> --- a/src/py/libcamera/py_main.cpp\n> +++ b/src/py/libcamera/py_main.cpp\n> @@ -155,6 +155,7 @@ PYBIND11_MODULE(_libcamera, m)\n> \tauto pyStreamFormats = py::class_(m, \"StreamFormats\");\n> \tauto pyFrameBufferAllocator = py::class_(m, \"FrameBufferAllocator\");\n> \tauto pyFrameBuffer = py::class_(m, \"FrameBuffer\");\n> +\tauto pyFrameBufferPlane = py::class_(pyFrameBuffer, \"Plane\");\n> \tauto pyStream = py::class_(m, \"Stream\");\n> \tauto pyControlId = py::class_(m, \"ControlId\");\n> \tauto pyControlInfo = py::class_(m, \"ControlInfo\");\n> @@ -408,31 +409,31 @@ PYBIND11_MODULE(_libcamera, m)\n> \t\t});\n> \n> \tpyFrameBuffer\n> -\t\t/* \\todo implement FrameBuffer::Plane properly */\n> -\t\t.def(py::init([](std::vector> planes, unsigned int cookie) {\n> -\t\t\tstd::vector v;\n> -\t\t\tfor (const auto &t : planes)\n> -\t\t\t\tv.push_back({ SharedFD(std::get<0>(t)), FrameBuffer::Plane::kInvalidOffset, std::get<1>(t) });\n> -\t\t\treturn new FrameBuffer(v, cookie);\n> -\t\t}))\n> +\t\t.def(py::init, unsigned int>(),\n> +\t\t py::arg(\"planes\"), py::arg(\"cookie\") = 0)\n> \t\t.def_property_readonly(\"metadata\", &FrameBuffer::metadata, py::return_value_policy::reference_internal)\n> -\t\t.def_property_readonly(\"num_planes\", [](const FrameBuffer &self) {\n> -\t\t\treturn self.planes().size();\n> -\t\t})\n> -\t\t.def(\"length\", [](FrameBuffer &self, uint32_t idx) {\n> -\t\t\tconst FrameBuffer::Plane &plane = self.planes()[idx];\n> -\t\t\treturn plane.length;\n> -\t\t})\n> -\t\t.def(\"fd\", [](FrameBuffer &self, uint32_t idx) {\n> -\t\t\tconst FrameBuffer::Plane &plane = self.planes()[idx];\n> -\t\t\treturn plane.fd.get();\n> -\t\t})\n> -\t\t.def(\"offset\", [](FrameBuffer &self, uint32_t idx) {\n> -\t\t\tconst FrameBuffer::Plane &plane = self.planes()[idx];\n> -\t\t\treturn plane.offset;\n> -\t\t})\n> +\t\t.def_property_readonly(\"planes\", &FrameBuffer::planes)\n> \t\t.def_property(\"cookie\", &FrameBuffer::cookie, &FrameBuffer::setCookie);\n> \n> +\tpyFrameBufferPlane\n> +\t\t.def(py::init())\n> +\t\t.def(py::init([](int fd, unsigned int offset, unsigned int length) {\n> +\t\t\tauto p = FrameBuffer::Plane();\n> +\t\t\tp.fd = SharedFD(fd);\n> +\t\t\tp.offset = offset;\n> +\t\t\tp.length = length;\n> +\t\t\treturn p;\n> +\t\t}), py::arg(\"fd\"), py::arg(\"offset\"), py::arg(\"length\"))\n> +\t\t.def_property(\"fd\",\n> +\t\t\t[](const FrameBuffer::Plane &self) {\n> +\t\t\t\treturn self.fd.get();\n> +\t\t\t},\n> +\t\t\t[](FrameBuffer::Plane &self, int fd) {\n> +\t\t\t\tself.fd = SharedFD(fd);\n> +\t\t\t})\n> +\t\t.def_readwrite(\"offset\", &FrameBuffer::Plane::offset)\n> +\t\t.def_readwrite(\"length\", &FrameBuffer::Plane::length);\n> +\n> \tpyStream\n> \t\t.def_property_readonly(\"configuration\", &Stream::configuration);\n> \n> diff --git a/src/py/libcamera/utils/MappedFrameBuffer.py b/src/py/libcamera/utils/MappedFrameBuffer.py\n> index fc2726b6..69315e76 100644\n> --- a/src/py/libcamera/utils/MappedFrameBuffer.py\n> +++ b/src/py/libcamera/utils/MappedFrameBuffer.py\n> @@ -21,8 +21,8 @@ class MappedFrameBuffer:\n> \n> bufinfos = {}\n> \n> - for i in range(fb.num_planes):\n> - fd = fb.fd(i)\n> + for p in fb.planes:\n\nCould we call the variable plane instead of p ? Same below.\n\nReviewed-by: Laurent Pinchart \n\n> + fd = p.fd\n> \n> if fd not in bufinfos:\n> buflen = os.lseek(fd, 0, os.SEEK_END)\n> @@ -30,11 +30,11 @@ class MappedFrameBuffer:\n> else:\n> buflen = bufinfos[fd]['buflen']\n> \n> - if fb.offset(i) > buflen or fb.offset(i) + fb.length(i) > buflen:\n> + if p.offset > buflen or p.offset + p.length > buflen:\n> raise RuntimeError(f'plane is out of buffer: buffer length={buflen}, ' +\n> - f'plane offset={fb.offset(i)}, plane length={fb.length(i)}')\n> + f'plane offset={p.offset}, plane length={p.length}')\n> \n> - bufinfos[fd]['maplen'] = max(bufinfos[fd]['maplen'], fb.offset(i) + fb.length(i))\n> + bufinfos[fd]['maplen'] = max(bufinfos[fd]['maplen'], p.offset + p.length)\n> \n> # mmap the buffers\n> \n> @@ -51,14 +51,14 @@ class MappedFrameBuffer:\n> \n> planes = []\n> \n> - for i in range(fb.num_planes):\n> - fd = fb.fd(i)\n> + for p in fb.planes:\n> + fd = p.fd\n> info = bufinfos[fd]\n> \n> mv = memoryview(info['map'])\n> \n> - start = fb.offset(i)\n> - end = fb.offset(i) + fb.length(i)\n> + start = p.offset\n> + end = p.offset + p.length\n> \n> mv = mv[start:end]\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 B9A1DBD160\n\tfor ;\n\tMon, 30 May 2022 09:21:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE04765633;\n\tMon, 30 May 2022 11:21:18 +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 CA81E6040B\n\tfor ;\n\tMon, 30 May 2022 11:21:16 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(lmontsouris-659-1-41-236.w92-154.abo.wanadoo.fr [92.154.76.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0E3EC6BD;\n\tMon, 30 May 2022 11:21:16 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653902478;\n\tbh=ktPcpmxUNrmQvdwNrmEd2DE55KrMEFgkQIMtuDewTYI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=zoGGhv4q/dKczWFJRF3hUAnbWpdoUeifJUF+HjQ9B6IQleRaGx0pJUFfQalgIFfm5\n\t3uUOu8Lvd1+pjW//Owa4TfyE1nyANJTgHWHMh6G0/XCFBVkXx1p1glnGAlcxmBDAAl\n\tWRo1k0UXzyN0A/5I43u+nIG+9ywievfjvHHnhGU6R9F8dGDuf0Dd3oxvz3lO7xJGB8\n\t+QApu7Z37n88I3bbrSKe8ssIy8IJcdGWwId1JxYCEAux+75MlJr8qcmJ1KuIQLgBUC\n\toXfxJp6hDohN5mQzqasF+Rts5Yugsmtet4WqW/95f8U/cF9QjmVVMM14eIOBj7R8/g\n\tzTA+vHUJpZvnA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653902476;\n\tbh=ktPcpmxUNrmQvdwNrmEd2DE55KrMEFgkQIMtuDewTYI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BhrsTxfusb936NYiJNmFG7Ei2w2YKd3P2BrlffYTkpAkIgg/tzm8PbCOON3te7R6d\n\tbPOYoJdmMwwkmcz1V4fJ6wENiQnh0EmicRirD5J0gl+NzUAs5ie25VKyzhIfD0S7RI\n\thoML81XFZTXPQUMFoULDnTlY5o92M2ONHZDNGA0g="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"BhrsTxfu\"; dkim-atps=neutral","Date":"Mon, 30 May 2022 12:21:13 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220527144447.94891-1-tomi.valkeinen@ideasonboard.com>\n\t<20220527144447.94891-24-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220527144447.94891-24-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 23/30] py: Implement\n\tFrameBufferPlane","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23237,"web_url":"https://patchwork.libcamera.org/comment/23237/","msgid":"<155a2e64-668f-04a5-c0eb-3a9e5c72c954@ideasonboard.com>","date":"2022-05-30T09:38:58","subject":"Re: [libcamera-devel] [PATCH v3 23/30] py: Implement\n\tFrameBufferPlane","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 30/05/2022 12:21, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> Thank you for the patch.\n> \n> On Fri, May 27, 2022 at 05:44:40PM +0300, Tomi Valkeinen wrote:\n>> Implement FrameBufferPlane class and adjust the methods and uses\n>> accordingly.\n>>\n>> Note that we don't expose the fd as a SharedFD, but as an int.\n> \n> This may cause use-after-free (or rather use-after-close) issues and/or\n> fd leaks, but I think that's OK for now. We can improve it later.\n\nYep. It felt odd to me to implement such a system level helper as \nSharedFD in Python. I'm not sure how this should be exposed.\n\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> src/py/cam/cam_kms.py | 6 +--\n>> src/py/cam/cam_qtgl.py | 2 +-\n>> src/py/libcamera/py_main.cpp | 45 +++++++++++----------\n>> src/py/libcamera/utils/MappedFrameBuffer.py | 18 ++++-----\n>> 4 files changed, 36 insertions(+), 35 deletions(-)\n>>\n>> diff --git a/src/py/cam/cam_kms.py b/src/py/cam/cam_kms.py\n>> index 49b00211..3d1e8c27 100644\n>> --- a/src/py/cam/cam_kms.py\n>> +++ b/src/py/cam/cam_kms.py\n>> @@ -131,10 +131,10 @@ class KMSRenderer:\n>> fds = []\n>> strides = []\n>> offsets = []\n>> - for i in range(fb.num_planes):\n>> - fds.append(fb.fd(i))\n>> + for p in fb.planes:\n>> + fds.append(p.fd)\n>> strides.append(cfg.stride)\n>> - offsets.append(fb.offset(i))\n>> + offsets.append(p.offset)\n>> \n>> drmfb = pykms.DmabufFramebuffer(self.card, w, h, fmt,\n>> fds, strides, offsets)\n>> diff --git a/src/py/cam/cam_qtgl.py b/src/py/cam/cam_qtgl.py\n>> index 4b43f51d..6cfbd347 100644\n>> --- a/src/py/cam/cam_qtgl.py\n>> +++ b/src/py/cam/cam_qtgl.py\n>> @@ -269,7 +269,7 @@ class MainWindow(QtWidgets.QWidget):\n>> EGL_WIDTH, w,\n>> EGL_HEIGHT, h,\n>> EGL_LINUX_DRM_FOURCC_EXT, fmt,\n>> - EGL_DMA_BUF_PLANE0_FD_EXT, fb.fd(0),\n>> + EGL_DMA_BUF_PLANE0_FD_EXT, fb.planes[0].fd,\n>> EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0,\n>> EGL_DMA_BUF_PLANE0_PITCH_EXT, cfg.stride,\n>> EGL_NONE,\n>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n>> index 52f70811..fcf009f0 100644\n>> --- a/src/py/libcamera/py_main.cpp\n>> +++ b/src/py/libcamera/py_main.cpp\n>> @@ -155,6 +155,7 @@ PYBIND11_MODULE(_libcamera, m)\n>> \tauto pyStreamFormats = py::class_(m, \"StreamFormats\");\n>> \tauto pyFrameBufferAllocator = py::class_(m, \"FrameBufferAllocator\");\n>> \tauto pyFrameBuffer = py::class_(m, \"FrameBuffer\");\n>> +\tauto pyFrameBufferPlane = py::class_(pyFrameBuffer, \"Plane\");\n>> \tauto pyStream = py::class_(m, \"Stream\");\n>> \tauto pyControlId = py::class_(m, \"ControlId\");\n>> \tauto pyControlInfo = py::class_(m, \"ControlInfo\");\n>> @@ -408,31 +409,31 @@ PYBIND11_MODULE(_libcamera, m)\n>> \t\t});\n>> \n>> \tpyFrameBuffer\n>> -\t\t/* \\todo implement FrameBuffer::Plane properly */\n>> -\t\t.def(py::init([](std::vector> planes, unsigned int cookie) {\n>> -\t\t\tstd::vector v;\n>> -\t\t\tfor (const auto &t : planes)\n>> -\t\t\t\tv.push_back({ SharedFD(std::get<0>(t)), FrameBuffer::Plane::kInvalidOffset, std::get<1>(t) });\n>> -\t\t\treturn new FrameBuffer(v, cookie);\n>> -\t\t}))\n>> +\t\t.def(py::init, unsigned int>(),\n>> +\t\t py::arg(\"planes\"), py::arg(\"cookie\") = 0)\n>> \t\t.def_property_readonly(\"metadata\", &FrameBuffer::metadata, py::return_value_policy::reference_internal)\n>> -\t\t.def_property_readonly(\"num_planes\", [](const FrameBuffer &self) {\n>> -\t\t\treturn self.planes().size();\n>> -\t\t})\n>> -\t\t.def(\"length\", [](FrameBuffer &self, uint32_t idx) {\n>> -\t\t\tconst FrameBuffer::Plane &plane = self.planes()[idx];\n>> -\t\t\treturn plane.length;\n>> -\t\t})\n>> -\t\t.def(\"fd\", [](FrameBuffer &self, uint32_t idx) {\n>> -\t\t\tconst FrameBuffer::Plane &plane = self.planes()[idx];\n>> -\t\t\treturn plane.fd.get();\n>> -\t\t})\n>> -\t\t.def(\"offset\", [](FrameBuffer &self, uint32_t idx) {\n>> -\t\t\tconst FrameBuffer::Plane &plane = self.planes()[idx];\n>> -\t\t\treturn plane.offset;\n>> -\t\t})\n>> +\t\t.def_property_readonly(\"planes\", &FrameBuffer::planes)\n>> \t\t.def_property(\"cookie\", &FrameBuffer::cookie, &FrameBuffer::setCookie);\n>> \n>> +\tpyFrameBufferPlane\n>> +\t\t.def(py::init())\n>> +\t\t.def(py::init([](int fd, unsigned int offset, unsigned int length) {\n>> +\t\t\tauto p = FrameBuffer::Plane();\n>> +\t\t\tp.fd = SharedFD(fd);\n>> +\t\t\tp.offset = offset;\n>> +\t\t\tp.length = length;\n>> +\t\t\treturn p;\n>> +\t\t}), py::arg(\"fd\"), py::arg(\"offset\"), py::arg(\"length\"))\n>> +\t\t.def_property(\"fd\",\n>> +\t\t\t[](const FrameBuffer::Plane &self) {\n>> +\t\t\t\treturn self.fd.get();\n>> +\t\t\t},\n>> +\t\t\t[](FrameBuffer::Plane &self, int fd) {\n>> +\t\t\t\tself.fd = SharedFD(fd);\n>> +\t\t\t})\n>> +\t\t.def_readwrite(\"offset\", &FrameBuffer::Plane::offset)\n>> +\t\t.def_readwrite(\"length\", &FrameBuffer::Plane::length);\n>> +\n>> \tpyStream\n>> \t\t.def_property_readonly(\"configuration\", &Stream::configuration);\n>> \n>> diff --git a/src/py/libcamera/utils/MappedFrameBuffer.py b/src/py/libcamera/utils/MappedFrameBuffer.py\n>> index fc2726b6..69315e76 100644\n>> --- a/src/py/libcamera/utils/MappedFrameBuffer.py\n>> +++ b/src/py/libcamera/utils/MappedFrameBuffer.py\n>> @@ -21,8 +21,8 @@ class MappedFrameBuffer:\n>> \n>> bufinfos = {}\n>> \n>> - for i in range(fb.num_planes):\n>> - fd = fb.fd(i)\n>> + for p in fb.planes:\n> \n> Could we call the variable plane instead of p ? Same below.\n\nSure.\n\n Tomi","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 542E5BD161\n\tfor ;\n\tMon, 30 May 2022 09:39:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B0EC65633;\n\tMon, 30 May 2022 11:39:03 +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 C09806040B\n\tfor ;\n\tMon, 30 May 2022 11:39:01 +0200 (CEST)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4062E6BD;\n\tMon, 30 May 2022 11:39:01 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653903543;\n\tbh=0Z2aczCILQ0tDtqbdsbqymhO6fbfnZS6JA7gyiCJI8E=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=lSxXqjyZ8+Ugxt/2oggBMAPLq7StRxC9fqgQnTpWIhkJQbz11u5pDgD4obR5WMjTF\n\tFcxxE1ldi/MWCxOHgZs+sZH6L5kx7rM4yPEUGIEP4t8peQtSksWq9Ep+3uD0izWZsg\n\txIuLvz8WW7yeUtCfYCGWjGwitCOx7mv+QCKP2gddobmLk+aPYet1vK/lIXUyhn24Tr\n\t53t9uKJkIxnVHptizyw9tNRipdx/zqGKr91LcRyGMSX+imrNHxZFkKXxhIhXrxReCi\n\tvsGVS9aWld7ClDLlZgkct6aj9dlsWKl7BnHtDTeoTNZ7rS5U4xRGonyIPQyd+uhgPD\n\t/maRxTG7xL0vg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653903541;\n\tbh=0Z2aczCILQ0tDtqbdsbqymhO6fbfnZS6JA7gyiCJI8E=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=jPsCWKWjCU/lelgIA1ggxquEBuy3EsDBPOiPHl9XCBjkBF1tsblI8jPTryukwGjwQ\n\tlavbmtgOeC/8WUqXrsmQVGeymNQe5dSkxL3W0i+HcwL1/kTLWlKiZusyuw0lEHa1Jy\n\twnnO0V37O90m8vDHYV7iTY0ofsZbfGeAWKQbnJyQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jPsCWKWj\"; dkim-atps=neutral","Message-ID":"<155a2e64-668f-04a5-c0eb-3a9e5c72c954@ideasonboard.com>","Date":"Mon, 30 May 2022 12:38:58 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220527144447.94891-1-tomi.valkeinen@ideasonboard.com>\n\t<20220527144447.94891-24-tomi.valkeinen@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 23/30] py: Implement\n\tFrameBufferPlane","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23239,"web_url":"https://patchwork.libcamera.org/comment/23239/","msgid":"","date":"2022-05-30T09:45:53","subject":"Re: [libcamera-devel] [PATCH v3 23/30] py: Implement\n\tFrameBufferPlane","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nOn Mon, May 30, 2022 at 12:38:58PM +0300, Tomi Valkeinen wrote:\n> On 30/05/2022 12:21, Laurent Pinchart wrote:\n> > On Fri, May 27, 2022 at 05:44:40PM +0300, Tomi Valkeinen wrote:\n> >> Implement FrameBufferPlane class and adjust the methods and uses\n> >> accordingly.\n> >>\n> >> Note that we don't expose the fd as a SharedFD, but as an int.\n> > \n> > This may cause use-after-free (or rather use-after-close) issues and/or\n> > fd leaks, but I think that's OK for now. We can improve it later.\n> \n> Yep. It felt odd to me to implement such a system level helper as \n> SharedFD in Python. I'm not sure how this should be exposed.\n\nPython doesn't seem to have a low-level file descriptor object. We could\nwrap the fd in a file using os.fdopen(), but that takes over the fd and\nthus would require a dup(), which isn't nice. We could also create\nbindings for SharedFD. Or something entirely different that would escape\nme right now.\n\n> >> Signed-off-by: Tomi Valkeinen \n> >> ---\n> >> src/py/cam/cam_kms.py | 6 +--\n> >> src/py/cam/cam_qtgl.py | 2 +-\n> >> src/py/libcamera/py_main.cpp | 45 +++++++++++----------\n> >> src/py/libcamera/utils/MappedFrameBuffer.py | 18 ++++-----\n> >> 4 files changed, 36 insertions(+), 35 deletions(-)\n> >>\n> >> diff --git a/src/py/cam/cam_kms.py b/src/py/cam/cam_kms.py\n> >> index 49b00211..3d1e8c27 100644\n> >> --- a/src/py/cam/cam_kms.py\n> >> +++ b/src/py/cam/cam_kms.py\n> >> @@ -131,10 +131,10 @@ class KMSRenderer:\n> >> fds = []\n> >> strides = []\n> >> offsets = []\n> >> - for i in range(fb.num_planes):\n> >> - fds.append(fb.fd(i))\n> >> + for p in fb.planes:\n> >> + fds.append(p.fd)\n> >> strides.append(cfg.stride)\n> >> - offsets.append(fb.offset(i))\n> >> + offsets.append(p.offset)\n> >> \n> >> drmfb = pykms.DmabufFramebuffer(self.card, w, h, fmt,\n> >> fds, strides, offsets)\n> >> diff --git a/src/py/cam/cam_qtgl.py b/src/py/cam/cam_qtgl.py\n> >> index 4b43f51d..6cfbd347 100644\n> >> --- a/src/py/cam/cam_qtgl.py\n> >> +++ b/src/py/cam/cam_qtgl.py\n> >> @@ -269,7 +269,7 @@ class MainWindow(QtWidgets.QWidget):\n> >> EGL_WIDTH, w,\n> >> EGL_HEIGHT, h,\n> >> EGL_LINUX_DRM_FOURCC_EXT, fmt,\n> >> - EGL_DMA_BUF_PLANE0_FD_EXT, fb.fd(0),\n> >> + EGL_DMA_BUF_PLANE0_FD_EXT, fb.planes[0].fd,\n> >> EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0,\n> >> EGL_DMA_BUF_PLANE0_PITCH_EXT, cfg.stride,\n> >> EGL_NONE,\n> >> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> >> index 52f70811..fcf009f0 100644\n> >> --- a/src/py/libcamera/py_main.cpp\n> >> +++ b/src/py/libcamera/py_main.cpp\n> >> @@ -155,6 +155,7 @@ PYBIND11_MODULE(_libcamera, m)\n> >> \tauto pyStreamFormats = py::class_(m, \"StreamFormats\");\n> >> \tauto pyFrameBufferAllocator = py::class_(m, \"FrameBufferAllocator\");\n> >> \tauto pyFrameBuffer = py::class_(m, \"FrameBuffer\");\n> >> +\tauto pyFrameBufferPlane = py::class_(pyFrameBuffer, \"Plane\");\n> >> \tauto pyStream = py::class_(m, \"Stream\");\n> >> \tauto pyControlId = py::class_(m, \"ControlId\");\n> >> \tauto pyControlInfo = py::class_(m, \"ControlInfo\");\n> >> @@ -408,31 +409,31 @@ PYBIND11_MODULE(_libcamera, m)\n> >> \t\t});\n> >> \n> >> \tpyFrameBuffer\n> >> -\t\t/* \\todo implement FrameBuffer::Plane properly */\n> >> -\t\t.def(py::init([](std::vector> planes, unsigned int cookie) {\n> >> -\t\t\tstd::vector v;\n> >> -\t\t\tfor (const auto &t : planes)\n> >> -\t\t\t\tv.push_back({ SharedFD(std::get<0>(t)), FrameBuffer::Plane::kInvalidOffset, std::get<1>(t) });\n> >> -\t\t\treturn new FrameBuffer(v, cookie);\n> >> -\t\t}))\n> >> +\t\t.def(py::init, unsigned int>(),\n> >> +\t\t py::arg(\"planes\"), py::arg(\"cookie\") = 0)\n> >> \t\t.def_property_readonly(\"metadata\", &FrameBuffer::metadata, py::return_value_policy::reference_internal)\n> >> -\t\t.def_property_readonly(\"num_planes\", [](const FrameBuffer &self) {\n> >> -\t\t\treturn self.planes().size();\n> >> -\t\t})\n> >> -\t\t.def(\"length\", [](FrameBuffer &self, uint32_t idx) {\n> >> -\t\t\tconst FrameBuffer::Plane &plane = self.planes()[idx];\n> >> -\t\t\treturn plane.length;\n> >> -\t\t})\n> >> -\t\t.def(\"fd\", [](FrameBuffer &self, uint32_t idx) {\n> >> -\t\t\tconst FrameBuffer::Plane &plane = self.planes()[idx];\n> >> -\t\t\treturn plane.fd.get();\n> >> -\t\t})\n> >> -\t\t.def(\"offset\", [](FrameBuffer &self, uint32_t idx) {\n> >> -\t\t\tconst FrameBuffer::Plane &plane = self.planes()[idx];\n> >> -\t\t\treturn plane.offset;\n> >> -\t\t})\n> >> +\t\t.def_property_readonly(\"planes\", &FrameBuffer::planes)\n> >> \t\t.def_property(\"cookie\", &FrameBuffer::cookie, &FrameBuffer::setCookie);\n> >> \n> >> +\tpyFrameBufferPlane\n> >> +\t\t.def(py::init())\n> >> +\t\t.def(py::init([](int fd, unsigned int offset, unsigned int length) {\n> >> +\t\t\tauto p = FrameBuffer::Plane();\n> >> +\t\t\tp.fd = SharedFD(fd);\n> >> +\t\t\tp.offset = offset;\n> >> +\t\t\tp.length = length;\n> >> +\t\t\treturn p;\n> >> +\t\t}), py::arg(\"fd\"), py::arg(\"offset\"), py::arg(\"length\"))\n> >> +\t\t.def_property(\"fd\",\n> >> +\t\t\t[](const FrameBuffer::Plane &self) {\n> >> +\t\t\t\treturn self.fd.get();\n> >> +\t\t\t},\n> >> +\t\t\t[](FrameBuffer::Plane &self, int fd) {\n> >> +\t\t\t\tself.fd = SharedFD(fd);\n> >> +\t\t\t})\n> >> +\t\t.def_readwrite(\"offset\", &FrameBuffer::Plane::offset)\n> >> +\t\t.def_readwrite(\"length\", &FrameBuffer::Plane::length);\n> >> +\n> >> \tpyStream\n> >> \t\t.def_property_readonly(\"configuration\", &Stream::configuration);\n> >> \n> >> diff --git a/src/py/libcamera/utils/MappedFrameBuffer.py b/src/py/libcamera/utils/MappedFrameBuffer.py\n> >> index fc2726b6..69315e76 100644\n> >> --- a/src/py/libcamera/utils/MappedFrameBuffer.py\n> >> +++ b/src/py/libcamera/utils/MappedFrameBuffer.py\n> >> @@ -21,8 +21,8 @@ class MappedFrameBuffer:\n> >> \n> >> bufinfos = {}\n> >> \n> >> - for i in range(fb.num_planes):\n> >> - fd = fb.fd(i)\n> >> + for p in fb.planes:\n> > \n> > Could we call the variable plane instead of p ? Same below.","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 CD880BD160\n\tfor ;\n\tMon, 30 May 2022 09:45:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 89DE565635;\n\tMon, 30 May 2022 11:45:56 +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 BDDF76040B\n\tfor ;\n\tMon, 30 May 2022 11:45:55 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(lmontsouris-659-1-41-236.w92-154.abo.wanadoo.fr [92.154.76.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6CC6AB90;\n\tMon, 30 May 2022 11:45:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653903956;\n\tbh=Ly0TKYveUefMthD5Y3Zfg457sSNlICQ5UzyKUybqRng=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=QgtQGk/l5vieuPnJP+NH7etRL0JEjUYhd9r4oWvw7qdyX7MPKXz8GHcgNTbd5uzO5\n\tVRs+Rb/SmVFPlaFwSN5SbdawXh7iSDCn8ohgc1r3kujIrGkA4Inw05msGSDTb2XOBK\n\turgztQM0KDQYBxdN28MwfhklhxMGtITE05Y0QRIqeqNp4QHEeZ+R+I6jUWIzCZb5Jo\n\tORhjOJ2SVkhpMvC3Dw4xdccEsAFNJ++Kvbf3HOr7A8PqHjZDbIzo6QpQhatOtTRwnd\n\tgO8YrahssLwLMoLutIGPyKESUEZtpQftE91i9Iec95CdK4o7s6PEmmBXACByQeJ6EH\n\tSVlY5dp5JGBrQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653903955;\n\tbh=Ly0TKYveUefMthD5Y3Zfg457sSNlICQ5UzyKUybqRng=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=evJW4MFxlNPWE4c4rBnAD6dy2nUckFKpZUxsVGN3py/X8w0s0DA70bFeAuksvR9n+\n\tCi7HnWhg8hfRxon3HAOKegG2hsQDA8+qS1Q8Zn0DL0H/bCsASMeBeXBPpJT3bXs+hY\n\tkSP5Ne7LKxzb5J0nVOlHO4ocOZ7WN3GPMxki0CRE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"evJW4MFx\"; dkim-atps=neutral","Date":"Mon, 30 May 2022 12:45:53 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220527144447.94891-1-tomi.valkeinen@ideasonboard.com>\n\t<20220527144447.94891-24-tomi.valkeinen@ideasonboard.com>\n\t\n\t<155a2e64-668f-04a5-c0eb-3a9e5c72c954@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<155a2e64-668f-04a5-c0eb-3a9e5c72c954@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 23/30] py: Implement\n\tFrameBufferPlane","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]