[libcamera-devel] [PATCH v7 04/13] Add Python bindings
Tomi Valkeinen
tomi.valkeinen at ideasonboard.com
Fri May 6 10:11:46 CEST 2022
Hi,
On 05/05/2022 20:32, Laurent Pinchart wrote:
> On Thu, May 05, 2022 at 02:51:34PM +0100, Kieran Bingham wrote:
>> Quoting Tomi Valkeinen (2022-05-05 11:40:55)
>>> Add libcamera Python bindings. pybind11 is used to generate the C++ <->
>>> Python layer.
>>>
>>> We use pybind11 'smart_holder' version to avoid issues with private
>>> destructors and shared_ptr. There is also an alternative solution here:
>>>
>>> https://github.com/pybind/pybind11/pull/2067
>>>
>>> Only a subset of libcamera classes are exposed. Implementing and testing
>>> the wrapper classes is challenging, and as such only classes that I have
>>> needed have been added so far.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
>>> ---
>>> meson.build | 1 +
>>> meson_options.txt | 5 +
>>> src/meson.build | 1 +
>>> src/py/libcamera/__init__.py | 12 +
>>> src/py/libcamera/meson.build | 44 ++
>>> src/py/libcamera/pyenums.cpp | 53 ++
>>> src/py/libcamera/pymain.cpp | 452 ++++++++++++++++++
>>> src/py/meson.build | 1 +
>>> subprojects/.gitignore | 3 +-
>>> subprojects/packagefiles/pybind11/meson.build | 8 +
>>> subprojects/pybind11.wrap | 8 +
>>> 11 files changed, 587 insertions(+), 1 deletion(-)
>>> create mode 100644 src/py/libcamera/__init__.py
>>> create mode 100644 src/py/libcamera/meson.build
>>> create mode 100644 src/py/libcamera/pyenums.cpp
>>> create mode 100644 src/py/libcamera/pymain.cpp
>>> create mode 100644 src/py/meson.build
>>> create mode 100644 subprojects/packagefiles/pybind11/meson.build
>>> create mode 100644 subprojects/pybind11.wrap
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 0124e7d3..60a911e0 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -177,6 +177,7 @@ summary({
>>> 'Tracing support': tracing_enabled,
>>> 'Android support': android_enabled,
>>> 'GStreamer support': gst_enabled,
>>> + 'Python bindings': pycamera_enabled,
>>> 'V4L2 emulation support': v4l2_enabled,
>>> 'cam application': cam_enabled,
>>> 'qcam application': qcam_enabled,
>>> diff --git a/meson_options.txt b/meson_options.txt
>>> index 2c80ad8b..ca00c78e 100644
>>> --- a/meson_options.txt
>>> +++ b/meson_options.txt
>>> @@ -58,3 +58,8 @@ option('v4l2',
>>> type : 'boolean',
>>> value : false,
>>> description : 'Compile the V4L2 compatibility layer')
>>> +
>>> +option('pycamera',
>>> + type : 'feature',
>>> + value : 'auto',
>>> + description : 'Enable libcamera Python bindings (experimental)')
>>> diff --git a/src/meson.build b/src/meson.build
>>> index e0ea9c35..34663a6f 100644
>>> --- a/src/meson.build
>>> +++ b/src/meson.build
>>> @@ -37,4 +37,5 @@ subdir('cam')
>>> subdir('qcam')
>>>
>>> subdir('gstreamer')
>>> +subdir('py')
>>> subdir('v4l2')
>>> diff --git a/src/py/libcamera/__init__.py b/src/py/libcamera/__init__.py
>>> new file mode 100644
>>> index 00000000..cd7512a2
>>> --- /dev/null
>>> +++ b/src/py/libcamera/__init__.py
>>> @@ -0,0 +1,12 @@
>>> +# SPDX-License-Identifier: LGPL-2.1-or-later
>>> +# Copyright (C) 2021, Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
>
> You may want to bump the copyright year :-) Same below.
>
>>> +
>>> +from ._libcamera import *
>>> +import mmap
>>> +
>>> +
>>> +def __FrameBuffer__mmap(self, plane):
>
> Let's record that more work is needed:
>
> # \todo Support multi-planar formats
This is implemented in a later patch in the series. I didn't want to
squash to highlight the change.
>>> + return mmap.mmap(self.fd(plane), self.length(plane), mmap.MAP_SHARED, mmap.PROT_READ)
>>> +
>>> +
>>> +FrameBuffer.mmap = __FrameBuffer__mmap
>>> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build
>>> new file mode 100644
>>> index 00000000..e1f5cf21
>>> --- /dev/null
>>> +++ b/src/py/libcamera/meson.build
>>> @@ -0,0 +1,44 @@
>>> +# SPDX-License-Identifier: CC0-1.0
>>> +
>>> +py3_dep = dependency('python3', required : get_option('pycamera'))
>>> +
>>> +if not py3_dep.found()
>>> + pycamera_enabled = false
>>> + subdir_done()
>>> +endif
>>> +
>>> +pycamera_enabled = true
>>> +
>>> +pybind11_proj = subproject('pybind11')
>>> +pybind11_dep = pybind11_proj.get_variable('pybind11_dep')
>>> +
>>> +pycamera_sources = files([
>>> + 'pymain.cpp',
>>> + 'pyenums.cpp',
>
> Alphabetical order please.
>
>>> +])
>>> +
>>> +pycamera_deps = [
>>> + libcamera_public,
>>> + py3_dep,
>>> + pybind11_dep,
>>> +]
>>> +
>>> +pycamera_args = ['-fvisibility=hidden']
>>> +pycamera_args += ['-Wno-shadow']
>>> +pycamera_args += ['-DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT']
>
> You could write this as
>
> pycamera_args = [
> '-fvisibility=hidden',
> '-Wno-shadow',
> '-DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT',
> ]
>
>>> +
>>> +destdir = get_option('libdir') + '/python' + py3_dep.version() + '/site-packages/libcamera'
>
> destdir = get_option('libdir') / ('python' + py3_dep.version()) / 'site-packages' / 'libcamera'
>
>>> +
>>> +pycamera = shared_module('_libcamera',
>>> + pycamera_sources,
>>> + install : true,
>>> + install_dir : destdir,
>>> + name_prefix : '',
>>> + dependencies : pycamera_deps,
>>> + cpp_args : pycamera_args)
>>> +
>>> +run_command('ln', '-fsT', '../../../../src/py/libcamera/__init__.py',
>>> + meson.current_build_dir() / '__init__.py',
>>> + check: true)
>>> +
>>> +install_data(['__init__.py'], install_dir : destdir)
>>> diff --git a/src/py/libcamera/pyenums.cpp b/src/py/libcamera/pyenums.cpp
>>> new file mode 100644
>>> index 00000000..39886656
>>> --- /dev/null
>>> +++ b/src/py/libcamera/pyenums.cpp
>>> @@ -0,0 +1,53 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2021, Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
>>> + *
>>> + * Python bindings
>
> This could be a bit more descriptive:
>
> * Python bindings - Enumerations
>
>>> + */
>>> +
>>> +#include <libcamera/libcamera.h>
>>> +
>>> +#include <pybind11/smart_holder.h>
>>> +
>>> +namespace py = pybind11;
>>> +
>>> +using namespace libcamera;
>>> +
>>> +void init_pyenums(py::module &m)
>>> +{
>>> + py::enum_<CameraConfiguration::Status>(m, "ConfigurationStatus")
>>> + .value("Valid", CameraConfiguration::Valid)
>>> + .value("Adjusted", CameraConfiguration::Adjusted)
>>> + .value("Invalid", CameraConfiguration::Invalid);
>
> It would be nice if C++ had some introspection capabilities that would
> allow this to be done automatically :-)
>
>>> +
>>> + py::enum_<StreamRole>(m, "StreamRole")
>>> + .value("StillCapture", StreamRole::StillCapture)
>>> + .value("Raw", StreamRole::Raw)
>>> + .value("VideoRecording", StreamRole::VideoRecording)
>>> + .value("Viewfinder", StreamRole::Viewfinder);
>>> +
>>> + py::enum_<Request::Status>(m, "RequestStatus")
>>> + .value("Pending", Request::RequestPending)
>
> I wonder if the C++ enum should turn into an enum class, and the
> enumerators renamed to drop the Request prefix.
Hmm, what are you suggesting? Changing the C++ enum to enum class? Isn't
that kind of a drastic change? In some situations enums and enum classes
behave quite differently, and in any case it's a big API change.
I'm not against it, I was surprised to see so many enums used in the C++
side, but... Maybe that's not part of thise series?
>>> + .value("Complete", Request::RequestComplete)
>>> + .value("Cancelled", Request::RequestCancelled);
>
> Nothing that needs to be changed now, by what is more pythonic between
>
> Request.Status.Pending
>
> and
>
> RequestStatus.Pending
>
> ?
Or Request.RequestPending.
I have no idea =). I like the first one best. I did add some of the more
recent enums inside the related class, but for these I just copied the
C++ side style.
>>> +
>>> + py::enum_<FrameMetadata::Status>(m, "FrameMetadataStatus")
>>> + .value("Success", FrameMetadata::FrameSuccess)
>
> Same here, an enum class may make sense.
>
>>> + .value("Error", FrameMetadata::FrameError)
>>> + .value("Cancelled", FrameMetadata::FrameCancelled);
>>> +
>>> + py::enum_<Request::ReuseFlag>(m, "ReuseFlag")
>>> + .value("Default", Request::ReuseFlag::Default)
>>> + .value("ReuseBuffers", Request::ReuseFlag::ReuseBuffers);
>>> +
>>> + py::enum_<ControlType>(m, "ControlType")
>>> + .value("None", ControlType::ControlTypeNone)
>
> Ditto.
>
>>> + .value("Bool", ControlType::ControlTypeBool)
>>> + .value("Byte", ControlType::ControlTypeByte)
>>> + .value("Integer32", ControlType::ControlTypeInteger32)
>>> + .value("Integer64", ControlType::ControlTypeInteger64)
>>> + .value("Float", ControlType::ControlTypeFloat)
>>> + .value("String", ControlType::ControlTypeString)
>>> + .value("Rectangle", ControlType::ControlTypeRectangle)
>>> + .value("Size", ControlType::ControlTypeSize);
>>> +}
>>> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp
>>> new file mode 100644
>>> index 00000000..54674caf
>>> --- /dev/null
>>> +++ b/src/py/libcamera/pymain.cpp
>>> @@ -0,0 +1,452 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2021, Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
>>> + *
>>> + * Python bindings
>>> + */
>>> +
>>> +/*
>>> + * To generate pylibcamera stubs:
>>> + * PYTHONPATH=build/src/py pybind11-stubgen --no-setup-py -o build/src/py libcamera
>
> What is this for ?
Oh, that's just a comment for myself on how to generate stubs for the
library. I need to add a bit more context here, or remove it.
>>> + */
>>> +
>>> +#include <chrono>
>>> +#include <fcntl.h>
>>> +#include <mutex>
>>> +#include <sys/eventfd.h>
>>> +#include <sys/mman.h>
>>> +#include <thread>
>>> +#include <unistd.h>
>>> +
>>> +#include <libcamera/libcamera.h>
>>> +
>>> +#include <pybind11/functional.h>
>>> +#include <pybind11/smart_holder.h>
>>> +#include <pybind11/stl.h>
>>> +#include <pybind11/stl_bind.h>
>>> +
>>> +namespace py = pybind11;
>>> +
>>> +using namespace std;
>
> You're using the std:: namespace qualifier explicitly in several
> location below. Could we do that everywhere and drop the using namespace
> directive ?
I'd prefer to use the 'using namespace' and drop the std:: in the code.
The few std:: uses are left-overs from copy pastes from examples...
>>> +using namespace libcamera;
>>> +
>>> +template<typename T>
>>> +static py::object ValueOrTuple(const ControlValue &cv)
>
> Could you rename this to valueOrTuple() to follow the libcamera coding
> style ? Or would that look awkward from a python bindings coding style
> point of view ?
No, I think it's fine.
> Same for ControlValueToPy and PyToControlValue.
>
>>> +{
>>> + if (cv.isArray()) {
>>> + const T *v = reinterpret_cast<const T *>(cv.data().data());
>>> + auto t = py::tuple(cv.numElements());
>>> +
>>> + for (size_t i = 0; i < cv.numElements(); ++i)
>>> + t[i] = v[i];
>>> +
>>> + return t;
>>> + }
>>> +
>>> + return py::cast(cv.get<T>());
>>> +}
>>> +
>>> +static py::object ControlValueToPy(const ControlValue &cv)
>>> +{
>>> + switch (cv.type()) {
>>> + case ControlTypeBool:
>>> + return ValueOrTuple<bool>(cv);
>>> + case ControlTypeByte:
>>> + return ValueOrTuple<uint8_t>(cv);
>>> + case ControlTypeInteger32:
>>> + return ValueOrTuple<int32_t>(cv);
>>> + case ControlTypeInteger64:
>>> + return ValueOrTuple<int64_t>(cv);
>>> + case ControlTypeFloat:
>>> + return ValueOrTuple<float>(cv);
>>> + case ControlTypeString:
>>> + return py::cast(cv.get<string>());
>>> + case ControlTypeRectangle: {
>
> /* \todo Add geometry classes to the Python bindings */
What's missing? The switch handles all the possible cases afaics.
>>> + const Rectangle *v = reinterpret_cast<const Rectangle *>(cv.data().data());
>>> + return py::make_tuple(v->x, v->y, v->width, v->height);
>>> + }
>>> + case ControlTypeSize: {
>>> + const Size *v = reinterpret_cast<const Size *>(cv.data().data());
>>> + return py::make_tuple(v->width, v->height);
>>> + }
>>> + case ControlTypeNone:
>>> + default:
>>> + throw runtime_error("Unsupported ControlValue type");
>>> + }
>>> +}
>>> +
>>> +static ControlValue PyToControlValue(const py::object &ob, ControlType type)
>>> +{
>>> + switch (type) {
>>> + case ControlTypeBool:
>>> + return ControlValue(ob.cast<bool>());
>>> + case ControlTypeByte:
>>> + return ControlValue(ob.cast<uint8_t>());
>>> + case ControlTypeInteger32:
>>> + return ControlValue(ob.cast<int32_t>());
>>> + case ControlTypeInteger64:
>>> + return ControlValue(ob.cast<int64_t>());
>>> + case ControlTypeFloat:
>>> + return ControlValue(ob.cast<float>());
>>> + case ControlTypeString:
>>> + return ControlValue(ob.cast<string>());
>>> + case ControlTypeRectangle:
>>> + case ControlTypeSize:
>
> A todo comment would be nice here too.
>
>>> + case ControlTypeNone:
>>> + default:
>>> + throw runtime_error("Control type not implemented");
>>> + }
>>> +}
>>> +
>>> +static weak_ptr<CameraManager> g_camera_manager;
>
> Coding style here too, gCameraManager, or would that look too awkward ?
> Same below.
>
>>> +static int g_eventfd;
>>> +static mutex g_reqlist_mutex;
>>> +static vector<Request *> g_reqlist;
>>> +
>>> +static void handleRequestCompleted(Request *req)
>>> +{
>>> + {
>>> + lock_guard guard(g_reqlist_mutex);
>>> + g_reqlist.push_back(req);
>>> + }
>>> +
>>> + uint64_t v = 1;
>>> + write(g_eventfd, &v, 8);
>>> +}
>>> +
>>> +void init_pyenums(py::module &m);
>>> +
>>> +PYBIND11_MODULE(_libcamera, m)
>>> +{
>>> + init_pyenums(m);
>>> +
>>> + /* Forward declarations */
>>> +
>>> + /*
>>> + * We need to declare all the classes here so that Python docstrings
>>> + * can be generated correctly.
>>> + * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings
>>> + */
>>> +
>>> + auto pyCameraManager = py::class_<CameraManager>(m, "CameraManager");
>>> + auto pyCamera = py::class_<Camera>(m, "Camera");
>>> + auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration");
>>> + auto pyStreamConfiguration = py::class_<StreamConfiguration>(m, "StreamConfiguration");
>>> + auto pyStreamFormats = py::class_<StreamFormats>(m, "StreamFormats");
>>> + auto pyFrameBufferAllocator = py::class_<FrameBufferAllocator>(m, "FrameBufferAllocator");
>>> + auto pyFrameBuffer = py::class_<FrameBuffer>(m, "FrameBuffer");
>>> + auto pyStream = py::class_<Stream>(m, "Stream");
>>> + auto pyControlId = py::class_<ControlId>(m, "ControlId");
>>> + auto pyRequest = py::class_<Request>(m, "Request");
>>> + auto pyFrameMetadata = py::class_<FrameMetadata>(m, "FrameMetadata");
>
> Could you please sort these alphabetically ?
Later additions introduce classes that depend on each other, and cannot
be sorted. Also, these are in the same order as they are in the file
below. So, I'd rather keep them as they are.
>>> +
>>> + /* Global functions */
>>> + m.def("logSetLevel", &logSetLevel);
>>> +
>>> + /* Classes */
>>> + pyCameraManager
>>> + .def_static("singleton", []() {
>>> + shared_ptr<CameraManager> cm = g_camera_manager.lock();
>>> + if (cm)
>>> + return cm;
>>> +
>>> + int fd = eventfd(0, 0);
>>> + if (fd == -1)
>>> + throw std::system_error(errno, std::generic_category(), "Failed to create eventfd");
>
> Some long lines like this one can be wrapped:
>
> throw std::system_error(errno, std::generic_category(),
> "Failed to create eventfd");
>>> +
>>> + cm = shared_ptr<CameraManager>(new CameraManager, [](auto p) {
>>> + close(g_eventfd);
>>> + g_eventfd = -1;
>>> + delete p;
>>> + });
>>> +
>>> + g_eventfd = fd;
>>> + g_camera_manager = cm;
>>> +
>>> + int ret = cm->start();
>>> + if (ret)
>>> + throw std::system_error(-ret, std::generic_category(), "Failed to start CameraManager");
>>> +
>>> + return cm;
>>> + })
>>> +
>>> + .def_property_readonly("version", &CameraManager::version)
>>> +
>>> + .def_property_readonly("efd", [](CameraManager &) {
>>> + return g_eventfd;
>>> + })
>>> +
>>> + .def("getReadyRequests", [](CameraManager &) {
>
> As we don't usually prefix getters with "get", maybe just
> "readyRequests" ?
Hmm, this is not a getter, at least not as I see a getter. A getter
returns the value of a field, and would be exposed as a property. This
is a function that does work.
>>> + vector<Request *> v;
>>> +
>>> + {
>>> + lock_guard guard(g_reqlist_mutex);
>>> + swap(v, g_reqlist);
>>> + }
>>> +
>>> + vector<py::object> ret;
>>> +
>>> + for (Request *req : v) {
>>> + py::object o = py::cast(req);
>>> + /* decrease the ref increased in Camera::queueRequest() */
>
> s/decrease/Decrease/
>
>>> + o.dec_ref();
>>> + ret.push_back(o);
>>> + }
>>> +
>>> + return ret;
>>> + })
>>> +
>>> + .def("get", py::overload_cast<const string &>(&CameraManager::get), py::keep_alive<0, 1>())
>>> +
>>> + .def("find", [](CameraManager &self, string str) {
>>> + std::transform(str.begin(), str.end(), str.begin(), ::tolower);
>>> +
>>> + for (auto c : self.cameras()) {
>>> + string id = c->id();
>>> +
>>> + std::transform(id.begin(), id.end(), id.begin(), ::tolower);
>
> Do we really want to ignore the case in the comparison here ?
>
>>> +
>>> + if (id.find(str) != string::npos)
>
> Actually, does this function really belong in the python bindings ? It
> seems to be used in unit tests only, where you could iterate over the
> cameras exposed by the cameras property instead.
I think I agree. I probably added this quite early, when I didn't know
how to expose a list of cameras.
>>> + return c;
>>> + }
>>> +
>>> + return shared_ptr<Camera>();
>>> + }, py::keep_alive<0, 1>())
>>> +
>>> + /* Create a list of Cameras, where each camera has a keep-alive to CameraManager */
>>> + .def_property_readonly("cameras", [](CameraManager &self) {
>>> + py::list l;
>>> +
>>> + for (auto &c : self.cameras()) {
>>> + py::object py_cm = py::cast(self);
>>> + py::object py_cam = py::cast(c);
>>> + py::detail::keep_alive_impl(py_cam, py_cm);
>>> + l.append(py_cam);
>>> + }
>>> +
>>> + return l;
>>> + });
>>> +
>>> + pyCamera
>>> + .def_property_readonly("id", &Camera::id)
>>> + .def("acquire", &Camera::acquire)
>>> + .def("release", &Camera::release)
>>> + .def("start", [](Camera &self) {
>>> + self.requestCompleted.connect(handleRequestCompleted);
>>
>> What happens if someone calls start() multiple times? Is that going to
>> mean the signal / slot will be connected multiple times?
>>
>> That could be problematic ... as I think it means it could then call the
>> request completed handler multiple times - but it may also already be
>> trapped by the recent work we did around there.
>>
>> So for now I think this is fine - but may be something to test or
>> consider later.
>
> Indeed. A \todo comment would be nice.
>
>>> +
>>> + int ret = self.start();
>>> + if (ret)
>>> + self.requestCompleted.disconnect(handleRequestCompleted);
>>> +
>>> + return ret;
>
> int ret = self.start();
> if (ret) {
> self.requestCompleted.disconnect(handleRequestCompleted);
> return ret;
> }
>
> return 0;
>
>>> + })
>>> +
>>> + .def("stop", [](Camera &self) {
>>> + int ret = self.stop();
>>> + if (!ret)
>>> + self.requestCompleted.disconnect(handleRequestCompleted);
>>> +
>>> + return ret;
>
> int ret = self.stop();
> if (ret)
> return ret;
>
> self.requestCompleted.disconnect(handleRequestCompleted);
> return 0;
>
>>> + })
>>> +
>>> + .def("__repr__", [](Camera &self) {
>>> + return "<libcamera.Camera '" + self.id() + "'>";
>>> + })
>>> +
>>> + /* Keep the camera alive, as StreamConfiguration contains a Stream* */
>>> + .def("generateConfiguration", &Camera::generateConfiguration, py::keep_alive<0, 1>())
>>> + .def("configure", &Camera::configure)
>>> +
>>> + .def("createRequest", &Camera::createRequest, py::arg("cookie") = 0)
>>> +
>>> + .def("queueRequest", [](Camera &self, Request *req) {
>>> + py::object py_req = py::cast(req);
>>> +
>
> /*
> * Increase the reference count, will be dropped in
> * getReadyRequests().
> */
>
> I wonder what happens if nobody calls getReadyRequests() though, for
> instance for the last requesuts when stopping the camera. If there's an
> issue there, please add a \todo comment somewhere.
The handled requests will then sit in the gReqList. What is the issue
you see? Memory leak? We "leak" in any case, as the CameraManager
singleton is always there, and the gReqList is essentially a field of
the camera manager singleton.
>>> + py_req.inc_ref();
>>> +
>>> + int ret = self.queueRequest(req);
>>> + if (ret)
>>> + py_req.dec_ref();
>>> +
>>> + return ret;
>>> + })
>>> +
>>> + .def_property_readonly("streams", [](Camera &self) {
>>> + py::set set;
>>> + for (auto &s : self.streams()) {
>>> + py::object py_self = py::cast(self);
>>> + py::object py_s = py::cast(s);
>>> + py::detail::keep_alive_impl(py_s, py_self);
>>> + set.add(py_s);
>>> + }
>>> + return set;
>>> + })
>>> +
>>> + .def("find_control", [](Camera &self, const string &name) {
>
> "findControl"
This is the python API, so "find_control" is the correct one. That said,
I mix up the styles in many places. To be honest, I don't know if a
direct binding should use the original names or pythonic names.
But I think I'll just go with the pythonic names.
>>> + const auto &controls = self.controls();
>>> +
>>> + auto it = find_if(controls.begin(), controls.end(),
>>> + [&name](const auto &kvp) { return kvp.first->name() == name; });
>>> +
>>> + if (it == controls.end())
>>> + throw runtime_error("Control not found");
>>> +
>>> + return it->first;
>>> + }, py::return_value_policy::reference_internal)
>>> +
>>> + .def_property_readonly("controls", [](Camera &self) {
>>> + py::dict ret;
>>> +
>>> + for (const auto &[id, ci] : self.controls()) {
>
> /* \todo Add bindings for the ControlInfo class */
>
>>> + ret[id->name().c_str()] = make_tuple<py::object>(ControlValueToPy(ci.min()),
>>> + ControlValueToPy(ci.max()),
>>> + ControlValueToPy(ci.def()));
>>> + }
>>> +
>>> + return ret;
>>> + })
>>> +
>>> + .def_property_readonly("properties", [](Camera &self) {
>>> + py::dict ret;
>>> +
>>> + for (const auto &[key, cv] : self.properties()) {
>>> + const ControlId *id = properties::properties.at(key);
>>> + py::object ob = ControlValueToPy(cv);
>>> +
>>> + ret[id->name().c_str()] = ob;
>>> + }
>>> +
>>> + return ret;
>>> + });
>>> +
>>> + pyCameraConfiguration
>>> + .def("__iter__", [](CameraConfiguration &self) {
>>> + return py::make_iterator<py::return_value_policy::reference_internal>(self);
>>> + }, py::keep_alive<0, 1>())
>>> + .def("__len__", [](CameraConfiguration &self) {
>>> + return self.size();
>>> + })
>>> + .def("validate", &CameraConfiguration::validate)
>>> + .def("at", py::overload_cast<unsigned int>(&CameraConfiguration::at), py::return_value_policy::reference_internal)
>
> Line wrap.
>
>>> + .def_property_readonly("size", &CameraConfiguration::size)
>>> + .def_property_readonly("empty", &CameraConfiguration::empty);
>>> +
>>> + pyStreamConfiguration
>>> + .def("toString", &StreamConfiguration::toString)
>>> + .def_property_readonly("stream", &StreamConfiguration::stream, py::return_value_policy::reference_internal)
>>> + .def_property(
>>> + "size",
>>> + [](StreamConfiguration &self) { return make_tuple(self.size.width, self.size.height); },
>>> + [](StreamConfiguration &self, tuple<uint32_t, uint32_t> size) { self.size.width = get<0>(size); self.size.height = get<1>(size); })
>
> That's a very long line too.
>
> [](StreamConfiguration &self) {
> return make_tuple(self.size.width, self.size.height);
> },
> [](StreamConfiguration &self, tuple<uint32_t, uint32_t> size) {
> self.size.width = get<0>(size);
> self.size.height = get<1>(size);
> })
>
> Same where applicable.
>
>>> + .def_property(
>>> + "pixelFormat",
>>> + [](StreamConfiguration &self) { return self.pixelFormat.toString(); },
>>> + [](StreamConfiguration &self, string fmt) { self.pixelFormat = PixelFormat::fromString(fmt); })
>>> + .def_readwrite("stride", &StreamConfiguration::stride)
>>> + .def_readwrite("frameSize", &StreamConfiguration::frameSize)
>>> + .def_readwrite("bufferCount", &StreamConfiguration::bufferCount)
>>> + .def_property_readonly("formats", &StreamConfiguration::formats, py::return_value_policy::reference_internal);
>>> +
>>> + pyStreamFormats
>>> + .def_property_readonly("pixelFormats", [](StreamFormats &self) {
>>> + vector<string> fmts;
>>> + for (auto &fmt : self.pixelformats())
>>> + fmts.push_back(fmt.toString());
>>> + return fmts;
>>> + })
>>> + .def("sizes", [](StreamFormats &self, const string &pixelFormat) {
>>> + auto fmt = PixelFormat::fromString(pixelFormat);
>>> + vector<tuple<uint32_t, uint32_t>> fmts;
>>> + for (const auto &s : self.sizes(fmt))
>>> + fmts.push_back(make_tuple(s.width, s.height));
>>> + return fmts;
>>> + })
>>> + .def("range", [](StreamFormats &self, const string &pixelFormat) {
>>> + auto fmt = PixelFormat::fromString(pixelFormat);
>>> + const auto &range = self.range(fmt);
>>> + return make_tuple(make_tuple(range.hStep, range.vStep),
>>> + make_tuple(range.min.width, range.min.height),
>>> + make_tuple(range.max.width, range.max.height));
>>> + });
>>> +
>>> + pyFrameBufferAllocator
>>> + .def(py::init<shared_ptr<Camera>>(), py::keep_alive<1, 2>())
>>> + .def("allocate", &FrameBufferAllocator::allocate)
>>> + .def_property_readonly("allocated", &FrameBufferAllocator::allocated)
>>> + /* Create a list of FrameBuffers, where each FrameBuffer has a keep-alive to FrameBufferAllocator */
>>> + .def("buffers", [](FrameBufferAllocator &self, Stream *stream) {
>>> + py::object py_self = py::cast(self);
>>> + py::list l;
>>> + for (auto &ub : self.buffers(stream)) {
>>> + py::object py_buf = py::cast(ub.get(), py::return_value_policy::reference_internal, py_self);
>>> + l.append(py_buf);
>>> + }
>>> + return l;
>>> + });
>>> +
>>> + pyFrameBuffer
>>> + /* TODO: implement FrameBuffer::Plane properly */
>
> s/TODO: implement/\\todo Implement/
>
>>> + .def(py::init([](vector<tuple<int, unsigned int>> planes, unsigned int cookie) {
>>> + vector<FrameBuffer::Plane> v;
>>> + for (const auto &t : planes)
>>> + v.push_back({ SharedFD(get<0>(t)), FrameBuffer::Plane::kInvalidOffset, get<1>(t) });
>>> + return new FrameBuffer(v, cookie);
>>> + }))
>>> + .def_property_readonly("metadata", &FrameBuffer::metadata, py::return_value_policy::reference_internal)
>>> + .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_property("cookie", &FrameBuffer::cookie, &FrameBuffer::setCookie);
>>> +
>>> + pyStream
>>> + .def_property_readonly("configuration", &Stream::configuration);
>>> +
>>> + pyControlId
>>> + .def_property_readonly("id", &ControlId::id)
>>> + .def_property_readonly("name", &ControlId::name)
>>> + .def_property_readonly("type", &ControlId::type);
>>> +
>>> + pyRequest
>>> + /* Fence is not supported, so we cannot expose addBuffer() directly */
>>> + .def("addBuffer", [](Request &self, const Stream *stream, FrameBuffer *buffer) {
>>> + return self.addBuffer(stream, buffer);
>>> + }, py::keep_alive<1, 3>()) /* Request keeps Framebuffer alive */
>>> + .def_property_readonly("status", &Request::status)
>>> + .def_property_readonly("buffers", &Request::buffers)
>>> + .def_property_readonly("cookie", &Request::cookie)
>>> + .def_property_readonly("hasPendingBuffers", &Request::hasPendingBuffers)
>>> + .def("set_control", [](Request &self, ControlId &id, py::object value) {
>>> + self.controls().set(id.id(), PyToControlValue(value, id.type()));
>>> + })
>>> + .def_property_readonly("metadata", [](Request &self) {
>>> + py::dict ret;
>>> +
>>> + for (const auto &[key, cv] : self.metadata()) {
>>> + const ControlId *id = controls::controls.at(key);
>>> + py::object ob = ControlValueToPy(cv);
>>> +
>>> + ret[id->name().c_str()] = ob;
>>> + }
>>> +
>>> + return ret;
>>> + })
>>> + /* As we add a keep_alive to the fb in addBuffers(), we can only allow reuse with ReuseBuffers. */
>
> That will be an annoying limitation. Can you add a todo comment to fix
> it (and line wrap this comment) ?
>
>>> + .def("reuse", [](Request &self) { self.reuse(Request::ReuseFlag::ReuseBuffers); });
>>> +
>>> + pyFrameMetadata
>>> + .def_readonly("status", &FrameMetadata::status)
>>> + .def_readonly("sequence", &FrameMetadata::sequence)
>>> + .def_readonly("timestamp", &FrameMetadata::timestamp)
>>> + /* temporary helper, to be removed */
>
> /* \todo Temporary helper, to be removed */
>
>>> + .def_property_readonly("bytesused", [](FrameMetadata &self) {
>>> + vector<unsigned int> v;
>>> + v.resize(self.planes().size());
>>> + transform(self.planes().begin(), self.planes().end(), v.begin(), [](const auto &p) { return p.bytesused; });
>>> + return v;
>>> + });
>>> +}
>>> diff --git a/src/py/meson.build b/src/py/meson.build
>>> new file mode 100644
>>> index 00000000..4ce9668c
>>> --- /dev/null
>>> +++ b/src/py/meson.build
>>> @@ -0,0 +1 @@
>>> +subdir('libcamera')
>>> diff --git a/subprojects/.gitignore b/subprojects/.gitignore
>>> index 391fde2c..0e194289 100644
>>> --- a/subprojects/.gitignore
>>> +++ b/subprojects/.gitignore
>>> @@ -1,3 +1,4 @@
>>> /googletest-release*
>>> /libyuv
>>> -/packagecache
>>> \ No newline at end of file
>>> +/packagecache
>>> +/pybind11
>>> diff --git a/subprojects/packagefiles/pybind11/meson.build b/subprojects/packagefiles/pybind11/meson.build
>>> new file mode 100644
>>> index 00000000..67e89aec
>>> --- /dev/null
>>> +++ b/subprojects/packagefiles/pybind11/meson.build
>>> @@ -0,0 +1,8 @@
>>> +project('pybind11', 'cpp',
>>> + version : '2.9.1',
>>> + license : 'BSD-3-Clause')
>>> +
>>> +pybind11_incdir = include_directories('include')
>>> +
>>> +pybind11_dep = declare_dependency(
>>> + include_directories : pybind11_incdir)
>
> 4 spaces for indentation.
>
>>> diff --git a/subprojects/pybind11.wrap b/subprojects/pybind11.wrap
>>> new file mode 100644
>>> index 00000000..2413e9ca
>>> --- /dev/null
>>> +++ b/subprojects/pybind11.wrap
>>> @@ -0,0 +1,8 @@
>>> +[wrap-git]
>>> +url = https://github.com/pybind/pybind11.git
>
> A comment here to mention this is the smart_holder branch could be nice.
>
>>> +revision = 82734801f23314b4c34d70a79509e060a2648e04
>>
>> Great - this is now pointing directly to the pybind sources so I think
>> that's much cleaner:
>>
>> It's hard to do a dedicated review on so much that I honestly don't
>> fully comprehend yet - but I think getting this in and working/building
>> on top is better for everyone, and as far as I know - it's already quite
>> well tested by RPi (albeit, an older version, so I hope David can rebase
>> and test sometime soon).
>>
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>>> +depth = 1
>>> +patch_directory = pybind11
>
> Do we actually have any patch ?
We have the meson.build file.
Tomi
More information about the libcamera-devel
mailing list