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

[Jacopo Mondi]

On Thu, Oct 19, 2023 at 12:40:34AM +0300, Laurent Pinchart wrote:
> On Wed, Sep 06, 2023 at 09:10:10AM +0200, Jacopo Mondi via libcamera-devel wrote:
> > 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:
> > > > 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 ?
>
> Could we at least have the same case for the C++ and Python applications
> ? Having to write '-o mirror' for one and '-o Mirror' for the other
> isn't nice.
>

Yes, I don't remeber why I went for capital letter for the py
implementation..

> And I also wonder if we should also support a rot0 value. It would allow
> users of the application to unconditionally pass the -o option, which
> may make usage of cam/cam.py easier from scripts.
>

True, I'll add it!

> > > 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 wish EXIF had names. As far as I can tell, there's no standardized
> names for orientations, which leads to everybody using their own naming
> scheme :-(
>
> > I considered 'mirror' more intuitive than 'rotate0Flip' for users.
>
> Are users of cam and cam.py that different from applications developers
> ? If not, maybe libcamera::Orientation::Mirror would also be a more
> intuitive name than rotate0Flip ?
>

True, but that would break the enumeration naming rule.. true, if
rotation is 0 it could be omitted, but then how would you name
Orientation::Rotate0 ?

> --
> Regards,
>
> Laurent Pinchart