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

[David Plowman]

Hi Laurent, Tomi

On Fri, 6 May 2022 at 16:24, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Tomi,
>
> 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...

David

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