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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sun, Jun 16, 2019 at 03:34:01PM +0200, Niklas Söderlund wrote:
> Add a new option to the cam tool that prints information about the
> configuration supplied by the user. If the option is specified,
> information about the configuration is printed after the configuration
> has been verified possibly adjusted by the camera.

s/verified/verified and/

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/cam/info.cpp    | 37 +++++++++++++++++++++++++++++++++++++
>  src/cam/info.h      | 14 ++++++++++++++
>  src/cam/main.cpp    | 13 +++++++++++++
>  src/cam/main.h      |  1 +
>  src/cam/meson.build |  1 +
>  5 files changed, 66 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..fe32ceb614e85794
> --- /dev/null
> +++ b/src/cam/info.cpp
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * info.cpp - Display stream information
> + */
> +
> +#include <iomanip>
> +#include <iostream>
> +
> +#include "info.h"
> +
> +using namespace libcamera;
> +
> +int infoConfiguration(const libcamera::CameraConfiguration &config)

Do you need the libcamera namespace qualifier as you use the namespace ?

> +{
> +	unsigned int index = 0;
> +	for (const StreamConfiguration &cfg : config) {
> +		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;
> +		}
> +
> +		index++;
> +	}
> +
> +	return 0;

The function never returns an error so you can make it void.

> +}
> diff --git a/src/cam/info.h b/src/cam/info.h
> new file mode 100644
> index 0000000000000000..65e31497e5cac358
> --- /dev/null
> +++ b/src/cam/info.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * info.h - Display stream information
> + */
> +#ifndef __CAM_INFO_H__
> +#define __CAM_INFO_H__
> +
> +#include <libcamera/camera.h>
> +
> +int infoConfiguration(const libcamera::CameraConfiguration &config);
> +
> +#endif /* __CAM_INFO_H__ */
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index 191fef3a3c8a2b64..7c68cb290d597430 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"

I would just move the infoConfiguration() function to this file
(possibly as a member of the CamApp class) and drop info.h and info.cpp.
It will only be called from a single place.

>  #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,16 @@ int CamApp::run()
>  			std::cout << "- " << cam->name() << std::endl;
>  	}
>  
> +	if (options_.isSet(OptInfo)) {
> +		if (!config_.get()) {

		if (!config) {

should do fine.

> +			std::cout << "Can't print information without a camera"

"Cannot print stream information without a camera"

> +				  << std::endl;
> +			return -ENODEV;

Maybe -EINVAL ?

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		}
> +
> +		infoConfiguration(*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..0997476bb335e446 100644
> --- a/src/cam/main.h
> +++ b/src/cam/main.h
> @@ -12,6 +12,7 @@ enum {
>  	OptCapture = 'C',
>  	OptFile = 'F',
>  	OptHelp = 'h',
> +	OptInfo = 'I',
>  	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