[PATCH 3/3] apps: cam: Support full orientation options

[Kieran Bingham]

Quoting Jacopo Mondi (2025-05-02 09:31:55)
> Hi Kieran
> 
> On Thu, May 01, 2025 at 03:16:09PM +0100, Kieran Bingham wrote:
> > The Orientation control was implemented in commit 7e5f1e1cedf5 "apps:
> 
> nit: could you use the canonical commit reference style ?

Argh - the brackets got dropped/lost somehow in escaping on the
commandline. But yes I'll fix this.

> The Orientation control was implemented in commit 7e5f1e1cedf5 ("apps:
>  cam: Add option to set stream orientation") but did not fully add all of
> 
> > cam: Add option to set stream orientation" but did not fully add all of
> > the supported Orientation types.
> >
> > The upcoming dewarp support on RkISP1 will provide full rotation
> > implementation options, including previously difficult '90' degree
> > rotations.
> >
> > Extend the cam application to support setting any of the valid options.
> >
> > The existing 'mirror' and 'flip' options are not consistent with the
> 
> Where are these existing already ?

In your commit you added at 7e5f1e1cedf5 in cam/camera_session.cpp in 2023 ;-)
> 
> > rest of the option definitions, but maintain them as useful aliases to
> > their corresponding type.
> >
> > Fixes: 7e5f1e1cedf5 ("apps: cam: Add option to set stream orientation")
> 
> Not really a fixes..

Indeed, hence below ... mayb I'll remove it in fact, it wasn't "broken"
just reduced functionality.

Mostly I only care about the tags as a way of highlighting things in the
release notes ;-)

> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > ---
> > It might be debatable if this really is a 'Fixes' as the existing commit
> > wasn't broken - just not fully implemented - however the full set of
> > Orientation types were there at the time the patch was merged.
> 
> Ah yes. Doesn't matter at all to me
> 
> >
> >  src/apps/cam/camera_session.cpp | 9 +++++++++
> >  src/apps/cam/main.cpp           | 5 ++++-
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> > index f63fcb228519..43188133d9cb 100644
> > --- a/src/apps/cam/camera_session.cpp
> > +++ b/src/apps/cam/camera_session.cpp
> > @@ -97,7 +97,16 @@ CameraSession::CameraSession(CameraManager *cm,
> >               std::string orientOpt = options_[OptOrientation].toString();
> >               static const std::map<std::string, libcamera::Orientation> orientations{
> >                       { "rot0", libcamera::Orientation::Rotate0 },
> > +                     { "rot90", libcamera::Orientation::Rotate90 },
> >                       { "rot180", libcamera::Orientation::Rotate180 },
> > +                     { "rot270", libcamera::Orientation::Rotate270 },
> > +
> > +                     { "rot0mirror", libcamera::Orientation::Rotate0Mirror },
> > +                     { "rot90mirror", libcamera::Orientation::Rotate90Mirror },
> > +                     { "rot180mirror", libcamera::Orientation::Rotate180Mirror },
> > +                     { "rot270mirror", libcamera::Orientation::Rotate270Mirror },
> > +
> > +                     /* Helpful aliases */
> >                       { "mirror", libcamera::Orientation::Rotate0Mirror },
> >                       { "flip", libcamera::Orientation::Rotate180Mirror },
> >               };
> > diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp
> > index fa266eca6d30..f1495a2db465 100644
> > --- a/src/apps/cam/main.cpp
> > +++ b/src/apps/cam/main.cpp
> > @@ -136,7 +136,10 @@ int CamApp::parseOptions(int argc, char *argv[])
> >                        OptCamera);
> >
> >       parser.addOption(OptOrientation, OptionString,
> > -                      "Desired image orientation (rot0, rot180, mirror, flip)",
> > +                      "Desired image orientation. Supported values:\n"
> > +                      "- rot0, rot90, rot180, rot270,\n"
> > +                      "- rot0mirror, rot90mirror, rot180mirror, rot270mirror,\n"
> > +                      "- mirror (alias for rot0mirror), flip (alias for rot180mirror)",
> >                        "orientation", ArgumentRequired, "orientation", false,
> >                        OptCamera);
> 
> Have you tested this on a platform that doesn't support
> transpositions?

No I haven't yet actually - but I don't think 'some platforms not
supporting' should limit the api options from cam.

I would expect that the platform should adjust or return invalid - and
if it doesn't - that's a fault of the pipeline handler - not cam.

> With the above confirmed
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> 


Thanks

Kieran

> Thanks
>   j
> 
> 
> >  #ifdef HAVE_KMS
> > --
> > 2.48.1
> >