[libcamera-devel] [PATCH 11/14] py: add geometry classes
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 17 11:48:13 CEST 2022
Hi Tomi,
On Tue, May 17, 2022 at 12:02:13PM +0300, Tomi Valkeinen wrote:
> On 17/05/2022 11:37, Laurent Pinchart wrote:
> > On Mon, May 16, 2022 at 05:10:19PM +0300, Tomi Valkeinen wrote:
> >> Add libcamera's geometry classes to the Python bindings.
> >>
> >> Note that this commit only adds the classes, but they are not used
> >> anywhere yet.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> >> ---
> >> src/py/libcamera/meson.build | 1 +
> >> src/py/libcamera/pygeometry.cpp | 104 ++++++++++++++++++++++++++++++++
> >> src/py/libcamera/pymain.cpp | 2 +
> >> 3 files changed, 107 insertions(+)
> >> create mode 100644 src/py/libcamera/pygeometry.cpp
> >>
> >> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build
> >> index af8ba6a5..12e006e7 100644
> >> --- a/src/py/libcamera/meson.build
> >> +++ b/src/py/libcamera/meson.build
> >> @@ -14,6 +14,7 @@ pybind11_dep = pybind11_proj.get_variable('pybind11_dep')
> >>
> >> pycamera_sources = files([
> >> 'pyenums.cpp',
> >> + 'pygeometry.cpp',
> >> 'pymain.cpp',
> >> ])
> >>
> >> diff --git a/src/py/libcamera/pygeometry.cpp b/src/py/libcamera/pygeometry.cpp
> >> new file mode 100644
> >> index 00000000..2cd1432e
> >> --- /dev/null
> >> +++ b/src/py/libcamera/pygeometry.cpp
> >> @@ -0,0 +1,104 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> >> + *
> >> + * Python bindings - Geometry classes
> >> + */
> >> +
> >> +#include <array>
> >> +
> >> +#include <libcamera/geometry.h>
> >> +#include <libcamera/libcamera.h>
> >> +
> >> +#include <pybind11/operators.h>
> >> +#include <pybind11/smart_holder.h>
> >> +#include <pybind11/stl.h>
> >> +
> >> +namespace py = pybind11;
> >> +
> >> +using namespace libcamera;
> >> +
> >> +void init_py_geometry(py::module &m)
> >> +{
> >> + py::class_<Point>(m, "Point")
> >> + .def(py::init<>())
> >> + .def(py::init<int, int>())
> >> + .def_readwrite("x", &Point::x)
> >> + .def_readwrite("y", &Point::y)
> >> + .def(py::self == py::self)
> >> + .def(-py::self)
> >> + .def("__str__", &Point::toString)
> >> + .def("__repr__", [](const Point &self) {
> >> + return "libcamera.Point(" + std::to_string(self.x) + ", " + std::to_string(self.y) + ")";
> >> + });
> >> +
> >> + py::class_<Size>(m, "Size")
> >> + .def(py::init<>())
> >> + .def(py::init<unsigned int, unsigned int>())
> >> + .def_readwrite("width", &Size::width)
> >> + .def_readwrite("height", &Size::height)
> >> + .def_property_readonly("is_null", &Size::isNull)
> >> + .def(py::self == py::self)
> >> + .def(py::self * float())
> >> + .def(py::self / float())
> >> + .def(py::self *= float())
> >> + .def(py::self /= float())
> >> + .def("__str__", &Size::toString)
> >> + .def("__repr__", [](const Size &self) {
> >> + return "libcamera.Size(" + std::to_string(self.width) + ", " + std::to_string(self.height) + ")";
> >> + });
> >> +
> >> + py::class_<SizeRange>(m, "SizeRange")
> >> + .def(py::init<>())
> >> + .def(py::init<Size>())
> >> + .def(py::init<Size, Size>())
> >> + .def(py::init<Size, Size, unsigned int, unsigned int>())
> >> + .def(py::init<>([](const std::array<unsigned int, 2> &s) {
> >> + return SizeRange(Size(std::get<0>(s), std::get<1>(s)));
> >> + }))
> >> + .def(py::init<>([](const std::array<unsigned int, 2> &min, const std::array<unsigned int, 2> &max) {
> >> + return SizeRange(Size(std::get<0>(min), std::get<1>(min)),
> >> + Size(std::get<0>(max), std::get<1>(max)));
> >> + }))
> >> + .def(py::init<>([](const std::array<unsigned int, 2> &min, const std::array<unsigned int, 2> &max,
> >> + unsigned int hStep, unsigned int vStep) {
> >> + return SizeRange(Size(std::get<0>(min), std::get<1>(min)),
> >> + Size(std::get<0>(max), std::get<1>(max)),
> >> + hStep, vStep);
> >> + }))
> >> + .def_readwrite("min", &SizeRange::min)
> >> + .def_readwrite("max", &SizeRange::max)
> >> + .def_readwrite("hStep", &SizeRange::hStep)
> >> + .def_readwrite("vStep", &SizeRange::vStep)
> >> + .def(py::self == py::self)
> >> + .def("__str__", &SizeRange::toString)
> >> + .def("__repr__", [](const SizeRange &self) {
> >> + return py::str("libcamera.SizeRange(({}, {}), ({}, {}), {}, {})")
> >> + .format(self.min.width, self.min.height,
> >> + self.max.width, self.max.height,
> >> + self.hStep, self.vStep);
> >> + });
> >> +
> >> + py::class_<Rectangle>(m, "Rectangle")
> >> + .def(py::init<>())
> >> + .def(py::init<int, int, Size>())
> >> + .def(py::init<>([](int x, int y, const std::array<unsigned int, 2> &s) {
> >> + return Rectangle(x, y, std::get<0>(s), std::get<1>(s));
> >> + }))
> >
> > I'm puzzled a bit by this constructor. Where do you use it, and could
> > then next constructor be used instead ? If it's meant to cover a common
> > use case, should the C++ API also implement this ?
>
> It allows:
>
> libcam.Rectangle(1, 2, (3, 4))
And then someone will request being able to write
libcam.Rectangle((1, 2), (3, 4))
and possibly even
libcam.Rectangle((1, 2), 3, 4)
:-)
In C++ it's a bit different thanks to implicit constructors and
initializer lists, so I agree that we may want to expose a similar
feature manually to make the code more readable. I'm just concerned
about the possible large number of combinations.
Is there a Python coding style rule (or rather API design rule) about
this ?
> It's more obvious with SizeRange:
>
> libcam.SizeRange((1, 2), (3, 4), 5, 6)
>
> instead of
>
> libcam.SizeRange(libcam.Size(1, 2), libcam.Size(3, 4), 5, 6)
>
> I really like tuples in python. I'd also like to do unpacking, like:
>
> w, h = libcam.Size(1, 2)
>
> I think I figured out how to do that, but after sending the series.
>
> These kind of features are perhaps something we need to do a decision
> on: include them, or keep the API as bare-bones as possible.
Good question. I personally prefer using s.width and s.height instead of
unpacking and using w and h in most cases, but that's probably a matter
of personal preference.
I'm not against such "small" differences between the C++ and Python
APIs, if it makes the API better. "better" of course needs to be defined
here, my go-to reference is usually "The Little Manual of API Design" by
Jasmin Blanchette (https://www.cs.vu.nl/~jbe248/api-design.pdf). A good
API should, in my opinion, have two important characteristics:
- Easy to learn and memorize: guessing how to achieve a task should give
the right answer. Consistency of the API is important here, but
exceptions are totally fine when they make the API easier to guess.
- Readability: an API piece (e.g. the name of a function on an
object) should do what you expect it to do when reading it.
> >> + .def(py::init<int, int, unsigned int, unsigned int>())
> >> + .def(py::init<Size>())
> >> + .def_readwrite("x", &Rectangle::x)
> >> + .def_readwrite("y", &Rectangle::y)
> >> + .def_readwrite("width", &Rectangle::width)
> >> + .def_readwrite("height", &Rectangle::height)
> >> + .def_property_readonly("is_null", &Rectangle::isNull)
> >> + .def_property_readonly("center", &Rectangle::center)
> >> + .def_property_readonly("size", &Rectangle::size)
> >> + .def_property_readonly("topLeft", &Rectangle::topLeft)
> >> + .def(py::self == py::self)
> >> + .def("__str__", &Rectangle::toString)
> >> + .def("__repr__", [](const Rectangle &self) {
> >> + return py::str("libcamera.Rectangle({}, {}, {}, {})")
> >> + .format(self.x, self.y, self.width, self.height);
> >> + });
> >
> > Could you add todo comments to remind that the other members of the
> > geometry classes should be implemented too ?
>
> Yes.
>
> >> +}
> >> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp
> >> index cc2ddee5..1dd70d9c 100644
> >> --- a/src/py/libcamera/pymain.cpp
> >> +++ b/src/py/libcamera/pymain.cpp
> >> @@ -137,11 +137,13 @@ static void handleRequestCompleted(Request *req)
> >>
> >> void init_pyenums(py::module &m);
> >> void init_pyenums_generated(py::module &m);
> >> +void init_py_geometry(py::module &m);
> >>
> >> PYBIND11_MODULE(_libcamera, m)
> >> {
> >> init_pyenums(m);
> >> init_pyenums_generated(m);
> >> + init_py_geometry(m);
> >
> > My OCD kicks in :-)
>
> Now that I look at it... Yes...
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list