[libcamera-devel] py: Improve print methods for Transform and ColorSpace objects
diff mbox series

Message ID 20220518090801.7229-1-david.plowman@raspberrypi.com
State New
Headers show
Series
  • [libcamera-devel] py: Improve print methods for Transform and ColorSpace objects
Related show

Commit Message

David Plowman May 18, 2022, 9:08 a.m. UTC 
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(+)

Comments

Laurent Pinchart May 18, 2022, 10:02 a.m. UTC | #1 
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)
David Plowman May 18, 2022, 11:26 a.m. UTC | #2 
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
Laurent Pinchart May 18, 2022, 11:36 a.m. UTC | #3 
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 ?
Tomi Valkeinen June 7, 2022, 1:29 p.m. UTC | #4 
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

Patch
diff mbox series 

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)