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

[Jacopo Mondi]

Hi Kieran

On Fri, May 02, 2025 at 11:05:39AM +0100, Kieran Bingham wrote:
> 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.

Absolutely, I wasn't implying that.

-Ideally- we should only list the supported rotations, but that's not
something our API supports afaict. And this is cam anyway, so best
effort at most.

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

Yes, just wanted to make sure passing in an invalid roation doesn't
cause unexpected issues.

>
> > With the above confirmed
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
>
>
> Thanks
>
> Kieran
>
> > Thanks
> >   j
> >
> >
> > >  #ifdef HAVE_KMS
> > > --
> > > 2.48.1
> > >