[libcamera-devel] [PATCH v5 12/12] py: cam: Add option to set stream orientation

[Tomi Valkeinen]

On 04/09/2023 22:07, Jacopo Mondi wrote:
> Hi Tomi
> 
> On Mon, Sep 04, 2023 at 08:18:11PM +0300, Tomi Valkeinen wrote:
>> On 01/09/2023 18:02, Jacopo Mondi via libcamera-devel wrote:
>>> Add a '--orientation|-o' option to the Python version of the cam test
>>> application to set an orientation to the image stream.
>>>
>>> Supported values are:
>>> - Rot180: Rotate 180 degrees
>>> - Flip: vertical flip
>>> - Mirror: horizontal flip
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>>> ---
>>>    src/py/cam/cam.py | 20 ++++++++++++++++++++
>>>    1 file changed, 20 insertions(+)
>>>
>>> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py
>>> index a2a115c164b4..84d8ca1716fd 100755
>>> --- a/src/py/cam/cam.py
>>> +++ b/src/py/cam/cam.py
>>> @@ -23,6 +23,7 @@ class CameraContext:
>>>        opt_metadata: bool
>>>        opt_save_frames: bool
>>>        opt_capture: int
>>> +    opt_orientation: str
>>>        stream_names: dict[libcam.Stream, str]
>>>        streams: list[libcam.Stream]
>>> @@ -146,6 +147,20 @@ class CameraContext:
>>>                if 'pixelformat' in stream_opts:
>>>                    stream_config.pixel_format = libcam.PixelFormat(stream_opts['pixelformat'])
>>> +        if self.opt_orientation is not None:
>>> +            orientation_map = {
>>> +                'Rot180': libcam.Orientation.Rotate180,
>>> +                'Mirror': libcam.Orientation.Rotate0Flip,
>>> +                'Flip': libcam.Orientation.Rotate180Flip,
>>> +            }
>>
>> Any reason to not support all orientations? In python you can get the name
>> of the enum as a string, and you could just compare that directly to the
>> string from the user. Also, you could lower-case both before comparison, so
>> that the user could say "-o rotate270".
>>
> 
> The three supported options are the ones supporte by the C++ version
> of cam, and I like the two to behave the same. The reason why the C++
> version of cam only supports the three above options is because
> they're usually the most common features as user expects. The list
> could indeed be expanded, but for now I would like the two versions to
> behave the same. Is this ok with you ?

Well... cam.py resembles the C++ version, but it's not identical. I 
initially thought I'd write a Python clone of the C++ cam, but then I 
realized that perhaps it doesn't make sense, and instead it's better to 
write a cam.py that is more Pythonic, does things differently than the 
C++ version when it makes sense, and, when possible, does extra things 
that are easy to do in Python.

Generally speaking, I'd rather support all features, as otherwise will 
they ever be tested?

If you use the enum names directly, you can just do:

self.opt_orientation.lower() in [x.lower() for x in 
libcam.Orientation.__members__]

However, I now realized you used different naming in the option, 
compared to the enum (e.g. mirror vs rotate0flip). For that you, of 
course, need a mapping there. And if that's the target, then I think the 
above code is fine.

However, and this goes a bit outside the topic here, why do you expose 
the enums via different names? Or, if those names are better, then why 
not use those "better" names for the enum? And if you specifically want 
to use EXIF spec names for the enum, then wouldn't it be better to 
expose the orientation in cam/cam.py also with the EXIF spec names?

  Tomi