| Message ID | 20220505104104.70841-10-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi, Thank you for the patch. 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@raspberrypi.com>. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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. > + 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. > + > + if (hflip) > + t ^= Transform::HFlip; > + if (vflip) > + t ^= Transform::VFlip; > + if (transpose) > + t ^= Transform::Transpose; > + return t; > + }), py::arg("rotation") = 0, py::arg("hflip") = false, > + py::arg("vflip") = false, py::arg("transpose") = false) > + .def(py::init([](Transform &other) { return other; })) > + .def("__repr__", [](Transform &self) { > + return "<libcamera.Transform '" + std::string(transformToString(self)) + "'>"; > + }) > + .def_property("hflip", > + [](Transform &self) { > + return !!(self & Transform::HFlip); > + }, > + [](Transform &self, bool hflip) { > + if (hflip) > + self |= Transform::HFlip; > + else > + self &= ~Transform::HFlip; > + }) > + .def_property("vflip", > + [](Transform &self) { > + return !!(self & Transform::VFlip); > + }, > + [](Transform &self, bool vflip) { > + if (vflip) > + self |= Transform::VFlip; > + else > + self &= ~Transform::VFlip; > + }) > + .def_property("transpose", > + [](Transform &self) { > + return !!(self & Transform::Transpose); > + }, > + [](Transform &self, bool transpose) { > + if (transpose) > + self |= Transform::Transpose; > + else > + self &= ~Transform::Transpose; > + }) > + .def("inverse", [](Transform &self) { return -self; }) > + .def("invert", [](Transform &self) { > + self = -self; > + }) > + .def("compose", [](Transform &self, Transform &other) { > + self = self * other; > + }); > }
On 05/05/2022 20:59, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > 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@raspberrypi.com>. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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. >> + 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. Tomi
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@raspberrypi.com>. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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 ? > >> + 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.
Hi Laurent, Tomi On Fri, 6 May 2022 at 16:24, Laurent Pinchart <laurent.pinchart@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@raspberrypi.com>. > > >> > > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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
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@raspberrypi.com>. > > > >> > > > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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.
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) { + bool ok; + + Transform t = transformFromRotation(rotation, &ok); + if (!ok) + throw std::runtime_error("Unable to create the transform"); + + if (hflip) + t ^= Transform::HFlip; + if (vflip) + t ^= Transform::VFlip; + if (transpose) + t ^= Transform::Transpose; + return t; + }), py::arg("rotation") = 0, py::arg("hflip") = false, + py::arg("vflip") = false, py::arg("transpose") = false) + .def(py::init([](Transform &other) { return other; })) + .def("__repr__", [](Transform &self) { + return "<libcamera.Transform '" + std::string(transformToString(self)) + "'>"; + }) + .def_property("hflip", + [](Transform &self) { + return !!(self & Transform::HFlip); + }, + [](Transform &self, bool hflip) { + if (hflip) + self |= Transform::HFlip; + else + self &= ~Transform::HFlip; + }) + .def_property("vflip", + [](Transform &self) { + return !!(self & Transform::VFlip); + }, + [](Transform &self, bool vflip) { + if (vflip) + self |= Transform::VFlip; + else + self &= ~Transform::VFlip; + }) + .def_property("transpose", + [](Transform &self) { + return !!(self & Transform::Transpose); + }, + [](Transform &self, bool transpose) { + if (transpose) + self |= Transform::Transpose; + else + self &= ~Transform::Transpose; + }) + .def("inverse", [](Transform &self) { return -self; }) + .def("invert", [](Transform &self) { + self = -self; + }) + .def("compose", [](Transform &self, Transform &other) { + self = self * other; + }); }
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@raspberrypi.com>. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- src/py/libcamera/pymain.cpp | 63 ++++++++++++++++++++++++++++++++++++- 1 file changed, 62 insertions(+), 1 deletion(-)