[libcamera-devel] [PATCH v5 11/12] apps: cam: Add option to set stream orientation

[Jacopo Mondi]

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 at ideasonboard.com>
> > Reviewed-by: David Plowman <david.plowman at 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