Message ID | 20220518090801.7229-1-david.plowman@raspberrypi.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Hi David, Thank you for the patch. On Wed, May 18, 2022 at 10:08:01AM +0100, David Plowman via libcamera-devel wrote: > They should now print out their meaningful string representations > instead of "object at <address>". > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/py/libcamera/pymain.cpp | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp > index fb89975c..2ba5fcbb 100644 > --- a/src/py/libcamera/pymain.cpp > +++ b/src/py/libcamera/pymain.cpp > @@ -567,6 +567,9 @@ PYBIND11_MODULE(_libcamera, m) > .def("__str__", [](Transform &self) { > return "<libcamera.Transform '" + std::string(transformToString(self)) + "'>"; > }) > + .def("__repr__", [](Transform &self) { > + return "<libcamera.Transform '" + std::string(transformToString(self)) + "'>"; > + }) Can we do the same as "[PATCH v2 12/13] py: add geometry classes" and return a string that would construct an identical Transform object when run as Python code ? Same below. > .def_property("hflip", > [](Transform &self) { > return !!(self & Transform::HFlip); > @@ -617,6 +620,9 @@ PYBIND11_MODULE(_libcamera, m) > .def("__str__", [](ColorSpace &self) { > return "<libcamera.ColorSpace '" + self.toString() + "'>"; > }) > + .def("__repr__", [](ColorSpace &self) { > + return "<libcamera.ColorSpace '" + self.toString() + "'>"; > + }) > .def_readwrite("primaries", &ColorSpace::primaries) > .def_readwrite("transferFunction", &ColorSpace::transferFunction) > .def_readwrite("ycbcrEncoding", &ColorSpace::ycbcrEncoding)
Hi Laurent Thanks for the comments. On Wed, 18 May 2022 at 11:02, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David, > > Thank you for the patch. > > On Wed, May 18, 2022 at 10:08:01AM +0100, David Plowman via libcamera-devel wrote: > > They should now print out their meaningful string representations > > instead of "object at <address>". > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/py/libcamera/pymain.cpp | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp > > index fb89975c..2ba5fcbb 100644 > > --- a/src/py/libcamera/pymain.cpp > > +++ b/src/py/libcamera/pymain.cpp > > @@ -567,6 +567,9 @@ PYBIND11_MODULE(_libcamera, m) > > .def("__str__", [](Transform &self) { > > return "<libcamera.Transform '" + std::string(transformToString(self)) + "'>"; > > }) > > + .def("__repr__", [](Transform &self) { > > + return "<libcamera.Transform '" + std::string(transformToString(self)) + "'>"; > > + }) > > Can we do the same as "[PATCH v2 12/13] py: add geometry classes" and > return a string that would construct an identical Transform object when > run as Python code ? Same below. Just to be clear then, you'd prefer a string like (for example) libcamera.Transform(hflip=False, vflip=False, transpose=False) ? > > > .def_property("hflip", > > [](Transform &self) { > > return !!(self & Transform::HFlip); > > @@ -617,6 +620,9 @@ PYBIND11_MODULE(_libcamera, m) > > .def("__str__", [](ColorSpace &self) { > > return "<libcamera.ColorSpace '" + self.toString() + "'>"; > > }) > > + .def("__repr__", [](ColorSpace &self) { > > + return "<libcamera.ColorSpace '" + self.toString() + "'>"; > > + }) > > .def_readwrite("primaries", &ColorSpace::primaries) > > .def_readwrite("transferFunction", &ColorSpace::transferFunction) > > .def_readwrite("ycbcrEncoding", &ColorSpace::ycbcrEncoding) And in this case I'd output (for example) libcamera.ColorSpace(libcamera.ColorSpace.Primaries.Rec709, libcamera.ColorSpace.TransferFunction.Srgb, libcamera.ColorSpace.YcbcrEncoding.Rec601, libcamera.ColorSpace.Range.Full) ? In this case I could perhaps "optimise" it to libcamera.ColorSpace.Jpeg() so I'd check for "short versions" first? Thanks David > > -- > Regards, > > Laurent Pinchart
Hi David, On Wed, May 18, 2022 at 12:26:06PM +0100, David Plowman wrote: > On Wed, 18 May 2022 at 11:02, Laurent Pinchart wrote: > > On Wed, May 18, 2022 at 10:08:01AM +0100, David Plowman via libcamera-devel wrote: > > > They should now print out their meaningful string representations > > > instead of "object at <address>". > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/py/libcamera/pymain.cpp | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp > > > index fb89975c..2ba5fcbb 100644 > > > --- a/src/py/libcamera/pymain.cpp > > > +++ b/src/py/libcamera/pymain.cpp > > > @@ -567,6 +567,9 @@ PYBIND11_MODULE(_libcamera, m) > > > .def("__str__", [](Transform &self) { > > > return "<libcamera.Transform '" + std::string(transformToString(self)) + "'>"; > > > }) > > > + .def("__repr__", [](Transform &self) { > > > + return "<libcamera.Transform '" + std::string(transformToString(self)) + "'>"; > > > + }) > > > > Can we do the same as "[PATCH v2 12/13] py: add geometry classes" and > > return a string that would construct an identical Transform object when > > run as Python code ? Same below. > > Just to be clear then, you'd prefer a string like (for example) > > libcamera.Transform(hflip=False, vflip=False, transpose=False) > > ? It's possibly a bit long. I wonder if we should expose the C++ presets. > > > .def_property("hflip", > > > [](Transform &self) { > > > return !!(self & Transform::HFlip); > > > @@ -617,6 +620,9 @@ PYBIND11_MODULE(_libcamera, m) > > > .def("__str__", [](ColorSpace &self) { > > > return "<libcamera.ColorSpace '" + self.toString() + "'>"; > > > }) > > > + .def("__repr__", [](ColorSpace &self) { > > > + return "<libcamera.ColorSpace '" + self.toString() + "'>"; > > > + }) > > > .def_readwrite("primaries", &ColorSpace::primaries) > > > .def_readwrite("transferFunction", &ColorSpace::transferFunction) > > > .def_readwrite("ycbcrEncoding", &ColorSpace::ycbcrEncoding) > > And in this case I'd output (for example) > > libcamera.ColorSpace(libcamera.ColorSpace.Primaries.Rec709, > libcamera.ColorSpace.TransferFunction.Srgb, > libcamera.ColorSpace.YcbcrEncoding.Rec601, > libcamera.ColorSpace.Range.Full) > > ? Ouch, that hurts. > In this case I could perhaps "optimise" it to > > libcamera.ColorSpace.Jpeg() > > so I'd check for "short versions" first? That would be good yes, but in the non-standard case, I wonder if such a long representation is acceptable. Tomi, any opinion ?
Hi, Sorry, missed this. On 18/05/2022 14:36, Laurent Pinchart via libcamera-devel wrote: > Hi David, > > On Wed, May 18, 2022 at 12:26:06PM +0100, David Plowman wrote: >> On Wed, 18 May 2022 at 11:02, Laurent Pinchart wrote: >>> On Wed, May 18, 2022 at 10:08:01AM +0100, David Plowman via libcamera-devel wrote: >>>> They should now print out their meaningful string representations >>>> instead of "object at <address>". >>>> >>>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> >>>> --- >>>> src/py/libcamera/pymain.cpp | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp >>>> index fb89975c..2ba5fcbb 100644 >>>> --- a/src/py/libcamera/pymain.cpp >>>> +++ b/src/py/libcamera/pymain.cpp >>>> @@ -567,6 +567,9 @@ PYBIND11_MODULE(_libcamera, m) >>>> .def("__str__", [](Transform &self) { >>>> return "<libcamera.Transform '" + std::string(transformToString(self)) + "'>"; >>>> }) >>>> + .def("__repr__", [](Transform &self) { >>>> + return "<libcamera.Transform '" + std::string(transformToString(self)) + "'>"; >>>> + }) >>> >>> Can we do the same as "[PATCH v2 12/13] py: add geometry classes" and >>> return a string that would construct an identical Transform object when >>> run as Python code ? Same below. >> >> Just to be clear then, you'd prefer a string like (for example) >> >> libcamera.Transform(hflip=False, vflip=False, transpose=False) >> >> ? > > It's possibly a bit long. I wonder if we should expose the C++ presets. I think this is still okayish. >>>> .def_property("hflip", >>>> [](Transform &self) { >>>> return !!(self & Transform::HFlip); >>>> @@ -617,6 +620,9 @@ PYBIND11_MODULE(_libcamera, m) >>>> .def("__str__", [](ColorSpace &self) { >>>> return "<libcamera.ColorSpace '" + self.toString() + "'>"; >>>> }) >>>> + .def("__repr__", [](ColorSpace &self) { >>>> + return "<libcamera.ColorSpace '" + self.toString() + "'>"; >>>> + }) >>>> .def_readwrite("primaries", &ColorSpace::primaries) >>>> .def_readwrite("transferFunction", &ColorSpace::transferFunction) >>>> .def_readwrite("ycbcrEncoding", &ColorSpace::ycbcrEncoding) >> >> And in this case I'd output (for example) >> >> libcamera.ColorSpace(libcamera.ColorSpace.Primaries.Rec709, >> libcamera.ColorSpace.TransferFunction.Srgb, >> libcamera.ColorSpace.YcbcrEncoding.Rec601, >> libcamera.ColorSpace.Range.Full) >> >> ? > > Ouch, that hurts. Yes, that's a bit long... >> In this case I could perhaps "optimise" it to >> >> libcamera.ColorSpace.Jpeg() >> >> so I'd check for "short versions" first? > > That would be good yes, but in the non-standard case, I wonder if such a > long representation is acceptable. Tomi, any opinion ? I don't know how useful it is to try to stick to the __repr__ rule of outputting a python code string. It's nice for simple classes, but it doesn't work for anything more complex. And I don't know if there's any practical use for the feature. It also doesn't work if implemented "naively", as e.g. I usually "import libcamera as libcam", and then "libcamera.Transform" doesn't work. I'm not sure how it could be made smarter. ColorSpace and Transform are kind of simple, but sounds like at least for ColorSpace it would require perhaps a lot of work to output something that's not overly long. And I just don't quite see the big benefit. However, there is a big benefit in outputting _something_ that describes the object, which is what the original patch does. So, perhaps for Transform, output "libcamera.Transform(hflip=False, vflip=False, transpose=False)" which should be trivial to implement, and for ColorSpace just use the C++ toString()? We can always improve it later. Tomi
diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp index fb89975c..2ba5fcbb 100644 --- a/src/py/libcamera/pymain.cpp +++ b/src/py/libcamera/pymain.cpp @@ -567,6 +567,9 @@ PYBIND11_MODULE(_libcamera, m) .def("__str__", [](Transform &self) { return "<libcamera.Transform '" + std::string(transformToString(self)) + "'>"; }) + .def("__repr__", [](Transform &self) { + return "<libcamera.Transform '" + std::string(transformToString(self)) + "'>"; + }) .def_property("hflip", [](Transform &self) { return !!(self & Transform::HFlip); @@ -617,6 +620,9 @@ PYBIND11_MODULE(_libcamera, m) .def("__str__", [](ColorSpace &self) { return "<libcamera.ColorSpace '" + self.toString() + "'>"; }) + .def("__repr__", [](ColorSpace &self) { + return "<libcamera.ColorSpace '" + self.toString() + "'>"; + }) .def_readwrite("primaries", &ColorSpace::primaries) .def_readwrite("transferFunction", &ColorSpace::transferFunction) .def_readwrite("ycbcrEncoding", &ColorSpace::ycbcrEncoding)
They should now print out their meaningful string representations instead of "object at <address>". Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/py/libcamera/pymain.cpp | 6 ++++++ 1 file changed, 6 insertions(+)