[libcamera-devel] [PATCH v7 13/13] py: implement MappedFrameBuffer
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri May 6 17:22:19 CEST 2022
Hi Tomi,
On Fri, May 06, 2022 at 05:44:23PM +0300, Tomi Valkeinen wrote:
> On 05/05/2022 21:24, Laurent Pinchart wrote:
> > On Thu, May 05, 2022 at 01:41:04PM +0300, Tomi Valkeinen wrote:
> >> Instead of just exposing plain mmap via fb.mmap(planenum), implement a
> >> MappedFrameBuffer class, similar to C++'s MappedFrameBuffer.
> >> MappedFrameBuffer mmaps the underlying filedescriptors and provides
> >> Python memoryviews for each plane.
> >>
> >> As an example, to save a Framebuffer to a file:
> >>
> >> with fb.mmap() as mfb:
> >> with open(filename, "wb") as f:
> >> for p in mfb.planes:
> >> f.write(p)
> >>
> >> The objects in mfb.planes are memoryviews that cover only the plane in
> >> question.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> >> ---
> >> src/py/cam/cam.py | 11 +++---
> >> src/py/cam/cam_qt.py | 6 +--
> >> src/py/libcamera/__init__.py | 72 +++++++++++++++++++++++++++++++++++-
> >> src/py/libcamera/pymain.cpp | 7 ++++
> >> 4 files changed, 86 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py
> >> index 4efa6459..c0ebb186 100755
> >> --- a/src/py/cam/cam.py
> >> +++ b/src/py/cam/cam.py
> >> @@ -329,9 +329,9 @@ def request_handler(state, ctx, req):
> >>
> >> crcs = []
> >> if ctx["opt-crc"]:
> >> - with fb.mmap(0) as b:
> >> - crc = binascii.crc32(b)
> >> - crcs.append(crc)
> >> + with fb.mmap() as mfb:
> >> + plane_crcs = [binascii.crc32(p) for p in mfb.planes]
> >> + crcs.append(plane_crcs)
> >>
> >> meta = fb.metadata
> >>
> >> @@ -347,10 +347,11 @@ def request_handler(state, ctx, req):
> >> print(f"\t{ctrl} = {val}")
> >>
> >> if ctx["opt-save-frames"]:
> >> - with fb.mmap(0) as b:
> >> + with fb.mmap() as mfb:
> >> filename = "frame-{}-{}-{}.data".format(ctx["id"], stream_name, ctx["reqs-completed"])
> >> with open(filename, "wb") as f:
> >> - f.write(b)
> >> + for p in mfb.planes:
> >> + f.write(p)
> >>
> >> state["renderer"].request_handler(ctx, req)
> >>
> >> diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py
> >> index 30fb7a1d..d394987b 100644
> >> --- a/src/py/cam/cam_qt.py
> >> +++ b/src/py/cam/cam_qt.py
> >> @@ -324,17 +324,17 @@ class MainWindow(QtWidgets.QWidget):
> >> controlsLayout.addStretch()
> >>
> >> def buf_to_qpixmap(self, stream, fb):
> >> - with fb.mmap(0) as b:
> >> + with fb.mmap() as mfb:
> >> cfg = stream.configuration
> >> w, h = cfg.size
> >> pitch = cfg.stride
> >>
> >> if cfg.pixelFormat == "MJPEG":
> >> - img = Image.open(BytesIO(b))
> >> + img = Image.open(BytesIO(mfb.planes[0]))
> >> qim = ImageQt(img).copy()
> >> pix = QtGui.QPixmap.fromImage(qim)
> >> else:
> >> - data = np.array(b, dtype=np.uint8)
> >> + data = np.array(mfb.planes[0], dtype=np.uint8)
> >> rgb = to_rgb(cfg.pixelFormat, cfg.size, data)
> >>
> >> if rgb is None:
> >> diff --git a/src/py/libcamera/__init__.py b/src/py/libcamera/__init__.py
> >> index cd7512a2..caf06af7 100644
> >> --- a/src/py/libcamera/__init__.py
> >> +++ b/src/py/libcamera/__init__.py
> >> @@ -1,12 +1,80 @@
> >> # SPDX-License-Identifier: LGPL-2.1-or-later
> >> # Copyright (C) 2021, Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> >>
> >> +from os import lseek, SEEK_END
> >> from ._libcamera import *
> >> import mmap
> >>
> >>
> >> -def __FrameBuffer__mmap(self, plane):
> >> - return mmap.mmap(self.fd(plane), self.length(plane), mmap.MAP_SHARED, mmap.PROT_READ)
> >> +def __FrameBuffer__mmap(self):
> >> + return MappedFrameBuffer(self)
> >>
> >>
> >> FrameBuffer.mmap = __FrameBuffer__mmap
> >
> > I'd move this after the MappedFrameBuffer class definition.
> >
> >> +
> >> +
> >> +class MappedFrameBuffer:
> >> + def __init__(self, fb):
> >> + self.fb = fb
> >> +
> >> + # Collect information about the buffers
> >> +
> >> + bufinfos = {}
> >> +
> >> + for i in range(fb.num_planes):
> >> + fd = fb.fd(i)
> >> +
> >> + if fd not in bufinfos:
> >> + buflen = lseek(fd, 0, SEEK_END)
> >> + bufinfos[fd] = {"maplen": 0, "buflen": buflen}
> >
> > Single quotes here too please.
> >
> >> + else:
> >> + buflen = bufinfos[fd]["buflen"]
> >> +
> >> + if fb.offset(i) > buflen or fb.offset(i) + fb.length(i) > buflen:
> >> + raise RuntimeError(f"plane is out of buffer: buffer length={buflen}, "
> >> + f"plane offset={fb.offset(i)}, plane length={fb.length(i)}")
> >> +
> >> + bufinfos[fd]["maplen"] = max(bufinfos[fd]["maplen"], fb.offset(i) + fb.length(i))
> >> +
> >> + # mmap the buffers
> >> +
> >> + maps = []
> >> +
> >> + for fd, info in bufinfos.items():
> >> + map = mmap.mmap(fd, info["maplen"], mmap.MAP_SHARED, mmap.PROT_READ | mmap.PROT_WRITE)
> >> + info["map"] = map
> >> + maps.append(map)
> >> +
> >> + self.maps = tuple(maps)
> >> +
> >> + # Create memoryviews for the planes
> >> +
> >> + planes = []
> >> +
> >> + for i in range(fb.num_planes):
> >> + fd = fb.fd(i)
> >> + info = bufinfos[fd]
> >> +
> >> + mv = memoryview(info["map"])
> >> +
> >> + start = fb.offset(i)
> >> + end = fb.offset(i) + fb.length(i)
> >> +
> >> + mv = mv[start:end]
> >> +
> >> + planes.append(mv)
> >> +
> >> + self.planes = tuple(planes)
> >> +
> >> + def __enter__(self):
> >> + return self
> >> +
> >> + def __exit__(self, exc_type, exc_value, exc_traceback):
> >> + for p in self.planes:
> >> + p.release()
> >> +
> >> + for mm in self.maps:
> >> + mm.close()
> >
> > This looks a bit unbalanced, with the mapping created in __init__.
> > Should the mapping code be moved to __enter__ ? Or is this meant to be
> > this way to avoid forcing the use of the "with" construct ? If so, are
> > __enter__ and __exit__ needed, given that the object will be destroyed
> > once execution exits from the "with" scope ?
>
> I'm not sure if contextmanager classes are supposed to support multiple
> enter & exit cycles. But I'll move the init code to enter. As you said,
> it doesn't look balanced.
It means that it won't be possible to write
mfb = fb.mmap()
for plane in mfb.planes:
...
but only
with fb.mmap() as mfb:
for plane in mfb.planes:
...
compared to open() which allows both
with open('file.name', 'rb') as f:
f.write(data)
and
f = open('file.name', 'rb')
f.write(data)
It may not be a problem, but I wonder if there's some recommendation for
this. It can be handled later anyway.
> >> +
> >> + def planes(self):
> >> + return self.planes
> >
> > Is this function actually used, or does the Python code access
> > self.planes as assigned in __init__ ? You may want to rename the member
> > to __planes, and turn this function into a getter for a read-only
> > property. I would also turn self.maps into self.__maps if not needed by
> > the users of this class.
>
> Right, I was a bit hasty there. I've hidden the fields, and expose this
> as a property.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list