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

[Jacopo Mondi]

Hi Tomi

On Tue, Sep 05, 2023 at 10:51:51AM +0300, Tomi Valkeinen via libcamera-devel wrote:
> 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?

I understand this point, but the only other Orientation that does not
include a transpose is: rotate0 (Identity) (which is equivalent to not
specifying a --orientation). All the other ones require a Transpose,
which none of our pipelines can do.

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

EXIF specs do not define names but only the numerical identifiers, and
I considered 'mirror' more intuitive than 'rotate0Flip' for users.

>
>  Tomi
>