[libcamera-devel] [PATCH v3 23/30] py: Implement FrameBufferPlane

[Tomi Valkeinen]

On 30/05/2022 12:21, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> 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.

>> 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.

Sure.

  Tomi