[libcamera-devel] [PATCH v7 09/13] py: add Transform

[Laurent Pinchart]

On Fri, May 06, 2022 at 04:40:01PM +0100, David Plowman wrote:
> On Fri, 6 May 2022 at 16:24, Laurent Pinchart wrote:
> > On Fri, May 06, 2022 at 02:25:50PM +0300, Tomi Valkeinen wrote:
> > > On 05/05/2022 20:59, Laurent Pinchart wrote:
> > > > On Thu, May 05, 2022 at 01:41:00PM +0300, Tomi Valkeinen wrote:
> > > >> Add binding for Transform.
> > > >>
> > > >> C++'s Transform is an enum class, but we expose it as a Python class so
> > > >> that it can be easily manipulated.
> > > >>
> > > >> Original version from David Plowman <david.plowman at raspberrypi.com>.
> > > >>
> > > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> > > >> ---
> > > >>   src/py/libcamera/pymain.cpp | 63 ++++++++++++++++++++++++++++++++++++-
> > > >>   1 file changed, 62 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp
> > > >> index db9d90ab..3d2393ab 100644
> > > >> --- a/src/py/libcamera/pymain.cpp
> > > >> +++ b/src/py/libcamera/pymain.cpp
> > > >> @@ -152,6 +152,7 @@ PYBIND11_MODULE(_libcamera, m)
> > > >>    auto pyControlId = py::class_<ControlId>(m, "ControlId");
> > > >>    auto pyRequest = py::class_<Request>(m, "Request");
> > > >>    auto pyFrameMetadata = py::class_<FrameMetadata>(m, "FrameMetadata");
> > > >> +  auto pyTransform = py::class_<Transform>(m, "Transform");
> > > >>
> > > >>    /* Global functions */
> > > >>    m.def("logSetLevel", &logSetLevel);
> > > >> @@ -342,7 +343,8 @@ PYBIND11_MODULE(_libcamera, m)
> > > >>            .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);
> > > >> +          .def_property_readonly("empty", &CameraConfiguration::empty)
> > > >> +          .def_readwrite("transform", &CameraConfiguration::transform);
> > > >>
> > > >>    pyStreamConfiguration
> > > >>            .def("toString", &StreamConfiguration::toString)
> > > >> @@ -462,4 +464,63 @@ PYBIND11_MODULE(_libcamera, m)
> > > >>                    transform(self.planes().begin(), self.planes().end(), v.begin(), [](const auto &p) { return p.bytesused; });
> > > >>                    return v;
> > > >>            });
> > > >> +
> > > >> +  pyTransform
> > > >> +          .def(py::init([](int rotation, bool hflip, bool vflip, bool transpose) {
> > > >
> > > > I'm not fond of the constructor, three bool arguments in a row are quite
> > > > error-prone. I'd rather expose the enum.
> > >
> > > It's not that bad with python, e.g.:
> > >
> > > Transform(transpose=True, vflip=True)
> > >
> > > I think that's more pythonic than or'ing enums.
> >
> > One could also write
> >
> >     Transform(0, true, false, true)
> >
> > which isn't nice. We can address that out later if needed, either
> > merging this patch already or postponing it. David, do you have any
> > opinion ?
> 
> Yes, what Tomi put is exactly how I am expecting to use it. I don't
> feel terribly strongly, but it did seem both handy and Python-ish. You
> can do
> 
> t = Transform(180)
> 
> t = Transform(hflip=True, vflip=True)
> 
> t = Transform()
> t.hflip = t.vflip =1
> 
> I agree that it can be misused by the determined, but I definitely
> went for convenience...

Time to document the Python bindings then ? :-)

> > > >> +                  bool ok;
> > > >> +
> > > >> +                  Transform t = transformFromRotation(rotation, &ok);
> > > >> +                  if (!ok)
> > > >> +                          throw std::runtime_error("Unable to create the transform");
> > > >
> > > > "Invalid rotation" would be a better message I think.
> > >
> > > And std::invalid_argument.
> >
> > Good point.

-- 
Regards,

Laurent Pinchart