[libcamera-devel] [PATCH v3 23/30] py: Implement FrameBufferPlane
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon May 30 11:45:53 CEST 2022
Hi Tomi,
On Mon, May 30, 2022 at 12:38:58PM +0300, Tomi Valkeinen wrote:
> On 30/05/2022 12:21, Laurent Pinchart wrote:
> > On Fri, May 27, 2022 at 05:44:40PM +0300, Tomi Valkeinen wrote:
> >> Implement FrameBufferPlane class and adjust the methods and uses
> >> accordingly.
> >>
> >> Note that we don't expose the fd as a SharedFD, but as an int.
> >
> > This may cause use-after-free (or rather use-after-close) issues and/or
> > fd leaks, but I think that's OK for now. We can improve it later.
>
> Yep. It felt odd to me to implement such a system level helper as
> SharedFD in Python. I'm not sure how this should be exposed.
Python doesn't seem to have a low-level file descriptor object. We could
wrap the fd in a file using os.fdopen(), but that takes over the fd and
thus would require a dup(), which isn't nice. We could also create
bindings for SharedFD. Or something entirely different that would escape
me right now.
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> >> ---
> >> src/py/cam/cam_kms.py | 6 +--
> >> src/py/cam/cam_qtgl.py | 2 +-
> >> src/py/libcamera/py_main.cpp | 45 +++++++++++----------
> >> src/py/libcamera/utils/MappedFrameBuffer.py | 18 ++++-----
> >> 4 files changed, 36 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/src/py/cam/cam_kms.py b/src/py/cam/cam_kms.py
> >> index 49b00211..3d1e8c27 100644
> >> --- a/src/py/cam/cam_kms.py
> >> +++ b/src/py/cam/cam_kms.py
> >> @@ -131,10 +131,10 @@ class KMSRenderer:
> >> fds = []
> >> strides = []
> >> offsets = []
> >> - for i in range(fb.num_planes):
> >> - fds.append(fb.fd(i))
> >> + for p in fb.planes:
> >> + fds.append(p.fd)
> >> strides.append(cfg.stride)
> >> - offsets.append(fb.offset(i))
> >> + offsets.append(p.offset)
> >>
> >> drmfb = pykms.DmabufFramebuffer(self.card, w, h, fmt,
> >> fds, strides, offsets)
> >> diff --git a/src/py/cam/cam_qtgl.py b/src/py/cam/cam_qtgl.py
> >> index 4b43f51d..6cfbd347 100644
> >> --- a/src/py/cam/cam_qtgl.py
> >> +++ b/src/py/cam/cam_qtgl.py
> >> @@ -269,7 +269,7 @@ class MainWindow(QtWidgets.QWidget):
> >> EGL_WIDTH, w,
> >> EGL_HEIGHT, h,
> >> EGL_LINUX_DRM_FOURCC_EXT, fmt,
> >> - EGL_DMA_BUF_PLANE0_FD_EXT, fb.fd(0),
> >> + EGL_DMA_BUF_PLANE0_FD_EXT, fb.planes[0].fd,
> >> EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0,
> >> EGL_DMA_BUF_PLANE0_PITCH_EXT, cfg.stride,
> >> EGL_NONE,
> >> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> >> index 52f70811..fcf009f0 100644
> >> --- a/src/py/libcamera/py_main.cpp
> >> +++ b/src/py/libcamera/py_main.cpp
> >> @@ -155,6 +155,7 @@ PYBIND11_MODULE(_libcamera, m)
> >> auto pyStreamFormats = py::class_<StreamFormats>(m, "StreamFormats");
> >> auto pyFrameBufferAllocator = py::class_<FrameBufferAllocator>(m, "FrameBufferAllocator");
> >> auto pyFrameBuffer = py::class_<FrameBuffer>(m, "FrameBuffer");
> >> + auto pyFrameBufferPlane = py::class_<FrameBuffer::Plane>(pyFrameBuffer, "Plane");
> >> auto pyStream = py::class_<Stream>(m, "Stream");
> >> auto pyControlId = py::class_<ControlId>(m, "ControlId");
> >> auto pyControlInfo = py::class_<ControlInfo>(m, "ControlInfo");
> >> @@ -408,31 +409,31 @@ PYBIND11_MODULE(_libcamera, m)
> >> });
> >>
> >> pyFrameBuffer
> >> - /* \todo implement FrameBuffer::Plane properly */
> >> - .def(py::init([](std::vector<std::tuple<int, unsigned int>> planes, unsigned int cookie) {
> >> - std::vector<FrameBuffer::Plane> v;
> >> - for (const auto &t : planes)
> >> - v.push_back({ SharedFD(std::get<0>(t)), FrameBuffer::Plane::kInvalidOffset, std::get<1>(t) });
> >> - return new FrameBuffer(v, cookie);
> >> - }))
> >> + .def(py::init<std::vector<FrameBuffer::Plane>, unsigned int>(),
> >> + py::arg("planes"), py::arg("cookie") = 0)
> >> .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;
> >> - })
> >> - .def("fd", [](FrameBuffer &self, uint32_t idx) {
> >> - 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_readonly("planes", &FrameBuffer::planes)
> >> .def_property("cookie", &FrameBuffer::cookie, &FrameBuffer::setCookie);
> >>
> >> + pyFrameBufferPlane
> >> + .def(py::init())
> >> + .def(py::init([](int fd, unsigned int offset, unsigned int length) {
> >> + auto p = FrameBuffer::Plane();
> >> + p.fd = SharedFD(fd);
> >> + p.offset = offset;
> >> + p.length = length;
> >> + return p;
> >> + }), py::arg("fd"), py::arg("offset"), py::arg("length"))
> >> + .def_property("fd",
> >> + [](const FrameBuffer::Plane &self) {
> >> + return self.fd.get();
> >> + },
> >> + [](FrameBuffer::Plane &self, int fd) {
> >> + self.fd = SharedFD(fd);
> >> + })
> >> + .def_readwrite("offset", &FrameBuffer::Plane::offset)
> >> + .def_readwrite("length", &FrameBuffer::Plane::length);
> >> +
> >> pyStream
> >> .def_property_readonly("configuration", &Stream::configuration);
> >>
> >> diff --git a/src/py/libcamera/utils/MappedFrameBuffer.py b/src/py/libcamera/utils/MappedFrameBuffer.py
> >> index fc2726b6..69315e76 100644
> >> --- a/src/py/libcamera/utils/MappedFrameBuffer.py
> >> +++ b/src/py/libcamera/utils/MappedFrameBuffer.py
> >> @@ -21,8 +21,8 @@ class MappedFrameBuffer:
> >>
> >> bufinfos = {}
> >>
> >> - for i in range(fb.num_planes):
> >> - fd = fb.fd(i)
> >> + for p in fb.planes:
> >
> > Could we call the variable plane instead of p ? Same below.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list