[3/3] apps: cam: Support full orientation options
diff mbox series

Message ID 20250501141609.717148-4-kieran.bingham@ideasonboard.com
State New
Headers show
Series
  • libcamera: rkisp1: Camera Sensor Mode Validation
Related show

Commit Message

Kieran Bingham May 1, 2025, 2:16 p.m. UTC 
The Orientation control was implemented in commit 7e5f1e1cedf5 "apps:
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
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")
Signed-off-by: Kieran Bingham <kieran.bingham@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.

 src/apps/cam/camera_session.cpp | 9 +++++++++
 src/apps/cam/main.cpp           | 5 ++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi May 2, 2025, 8:31 a.m. UTC | #1 
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 ?

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 ?

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

> Signed-off-by: Kieran Bingham <kieran.bingham@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?

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

Thanks
  j


>  #ifdef HAVE_KMS
> --
> 2.48.1
>
Kieran Bingham May 2, 2025, 10:05 a.m. UTC | #2 
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@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@ideasonboard.com>
> 


Thanks

Kieran

> Thanks
>   j
> 
> 
> >  #ifdef HAVE_KMS
> > --
> > 2.48.1
> >
Jacopo Mondi May 2, 2025, 10:15 a.m. UTC | #3 
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@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@ideasonboard.com>
> >
>
>
> Thanks
>
> Kieran
>
> > Thanks
> >   j
> >
> >
> > >  #ifdef HAVE_KMS
> > > --
> > > 2.48.1
> > >

Patch
diff mbox series 

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);
 #ifdef HAVE_KMS