[libcamera-devel,v5,11/12] apps: cam: Add option to set stream orientation
diff mbox series

Message ID 20230901150215.11585-12-jacopo.mondi@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Replace CameraConfiguration::transform
Related show

Commit Message

Jacopo Mondi Sept. 1, 2023, 3:02 p.m. UTC 
Add a '--orientation|-o' option to the cam test application to set
an orientation to the image stream.

Supported values are the ones obtained by applying flips to the camera
sensor:
- rot180: Rotate 180 degrees
- flip: vertical flip
- mirror: horizontal flip

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/apps/cam/camera_session.cpp | 17 +++++++++++++++++
 src/apps/cam/main.cpp           |  5 +++++
 src/apps/cam/main.h             |  1 +
 3 files changed, 23 insertions(+)

Comments

Laurent Pinchart Oct. 18, 2023, 9:31 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Fri, Sep 01, 2023 at 05:02:14PM +0200, Jacopo Mondi via libcamera-devel wrote:
> Add a '--orientation|-o' option to the cam test application to set

Hmmmm... '-o' is usually used as a short form of '--output'. Do we want
to keep it free for that purpose in case we add an output argument later
? I don't mind either way, I suppose we'll still b e able to change it.

> an orientation to the image stream.
> 
> Supported values are the ones obtained by applying flips to the camera
> sensor:
> - rot180: Rotate 180 degrees
> - flip: vertical flip
> - mirror: horizontal flip
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/apps/cam/camera_session.cpp | 17 +++++++++++++++++
>  src/apps/cam/main.cpp           |  5 +++++
>  src/apps/cam/main.h             |  1 +
>  3 files changed, 23 insertions(+)
> 
> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> index 066e397b5f47..1cf7a245aaa7 100644
> --- a/src/apps/cam/camera_session.cpp
> +++ b/src/apps/cam/camera_session.cpp
> @@ -65,6 +65,23 @@ CameraSession::CameraSession(CameraManager *cm,
>  		return;
>  	}
>  
> +	if (options_.isSet(OptOrientation)) {
> +		std::string orient = options_[OptOrientation].toString();
> +		if (orient == "rot180") {
> +			config->orientation =
> +				libcamera::Orientation::rotate180;
> +		} else if (orient == "mirror") {
> +			config->orientation =
> +				libcamera::Orientation::rotate0Flip;

This makes me wonder if we shouldn't call the orientation rotate0Mirror.
What do you think ?

> +		} else if (orient == "flip") {
> +			config->orientation =
> +				libcamera::Orientation::rotate180Flip;
> +		} else {
> +			std::cerr << "Invalid orientation " << orient << std::endl;
> +			return;
> +		}

This would be more C++, and easier to extend:

		static const std::map<std::string, libcamera::Orientation> orientations{
			{ "rot180", libcamera::Orientation::rotate180 },
			{ "mirror", libcamera::Orientation::rotate0Flip },
			{ "flip", libcamera::Orientation::rotate180Flip },
		};

		auto orientation = orientations.find(options_[OptOrientation].toString());
		if (orientation == orientations.end()) {
			std::cerr << "Invalid orientation " << orient << std::endl;
			return;
		}

		config->orientation = orientation.second;

Entirely up to you.

> +	}
> +
>  	/* Apply configuration if explicitly requested. */
>  	if (StreamKeyValueParser::updateConfiguration(config.get(),
>  						      options_[OptStream])) {
> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp
> index 5d8a57bc14e5..d68243edac3f 100644
> --- a/src/apps/cam/main.cpp
> +++ b/src/apps/cam/main.cpp
> @@ -133,6 +133,11 @@ int CamApp::parseOptions(int argc, char *argv[])
>  			 "Capture until interrupted by user or until <count> frames captured",
>  			 "capture", ArgumentOptional, "count", false,
>  			 OptCamera);
> +
> +	parser.addOption(OptOrientation, OptionString,
> +			 "Desired image orientation (rot180, mirror, flip)",
> +			 "orientation", ArgumentRequired, "orientation", false,
> +			 OptCamera);
>  #ifdef HAVE_KMS
>  	parser.addOption(OptDisplay, OptionString,
>  			 "Display viewfinder through DRM/KMS on specified connector",
> diff --git a/src/apps/cam/main.h b/src/apps/cam/main.h
> index 526aecece3f3..4aa959b32e13 100644
> --- a/src/apps/cam/main.h
> +++ b/src/apps/cam/main.h
> @@ -17,6 +17,7 @@ enum {
>  	OptList = 'l',
>  	OptListProperties = 'p',
>  	OptMonitor = 'm',
> +	OptOrientation = 'o',
>  	OptSDL = 'S',
>  	OptStream = 's',
>  	OptListControls = 256,
Jacopo Mondi Oct. 19, 2023, 11:55 a.m. UTC | #2 
Hi Laurent

On Thu, Oct 19, 2023 at 12:31:10AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Sep 01, 2023 at 05:02:14PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > Add a '--orientation|-o' option to the cam test application to set
>
> Hmmmm... '-o' is usually used as a short form of '--output'. Do we want
> to keep it free for that purpose in case we add an output argument later
> ? I don't mind either way, I suppose we'll still b e able to change it.
>

I presume so, and since we already have different options to select
file output, SDL output or DRM/KMS output, it feels like a -o option
to 'cam' is probably not urgent ?

> > an orientation to the image stream.
> >
> > Supported values are the ones obtained by applying flips to the camera
> > sensor:
> > - rot180: Rotate 180 degrees
> > - flip: vertical flip
> > - mirror: horizontal flip
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/apps/cam/camera_session.cpp | 17 +++++++++++++++++
> >  src/apps/cam/main.cpp           |  5 +++++
> >  src/apps/cam/main.h             |  1 +
> >  3 files changed, 23 insertions(+)
> >
> > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> > index 066e397b5f47..1cf7a245aaa7 100644
> > --- a/src/apps/cam/camera_session.cpp
> > +++ b/src/apps/cam/camera_session.cpp
> > @@ -65,6 +65,23 @@ CameraSession::CameraSession(CameraManager *cm,
> >  		return;
> >  	}
> >
> > +	if (options_.isSet(OptOrientation)) {
> > +		std::string orient = options_[OptOrientation].toString();
> > +		if (orient == "rot180") {
> > +			config->orientation =
> > +				libcamera::Orientation::rotate180;
> > +		} else if (orient == "mirror") {
> > +			config->orientation =
> > +				libcamera::Orientation::rotate0Flip;
>
> This makes me wonder if we shouldn't call the orientation rotate0Mirror.
> What do you think ?
>

Yes, sounds much better

> > +		} else if (orient == "flip") {
> > +			config->orientation =
> > +				libcamera::Orientation::rotate180Flip;
> > +		} else {
> > +			std::cerr << "Invalid orientation " << orient << std::endl;
> > +			return;
> > +		}
>
> This would be more C++, and easier to extend:
>
> 		static const std::map<std::string, libcamera::Orientation> orientations{
> 			{ "rot180", libcamera::Orientation::rotate180 },
> 			{ "mirror", libcamera::Orientation::rotate0Flip },
> 			{ "flip", libcamera::Orientation::rotate180Flip },
> 		};
>
> 		auto orientation = orientations.find(options_[OptOrientation].toString());
> 		if (orientation == orientations.end()) {
> 			std::cerr << "Invalid orientation " << orient << std::endl;
> 			return;
> 		}
>
> 		config->orientation = orientation.second;
>
> Entirely up to you.

Nicer, I'll take it in

Thanks
  j

>
> > +	}
> > +
> >  	/* Apply configuration if explicitly requested. */
> >  	if (StreamKeyValueParser::updateConfiguration(config.get(),
> >  						      options_[OptStream])) {
> > diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp
> > index 5d8a57bc14e5..d68243edac3f 100644
> > --- a/src/apps/cam/main.cpp
> > +++ b/src/apps/cam/main.cpp
> > @@ -133,6 +133,11 @@ int CamApp::parseOptions(int argc, char *argv[])
> >  			 "Capture until interrupted by user or until <count> frames captured",
> >  			 "capture", ArgumentOptional, "count", false,
> >  			 OptCamera);
> > +
> > +	parser.addOption(OptOrientation, OptionString,
> > +			 "Desired image orientation (rot180, mirror, flip)",
> > +			 "orientation", ArgumentRequired, "orientation", false,
> > +			 OptCamera);
> >  #ifdef HAVE_KMS
> >  	parser.addOption(OptDisplay, OptionString,
> >  			 "Display viewfinder through DRM/KMS on specified connector",
> > diff --git a/src/apps/cam/main.h b/src/apps/cam/main.h
> > index 526aecece3f3..4aa959b32e13 100644
> > --- a/src/apps/cam/main.h
> > +++ b/src/apps/cam/main.h
> > @@ -17,6 +17,7 @@ enum {
> >  	OptList = 'l',
> >  	OptListProperties = 'p',
> >  	OptMonitor = 'm',
> > +	OptOrientation = 'o',
> >  	OptSDL = 'S',
> >  	OptStream = 's',
> >  	OptListControls = 256,
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
index 066e397b5f47..1cf7a245aaa7 100644
--- a/src/apps/cam/camera_session.cpp
+++ b/src/apps/cam/camera_session.cpp
@@ -65,6 +65,23 @@  CameraSession::CameraSession(CameraManager *cm,
 		return;
 	}
 
+	if (options_.isSet(OptOrientation)) {
+		std::string orient = options_[OptOrientation].toString();
+		if (orient == "rot180") {
+			config->orientation =
+				libcamera::Orientation::rotate180;
+		} else if (orient == "mirror") {
+			config->orientation =
+				libcamera::Orientation::rotate0Flip;
+		} else if (orient == "flip") {
+			config->orientation =
+				libcamera::Orientation::rotate180Flip;
+		} else {
+			std::cerr << "Invalid orientation " << orient << std::endl;
+			return;
+		}
+	}
+
 	/* Apply configuration if explicitly requested. */
 	if (StreamKeyValueParser::updateConfiguration(config.get(),
 						      options_[OptStream])) {
diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp
index 5d8a57bc14e5..d68243edac3f 100644
--- a/src/apps/cam/main.cpp
+++ b/src/apps/cam/main.cpp
@@ -133,6 +133,11 @@  int CamApp::parseOptions(int argc, char *argv[])
 			 "Capture until interrupted by user or until <count> frames captured",
 			 "capture", ArgumentOptional, "count", false,
 			 OptCamera);
+
+	parser.addOption(OptOrientation, OptionString,
+			 "Desired image orientation (rot180, mirror, flip)",
+			 "orientation", ArgumentRequired, "orientation", false,
+			 OptCamera);
 #ifdef HAVE_KMS
 	parser.addOption(OptDisplay, OptionString,
 			 "Display viewfinder through DRM/KMS on specified connector",
diff --git a/src/apps/cam/main.h b/src/apps/cam/main.h
index 526aecece3f3..4aa959b32e13 100644
--- a/src/apps/cam/main.h
+++ b/src/apps/cam/main.h
@@ -17,6 +17,7 @@  enum {
 	OptList = 'l',
 	OptListProperties = 'p',
 	OptMonitor = 'm',
+	OptOrientation = 'o',
 	OptSDL = 'S',
 	OptStream = 's',
 	OptListControls = 256,