[libcamera-devel,v7,04/13] Add Python bindings
diff mbox series

Message ID 20220505104104.70841-5-tomi.valkeinen@ideasonboard.com
State Accepted
Headers show
Series
  • Python bindings
Related show

Commit Message

Tomi Valkeinen May 5, 2022, 10:40 a.m. UTC
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@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

Comments

Kieran Bingham May 5, 2022, 1:51 p.m. UTC | #1
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@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@ideasonboard.com>
> +
> +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)
> +
> +
> +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',
> +])
> +
> +pycamera_deps = [
> +    libcamera_public,
> +    py3_dep,
> +    pybind11_dep,
> +]
> +
> +pycamera_args = ['-fvisibility=hidden']
> +pycamera_args += ['-Wno-shadow']
> +pycamera_args += ['-DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT']
> +
> +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@ideasonboard.com>
> + *
> + * Python bindings
> + */
> +
> +#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);
> +
> +       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)
> +               .value("Complete", Request::RequestComplete)
> +               .value("Cancelled", Request::RequestCancelled);
> +
> +       py::enum_<FrameMetadata::Status>(m, "FrameMetadataStatus")
> +               .value("Success", FrameMetadata::FrameSuccess)
> +               .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)
> +               .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@ideasonboard.com>
> + *
> + * Python bindings
> + */
> +
> +/*
> + * To generate pylibcamera stubs:
> + * PYTHONPATH=build/src/py pybind11-stubgen --no-setup-py -o build/src/py libcamera
> + */
> +
> +#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;
> +using namespace libcamera;
> +
> +template<typename T>
> +static py::object ValueOrTuple(const ControlValue &cv)
> +{
> +       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: {
> +               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:
> +       case ControlTypeNone:
> +       default:
> +               throw runtime_error("Control type not implemented");
> +       }
> +}
> +
> +static weak_ptr<CameraManager> g_camera_manager;
> +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");
> +
> +       /* 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");
> +
> +                       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 &) {
> +                       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() */
> +                               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);
> +
> +                               if (id.find(str) != string::npos)
> +                                       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.


> +
> +                       int ret = self.start();
> +                       if (ret)
> +                               self.requestCompleted.disconnect(handleRequestCompleted);
> +
> +                       return ret;
> +               })
> +
> +               .def("stop", [](Camera &self) {
> +                       int ret = self.stop();
> +                       if (!ret)
> +                               self.requestCompleted.disconnect(handleRequestCompleted);
> +
> +                       return ret;
> +               })
> +
> +               .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);
> +
> +                       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) {
> +                       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()) {
> +                               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)
> +               .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); })
> +               .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 */
> +               .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. */
> +               .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 */
> +               .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)
> 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
> +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@ideasonboard.com>



> +depth = 1
> +patch_directory = pybind11
> +
> +[provide]
> +pybind11 = pybind11_dep
> -- 
> 2.34.1
>
Tomi Valkeinen May 5, 2022, 2:03 p.m. UTC | #2
On 05/05/2022 16:51, Kieran Bingham wrote:

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

Good point. I'll look at it.

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

Yes, I think David rebasing their work and testing it is an important 
step before we can consider merging this.

Hint, hint, David! ;)

Note that the API has changed a bit. The fb mmap is different, and the 
ColorSpace enums are nested. I think those are the only API changes.

Another thing, Python bindings are currently enabled by default if the 
dependencies are there. I was wondering if the bindings should be 
disabled by default until we're more confident that they build fine.

  Tomi
Kieran Bingham May 5, 2022, 2:17 p.m. UTC | #3
Quoting Tomi Valkeinen (2022-05-05 15:03:59)
> On 05/05/2022 16:51, Kieran Bingham wrote:
> 
> >> +       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.
> 
> Good point. I'll look at it.
> 
> >> 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
> >> +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).
> 
> Yes, I think David rebasing their work and testing it is an important 
> step before we can consider merging this.
> 
> Hint, hint, David! ;)
> 
> Note that the API has changed a bit. The fb mmap is different, and the 
> ColorSpace enums are nested. I think those are the only API changes.
> 
> Another thing, Python bindings are currently enabled by default if the 
> dependencies are there. I was wondering if the bindings should be 
> disabled by default until we're more confident that they build fine.

I'm fine with them building when dependencies are met. If that catches
failures early - that's better in my opinion.

--
Kieran


> 
>   Tomi
Laurent Pinchart May 5, 2022, 5:32 p.m. UTC | #4
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@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@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

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

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

?

> > +
> > +       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@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 ?

> > + */
> > +
> > +#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 ?

> > +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 ?

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 */

> > +               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 ?

