[libcamera-devel] [PATCH 16/17] cam: Add --info option to print information about stream(s)

[Laurent Pinchart]

Hi Niklas,

On Wed, May 29, 2019 at 11:50:47PM +0100, Kieran Bingham wrote:
> On 27/05/2019 01:15, Niklas Söderlund wrote:
> > Add a new option to cam tool which prints information about the

s/to cam tool/to the cam tool/
s/which/that/

> > configuration supplied by the user.
> 
> 
> 
> > If specified information is printed
> > about the configuration after its been verified and possibly adjusted by
> > the camera and information about each stream in the camera
> > configuration.
> 
> That paragraph is hard to interpret. But it's late so perhaps I'm just
> tired.
> 
> Do you mean something like:
> 
> If provided by the pipeline handler, information is printed about the
> configuration after it has been verified (and potentially adjusted) by
> the camera and information about each stream in the camera configuration
> is presented.

It was hard to parse for me indeed. I think Niklas meant "If the option
is specified, information about the configuration is printed after the
configuration has been verified ..."

> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/cam/info.cpp    | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >  src/cam/info.h      | 18 ++++++++++++++++++
> >  src/cam/main.cpp    |  8 ++++++++
> >  src/cam/main.h      |  1 +
> >  src/cam/meson.build |  1 +
> >  5 files changed, 72 insertions(+)
> >  create mode 100644 src/cam/info.cpp
> >  create mode 100644 src/cam/info.h
> > 
> > diff --git a/src/cam/info.cpp b/src/cam/info.cpp
> > new file mode 100644
> > index 0000000000000000..35271942600494c7
> > --- /dev/null
> > +++ b/src/cam/info.cpp
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * info.cpp - Displat stream information

s/Displat/Display/

> > + */
> > +
> > +#include <iomanip>
> > +#include <iostream>
> > +
> > +#include "info.h"
> > +
> > +using namespace libcamera;
> > +
> > +int Info::run(const CameraConfiguration *config)
> > +{
> > +	if (!config) {
> > +		std::cout
> > +			<< "Can't display information, no configuration"
> > +			<< std::endl;
> > +		return -ENODEV;
> > +	}

Is this possible ? I would remove this check and pass the configuration
by const reference.

> > +
> > +	unsigned int index = 0;
> > +	for (const StreamConfiguration cfg : *config) {

	     const StreamConfiguration &cfg

> > +		std::cout << index << ": " << cfg.toString() << std::endl;
> > +
> > +		const StreamFormats &formats = cfg.formats();
> > +		for (unsigned int pixelformat : formats.pixelformats()) {
> > +			std::cout << " * Pixelformat: 0x" << std::hex
> > +				  << std::setw(8) << pixelformat << " "
> > +				  << formats.range(pixelformat).toString()
> > +				  << std::endl;
> > +
> > +			for (const Size &size : formats.sizes(pixelformat))
> > +				std::cout << "  - " << size.toString()
> > +					  << std::endl;
> > +		}
> 
> This looks quite nice:
> 
> ./build/src/cam/cam -c "HP Wide Vision FHD Camera: HP W" -i
> Using camera HP Wide Vision FHD Camera: HP W
> 0: 1920x1080-0x47504a4d
>  * Pixelformat: 0x47504a4d Width: 176-1920 Height: 144-1080 hStep: 1
> vStep: 1
>   - 176x144
>   - 320x240
>   - 352x288
>   - 640x360
>   - 640x480
>   - 1280x720
>   - 1920x1080
> 
> Except for this bit:
>  * Pixelformat: 0x47504a4d Width: 176-1920 Height: 144-1080 hStep: 1
> vStep: 1
> 
>  - 176-1920 ?
> 
> Perhaps should we format this as
>  * Pixelformat 0x47504a4d 176x144 -> 1920x1080 ?
> 
> Infact, that's a print option in the formats.range() ...
> 
> > +
> > +		index++;
> > +	}
> > +
> > +	return 0;
> > +}
> > diff --git a/src/cam/info.h b/src/cam/info.h
> > new file mode 100644
> > index 0000000000000000..72fd446491f95033
> > --- /dev/null
> > +++ b/src/cam/info.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * info.h - Displat stream information
> > + */
> > +#ifndef __CAM_INFO_H__
> > +#define __CAM_INFO_H__
> > +
> > +#include <libcamera/camera.h>
> > +
> > +class Info
> > +{
> > +public:
> > +	int run(const libcamera::CameraConfiguration *config);
> > +};

You don't need a class for this, you can create a global function.

> > +
> > +#endif /* __CAM_INFO_H__ */
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index 25538f5ba95552d6..bf7a92b51d27c890 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -13,6 +13,7 @@
> >  
> >  #include "capture.h"
> >  #include "event_loop.h"
> > +#include "info.h"
> >  #include "main.h"
> >  #include "options.h"
> >  
> > @@ -162,6 +163,8 @@ int CamApp::parseOptions(int argc, char *argv[])
> >  			 "Set configuration of a camera stream", "stream", true);
> >  	parser.addOption(OptHelp, OptionNone, "Display this help message",
> >  			 "help");
> > +	parser.addOption(OptInfo, OptionNone,
> > +			 "Display information about stream(s)", "info");
> >  	parser.addOption(OptList, OptionNone, "List all cameras", "list");
> >  
> >  	options_ = parser.parse(argc, argv);
> > @@ -259,6 +262,11 @@ int CamApp::run()
> >  			std::cout << "- " << cam->name() << std::endl;
> >  	}
> >  
> > +	if (options_.isSet(OptInfo)) {
> > +		Info info;
> > +		info.run(config_.get());
> > +	}
> > +
> >  	if (options_.isSet(OptCapture)) {
> >  		Capture capture(camera_.get(), config_.get());
> >  		return capture.run(loop_, options_);
> > diff --git a/src/cam/main.h b/src/cam/main.h
> > index fff81b1f6c860b57..6324c042f89e7396 100644
> > --- a/src/cam/main.h
> > +++ b/src/cam/main.h
> > @@ -12,6 +12,7 @@ enum {
> >  	OptCapture = 'C',
> >  	OptFile = 'F',
> >  	OptHelp = 'h',
> > +	OptInfo = 'i',

-i is usually used for input files, I wonder if it could make sense to
reserve it for that purpose. One option could be to add a verbose
argument instead, but one may argue that controlling the display of
supported formats explicitly could also be useful.

> >  	OptList = 'l',
> >  	OptStream = 's',
> >  };
> > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > index 478346c59590631d..ee5b28421e4c1235 100644
> > --- a/src/cam/meson.build
> > +++ b/src/cam/meson.build
> > @@ -2,6 +2,7 @@ cam_sources = files([
> >      'buffer_writer.cpp',
> >      'capture.cpp',
> >      'event_loop.cpp',
> > +    'info.cpp',
> >      'main.cpp',
> >      'options.cpp',
> >  ])

-- 
Regards,

Laurent Pinchart