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

[Laurent Pinchart]

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.

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.

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

-- 
Regards,

Laurent Pinchart