> > +
> > +       /* 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" ?

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

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

> > +                       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"

> > +                       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@ideasonboard.com>
> 
> > +depth = 1
> > +patch_directory = pybind11

Do we actually have any patch ?

All comments are minor, once addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +
> > +[provide]
> > +pybind11 = pybind11_dep
Tomi Valkeinen May 6, 2022, 8:11 a.m. UTC | #5
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@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@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@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@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@ideasonboard.com>
>>
>>> +depth = 1
>>> +patch_directory = pybind11
> 
> Do we actually have any patch ?

We have the meson.build file.

  Tomi
Laurent Pinchart May 6, 2022, 10:01 a.m. UTC | #6
Hi Tomi,

On Fri, May 06, 2022 at 11:11:46AM +0300, Tomi Valkeinen wrote:
> 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@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

[snip]

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

[snip]

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

I noticed that after reviewing this patch :-)

> >>> +    return mmap.mmap(self.fd(plane), self.length(plane), mmap.MAP_SHARED, mmap.PROT_READ)
> >>> +
> >>> +
> >>> +FrameBuffer.mmap = __FrameBuffer__mmap

[snip]

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

[snip]

> >>> +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?

Sorry to have been unclear, I didn't mean doing it as part of this
series. It's an open question to get feedback on the idea.

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

I'm fine keeping it if the comment is expanded to explain why it's
useful :-)

> >>> + */
> >>> +
> >>> +#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...

For the libcamera namespace I agree as the name is long (although an
alias with `using namespace cam = libcamera` may be a good idea), but
for std we try to make it explicit to avoid namespace clashes. Just
`vector` looks quite alien compared to the rest of the libcamera code
base. On a related note, I have on my todo list to drop the `using
namespace std` from unit tests.

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

I meant adding bindings for the geometry classes themselves. At the
moment they're translated to tuples.

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

I would have proposed to change the order below too :-) But if there are
dependencies, it's indeed an issue :-( Would it make sense to make them
mostly sorted ?

> >>> +
> >>> +       /* 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.

OK.

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

Yes, memory leaks, and possible crashes if we clean up things in the
wrong order. This can be addressed later, but a comment that states the
issue could be nice.

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

I'm not sure either :-) What bothers me is that you have "createRequest"
above and "find_control" here. We should at least be consistent.

I think I have a preference for native names, as that will make it
easier to navigate documentation and code between C++ and Python.

> 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@ideasonboard.com>
> >>
> >>> +depth = 1
> >>> +patch_directory = pybind11
> > 
> > Do we actually have any patch ?
> 
> We have the meson.build file.

I got confused by the fact that patch_directory points to an overlay
directory, not a directory containing patches. My bad.

Patch
diff mbox series

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@ideasonboard.com>
+
+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)
+
+
+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',
+])
+
+pycamera_deps = [
+    libcamera_public,
+    py3_dep,
+    pybind11_dep,
+]
+
+pycamera_args = ['-fvisibility=hidden']
+pycamera_args += ['-Wno-shadow']
+pycamera_args += ['-DPYBIND11_USE_SMART_HOLDER_AS_DEFAULT']
+
+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@ideasonboard.com>
+ *
+ * Python bindings
+ */
+
+#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);
+
+	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)
+		.value("Complete", Request::RequestComplete)
+		.value("Cancelled", Request::RequestCancelled);
+
+	py::enum_<FrameMetadata::Status>(m, "FrameMetadataStatus")
+		.value("Success", FrameMetadata::FrameSuccess)
+		.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)
+		.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@ideasonboard.com>
+ *
+ * Python bindings
+ */
+
+/*
+ * To generate pylibcamera stubs:
+ * PYTHONPATH=build/src/py pybind11-stubgen --no-setup-py -o build/src/py libcamera
+ */
+
+#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;
+using namespace libcamera;
+
+template<typename T>
+static py::object ValueOrTuple(const ControlValue &cv)
+{
+	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: {
+		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:
+	case ControlTypeNone:
+	default:
+		throw runtime_error("Control type not implemented");
+	}
+}
+
+static weak_ptr<CameraManager> g_camera_manager;
+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");
+
+	/* 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");
+
+			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 &) {
+			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() */
+				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);
+
+				if (id.find(str) != string::npos)
+					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);
+
+			int ret = self.start();
+			if (ret)
+				self.requestCompleted.disconnect(handleRequestCompleted);
+
+			return ret;
+		})
+
+		.def("stop", [](Camera &self) {
+			int ret = self.stop();
+			if (!ret)
+				self.requestCompleted.disconnect(handleRequestCompleted);
+
+			return ret;
+		})
+
+		.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);
+
+			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) {
+			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()) {
+				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)
+		.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); })
+		.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 */
+		.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. */
+		.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 */
+		.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)
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
+revision = 82734801f23314b4c34d70a79509e060a2648e04
+depth = 1
+patch_directory = pybind11
+
+[provide]
+pybind11 = pybind11_dep