| Message ID | 20220505104104.70841-14-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi, Thank you for the patch. 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@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@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 ? > + > + 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. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp > index b9b52f6b..73d29479 100644 > --- a/src/py/libcamera/pymain.cpp > +++ b/src/py/libcamera/pymain.cpp > @@ -439,6 +439,9 @@ PYBIND11_MODULE(_libcamera, m) > return new FrameBuffer(v, cookie); > })) > .def_property_readonly("metadata", &FrameBuffer::metadata, py::return_value_policy::reference_internal) > + .def_property_readonly("num_planes", [](const FrameBuffer &self) { > + return self.planes().size(); > + }) > .def("length", [](FrameBuffer &self, uint32_t idx) { > const FrameBuffer::Plane &plane = self.planes()[idx]; > return plane.length; > @@ -447,6 +450,10 @@ PYBIND11_MODULE(_libcamera, m) > const FrameBuffer::Plane &plane = self.planes()[idx]; > return plane.fd.get(); > }) > + .def("offset", [](FrameBuffer &self, uint32_t idx) { > + const FrameBuffer::Plane &plane = self.planes()[idx]; > + return plane.offset; > + }) > .def_property("cookie", &FrameBuffer::cookie, &FrameBuffer::setCookie); > > pyStream
On 05/05/2022 21:24, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > 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@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@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. >> + >> + 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. Tomi
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@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@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.
On 06/05/2022 18:22, Laurent Pinchart wrote: > 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@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@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. Right. That's why I wrote the first version as I did. I'll try to find time to read more on the contextmanager recommendations at some point. Tomi
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@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 + + +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} + 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() + + def planes(self): + return self.planes diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp index b9b52f6b..73d29479 100644 --- a/src/py/libcamera/pymain.cpp +++ b/src/py/libcamera/pymain.cpp @@ -439,6 +439,9 @@ PYBIND11_MODULE(_libcamera, m) return new FrameBuffer(v, cookie); })) .def_property_readonly("metadata", &FrameBuffer::metadata, py::return_value_policy::reference_internal) + .def_property_readonly("num_planes", [](const FrameBuffer &self) { + return self.planes().size(); + }) .def("length", [](FrameBuffer &self, uint32_t idx) { const FrameBuffer::Plane &plane = self.planes()[idx]; return plane.length; @@ -447,6 +450,10 @@ PYBIND11_MODULE(_libcamera, m) const FrameBuffer::Plane &plane = self.planes()[idx]; return plane.fd.get(); }) + .def("offset", [](FrameBuffer &self, uint32_t idx) { + const FrameBuffer::Plane &plane = self.planes()[idx]; + return plane.offset; + }) .def_property("cookie", &FrameBuffer::cookie, &FrameBuffer::setCookie); pyStream
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@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(-)