[{"id":22871,"web_url":"https://patchwork.libcamera.org/comment/22871/","msgid":"","date":"2022-05-05T18:24:33","subject":"Re: [libcamera-devel] [PATCH v7 13/13] py: implement\n\tMappedFrameBuffer","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 Thu, May 05, 2022 at 01:41:04PM +0300, Tomi Valkeinen wrote:\n> Instead of just exposing plain mmap via fb.mmap(planenum), implement a\n> MappedFrameBuffer class, similar to C++'s MappedFrameBuffer.\n> MappedFrameBuffer mmaps the underlying filedescriptors and provides\n> Python memoryviews for each plane.\n>\n> As an example, to save a Framebuffer to a file:\n> \n> with fb.mmap() as mfb:\n> \twith open(filename, \"wb\") as f:\n> \t\tfor p in mfb.planes:\n> \t\t\tf.write(p)\n> \n> The objects in mfb.planes are memoryviews that cover only the plane in\n> question.\n> \n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/cam/cam.py | 11 +++---\n> src/py/cam/cam_qt.py | 6 +--\n> src/py/libcamera/__init__.py | 72 +++++++++++++++++++++++++++++++++++-\n> src/py/libcamera/pymain.cpp | 7 ++++\n> 4 files changed, 86 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py\n> index 4efa6459..c0ebb186 100755\n> --- a/src/py/cam/cam.py\n> +++ b/src/py/cam/cam.py\n> @@ -329,9 +329,9 @@ def request_handler(state, ctx, req):\n> \n> crcs = []\n> if ctx[\"opt-crc\"]:\n> - with fb.mmap(0) as b:\n> - crc = binascii.crc32(b)\n> - crcs.append(crc)\n> + with fb.mmap() as mfb:\n> + plane_crcs = [binascii.crc32(p) for p in mfb.planes]\n> + crcs.append(plane_crcs)\n> \n> meta = fb.metadata\n> \n> @@ -347,10 +347,11 @@ def request_handler(state, ctx, req):\n> print(f\"\\t{ctrl} = {val}\")\n> \n> if ctx[\"opt-save-frames\"]:\n> - with fb.mmap(0) as b:\n> + with fb.mmap() as mfb:\n> filename = \"frame-{}-{}-{}.data\".format(ctx[\"id\"], stream_name, ctx[\"reqs-completed\"])\n> with open(filename, \"wb\") as f:\n> - f.write(b)\n> + for p in mfb.planes:\n> + f.write(p)\n> \n> state[\"renderer\"].request_handler(ctx, req)\n> \n> diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py\n> index 30fb7a1d..d394987b 100644\n> --- a/src/py/cam/cam_qt.py\n> +++ b/src/py/cam/cam_qt.py\n> @@ -324,17 +324,17 @@ class MainWindow(QtWidgets.QWidget):\n> controlsLayout.addStretch()\n> \n> def buf_to_qpixmap(self, stream, fb):\n> - with fb.mmap(0) as b:\n> + with fb.mmap() as mfb:\n> cfg = stream.configuration\n> w, h = cfg.size\n> pitch = cfg.stride\n> \n> if cfg.pixelFormat == \"MJPEG\":\n> - img = Image.open(BytesIO(b))\n> + img = Image.open(BytesIO(mfb.planes[0]))\n> qim = ImageQt(img).copy()\n> pix = QtGui.QPixmap.fromImage(qim)\n> else:\n> - data = np.array(b, dtype=np.uint8)\n> + data = np.array(mfb.planes[0], dtype=np.uint8)\n> rgb = to_rgb(cfg.pixelFormat, cfg.size, data)\n> \n> if rgb is None:\n> diff --git a/src/py/libcamera/__init__.py b/src/py/libcamera/__init__.py\n> index cd7512a2..caf06af7 100644\n> --- a/src/py/libcamera/__init__.py\n> +++ b/src/py/libcamera/__init__.py\n> @@ -1,12 +1,80 @@\n> # SPDX-License-Identifier: LGPL-2.1-or-later\n> # Copyright (C) 2021, Tomi Valkeinen \n> \n> +from os import lseek, SEEK_END\n> from ._libcamera import *\n> import mmap\n> \n> \n> -def __FrameBuffer__mmap(self, plane):\n> - return mmap.mmap(self.fd(plane), self.length(plane), mmap.MAP_SHARED, mmap.PROT_READ)\n> +def __FrameBuffer__mmap(self):\n> + return MappedFrameBuffer(self)\n> \n> \n> FrameBuffer.mmap = __FrameBuffer__mmap\n\nI'd move this after the MappedFrameBuffer class definition.\n\n> +\n> +\n> +class MappedFrameBuffer:\n> + def __init__(self, fb):\n> + self.fb = fb\n> +\n> + # Collect information about the buffers\n> +\n> + bufinfos = {}\n> +\n> + for i in range(fb.num_planes):\n> + fd = fb.fd(i)\n> +\n> + if fd not in bufinfos:\n> + buflen = lseek(fd, 0, SEEK_END)\n> + bufinfos[fd] = {\"maplen\": 0, \"buflen\": buflen}\n\nSingle quotes here too please.\n\n> + else:\n> + buflen = bufinfos[fd][\"buflen\"]\n> +\n> + if fb.offset(i) > buflen or fb.offset(i) + fb.length(i) > 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> +\n> + bufinfos[fd][\"maplen\"] = max(bufinfos[fd][\"maplen\"], fb.offset(i) + fb.length(i))\n> +\n> + # mmap the buffers\n> +\n> + maps = []\n> +\n> + for fd, info in bufinfos.items():\n> + map = mmap.mmap(fd, info[\"maplen\"], mmap.MAP_SHARED, mmap.PROT_READ | mmap.PROT_WRITE)\n> + info[\"map\"] = map\n> + maps.append(map)\n> +\n> + self.maps = tuple(maps)\n> +\n> + # Create memoryviews for the planes\n> +\n> + planes = []\n> +\n> + for i in range(fb.num_planes):\n> + fd = fb.fd(i)\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> +\n> + mv = mv[start:end]\n> +\n> + planes.append(mv)\n> +\n> + self.planes = tuple(planes)\n> +\n> + def __enter__(self):\n> + return self\n> +\n> + def __exit__(self, exc_type, exc_value, exc_traceback):\n> + for p in self.planes:\n> + p.release()\n> +\n> + for mm in self.maps:\n> + mm.close()\n\nThis looks a bit unbalanced, with the mapping created in __init__.\nShould the mapping code be moved to __enter__ ? Or is this meant to be\nthis way to avoid forcing the use of the \"with\" construct ? If so, are\n__enter__ and __exit__ needed, given that the object will be destroyed\nonce execution exits from the \"with\" scope ?\n\n> +\n> + def planes(self):\n> + return self.planes\n\nIs this function actually used, or does the Python code access\nself.planes as assigned in __init__ ? You may want to rename the member\nto __planes, and turn this function into a getter for a read-only\nproperty. I would also turn self.maps into self.__maps if not needed by\nthe users of this class.\n\nReviewed-by: Laurent Pinchart \n\n> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp\n> index b9b52f6b..73d29479 100644\n> --- a/src/py/libcamera/pymain.cpp\n> +++ b/src/py/libcamera/pymain.cpp\n> @@ -439,6 +439,9 @@ PYBIND11_MODULE(_libcamera, m)\n> \t\t\treturn new FrameBuffer(v, cookie);\n> \t\t}))\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> @@ -447,6 +450,10 @@ PYBIND11_MODULE(_libcamera, m)\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(\"cookie\", &FrameBuffer::cookie, &FrameBuffer::setCookie);\n> \n> \tpyStream","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 B0F98C0F2A\n\tfor ;\n\tThu, 5 May 2022 18:24:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D371603AB;\n\tThu, 5 May 2022 20:24:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D56C603AB\n\tfor ;\n\tThu, 5 May 2022 20:24:38 +0200 (CEST)","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 623B5492;\n\tThu, 5 May 2022 20:24:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1651775083;\n\tbh=hYYk10KrTusYTcgIjg50crDPiVNwp3q5Q1znY2KaExE=;\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=GWURgr+m/bHM3vGaL9YSK8lmzN8cihyBXpB+54CIksrKl/5pSFFTgJb+UekbpKS/W\n\t/yBkYTPX8lopkgQU8EA4wc+JdfThIKVUsJgUAYyfeTWCx3eHQBg/I6Oye66EHe7QNT\n\tZq+i9QMWPMq0kvQzojpf9gpmhTIe612EZ7VrgH9oq3b4YlcxBgthJHIhcyBKQv3BC5\n\t4+p/RckBIbU/tCjdZzQ6BF6SYYDVNBtXMOzY8CeHS3phdq4gk5I1AlDS6HERiMcAvL\n\t2xUN8jXT/yg02K3bXCAucMNmwui1SnyXhoeP6kbLBYY9rTD8OcZ+M6hwOaw67WwrQS\n\tPECFfJNZOg4/w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1651775077;\n\tbh=hYYk10KrTusYTcgIjg50crDPiVNwp3q5Q1znY2KaExE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WRdsthbW+ic8UWw6Yzzlcn76umtqq9bu0uNfeAkjxt5lBOzmhh85gU8rVjrNiwPeV\n\tSFODV+V5fzsPi965HfkI0nA9WatUO7wN/rxAowZoN5zzuj52giL7wl7h0IxXnKZK4k\n\t3S1H/3OMZLSi6IMt0cFjhmklunCa2rxsS3aRar+c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WRdsthbW\"; dkim-atps=neutral","Date":"Thu, 5 May 2022 21:24:33 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220505104104.70841-1-tomi.valkeinen@ideasonboard.com>\n\t<20220505104104.70841-14-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220505104104.70841-14-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v7 13/13] py: implement\n\tMappedFrameBuffer","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":22885,"web_url":"https://patchwork.libcamera.org/comment/22885/","msgid":"<04d7b8ca-b851-154f-67f4-3423501b953a@ideasonboard.com>","date":"2022-05-06T14:44:23","subject":"Re: [libcamera-devel] [PATCH v7 13/13] py: implement\n\tMappedFrameBuffer","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 05/05/2022 21:24, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> Thank you for the patch.\n> \n> On Thu, May 05, 2022 at 01:41:04PM +0300, Tomi Valkeinen wrote:\n>> Instead of just exposing plain mmap via fb.mmap(planenum), implement a\n>> MappedFrameBuffer class, similar to C++'s MappedFrameBuffer.\n>> MappedFrameBuffer mmaps the underlying filedescriptors and provides\n>> Python memoryviews for each plane.\n>>\n>> As an example, to save a Framebuffer to a file:\n>>\n>> with fb.mmap() as mfb:\n>> \twith open(filename, \"wb\") as f:\n>> \t\tfor p in mfb.planes:\n>> \t\t\tf.write(p)\n>>\n>> The objects in mfb.planes are memoryviews that cover only the plane in\n>> question.\n>>\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> src/py/cam/cam.py | 11 +++---\n>> src/py/cam/cam_qt.py | 6 +--\n>> src/py/libcamera/__init__.py | 72 +++++++++++++++++++++++++++++++++++-\n>> src/py/libcamera/pymain.cpp | 7 ++++\n>> 4 files changed, 86 insertions(+), 10 deletions(-)\n>>\n>> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py\n>> index 4efa6459..c0ebb186 100755\n>> --- a/src/py/cam/cam.py\n>> +++ b/src/py/cam/cam.py\n>> @@ -329,9 +329,9 @@ def request_handler(state, ctx, req):\n>> \n>> crcs = []\n>> if ctx[\"opt-crc\"]:\n>> - with fb.mmap(0) as b:\n>> - crc = binascii.crc32(b)\n>> - crcs.append(crc)\n>> + with fb.mmap() as mfb:\n>> + plane_crcs = [binascii.crc32(p) for p in mfb.planes]\n>> + crcs.append(plane_crcs)\n>> \n>> meta = fb.metadata\n>> \n>> @@ -347,10 +347,11 @@ def request_handler(state, ctx, req):\n>> print(f\"\\t{ctrl} = {val}\")\n>> \n>> if ctx[\"opt-save-frames\"]:\n>> - with fb.mmap(0) as b:\n>> + with fb.mmap() as mfb:\n>> filename = \"frame-{}-{}-{}.data\".format(ctx[\"id\"], stream_name, ctx[\"reqs-completed\"])\n>> with open(filename, \"wb\") as f:\n>> - f.write(b)\n>> + for p in mfb.planes:\n>> + f.write(p)\n>> \n>> state[\"renderer\"].request_handler(ctx, req)\n>> \n>> diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py\n>> index 30fb7a1d..d394987b 100644\n>> --- a/src/py/cam/cam_qt.py\n>> +++ b/src/py/cam/cam_qt.py\n>> @@ -324,17 +324,17 @@ class MainWindow(QtWidgets.QWidget):\n>> controlsLayout.addStretch()\n>> \n>> def buf_to_qpixmap(self, stream, fb):\n>> - with fb.mmap(0) as b:\n>> + with fb.mmap() as mfb:\n>> cfg = stream.configuration\n>> w, h = cfg.size\n>> pitch = cfg.stride\n>> \n>> if cfg.pixelFormat == \"MJPEG\":\n>> - img = Image.open(BytesIO(b))\n>> + img = Image.open(BytesIO(mfb.planes[0]))\n>> qim = ImageQt(img).copy()\n>> pix = QtGui.QPixmap.fromImage(qim)\n>> else:\n>> - data = np.array(b, dtype=np.uint8)\n>> + data = np.array(mfb.planes[0], dtype=np.uint8)\n>> rgb = to_rgb(cfg.pixelFormat, cfg.size, data)\n>> \n>> if rgb is None:\n>> diff --git a/src/py/libcamera/__init__.py b/src/py/libcamera/__init__.py\n>> index cd7512a2..caf06af7 100644\n>> --- a/src/py/libcamera/__init__.py\n>> +++ b/src/py/libcamera/__init__.py\n>> @@ -1,12 +1,80 @@\n>> # SPDX-License-Identifier: LGPL-2.1-or-later\n>> # Copyright (C) 2021, Tomi Valkeinen \n>> \n>> +from os import lseek, SEEK_END\n>> from ._libcamera import *\n>> import mmap\n>> \n>> \n>> -def __FrameBuffer__mmap(self, plane):\n>> - return mmap.mmap(self.fd(plane), self.length(plane), mmap.MAP_SHARED, mmap.PROT_READ)\n>> +def __FrameBuffer__mmap(self):\n>> + return MappedFrameBuffer(self)\n>> \n>> \n>> FrameBuffer.mmap = __FrameBuffer__mmap\n> \n> I'd move this after the MappedFrameBuffer class definition.\n> \n>> +\n>> +\n>> +class MappedFrameBuffer:\n>> + def __init__(self, fb):\n>> + self.fb = fb\n>> +\n>> + # Collect information about the buffers\n>> +\n>> + bufinfos = {}\n>> +\n>> + for i in range(fb.num_planes):\n>> + fd = fb.fd(i)\n>> +\n>> + if fd not in bufinfos:\n>> + buflen = lseek(fd, 0, SEEK_END)\n>> + bufinfos[fd] = {\"maplen\": 0, \"buflen\": buflen}\n> \n> Single quotes here too please.\n> \n>> + else:\n>> + buflen = bufinfos[fd][\"buflen\"]\n>> +\n>> + if fb.offset(i) > buflen or fb.offset(i) + fb.length(i) > 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>> +\n>> + bufinfos[fd][\"maplen\"] = max(bufinfos[fd][\"maplen\"], fb.offset(i) + fb.length(i))\n>> +\n>> + # mmap the buffers\n>> +\n>> + maps = []\n>> +\n>> + for fd, info in bufinfos.items():\n>> + map = mmap.mmap(fd, info[\"maplen\"], mmap.MAP_SHARED, mmap.PROT_READ | mmap.PROT_WRITE)\n>> + info[\"map\"] = map\n>> + maps.append(map)\n>> +\n>> + self.maps = tuple(maps)\n>> +\n>> + # Create memoryviews for the planes\n>> +\n>> + planes = []\n>> +\n>> + for i in range(fb.num_planes):\n>> + fd = fb.fd(i)\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>> +\n>> + mv = mv[start:end]\n>> +\n>> + planes.append(mv)\n>> +\n>> + self.planes = tuple(planes)\n>> +\n>> + def __enter__(self):\n>> + return self\n>> +\n>> + def __exit__(self, exc_type, exc_value, exc_traceback):\n>> + for p in self.planes:\n>> + p.release()\n>> +\n>> + for mm in self.maps:\n>> + mm.close()\n> \n> This looks a bit unbalanced, with the mapping created in __init__.\n> Should the mapping code be moved to __enter__ ? Or is this meant to be\n> this way to avoid forcing the use of the \"with\" construct ? If so, are\n> __enter__ and __exit__ needed, given that the object will be destroyed\n> once execution exits from the \"with\" scope ?\n\nI'm not sure if contextmanager classes are supposed to support multiple \nenter & exit cycles. But I'll move the init code to enter. As you said, \nit doesn't look balanced.\n\n>> +\n>> + def planes(self):\n>> + return self.planes\n> \n> Is this function actually used, or does the Python code access\n> self.planes as assigned in __init__ ? You may want to rename the member\n> to __planes, and turn this function into a getter for a read-only\n> property. I would also turn self.maps into self.__maps if not needed by\n> the users of this class.\n\nRight, I was a bit hasty there. I've hidden the fields, and expose this \nas a property.\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 C1E7EC3256\n\tfor ;\n\tFri, 6 May 2022 14:44:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19DAF65646;\n\tFri, 6 May 2022 16:44:28 +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 CEA56604A3\n\tfor ;\n\tFri, 6 May 2022 16:44:26 +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 10498492;\n\tFri, 6 May 2022 16:44:26 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1651848268;\n\tbh=mDWvL6GwkfEv+ggc2zlfIjWT/EOwSubo8MmSJEulnXQ=;\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=DggFdvqx+EytA1QsiD9fkES1BrN2wXjgmMqWRgsGTk3jZ/HUIRxFjFg8z1viQBrLU\n\t6KeWlzreX68We6qw4M7j9ffgzOfHKhnyFF58T+nfdaGLYR1h/DaBUnLZd88A8tTAlT\n\tgSvnCxHxSc62y3hdnGzFrILmN6I5PJeDPOVNHRn+/UgplwZdDGcYD5Fb00XyX+ZY7h\n\tCaZWOhfHAWJrZ6Vg34eCTkqnIFbG50YMJYjG/m8HBywLJKFq5+hBVCgyDXkuE1wVRD\n\tPXvE0gy1KwQNblsP+Py2OOdmX3O9zgL8Xa7CnGLBAYJDNboR7cUXkJgjJoadxRABfX\n\tOhNYVSR8kOdIw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1651848266;\n\tbh=mDWvL6GwkfEv+ggc2zlfIjWT/EOwSubo8MmSJEulnXQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=N7t4R3oB3wjvYIX0tc7gnJo0SkdVKuP2YZLzrq8C3/vqO6nE3A9+u5nG4f7HEbzuY\n\t4FewVM6vBbZbESYOIInD+B4w85WjKSACUgumIMzmr1cBROIv0fJ0fIkZbIgVxVZShv\n\t2NKcpirXISLCEm8TLjVwhSYuM6z26jnEtbeu7e6M="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"N7t4R3oB\"; dkim-atps=neutral","Message-ID":"<04d7b8ca-b851-154f-67f4-3423501b953a@ideasonboard.com>","Date":"Fri, 6 May 2022 17:44:23 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.8.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220505104104.70841-1-tomi.valkeinen@ideasonboard.com>\n\t<20220505104104.70841-14-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 v7 13/13] py: implement\n\tMappedFrameBuffer","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":22887,"web_url":"https://patchwork.libcamera.org/comment/22887/","msgid":"","date":"2022-05-06T15:22:19","subject":"Re: [libcamera-devel] [PATCH v7 13/13] py: implement\n\tMappedFrameBuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nOn Fri, May 06, 2022 at 05:44:23PM +0300, Tomi Valkeinen wrote:\n> On 05/05/2022 21:24, Laurent Pinchart wrote:\n> > On Thu, May 05, 2022 at 01:41:04PM +0300, Tomi Valkeinen wrote:\n> >> Instead of just exposing plain mmap via fb.mmap(planenum), implement a\n> >> MappedFrameBuffer class, similar to C++'s MappedFrameBuffer.\n> >> MappedFrameBuffer mmaps the underlying filedescriptors and provides\n> >> Python memoryviews for each plane.\n> >>\n> >> As an example, to save a Framebuffer to a file:\n> >>\n> >> with fb.mmap() as mfb:\n> >> \twith open(filename, \"wb\") as f:\n> >> \t\tfor p in mfb.planes:\n> >> \t\t\tf.write(p)\n> >>\n> >> The objects in mfb.planes are memoryviews that cover only the plane in\n> >> question.\n> >>\n> >> Signed-off-by: Tomi Valkeinen \n> >> ---\n> >> src/py/cam/cam.py | 11 +++---\n> >> src/py/cam/cam_qt.py | 6 +--\n> >> src/py/libcamera/__init__.py | 72 +++++++++++++++++++++++++++++++++++-\n> >> src/py/libcamera/pymain.cpp | 7 ++++\n> >> 4 files changed, 86 insertions(+), 10 deletions(-)\n> >>\n> >> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py\n> >> index 4efa6459..c0ebb186 100755\n> >> --- a/src/py/cam/cam.py\n> >> +++ b/src/py/cam/cam.py\n> >> @@ -329,9 +329,9 @@ def request_handler(state, ctx, req):\n> >> \n> >> crcs = []\n> >> if ctx[\"opt-crc\"]:\n> >> - with fb.mmap(0) as b:\n> >> - crc = binascii.crc32(b)\n> >> - crcs.append(crc)\n> >> + with fb.mmap() as mfb:\n> >> + plane_crcs = [binascii.crc32(p) for p in mfb.planes]\n> >> + crcs.append(plane_crcs)\n> >> \n> >> meta = fb.metadata\n> >> \n> >> @@ -347,10 +347,11 @@ def request_handler(state, ctx, req):\n> >> print(f\"\\t{ctrl} = {val}\")\n> >> \n> >> if ctx[\"opt-save-frames\"]:\n> >> - with fb.mmap(0) as b:\n> >> + with fb.mmap() as mfb:\n> >> filename = \"frame-{}-{}-{}.data\".format(ctx[\"id\"], stream_name, ctx[\"reqs-completed\"])\n> >> with open(filename, \"wb\") as f:\n> >> - f.write(b)\n> >> + for p in mfb.planes:\n> >> + f.write(p)\n> >> \n> >> state[\"renderer\"].request_handler(ctx, req)\n> >> \n> >> diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py\n> >> index 30fb7a1d..d394987b 100644\n> >> --- a/src/py/cam/cam_qt.py\n> >> +++ b/src/py/cam/cam_qt.py\n> >> @@ -324,17 +324,17 @@ class MainWindow(QtWidgets.QWidget):\n> >> controlsLayout.addStretch()\n> >> \n> >> def buf_to_qpixmap(self, stream, fb):\n> >> - with fb.mmap(0) as b:\n> >> + with fb.mmap() as mfb:\n> >> cfg = stream.configuration\n> >> w, h = cfg.size\n> >> pitch = cfg.stride\n> >> \n> >> if cfg.pixelFormat == \"MJPEG\":\n> >> - img = Image.open(BytesIO(b))\n> >> + img = Image.open(BytesIO(mfb.planes[0]))\n> >> qim = ImageQt(img).copy()\n> >> pix = QtGui.QPixmap.fromImage(qim)\n> >> else:\n> >> - data = np.array(b, dtype=np.uint8)\n> >> + data = np.array(mfb.planes[0], dtype=np.uint8)\n> >> rgb = to_rgb(cfg.pixelFormat, cfg.size, data)\n> >> \n> >> if rgb is None:\n> >> diff --git a/src/py/libcamera/__init__.py b/src/py/libcamera/__init__.py\n> >> index cd7512a2..caf06af7 100644\n> >> --- a/src/py/libcamera/__init__.py\n> >> +++ b/src/py/libcamera/__init__.py\n> >> @@ -1,12 +1,80 @@\n> >> # SPDX-License-Identifier: LGPL-2.1-or-later\n> >> # Copyright (C) 2021, Tomi Valkeinen \n> >> \n> >> +from os import lseek, SEEK_END\n> >> from ._libcamera import *\n> >> import mmap\n> >> \n> >> \n> >> -def __FrameBuffer__mmap(self, plane):\n> >> - return mmap.mmap(self.fd(plane), self.length(plane), mmap.MAP_SHARED, mmap.PROT_READ)\n> >> +def __FrameBuffer__mmap(self):\n> >> + return MappedFrameBuffer(self)\n> >> \n> >> \n> >> FrameBuffer.mmap = __FrameBuffer__mmap\n> > \n> > I'd move this after the MappedFrameBuffer class definition.\n> > \n> >> +\n> >> +\n> >> +class MappedFrameBuffer:\n> >> + def __init__(self, fb):\n> >> + self.fb = fb\n> >> +\n> >> + # Collect information about the buffers\n> >> +\n> >> + bufinfos = {}\n> >> +\n> >> + for i in range(fb.num_planes):\n> >> + fd = fb.fd(i)\n> >> +\n> >> + if fd not in bufinfos:\n> >> + buflen = lseek(fd, 0, SEEK_END)\n> >> + bufinfos[fd] = {\"maplen\": 0, \"buflen\": buflen}\n> > \n> > Single quotes here too please.\n> > \n> >> + else:\n> >> + buflen = bufinfos[fd][\"buflen\"]\n> >> +\n> >> + if fb.offset(i) > buflen or fb.offset(i) + fb.length(i) > 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> >> +\n> >> + bufinfos[fd][\"maplen\"] = max(bufinfos[fd][\"maplen\"], fb.offset(i) + fb.length(i))\n> >> +\n> >> + # mmap the buffers\n> >> +\n> >> + maps = []\n> >> +\n> >> + for fd, info in bufinfos.items():\n> >> + map = mmap.mmap(fd, info[\"maplen\"], mmap.MAP_SHARED, mmap.PROT_READ | mmap.PROT_WRITE)\n> >> + info[\"map\"] = map\n> >> + maps.append(map)\n> >> +\n> >> + self.maps = tuple(maps)\n> >> +\n> >> + # Create memoryviews for the planes\n> >> +\n> >> + planes = []\n> >> +\n> >> + for i in range(fb.num_planes):\n> >> + fd = fb.fd(i)\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> >> +\n> >> + mv = mv[start:end]\n> >> +\n> >> + planes.append(mv)\n> >> +\n> >> + self.planes = tuple(planes)\n> >> +\n> >> + def __enter__(self):\n> >> + return self\n> >> +\n> >> + def __exit__(self, exc_type, exc_value, exc_traceback):\n> >> + for p in self.planes:\n> >> + p.release()\n> >> +\n> >> + for mm in self.maps:\n> >> + mm.close()\n> > \n> > This looks a bit unbalanced, with the mapping created in __init__.\n> > Should the mapping code be moved to __enter__ ? Or is this meant to be\n> > this way to avoid forcing the use of the \"with\" construct ? If so, are\n> > __enter__ and __exit__ needed, given that the object will be destroyed\n> > once execution exits from the \"with\" scope ?\n> \n> I'm not sure if contextmanager classes are supposed to support multiple \n> enter & exit cycles. But I'll move the init code to enter. As you said, \n> it doesn't look balanced.\n\nIt means that it won't be possible to write\n\n mfb = fb.mmap()\n for plane in mfb.planes:\n ...\n\nbut only\n\n with fb.mmap() as mfb:\n for plane in mfb.planes:\n ...\n\ncompared to open() which allows both\n\n with open('file.name', 'rb') as f:\n f.write(data)\n\nand\n\n f = open('file.name', 'rb')\n f.write(data)\n\nIt may not be a problem, but I wonder if there's some recommendation for\nthis. It can be handled later anyway.\n\n> >> +\n> >> + def planes(self):\n> >> + return self.planes\n> > \n> > Is this function actually used, or does the Python code access\n> > self.planes as assigned in __init__ ? You may want to rename the member\n> > to __planes, and turn this function into a getter for a read-only\n> > property. I would also turn self.maps into self.__maps if not needed by\n> > the users of this class.\n> \n> Right, I was a bit hasty there. I've hidden the fields, and expose this \n> as a property.","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 9873FC0F2A\n\tfor ;\n\tFri, 6 May 2022 15:22:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB8E961655;\n\tFri, 6 May 2022 17:22:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 60A5A604A3\n\tfor ;\n\tFri, 6 May 2022 17:22:24 +0200 (CEST)","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 7A014835;\n\tFri, 6 May 2022 17:22:23 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1651850545;\n\tbh=Fnt4d4/j4mc1tZqX/V1q/02NBzqiurvTuepQL5s99/4=;\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=FHwJMv1EAr4b6jAA+39DmmHk4dq8J5X29mbY9tB+aiWWg8xX1gqwypC5ZXsYw9JdF\n\to45nAro0DIFTKeMPKXDljXt0KzhDB+OSJhk2wcPHsyc2ze2GafMvBDq0O6INYb94gr\n\tIaFdnvf7Q5Rs4srYDlK6MAPL/0AaUUuETQt70k5EuA9UfNL2L3fWaiK6NXVFWPbS4J\n\t9NMraiJvZ+9AKn8g7MzM58iGgt9H0R/4ePg/eSgzHjBgkznDNT17Dqc5sNQFJkwy8f\n\t4YU3dhhCDUvTmDTfHW/WS3e4aKO1acNWtE0+r1raEp5dBc1H6p7gUulGl2AmPXRdUm\n\t43Zn2VlxJc0jg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1651850543;\n\tbh=Fnt4d4/j4mc1tZqX/V1q/02NBzqiurvTuepQL5s99/4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ttRgCZ74EJeyhpY2ZcrcOo/YNOYjF23ob8tdplqI1wVB/18KwSnMbHKWn6/Qc6TlJ\n\tNSFJ8u6E+Tau3uAC0spPBiMCsQmbS+VDLgFcOK9YjXqmQBqkCV8EAQHMGxyS+ySf34\n\tVq/PkVZv60o59QuobKJvR7sEmopgTv1oxF0z65nI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ttRgCZ74\"; dkim-atps=neutral","Date":"Fri, 6 May 2022 18:22:19 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220505104104.70841-1-tomi.valkeinen@ideasonboard.com>\n\t<20220505104104.70841-14-tomi.valkeinen@ideasonboard.com>\n\t\n\t<04d7b8ca-b851-154f-67f4-3423501b953a@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<04d7b8ca-b851-154f-67f4-3423501b953a@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v7 13/13] py: implement\n\tMappedFrameBuffer","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":22894,"web_url":"https://patchwork.libcamera.org/comment/22894/","msgid":"<86671ca1-c81c-a0c0-b9c8-ed2064d909ea@ideasonboard.com>","date":"2022-05-06T16:45:28","subject":"Re: [libcamera-devel] [PATCH v7 13/13] py: implement\n\tMappedFrameBuffer","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 06/05/2022 18:22, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> On Fri, May 06, 2022 at 05:44:23PM +0300, Tomi Valkeinen wrote:\n>> On 05/05/2022 21:24, Laurent Pinchart wrote:\n>>> On Thu, May 05, 2022 at 01:41:04PM +0300, Tomi Valkeinen wrote:\n>>>> Instead of just exposing plain mmap via fb.mmap(planenum), implement a\n>>>> MappedFrameBuffer class, similar to C++'s MappedFrameBuffer.\n>>>> MappedFrameBuffer mmaps the underlying filedescriptors and provides\n>>>> Python memoryviews for each plane.\n>>>>\n>>>> As an example, to save a Framebuffer to a file:\n>>>>\n>>>> with fb.mmap() as mfb:\n>>>> \twith open(filename, \"wb\") as f:\n>>>> \t\tfor p in mfb.planes:\n>>>> \t\t\tf.write(p)\n>>>>\n>>>> The objects in mfb.planes are memoryviews that cover only the plane in\n>>>> question.\n>>>>\n>>>> Signed-off-by: Tomi Valkeinen \n>>>> ---\n>>>> src/py/cam/cam.py | 11 +++---\n>>>> src/py/cam/cam_qt.py | 6 +--\n>>>> src/py/libcamera/__init__.py | 72 +++++++++++++++++++++++++++++++++++-\n>>>> src/py/libcamera/pymain.cpp | 7 ++++\n>>>> 4 files changed, 86 insertions(+), 10 deletions(-)\n>>>>\n>>>> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py\n>>>> index 4efa6459..c0ebb186 100755\n>>>> --- a/src/py/cam/cam.py\n>>>> +++ b/src/py/cam/cam.py\n>>>> @@ -329,9 +329,9 @@ def request_handler(state, ctx, req):\n>>>> \n>>>> crcs = []\n>>>> if ctx[\"opt-crc\"]:\n>>>> - with fb.mmap(0) as b:\n>>>> - crc = binascii.crc32(b)\n>>>> - crcs.append(crc)\n>>>> + with fb.mmap() as mfb:\n>>>> + plane_crcs = [binascii.crc32(p) for p in mfb.planes]\n>>>> + crcs.append(plane_crcs)\n>>>> \n>>>> meta = fb.metadata\n>>>> \n>>>> @@ -347,10 +347,11 @@ def request_handler(state, ctx, req):\n>>>> print(f\"\\t{ctrl} = {val}\")\n>>>> \n>>>> if ctx[\"opt-save-frames\"]:\n>>>> - with fb.mmap(0) as b:\n>>>> + with fb.mmap() as mfb:\n>>>> filename = \"frame-{}-{}-{}.data\".format(ctx[\"id\"], stream_name, ctx[\"reqs-completed\"])\n>>>> with open(filename, \"wb\") as f:\n>>>> - f.write(b)\n>>>> + for p in mfb.planes:\n>>>> + f.write(p)\n>>>> \n>>>> state[\"renderer\"].request_handler(ctx, req)\n>>>> \n>>>> diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py\n>>>> index 30fb7a1d..d394987b 100644\n>>>> --- a/src/py/cam/cam_qt.py\n>>>> +++ b/src/py/cam/cam_qt.py\n>>>> @@ -324,17 +324,17 @@ class MainWindow(QtWidgets.QWidget):\n>>>> controlsLayout.addStretch()\n>>>> \n>>>> def buf_to_qpixmap(self, stream, fb):\n>>>> - with fb.mmap(0) as b:\n>>>> + with fb.mmap() as mfb:\n>>>> cfg = stream.configuration\n>>>> w, h = cfg.size\n>>>> pitch = cfg.stride\n>>>> \n>>>> if cfg.pixelFormat == \"MJPEG\":\n>>>> - img = Image.open(BytesIO(b))\n>>>> + img = Image.open(BytesIO(mfb.planes[0]))\n>>>> qim = ImageQt(img).copy()\n>>>> pix = QtGui.QPixmap.fromImage(qim)\n>>>> else:\n>>>> - data = np.array(b, dtype=np.uint8)\n>>>> + data = np.array(mfb.planes[0], dtype=np.uint8)\n>>>> rgb = to_rgb(cfg.pixelFormat, cfg.size, data)\n>>>> \n>>>> if rgb is None:\n>>>> diff --git a/src/py/libcamera/__init__.py b/src/py/libcamera/__init__.py\n>>>> index cd7512a2..caf06af7 100644\n>>>> --- a/src/py/libcamera/__init__.py\n>>>> +++ b/src/py/libcamera/__init__.py\n>>>> @@ -1,12 +1,80 @@\n>>>> # SPDX-License-Identifier: LGPL-2.1-or-later\n>>>> # Copyright (C) 2021, Tomi Valkeinen \n>>>> \n>>>> +from os import lseek, SEEK_END\n>>>> from ._libcamera import *\n>>>> import mmap\n>>>> \n>>>> \n>>>> -def __FrameBuffer__mmap(self, plane):\n>>>> - return mmap.mmap(self.fd(plane), self.length(plane), mmap.MAP_SHARED, mmap.PROT_READ)\n>>>> +def __FrameBuffer__mmap(self):\n>>>> + return MappedFrameBuffer(self)\n>>>> \n>>>> \n>>>> FrameBuffer.mmap = __FrameBuffer__mmap\n>>>\n>>> I'd move this after the MappedFrameBuffer class definition.\n>>>\n>>>> +\n>>>> +\n>>>> +class MappedFrameBuffer:\n>>>> + def __init__(self, fb):\n>>>> + self.fb = fb\n>>>> +\n>>>> + # Collect information about the buffers\n>>>> +\n>>>> + bufinfos = {}\n>>>> +\n>>>> + for i in range(fb.num_planes):\n>>>> + fd = fb.fd(i)\n>>>> +\n>>>> + if fd not in bufinfos:\n>>>> + buflen = lseek(fd, 0, SEEK_END)\n>>>> + bufinfos[fd] = {\"maplen\": 0, \"buflen\": buflen}\n>>>\n>>> Single quotes here too please.\n>>>\n>>>> + else:\n>>>> + buflen = bufinfos[fd][\"buflen\"]\n>>>> +\n>>>> + if fb.offset(i) > buflen or fb.offset(i) + fb.length(i) > 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>>>> +\n>>>> + bufinfos[fd][\"maplen\"] = max(bufinfos[fd][\"maplen\"], fb.offset(i) + fb.length(i))\n>>>> +\n>>>> + # mmap the buffers\n>>>> +\n>>>> + maps = []\n>>>> +\n>>>> + for fd, info in bufinfos.items():\n>>>> + map = mmap.mmap(fd, info[\"maplen\"], mmap.MAP_SHARED, mmap.PROT_READ | mmap.PROT_WRITE)\n>>>> + info[\"map\"] = map\n>>>> + maps.append(map)\n>>>> +\n>>>> + self.maps = tuple(maps)\n>>>> +\n>>>> + # Create memoryviews for the planes\n>>>> +\n>>>> + planes = []\n>>>> +\n>>>> + for i in range(fb.num_planes):\n>>>> + fd = fb.fd(i)\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>>>> +\n>>>> + mv = mv[start:end]\n>>>> +\n>>>> + planes.append(mv)\n>>>> +\n>>>> + self.planes = tuple(planes)\n>>>> +\n>>>> + def __enter__(self):\n>>>> + return self\n>>>> +\n>>>> + def __exit__(self, exc_type, exc_value, exc_traceback):\n>>>> + for p in self.planes:\n>>>> + p.release()\n>>>> +\n>>>> + for mm in self.maps:\n>>>> + mm.close()\n>>>\n>>> This looks a bit unbalanced, with the mapping created in __init__.\n>>> Should the mapping code be moved to __enter__ ? Or is this meant to be\n>>> this way to avoid forcing the use of the \"with\" construct ? If so, are\n>>> __enter__ and __exit__ needed, given that the object will be destroyed\n>>> once execution exits from the \"with\" scope ?\n>>\n>> I'm not sure if contextmanager classes are supposed to support multiple\n>> enter & exit cycles. But I'll move the init code to enter. As you said,\n>> it doesn't look balanced.\n> \n> It means that it won't be possible to write\n> \n> mfb = fb.mmap()\n> for plane in mfb.planes:\n> ...\n> \n> but only\n> \n> with fb.mmap() as mfb:\n> for plane in mfb.planes:\n> ...\n> \n> compared to open() which allows both\n> \n> with open('file.name', 'rb') as f:\n> f.write(data)\n> \n> and\n> \n> f = open('file.name', 'rb')\n> f.write(data)\n> \n> It may not be a problem, but I wonder if there's some recommendation for\n> this. It can be handled later anyway.\n\nRight. That's why I wrote the first version as I did. I'll try to find \ntime to read more on the contextmanager recommendations at some point.\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 82BE1C0F2A\n\tfor ;\n\tFri, 6 May 2022 16:45:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C91C165646;\n\tFri, 6 May 2022 18:45:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 00BDD604A3\n\tfor ;\n\tFri, 6 May 2022 18:45:31 +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 3316E492;\n\tFri, 6 May 2022 18:45:31 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1651855532;\n\tbh=PBEDYDrXerjL25P/M6gK/YPWyJo5j4q5toSN2MM1YOE=;\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=3AdmbyQjRq3WKoYlJeKXy3eP+7eWrm7I4biYEHTzVpmzqwTvJUo9vGnalh3TshV5y\n\tgTlwlK3L+grP3pIQ990gFr4zAzUUOa/v6NiZ2X5JmMqXf+VCUzdOTuA4wSuHq9YJxU\n\tRWGIGkoO8jh5CS+r1xj/m50iWuAcqmsR2YiBJNUcdpeX+ixmur04+Dsbl6yOjYpjSR\n\ttT2ZKXGpF00TFIxpXL8/a8lpmNSEL10AQAMZ/A6Ef4vzOzkFicTqZxsREoGqlt14pQ\n\tiY7ujj+AI4GXiLYUg9R7skpEjK1WdF2JrSi5/oMyefmMjaqa+8Az0yv7uGvo/0W3Z3\n\tl0anc3Dq6z7cw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1651855531;\n\tbh=PBEDYDrXerjL25P/M6gK/YPWyJo5j4q5toSN2MM1YOE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=t5KI4p+Ws5/xySQvcLLpHn1mJIziDf3bempGTj5zR8PfMpojexFqs53bW43YSRU1A\n\tZB7Pco2i65sBLKMe0JWyH+EpWFMCTdkAugy8hgFhafO8N2WJFYHLU0x9IaH4RYtOf9\n\twgoqe9rqiLEKYElJ8RLvGTYf99ieIKwfxGnuaVqA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"t5KI4p+W\"; dkim-atps=neutral","Message-ID":"<86671ca1-c81c-a0c0-b9c8-ed2064d909ea@ideasonboard.com>","Date":"Fri, 6 May 2022 19:45:28 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.8.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220505104104.70841-1-tomi.valkeinen@ideasonboard.com>\n\t<20220505104104.70841-14-tomi.valkeinen@ideasonboard.com>\n\t\n\t<04d7b8ca-b851-154f-67f4-3423501b953a@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 v7 13/13] py: implement\n\tMappedFrameBuffer","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\" "}}]