[libcamera-devel] [PATCH] py: Improve print methods for Transform and ColorSpace objects

[Tomi Valkeinen]

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