[libcamera-devel] [PATCH v7 04/13] Add Python bindings
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri May 6 12:01:47 CEST 2022
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 at ideasonboard.com>
> >>> ---
> >>> meson.build | 1 +
> >>> meson_options.txt | 5 +
> >>> src/meson.build | 1 +
> >>> src/py/libcamera/__init__.py | 12 +
> >>> src/py/libcamera/meson.build | 44 ++
> >>> src/py/libcamera/pyenums.cpp | 53 ++
> >>> src/py/libcamera/pymain.cpp | 452 ++++++++++++++++++
> >>> src/py/meson.build | 1 +
> >>> subprojects/.gitignore | 3 +-
> >>> subprojects/packagefiles/pybind11/meson.build | 8 +
> >>> subprojects/pybind11.wrap | 8 +
> >>> 11 files changed, 587 insertions(+), 1 deletion(-)
> >>> create mode 100644 src/py/libcamera/__init__.py
> >>> create mode 100644 src/py/libcamera/meson.build
> >>> create mode 100644 src/py/libcamera/pyenums.cpp
> >>> create mode 100644 src/py/libcamera/pymain.cpp
> >>> create mode 100644 src/py/meson.build
> >>> create mode 100644 subprojects/packagefiles/pybind11/meson.build
> >>> create mode 100644 subprojects/pybind11.wrap
[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 at ideasonboard.com>
> >>> + *
> >>> + * Python bindings
> >>> + */
> >>> +
> >>> +/*
> >>> + * To generate pylibcamera stubs:
> >>> + * PYTHONPATH=build/src/py pybind11-stubgen --no-setup-py -o build/src/py libcamera
> >
> > What is this for ?
>
> Oh, that's just a comment for myself on how to generate stubs for the
> library. I need to add a bit more context here, or remove it.
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 at 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.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list