[libcamera-devel,RFC,4/4] apps: cam: Add option to set stream orientation
diff mbox series

Message ID 20230620142904.72600-5-jacopo.mondi@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Replace CameraConfiguration::transform
Related show

Commit Message

Jacopo Mondi June 20, 2023, 2:29 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>
---
 src/apps/cam/camera_session.cpp | 17 +++++++++++++++++
 src/apps/cam/main.cpp           |  5 +++++
 src/apps/cam/main.h             |  1 +
 3 files changed, 23 insertions(+)

--
2.40.1

Comments

Umang Jain June 21, 2023, 5:34 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On 6/20/23 7:59 PM, Jacopo Mondi via libcamera-devel wrote:
> 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

 From an application PoV, it should be able to set every possible 
rotation defined in enum Orientation, I think.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.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 8fcec6304d66..2d6a1440cde2 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::CameraConfiguration::rotate180;
> +		} else if (orient == "mirror") {
> +			config->orientation =
> +				libcamera::CameraConfiguration::flipRotate0;
> +		} else if (orient == "flip") {
> +			config->orientation =
> +				libcamera::CameraConfiguration::flipRotate180;
> +		} else {
> +			std::cerr << "Invalid orientation " << orient << std::endl;
> +			return;

If we want to restrict the orientation not supported by libcamera, we 
should restrict them at libcamera level itself.

Restricting them at application level doesn't make sense to me.
> +		}
> +	}
> +
>   	/* 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..8698d5eedcf8 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,
> +			 "The camera stream 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,
> --
> 2.40.1
>
Jacopo Mondi June 21, 2023, 7:41 a.m. UTC | #2 
Hi Umang

On Wed, Jun 21, 2023 at 11:04:48AM +0530, Umang Jain wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On 6/20/23 7:59 PM, Jacopo Mondi via libcamera-devel wrote:
> > 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
>
> From an application PoV, it should be able to set every possible rotation
> defined in enum Orientation, I think.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.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 8fcec6304d66..2d6a1440cde2 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::CameraConfiguration::rotate180;
> > +		} else if (orient == "mirror") {
> > +			config->orientation =
> > +				libcamera::CameraConfiguration::flipRotate0;
> > +		} else if (orient == "flip") {
> > +			config->orientation =
> > +				libcamera::CameraConfiguration::flipRotate180;
> > +		} else {
> > +			std::cerr << "Invalid orientation " << orient << std::endl;
> > +			return;
>
> If we want to restrict the orientation not supported by libcamera, we should
> restrict them at libcamera level itself.
>
> Restricting them at application level doesn't make sense to me.

Remember 'cam' is a test tool, not a full application.

I had the same thought you had, but then I wondered what was the
purpose of exposing a value we know cannot be used. The same reasoning
could be made for 'rotation0': does it make sense to have as an option
something we know it's the 'default' ?

> > +		}
> > +	}
> > +
> >   	/* 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..8698d5eedcf8 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,
> > +			 "The camera stream 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,
> > --
> > 2.40.1
> >
>

Patch
diff mbox series 

diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
index 8fcec6304d66..2d6a1440cde2 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::CameraConfiguration::rotate180;
+		} else if (orient == "mirror") {
+			config->orientation =
+				libcamera::CameraConfiguration::flipRotate0;
+		} else if (orient == "flip") {
+			config->orientation =
+				libcamera::CameraConfiguration::flipRotate180;
+		} 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..8698d5eedcf8 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,
+			 "The camera stream 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,