[libcamera-devel,1/2] cam: capture: Break out capture to a new class

1267 diff mbox series

Reduce the complexity of main.cpp by compartmentalize the capture logic
in its own class. There is no functional change.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/cam/capture.cpp | 244 ++++++++++++++++++++++++++++++++++++++++++++
 src/cam/capture.h   |  42 ++++++++
 src/cam/main.cpp    | 236 +-----------------------------------------
 src/cam/main.h      |  19 ++++
 src/cam/meson.build |   1 +
 5 files changed, 310 insertions(+), 232 deletions(-)
 create mode 100644 src/cam/capture.cpp
 create mode 100644 src/cam/capture.h
 create mode 100644 src/cam/main.h

Hi Niklas,

On 23/05/2019 01:55, Niklas Söderlund wrote:
> Reduce the complexity of main.cpp by compartmentalize the capture logic

.. compartmentalising the capture logic (you can /s/z/ if you're american)

> in its own class. There is no functional change.

/in its/in to its/

> 

This looks good to me, and as far as I can tell it is just some code
move, so no real blocker.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/cam/capture.cpp | 244 ++++++++++++++++++++++++++++++++++++++++++++
>  src/cam/capture.h   |  42 ++++++++
>  src/cam/main.cpp    | 236 +-----------------------------------------
>  src/cam/main.h      |  19 ++++
>  src/cam/meson.build |   1 +
>  5 files changed, 310 insertions(+), 232 deletions(-)
>  create mode 100644 src/cam/capture.cpp
>  create mode 100644 src/cam/capture.h
>  create mode 100644 src/cam/main.h
> 
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> new file mode 100644
> index 0000000000000000..91f65e8cf23c888d
> --- /dev/null
> +++ b/src/cam/capture.cpp
> @@ -0,0 +1,244 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * capture.cpp - Cam capture
> + */
> +
> +#include <climits>
> +#include <iomanip>
> +#include <iostream>
> +#include <sstream>
> +
> +#include "capture.h"
> +#include "main.h"
> +
> +using namespace libcamera;
> +
> +Capture::Capture()
> +	: camera_(nullptr), writer_(nullptr)
> +{
> +}
> +
> +int Capture::run(libcamera::Camera *camera, EventLoop *loop,
> +		 const OptionsParser::Options &options)
> +{
> +	int ret;
> +
> +	if (!camera) {
> +		std::cout << "Can't capture without a camera" << std::endl;
> +		return -ENODEV;
> +	}
> +
> +	camera_ = camera;
> +
> +	ret = prepareConfig(options);
> +	if (ret) {
> +		std::cout << "Failed to prepare camera configuration" << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	if (options.isSet(OptFile)) {
> +		if (!options[OptFile].toString().empty())
> +			writer_ = new BufferWriter(options[OptFile]);
> +		else
> +			writer_ = new BufferWriter();
> +	}
> +
> +	ret = capture(loop);
> +
> +	if (options.isSet(OptFile))
> +		delete writer_;
> +
> +	return ret;
> +}
> +
> +int Capture::prepareConfig(const OptionsParser::Options &options)
> +{
> +	StreamRoles roles;
> +
> +	/* If no configuration is provided assume a single video stream. */
> +	if (!options.isSet(OptStream)) {
> +		config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> +		return 0;
> +	}
> +
> +	const std::vector<OptionValue> &streamOptions =
> +		options[OptStream].toArray();
> +
> +	/* Use roles and get a default configuration. */
> +	for (auto const &value : streamOptions) {
> +		KeyValueParser::Options conf = value.toKeyValues();
> +
> +		if (!conf.isSet("role")) {
> +			roles.push_back(StreamRole::VideoRecording);
> +		} else if (conf["role"].toString() == "viewfinder") {
> +			roles.push_back(StreamRole::Viewfinder);
> +		} else if (conf["role"].toString() == "video") {
> +			roles.push_back(StreamRole::VideoRecording);
> +		} else if (conf["role"].toString() == "still") {
> +			roles.push_back(StreamRole::StillCapture);
> +		} else {
> +			std::cerr << "Unknown stream role "
> +				  << conf["role"].toString() << std::endl;
> +			return -EINVAL;
> +		}
> +	}
> +
> +	config_ = camera_->generateConfiguration(roles);
> +	if (!config_ || config_->size() != roles.size()) {
> +		std::cerr << "Failed to get default stream configuration"
> +			  << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	/* Apply configuration explicitly requested. */
> +	unsigned int i = 0;
> +	for (auto const &value : streamOptions) {
> +		KeyValueParser::Options conf = value.toKeyValues();
> +		StreamConfiguration &cfg = config_->at(i++);
> +
> +		if (conf.isSet("width"))
> +			cfg.size.width = conf["width"];
> +
> +		if (conf.isSet("height"))
> +			cfg.size.height = conf["height"];
> +
> +		/* TODO: Translate 4CC string to ID. */
> +		if (conf.isSet("pixelformat"))
> +			cfg.pixelFormat = conf["pixelformat"];
> +	}
> +
> +	return 0;
> +}
> +
> +int Capture::capture(EventLoop *loop)
> +{
> +	int ret;
> +
> +	ret = camera_->configure(config_.get());
> +	if (ret < 0) {
> +		std::cout << "Failed to configure camera" << std::endl;
> +		return ret;
> +	}
> +
> +	streamName_.clear();
> +
> +	for (unsigned int index = 0; index < config_->size(); ++index) {
> +		StreamConfiguration &cfg = config_->at(index);
> +		streamName_[cfg.stream()] = "stream" + std::to_string(index);
> +	}
> +
> +	ret = camera_->allocateBuffers();
> +	if (ret) {
> +		std::cerr << "Failed to allocate buffers" << std::endl;
> +		return ret;
> +	}
> +
> +	camera_->requestCompleted.connect(this, &Capture::requestComplete);
> +
> +	/* Identify the stream with the least number of buffers. */
> +	unsigned int nbuffers = UINT_MAX;
> +	for (StreamConfiguration &cfg : *config_) {
> +		Stream *stream = cfg.stream();
> +		nbuffers = std::min(nbuffers, stream->bufferPool().count());
> +	}
> +
> +	/*
> +	 * TODO: make cam tool smarter to support still capture by for
> +	 * example pushing a button. For now run all streams all the time.
> +	 */
> +
> +	std::vector<Request *> requests;
> +	for (unsigned int i = 0; i < nbuffers; i++) {
> +		Request *request = camera_->createRequest();
> +		if (!request) {
> +			std::cerr << "Can't create request" << std::endl;
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		std::map<Stream *, Buffer *> map;
> +		for (StreamConfiguration &cfg : *config_) {
> +			Stream *stream = cfg.stream();
> +			map[stream] = &stream->bufferPool().buffers()[i];
> +		}
> +
> +		ret = request->setBuffers(map);
> +		if (ret < 0) {
> +			std::cerr << "Can't set buffers for request" << std::endl;
> +			goto out;
> +		}
> +
> +		requests.push_back(request);
> +	}
> +
> +	ret = camera_->start();
> +	if (ret) {
> +		std::cout << "Failed to start capture" << std::endl;
> +		goto out;
> +	}
> +
> +	for (Request *request : requests) {
> +		ret = camera_->queueRequest(request);
> +		if (ret < 0) {
> +			std::cerr << "Can't queue request" << std::endl;
> +			goto out;
> +		}
> +	}
> +
> +	std::cout << "Capture until user interrupts by SIGINT" << std::endl;
> +	ret = loop->exec();
> +
> +	ret = camera_->stop();
> +	if (ret)
> +		std::cout << "Failed to stop capture" << std::endl;
> +out:
> +	camera_->freeBuffers();
> +
> +	return ret;
> +}
> +
> +void Capture::requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> +{
> +	static uint64_t now, last = 0;
> +	double fps = 0.0;
> +
> +	if (request->status() == Request::RequestCancelled)
> +		return;
> +
> +	struct timespec time;
> +	clock_gettime(CLOCK_MONOTONIC, &time);
> +	now = time.tv_sec * 1000 + time.tv_nsec / 1000000;
> +	fps = now - last;
> +	fps = last && fps ? 1000.0 / fps : 0.0;
> +	last = now;
> +
> +	std::stringstream info;
> +	info << "fps: " << std::fixed << std::setprecision(2) << fps;
> +
> +	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> +		Stream *stream = it->first;
> +		Buffer *buffer = it->second;
> +		const std::string &name = streamName_[stream];
> +
> +		info << " " << name
> +		     << " (" << buffer->index() << ")"
> +		     << " seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()
> +		     << " bytesused: " << buffer->bytesused();
> +
> +		if (writer_)
> +			writer_->write(buffer, name);
> +	}
> +
> +	std::cout << info.str() << std::endl;
> +
> +	request = camera_->createRequest();
> +	if (!request) {
> +		std::cerr << "Can't create request" << std::endl;
> +		return;
> +	}
> +
> +	request->setBuffers(buffers);
> +	camera_->queueRequest(request);
> +}
> diff --git a/src/cam/capture.h b/src/cam/capture.h
> new file mode 100644
> index 0000000000000000..728b1d22b159b046
> --- /dev/null
> +++ b/src/cam/capture.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * capture.h - Cam capture
> + */
> +#ifndef __CAM_CAPTURE_H__
> +#define __CAM_CAPTURE_H__
> +
> +#include <memory>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/request.h>
> +#include <libcamera/stream.h>
> +
> +#include "buffer_writer.h"
> +#include "event_loop.h"
> +#include "options.h"
> +
> +class Capture
> +{
> +public:
> +	Capture();
> +
> +	int run(libcamera::Camera *camera, EventLoop *loop,
> +		const OptionsParser::Options &options);
> +private:
> +	int prepareConfig(const OptionsParser::Options &options);
> +
> +	int capture(EventLoop *loop);
> +
> +	void requestComplete(libcamera::Request *request,
> +			     const std::map<libcamera::Stream *, libcamera::Buffer *> &buffers);
> +
> +	libcamera::Camera *camera_;
> +	std::unique_ptr<libcamera::CameraConfiguration> config_;
> +
> +	std::map<libcamera::Stream *, std::string> streamName_;
> +	BufferWriter *writer_;
> +};
> +
> +#endif /* __CAM_CAPTURE_H__ */
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index 5ecd7e0e38d768bb..fe7d4f90dbf14ffd 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -5,37 +5,22 @@
>   * main.cpp - cam - The libcamera swiss army knife
>   */
>  
> -#include <algorithm>
> -#include <iomanip>
>  #include <iostream>
> -#include <limits.h>
> -#include <map>
>  #include <signal.h>
> -#include <sstream>
>  #include <string.h>
>  
>  #include <libcamera/libcamera.h>
>  
> -#include "buffer_writer.h"
> +#include "capture.h"
>  #include "event_loop.h"
> +#include "main.h"
>  #include "options.h"
>  
>  using namespace libcamera;
>  
>  OptionsParser::Options options;
>  std::shared_ptr<Camera> camera;
> -std::map<Stream *, std::string> streamInfo;
>  EventLoop *loop;
> -BufferWriter *writer;
> -
> -enum {
> -	OptCamera = 'c',
> -	OptCapture = 'C',
> -	OptFile = 'F',
> -	OptHelp = 'h',
> -	OptList = 'l',
> -	OptStream = 's',
> -};
>  
>  void signalHandler(int signal)
>  {
> @@ -85,201 +70,6 @@ static int parseOptions(int argc, char *argv[])
>  	return 0;
>  }
>  
> -static std::unique_ptr<CameraConfiguration> prepareCameraConfig()
> -{
> -	StreamRoles roles;
> -
> -	/* If no configuration is provided assume a single video stream. */
> -	if (!options.isSet(OptStream))
> -		return camera->generateConfiguration({ StreamRole::VideoRecording });
> -
> -	const std::vector<OptionValue> &streamOptions =
> -		options[OptStream].toArray();
> -
> -	/* Use roles and get a default configuration. */
> -	for (auto const &value : streamOptions) {
> -		KeyValueParser::Options conf = value.toKeyValues();
> -
> -		if (!conf.isSet("role")) {
> -			roles.push_back(StreamRole::VideoRecording);
> -		} else if (conf["role"].toString() == "viewfinder") {
> -			roles.push_back(StreamRole::Viewfinder);
> -		} else if (conf["role"].toString() == "video") {
> -			roles.push_back(StreamRole::VideoRecording);
> -		} else if (conf["role"].toString() == "still") {
> -			roles.push_back(StreamRole::StillCapture);
> -		} else {
> -			std::cerr << "Unknown stream role "
> -				  << conf["role"].toString() << std::endl;
> -			return nullptr;
> -		}
> -	}
> -
> -	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
> -	if (!config || config->size() != roles.size()) {
> -		std::cerr << "Failed to get default stream configuration"
> -			  << std::endl;
> -		return nullptr;
> -	}
> -
> -	/* Apply configuration explicitly requested. */
> -	unsigned int i = 0;
> -	for (auto const &value : streamOptions) {
> -		KeyValueParser::Options conf = value.toKeyValues();
> -		StreamConfiguration &cfg = config->at(i++);
> -
> -		if (conf.isSet("width"))
> -			cfg.size.width = conf["width"];
> -
> -		if (conf.isSet("height"))
> -			cfg.size.height = conf["height"];
> -
> -		/* TODO: Translate 4CC string to ID. */
> -		if (conf.isSet("pixelformat"))
> -			cfg.pixelFormat = conf["pixelformat"];
> -	}
> -
> -	return config;
> -}
> -
> -static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> -{
> -	static uint64_t now, last = 0;
> -	double fps = 0.0;
> -
> -	if (request->status() == Request::RequestCancelled)
> -		return;
> -
> -	struct timespec time;
> -	clock_gettime(CLOCK_MONOTONIC, &time);
> -	now = time.tv_sec * 1000 + time.tv_nsec / 1000000;
> -	fps = now - last;
> -	fps = last && fps ? 1000.0 / fps : 0.0;
> -	last = now;
> -
> -	std::stringstream info;
> -	info << "fps: " << std::fixed << std::setprecision(2) << fps;
> -
> -	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> -		Stream *stream = it->first;
> -		Buffer *buffer = it->second;
> -		const std::string &name = streamInfo[stream];
> -
> -		info << " " << name
> -		     << " (" << buffer->index() << ")"
> -		     << " seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()
> -		     << " bytesused: " << buffer->bytesused();
> -
> -		if (writer)
> -			writer->write(buffer, name);
> -	}
> -
> -	std::cout << info.str() << std::endl;
> -
> -	request = camera->createRequest();
> -	if (!request) {
> -		std::cerr << "Can't create request" << std::endl;
> -		return;
> -	}
> -
> -	request->setBuffers(buffers);
> -	camera->queueRequest(request);
> -}
> -
> -static int capture()
> -{
> -	int ret;
> -
> -	std::unique_ptr<CameraConfiguration> config = prepareCameraConfig();
> -	if (!config) {
> -		std::cout << "Failed to prepare camera configuration" << std::endl;
> -		return -EINVAL;
> -	}
> -
> -	ret = camera->configure(config.get());
> -	if (ret < 0) {
> -		std::cout << "Failed to configure camera" << std::endl;
> -		return ret;
> -	}
> -
> -	streamInfo.clear();
> -
> -	for (unsigned int index = 0; index < config->size(); ++index) {
> -		StreamConfiguration &cfg = config->at(index);
> -		streamInfo[cfg.stream()] = "stream" + std::to_string(index);
> -	}
> -
> -	ret = camera->allocateBuffers();
> -	if (ret) {
> -		std::cerr << "Failed to allocate buffers"
> -			  << std::endl;
> -		return ret;
> -	}
> -
> -	camera->requestCompleted.connect(requestComplete);
> -
> -	/* Identify the stream with the least number of buffers. */
> -	unsigned int nbuffers = UINT_MAX;
> -	for (StreamConfiguration &cfg : *config) {
> -		Stream *stream = cfg.stream();
> -		nbuffers = std::min(nbuffers, stream->bufferPool().count());
> -	}
> -
> -	/*
> -	 * TODO: make cam tool smarter to support still capture by for
> -	 * example pushing a button. For now run all streams all the time.
> -	 */
> -
> -	std::vector<Request *> requests;
> -	for (unsigned int i = 0; i < nbuffers; i++) {
> -		Request *request = camera->createRequest();
> -		if (!request) {
> -			std::cerr << "Can't create request" << std::endl;
> -			ret = -ENOMEM;
> -			goto out;
> -		}
> -
> -		std::map<Stream *, Buffer *> map;
> -		for (StreamConfiguration &cfg : *config) {
> -			Stream *stream = cfg.stream();
> -			map[stream] = &stream->bufferPool().buffers()[i];
> -		}
> -
> -		ret = request->setBuffers(map);
> -		if (ret < 0) {
> -			std::cerr << "Can't set buffers for request" << std::endl;
> -			goto out;
> -		}
> -
> -		requests.push_back(request);
> -	}
> -
> -	ret = camera->start();
> -	if (ret) {
> -		std::cout << "Failed to start capture" << std::endl;
> -		goto out;
> -	}
> -
> -	for (Request *request : requests) {
> -		ret = camera->queueRequest(request);
> -		if (ret < 0) {
> -			std::cerr << "Can't queue request" << std::endl;
> -			goto out;
> -		}
> -	}
> -
> -	std::cout << "Capture until user interrupts by SIGINT" << std::endl;
> -	ret = loop->exec();
> -
> -	ret = camera->stop();
> -	if (ret)
> -		std::cout << "Failed to stop capture" << std::endl;
> -out:
> -	camera->freeBuffers();
> -
> -	return ret;
> -}
> -
>  int main(int argc, char **argv)
>  {
>  	int ret;
> @@ -327,26 +117,8 @@ int main(int argc, char **argv)
>  	}
>  
>  	if (options.isSet(OptCapture)) {
> -		if (!camera) {
> -			std::cout << "Can't capture without a camera"
> -				  << std::endl;
> -			ret = EXIT_FAILURE;
> -			goto out;
> -		}
> -
> -		if (options.isSet(OptFile)) {
> -			if (!options[OptFile].toString().empty())
> -				writer = new BufferWriter(options[OptFile]);
> -			else
> -				writer = new BufferWriter();
> -		}
> -
> -		capture();
> -
> -		if (options.isSet(OptFile)) {
> -			delete writer;
> -			writer = nullptr;
> -		}
> +		Capture capture;
> +		ret = capture.run(camera.get(), loop, options);
>  	}
>  
>  	if (camera) {
> diff --git a/src/cam/main.h b/src/cam/main.h
> new file mode 100644
> index 0000000000000000..a48bde620dc957f0
> --- /dev/null
> +++ b/src/cam/main.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * main.h - Cam application
> + */
> +#ifndef __CAM_MAIN_H__
> +#define __CAM_MAIN_H__
> +
> +enum {
> +	OptCamera = 'c',
> +	OptCapture = 'C',
> +	OptFile = 'F',
> +	OptHelp = 'h',
> +	OptList = 'l',
> +	OptStream = 's',
> +};
> +
> +#endif /* __CAM_CAPTURE_H__ */
> diff --git a/src/cam/meson.build b/src/cam/meson.build
> index 851295091d0d5132..6d27b57393584fac 100644
> --- a/src/cam/meson.build
> +++ b/src/cam/meson.build
> @@ -1,5 +1,6 @@
>  cam_sources = files([
>      'buffer_writer.cpp',
> +    'capture.cpp',
>      'event_loop.cpp',
>      'main.cpp',
>      'options.cpp',
>

Hi Niklas,

Thank you for the patch.

On Thu, May 23, 2019 at 02:55:33AM +0200, Niklas Söderlund wrote:
> Reduce the complexity of main.cpp by compartmentalize the capture logic
> in its own class. There is no functional change.

I'll refer to Kieran's excellent British review here, except that I
think "in to" should be "into" :-)

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/cam/capture.cpp | 244 ++++++++++++++++++++++++++++++++++++++++++++
>  src/cam/capture.h   |  42 ++++++++
>  src/cam/main.cpp    | 236 +-----------------------------------------
>  src/cam/main.h      |  19 ++++
>  src/cam/meson.build |   1 +
>  5 files changed, 310 insertions(+), 232 deletions(-)
>  create mode 100644 src/cam/capture.cpp
>  create mode 100644 src/cam/capture.h
>  create mode 100644 src/cam/main.h
> 
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> new file mode 100644
> index 0000000000000000..91f65e8cf23c888d
> --- /dev/null
> +++ b/src/cam/capture.cpp
> @@ -0,0 +1,244 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * capture.cpp - Cam capture
> + */
> +
> +#include <climits>
> +#include <iomanip>
> +#include <iostream>
> +#include <sstream>
> +
> +#include "capture.h"
> +#include "main.h"
> +
> +using namespace libcamera;
> +
> +Capture::Capture()
> +	: camera_(nullptr), writer_(nullptr)
> +{
> +}
> +
> +int Capture::run(libcamera::Camera *camera, EventLoop *loop,

No need for the libcamera:: namespace qualifier given the using
directive above.

> +		 const OptionsParser::Options &options)
> +{
> +	int ret;
> +
> +	if (!camera) {
> +		std::cout << "Can't capture without a camera" << std::endl;
> +		return -ENODEV;
> +	}
> +
> +	camera_ = camera;
> +
> +	ret = prepareConfig(options);
> +	if (ret) {
> +		std::cout << "Failed to prepare camera configuration" << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	if (options.isSet(OptFile)) {
> +		if (!options[OptFile].toString().empty())
> +			writer_ = new BufferWriter(options[OptFile]);
> +		else
> +			writer_ = new BufferWriter();
> +	}
> +

Should we move the camera->config() and buffer allocation code here, out
of Capture::capture() ? They don't really belong to the capture loop
itself, and I think the change would get rid of the out: label in that
function.

> +	ret = capture(loop);
> +
> +	if (options.isSet(OptFile))
> +		delete writer_;

I would add a writer_ = nullptr here just in case we want to run
multiple capture sessions.

> +
> +	return ret;
> +}
> +
> +int Capture::prepareConfig(const OptionsParser::Options &options)
> +{
> +	StreamRoles roles;
> +
> +	/* If no configuration is provided assume a single video stream. */
> +	if (!options.isSet(OptStream)) {
> +		config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });

Should we guard against generateConfiguration() returning nullptr ?
Obviously something would be very wrong, but not crashing would still be
nice.

> +		return 0;
> +	}
> +
> +	const std::vector<OptionValue> &streamOptions =
> +		options[OptStream].toArray();
> +
> +	/* Use roles and get a default configuration. */
> +	for (auto const &value : streamOptions) {
> +		KeyValueParser::Options conf = value.toKeyValues();
> +
> +		if (!conf.isSet("role")) {
> +			roles.push_back(StreamRole::VideoRecording);
> +		} else if (conf["role"].toString() == "viewfinder") {
> +			roles.push_back(StreamRole::Viewfinder);
> +		} else if (conf["role"].toString() == "video") {
> +			roles.push_back(StreamRole::VideoRecording);
> +		} else if (conf["role"].toString() == "still") {
> +			roles.push_back(StreamRole::StillCapture);
> +		} else {
> +			std::cerr << "Unknown stream role "
> +				  << conf["role"].toString() << std::endl;
> +			return -EINVAL;
> +		}
> +	}
> +
> +	config_ = camera_->generateConfiguration(roles);
> +	if (!config_ || config_->size() != roles.size()) {
> +		std::cerr << "Failed to get default stream configuration"
> +			  << std::endl;
> +		return -EINVAL;
> +	}
> +
> +	/* Apply configuration explicitly requested. */
> +	unsigned int i = 0;
> +	for (auto const &value : streamOptions) {
> +		KeyValueParser::Options conf = value.toKeyValues();
> +		StreamConfiguration &cfg = config_->at(i++);
> +
> +		if (conf.isSet("width"))
> +			cfg.size.width = conf["width"];
> +
> +		if (conf.isSet("height"))
> +			cfg.size.height = conf["height"];
> +
> +		/* TODO: Translate 4CC string to ID. */
> +		if (conf.isSet("pixelformat"))
> +			cfg.pixelFormat = conf["pixelformat"];
> +	}
> +
> +	return 0;
> +}
> +
> +int Capture::capture(EventLoop *loop)
> +{
> +	int ret;
> +
> +	ret = camera_->configure(config_.get());
> +	if (ret < 0) {
> +		std::cout << "Failed to configure camera" << std::endl;
> +		return ret;
> +	}
> +
> +	streamName_.clear();
> +
> +	for (unsigned int index = 0; index < config_->size(); ++index) {
> +		StreamConfiguration &cfg = config_->at(index);
> +		streamName_[cfg.stream()] = "stream" + std::to_string(index);
> +	}
> +
> +	ret = camera_->allocateBuffers();
> +	if (ret) {
> +		std::cerr << "Failed to allocate buffers" << std::endl;
> +		return ret;
> +	}
> +
> +	camera_->requestCompleted.connect(this, &Capture::requestComplete);
> +
> +	/* Identify the stream with the least number of buffers. */
> +	unsigned int nbuffers = UINT_MAX;
> +	for (StreamConfiguration &cfg : *config_) {
> +		Stream *stream = cfg.stream();
> +		nbuffers = std::min(nbuffers, stream->bufferPool().count());
> +	}
> +
> +	/*
> +	 * TODO: make cam tool smarter to support still capture by for
> +	 * example pushing a button. For now run all streams all the time.
> +	 */
> +
> +	std::vector<Request *> requests;
> +	for (unsigned int i = 0; i < nbuffers; i++) {
> +		Request *request = camera_->createRequest();
> +		if (!request) {
> +			std::cerr << "Can't create request" << std::endl;
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +
> +		std::map<Stream *, Buffer *> map;
> +		for (StreamConfiguration &cfg : *config_) {
> +			Stream *stream = cfg.stream();
> +			map[stream] = &stream->bufferPool().buffers()[i];
> +		}
> +
> +		ret = request->setBuffers(map);
> +		if (ret < 0) {
> +			std::cerr << "Can't set buffers for request" << std::endl;
> +			goto out;
> +		}
> +
> +		requests.push_back(request);
> +	}
> +
> +	ret = camera_->start();
> +	if (ret) {
> +		std::cout << "Failed to start capture" << std::endl;
> +		goto out;
> +	}
> +
> +	for (Request *request : requests) {
> +		ret = camera_->queueRequest(request);
> +		if (ret < 0) {
> +			std::cerr << "Can't queue request" << std::endl;
> +			goto out;
> +		}
> +	}
> +
> +	std::cout << "Capture until user interrupts by SIGINT" << std::endl;
> +	ret = loop->exec();
> +
> +	ret = camera_->stop();
> +	if (ret)
> +		std::cout << "Failed to stop capture" << std::endl;
> +out:
> +	camera_->freeBuffers();

Maybe a config_.reset() here to make it explicit that we don't need the
config anymore ? Otherwise this will result in a reference to the camera
being stored internally (at least for some pipeline handlers), and if
the Capture class doesn't get deleted before the camera manager is
stopped, there will be a warning.

> +
> +	return ret;
> +}
> +
> +void Capture::requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> +{
> +	static uint64_t now, last = 0;

You can now move these to member variables :-)

> +	double fps = 0.0;
> +
> +	if (request->status() == Request::RequestCancelled)
> +		return;
> +
> +	struct timespec time;
> +	clock_gettime(CLOCK_MONOTONIC, &time);
> +	now = time.tv_sec * 1000 + time.tv_nsec / 1000000;
> +	fps = now - last;
> +	fps = last && fps ? 1000.0 / fps : 0.0;
> +	last = now;
> +
> +	std::stringstream info;
> +	info << "fps: " << std::fixed << std::setprecision(2) << fps;
> +
> +	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> +		Stream *stream = it->first;
> +		Buffer *buffer = it->second;
> +		const std::string &name = streamName_[stream];
> +
> +		info << " " << name
> +		     << " (" << buffer->index() << ")"
> +		     << " seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()
> +		     << " bytesused: " << buffer->bytesused();
> +
> +		if (writer_)
> +			writer_->write(buffer, name);
> +	}
> +
> +	std::cout << info.str() << std::endl;
> +
> +	request = camera_->createRequest();
> +	if (!request) {
> +		std::cerr << "Can't create request" << std::endl;
> +		return;
> +	}
> +
> +	request->setBuffers(buffers);
> +	camera_->queueRequest(request);
> +}
> diff --git a/src/cam/capture.h b/src/cam/capture.h
> new file mode 100644
> index 0000000000000000..728b1d22b159b046
> --- /dev/null
> +++ b/src/cam/capture.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * capture.h - Cam capture
> + */
> +#ifndef __CAM_CAPTURE_H__
> +#define __CAM_CAPTURE_H__
> +
> +#include <memory>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/request.h>
> +#include <libcamera/stream.h>
> +
> +#include "buffer_writer.h"
> +#include "event_loop.h"
> +#include "options.h"
> +
> +class Capture
> +{
> +public:
> +	Capture();
> +
> +	int run(libcamera::Camera *camera, EventLoop *loop,
> +		const OptionsParser::Options &options);

I like how the options are passed as a const reference.

> +private:
> +	int prepareConfig(const OptionsParser::Options &options);
> +
> +	int capture(EventLoop *loop);
> +
> +	void requestComplete(libcamera::Request *request,
> +			     const std::map<libcamera::Stream *, libcamera::Buffer *> &buffers);
> +
> +	libcamera::Camera *camera_;
> +	std::unique_ptr<libcamera::CameraConfiguration> config_;
> +
> +	std::map<libcamera::Stream *, std::string> streamName_;
> +	BufferWriter *writer_;
> +};
> +
> +#endif /* __CAM_CAPTURE_H__ */
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index 5ecd7e0e38d768bb..fe7d4f90dbf14ffd 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -5,37 +5,22 @@
>   * main.cpp - cam - The libcamera swiss army knife
>   */
>  
> -#include <algorithm>
> -#include <iomanip>
>  #include <iostream>
> -#include <limits.h>
> -#include <map>
>  #include <signal.h>
> -#include <sstream>
>  #include <string.h>
>  
>  #include <libcamera/libcamera.h>
>  
> -#include "buffer_writer.h"
> +#include "capture.h"
>  #include "event_loop.h"
> +#include "main.h"
>  #include "options.h"
>  
>  using namespace libcamera;
>  
>  OptionsParser::Options options;
>  std::shared_ptr<Camera> camera;
> -std::map<Stream *, std::string> streamInfo;
>  EventLoop *loop;
> -BufferWriter *writer;
> -
> -enum {
> -	OptCamera = 'c',
> -	OptCapture = 'C',
> -	OptFile = 'F',
> -	OptHelp = 'h',
> -	OptList = 'l',
> -	OptStream = 's',
> -};
>  
>  void signalHandler(int signal)
>  {
> @@ -85,201 +70,6 @@ static int parseOptions(int argc, char *argv[])
>  	return 0;
>  }
>  
> -static std::unique_ptr<CameraConfiguration> prepareCameraConfig()
> -{
> -	StreamRoles roles;
> -
> -	/* If no configuration is provided assume a single video stream. */
> -	if (!options.isSet(OptStream))
> -		return camera->generateConfiguration({ StreamRole::VideoRecording });
> -
> -	const std::vector<OptionValue> &streamOptions =
> -		options[OptStream].toArray();
> -
> -	/* Use roles and get a default configuration. */
> -	for (auto const &value : streamOptions) {
> -		KeyValueParser::Options conf = value.toKeyValues();
> -
> -		if (!conf.isSet("role")) {
> -			roles.push_back(StreamRole::VideoRecording);
> -		} else if (conf["role"].toString() == "viewfinder") {
> -			roles.push_back(StreamRole::Viewfinder);
> -		} else if (conf["role"].toString() == "video") {
> -			roles.push_back(StreamRole::VideoRecording);
> -		} else if (conf["role"].toString() == "still") {
> -			roles.push_back(StreamRole::StillCapture);
> -		} else {
> -			std::cerr << "Unknown stream role "
> -				  << conf["role"].toString() << std::endl;
> -			return nullptr;
> -		}
> -	}
> -
> -	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
> -	if (!config || config->size() != roles.size()) {
> -		std::cerr << "Failed to get default stream configuration"
> -			  << std::endl;
> -		return nullptr;
> -	}
> -
> -	/* Apply configuration explicitly requested. */
> -	unsigned int i = 0;
> -	for (auto const &value : streamOptions) {
> -		KeyValueParser::Options conf = value.toKeyValues();
> -		StreamConfiguration &cfg = config->at(i++);
> -
> -		if (conf.isSet("width"))
> -			cfg.size.width = conf["width"];
> -
> -		if (conf.isSet("height"))
> -			cfg.size.height = conf["height"];
> -
> -		/* TODO: Translate 4CC string to ID. */
> -		if (conf.isSet("pixelformat"))
> -			cfg.pixelFormat = conf["pixelformat"];
> -	}
> -
> -	return config;
> -}
> -
> -static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> -{
> -	static uint64_t now, last = 0;
> -	double fps = 0.0;
> -
> -	if (request->status() == Request::RequestCancelled)
> -		return;
> -
> -	struct timespec time;
> -	clock_gettime(CLOCK_MONOTONIC, &time);
> -	now = time.tv_sec * 1000 + time.tv_nsec / 1000000;
> -	fps = now - last;
> -	fps = last && fps ? 1000.0 / fps : 0.0;
> -	last = now;
> -
> -	std::stringstream info;
> -	info << "fps: " << std::fixed << std::setprecision(2) << fps;
> -
> -	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> -		Stream *stream = it->first;
> -		Buffer *buffer = it->second;
> -		const std::string &name = streamInfo[stream];
> -
> -		info << " " << name
> -		     << " (" << buffer->index() << ")"
> -		     << " seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()
> -		     << " bytesused: " << buffer->bytesused();
> -
> -		if (writer)
> -			writer->write(buffer, name);
> -	}
> -
> -	std::cout << info.str() << std::endl;
> -
> -	request = camera->createRequest();
> -	if (!request) {
> -		std::cerr << "Can't create request" << std::endl;
> -		return;
> -	}
> -
> -	request->setBuffers(buffers);
> -	camera->queueRequest(request);
> -}
> -
> -static int capture()
> -{
> -	int ret;
> -
> -	std::unique_ptr<CameraConfiguration> config = prepareCameraConfig();
> -	if (!config) {
> -		std::cout << "Failed to prepare camera configuration" << std::endl;
> -		return -EINVAL;
> -	}
> -
> -	ret = camera->configure(config.get());
> -	if (ret < 0) {
> -		std::cout << "Failed to configure camera" << std::endl;
> -		return ret;
> -	}
> -
> -	streamInfo.clear();
> -
> -	for (unsigned int index = 0; index < config->size(); ++index) {
> -		StreamConfiguration &cfg = config->at(index);
> -		streamInfo[cfg.stream()] = "stream" + std::to_string(index);
> -	}
> -
> -	ret = camera->allocateBuffers();
> -	if (ret) {
> -		std::cerr << "Failed to allocate buffers"
> -			  << std::endl;
> -		return ret;
> -	}
> -
> -	camera->requestCompleted.connect(requestComplete);
> -
> -	/* Identify the stream with the least number of buffers. */
> -	unsigned int nbuffers = UINT_MAX;
> -	for (StreamConfiguration &cfg : *config) {
> -		Stream *stream = cfg.stream();
> -		nbuffers = std::min(nbuffers, stream->bufferPool().count());
> -	}
> -
> -	/*
> -	 * TODO: make cam tool smarter to support still capture by for
> -	 * example pushing a button. For now run all streams all the time.
> -	 */
> -
> -	std::vector<Request *> requests;
> -	for (unsigned int i = 0; i < nbuffers; i++) {
> -		Request *request = camera->createRequest();
> -		if (!request) {
> -			std::cerr << "Can't create request" << std::endl;
> -			ret = -ENOMEM;
> -			goto out;
> -		}
> -
> -		std::map<Stream *, Buffer *> map;
> -		for (StreamConfiguration &cfg : *config) {
> -			Stream *stream = cfg.stream();
> -			map[stream] = &stream->bufferPool().buffers()[i];
> -		}
> -
> -		ret = request->setBuffers(map);
> -		if (ret < 0) {
> -			std::cerr << "Can't set buffers for request" << std::endl;
> -			goto out;
> -		}
> -
> -		requests.push_back(request);
> -	}
> -
> -	ret = camera->start();
> -	if (ret) {
> -		std::cout << "Failed to start capture" << std::endl;
> -		goto out;
> -	}
> -
> -	for (Request *request : requests) {
> -		ret = camera->queueRequest(request);
> -		if (ret < 0) {
> -			std::cerr << "Can't queue request" << std::endl;
> -			goto out;
> -		}
> -	}
> -
> -	std::cout << "Capture until user interrupts by SIGINT" << std::endl;
> -	ret = loop->exec();
> -
> -	ret = camera->stop();
> -	if (ret)
> -		std::cout << "Failed to stop capture" << std::endl;
> -out:
> -	camera->freeBuffers();
> -
> -	return ret;
> -}
> -
>  int main(int argc, char **argv)
>  {
>  	int ret;
> @@ -327,26 +117,8 @@ int main(int argc, char **argv)
>  	}
>  
>  	if (options.isSet(OptCapture)) {
> -		if (!camera) {
> -			std::cout << "Can't capture without a camera"
> -				  << std::endl;
> -			ret = EXIT_FAILURE;
> -			goto out;
> -		}
> -
> -		if (options.isSet(OptFile)) {
> -			if (!options[OptFile].toString().empty())
> -				writer = new BufferWriter(options[OptFile]);
> -			else
> -				writer = new BufferWriter();
> -		}
> -
> -		capture();
> -
> -		if (options.isSet(OptFile)) {
> -			delete writer;
> -			writer = nullptr;
> -		}
> +		Capture capture;
> +		ret = capture.run(camera.get(), loop, options);

Should we move the event loop to the Capture class ? We may add more
users of the loop here, but in that case I think loop->exec() should
stay in main() (which would require splitting Capture::run() into a
start and stop).

>  	}
>  
>  	if (camera) {
> diff --git a/src/cam/main.h b/src/cam/main.h
> new file mode 100644
> index 0000000000000000..a48bde620dc957f0
> --- /dev/null
> +++ b/src/cam/main.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * main.h - Cam application
> + */
> +#ifndef __CAM_MAIN_H__
> +#define __CAM_MAIN_H__
> +
> +enum {
> +	OptCamera = 'c',
> +	OptCapture = 'C',
> +	OptFile = 'F',
> +	OptHelp = 'h',
> +	OptList = 'l',
> +	OptStream = 's',
> +};
> +
> +#endif /* __CAM_CAPTURE_H__ */

__CAM_MAIN_H__

> diff --git a/src/cam/meson.build b/src/cam/meson.build
> index 851295091d0d5132..6d27b57393584fac 100644
> --- a/src/cam/meson.build
> +++ b/src/cam/meson.build
> @@ -1,5 +1,6 @@
>  cam_sources = files([
>      'buffer_writer.cpp',
> +    'capture.cpp',
>      'event_loop.cpp',
>      'main.cpp',
>      'options.cpp',

On 23/05/2019 10:29, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thu, May 23, 2019 at 02:55:33AM +0200, Niklas Söderlund wrote:
>> Reduce the complexity of main.cpp by compartmentalize the capture logic
>> in its own class. There is no functional change.
> 
> I'll refer to Kieran's excellent British review here, except that I
> think "in to" should be "into" :-)

Ahem - yes, I think 'into' is more correct than 'in to' in this instance.

--
KB

> 
>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>> ---
>>  src/cam/capture.cpp | 244 ++++++++++++++++++++++++++++++++++++++++++++
>>  src/cam/capture.h   |  42 ++++++++
>>  src/cam/main.cpp    | 236 +-----------------------------------------
>>  src/cam/main.h      |  19 ++++
>>  src/cam/meson.build |   1 +
>>  5 files changed, 310 insertions(+), 232 deletions(-)
>>  create mode 100644 src/cam/capture.cpp
>>  create mode 100644 src/cam/capture.h
>>  create mode 100644 src/cam/main.h
>>
>> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
>> new file mode 100644
>> index 0000000000000000..91f65e8cf23c888d
>> --- /dev/null
>> +++ b/src/cam/capture.cpp
>> @@ -0,0 +1,244 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * capture.cpp - Cam capture
>> + */
>> +
>> +#include <climits>
>> +#include <iomanip>
>> +#include <iostream>
>> +#include <sstream>
>> +
>> +#include "capture.h"
>> +#include "main.h"
>> +
>> +using namespace libcamera;
>> +
>> +Capture::Capture()
>> +	: camera_(nullptr), writer_(nullptr)
>> +{
>> +}
>> +
>> +int Capture::run(libcamera::Camera *camera, EventLoop *loop,
> 
> No need for the libcamera:: namespace qualifier given the using
> directive above.
> 
>> +		 const OptionsParser::Options &options)
>> +{
>> +	int ret;
>> +
>> +	if (!camera) {
>> +		std::cout << "Can't capture without a camera" << std::endl;
>> +		return -ENODEV;
>> +	}
>> +
>> +	camera_ = camera;
>> +
>> +	ret = prepareConfig(options);
>> +	if (ret) {
>> +		std::cout << "Failed to prepare camera configuration" << std::endl;
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (options.isSet(OptFile)) {
>> +		if (!options[OptFile].toString().empty())
>> +			writer_ = new BufferWriter(options[OptFile]);
>> +		else
>> +			writer_ = new BufferWriter();
>> +	}
>> +
> 
> Should we move the camera->config() and buffer allocation code here, out
> of Capture::capture() ? They don't really belong to the capture loop
> itself, and I think the change would get rid of the out: label in that
> function.
> 
>> +	ret = capture(loop);
>> +
>> +	if (options.isSet(OptFile))
>> +		delete writer_;
> 
> I would add a writer_ = nullptr here just in case we want to run
> multiple capture sessions.
> 
>> +
>> +	return ret;
>> +}
>> +
>> +int Capture::prepareConfig(const OptionsParser::Options &options)
>> +{
>> +	StreamRoles roles;
>> +
>> +	/* If no configuration is provided assume a single video stream. */
>> +	if (!options.isSet(OptStream)) {
>> +		config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> 
> Should we guard against generateConfiguration() returning nullptr ?
> Obviously something would be very wrong, but not crashing would still be
> nice.
> 
>> +		return 0;
>> +	}
>> +
>> +	const std::vector<OptionValue> &streamOptions =
>> +		options[OptStream].toArray();
>> +
>> +	/* Use roles and get a default configuration. */
>> +	for (auto const &value : streamOptions) {
>> +		KeyValueParser::Options conf = value.toKeyValues();
>> +
>> +		if (!conf.isSet("role")) {
>> +			roles.push_back(StreamRole::VideoRecording);
>> +		} else if (conf["role"].toString() == "viewfinder") {
>> +			roles.push_back(StreamRole::Viewfinder);
>> +		} else if (conf["role"].toString() == "video") {
>> +			roles.push_back(StreamRole::VideoRecording);
>> +		} else if (conf["role"].toString() == "still") {
>> +			roles.push_back(StreamRole::StillCapture);
>> +		} else {
>> +			std::cerr << "Unknown stream role "
>> +				  << conf["role"].toString() << std::endl;
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	config_ = camera_->generateConfiguration(roles);
>> +	if (!config_ || config_->size() != roles.size()) {
>> +		std::cerr << "Failed to get default stream configuration"
>> +			  << std::endl;
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Apply configuration explicitly requested. */
>> +	unsigned int i = 0;
>> +	for (auto const &value : streamOptions) {
>> +		KeyValueParser::Options conf = value.toKeyValues();
>> +		StreamConfiguration &cfg = config_->at(i++);
>> +
>> +		if (conf.isSet("width"))
>> +			cfg.size.width = conf["width"];
>> +
>> +		if (conf.isSet("height"))
>> +			cfg.size.height = conf["height"];
>> +
>> +		/* TODO: Translate 4CC string to ID. */
>> +		if (conf.isSet("pixelformat"))
>> +			cfg.pixelFormat = conf["pixelformat"];
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int Capture::capture(EventLoop *loop)
>> +{
>> +	int ret;
>> +
>> +	ret = camera_->configure(config_.get());
>> +	if (ret < 0) {
>> +		std::cout << "Failed to configure camera" << std::endl;
>> +		return ret;
>> +	}
>> +
>> +	streamName_.clear();
>> +
>> +	for (unsigned int index = 0; index < config_->size(); ++index) {
>> +		StreamConfiguration &cfg = config_->at(index);
>> +		streamName_[cfg.stream()] = "stream" + std::to_string(index);
>> +	}
>> +
>> +	ret = camera_->allocateBuffers();
>> +	if (ret) {
>> +		std::cerr << "Failed to allocate buffers" << std::endl;
>> +		return ret;
>> +	}
>> +
>> +	camera_->requestCompleted.connect(this, &Capture::requestComplete);
>> +
>> +	/* Identify the stream with the least number of buffers. */
>> +	unsigned int nbuffers = UINT_MAX;
>> +	for (StreamConfiguration &cfg : *config_) {
>> +		Stream *stream = cfg.stream();
>> +		nbuffers = std::min(nbuffers, stream->bufferPool().count());
>> +	}
>> +
>> +	/*
>> +	 * TODO: make cam tool smarter to support still capture by for
>> +	 * example pushing a button. For now run all streams all the time.
>> +	 */
>> +
>> +	std::vector<Request *> requests;
>> +	for (unsigned int i = 0; i < nbuffers; i++) {
>> +		Request *request = camera_->createRequest();
>> +		if (!request) {
>> +			std::cerr << "Can't create request" << std::endl;
>> +			ret = -ENOMEM;
>> +			goto out;
>> +		}
>> +
>> +		std::map<Stream *, Buffer *> map;
>> +		for (StreamConfiguration &cfg : *config_) {
>> +			Stream *stream = cfg.stream();
>> +			map[stream] = &stream->bufferPool().buffers()[i];
>> +		}
>> +
>> +		ret = request->setBuffers(map);
>> +		if (ret < 0) {
>> +			std::cerr << "Can't set buffers for request" << std::endl;
>> +			goto out;
>> +		}
>> +
>> +		requests.push_back(request);
>> +	}
>> +
>> +	ret = camera_->start();
>> +	if (ret) {
>> +		std::cout << "Failed to start capture" << std::endl;
>> +		goto out;
>> +	}
>> +
>> +	for (Request *request : requests) {
>> +		ret = camera_->queueRequest(request);
>> +		if (ret < 0) {
>> +			std::cerr << "Can't queue request" << std::endl;
>> +			goto out;
>> +		}
>> +	}
>> +
>> +	std::cout << "Capture until user interrupts by SIGINT" << std::endl;
>> +	ret = loop->exec();
>> +
>> +	ret = camera_->stop();
>> +	if (ret)
>> +		std::cout << "Failed to stop capture" << std::endl;
>> +out:
>> +	camera_->freeBuffers();
> 
> Maybe a config_.reset() here to make it explicit that we don't need the
> config anymore ? Otherwise this will result in a reference to the camera
> being stored internally (at least for some pipeline handlers), and if
> the Capture class doesn't get deleted before the camera manager is
> stopped, there will be a warning.
> 
>> +
>> +	return ret;
>> +}
>> +
>> +void Capture::requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
>> +{
>> +	static uint64_t now, last = 0;
> 
> You can now move these to member variables :-)
> 
>> +	double fps = 0.0;
>> +
>> +	if (request->status() == Request::RequestCancelled)
>> +		return;
>> +
>> +	struct timespec time;
>> +	clock_gettime(CLOCK_MONOTONIC, &time);
>> +	now = time.tv_sec * 1000 + time.tv_nsec / 1000000;
>> +	fps = now - last;
>> +	fps = last && fps ? 1000.0 / fps : 0.0;
>> +	last = now;
>> +
>> +	std::stringstream info;
>> +	info << "fps: " << std::fixed << std::setprecision(2) << fps;
>> +
>> +	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
>> +		Stream *stream = it->first;
>> +		Buffer *buffer = it->second;
>> +		const std::string &name = streamName_[stream];
>> +
>> +		info << " " << name
>> +		     << " (" << buffer->index() << ")"
>> +		     << " seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()
>> +		     << " bytesused: " << buffer->bytesused();
>> +
>> +		if (writer_)
>> +			writer_->write(buffer, name);
>> +	}
>> +
>> +	std::cout << info.str() << std::endl;
>> +
>> +	request = camera_->createRequest();
>> +	if (!request) {
>> +		std::cerr << "Can't create request" << std::endl;
>> +		return;
>> +	}
>> +
>> +	request->setBuffers(buffers);
>> +	camera_->queueRequest(request);
>> +}
>> diff --git a/src/cam/capture.h b/src/cam/capture.h
>> new file mode 100644
>> index 0000000000000000..728b1d22b159b046
>> --- /dev/null
>> +++ b/src/cam/capture.h
>> @@ -0,0 +1,42 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * capture.h - Cam capture
>> + */
>> +#ifndef __CAM_CAPTURE_H__
>> +#define __CAM_CAPTURE_H__
>> +
>> +#include <memory>
>> +
>> +#include <libcamera/camera.h>
>> +#include <libcamera/request.h>
>> +#include <libcamera/stream.h>
>> +
>> +#include "buffer_writer.h"
>> +#include "event_loop.h"
>> +#include "options.h"
>> +
>> +class Capture
>> +{
>> +public:
>> +	Capture();
>> +
>> +	int run(libcamera::Camera *camera, EventLoop *loop,
>> +		const OptionsParser::Options &options);
> 
> I like how the options are passed as a const reference.
> 
>> +private:
>> +	int prepareConfig(const OptionsParser::Options &options);
>> +
>> +	int capture(EventLoop *loop);
>> +
>> +	void requestComplete(libcamera::Request *request,
>> +			     const std::map<libcamera::Stream *, libcamera::Buffer *> &buffers);
>> +
>> +	libcamera::Camera *camera_;
>> +	std::unique_ptr<libcamera::CameraConfiguration> config_;
>> +
>> +	std::map<libcamera::Stream *, std::string> streamName_;
>> +	BufferWriter *writer_;
>> +};
>> +
>> +#endif /* __CAM_CAPTURE_H__ */
>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
>> index 5ecd7e0e38d768bb..fe7d4f90dbf14ffd 100644
>> --- a/src/cam/main.cpp
>> +++ b/src/cam/main.cpp
>> @@ -5,37 +5,22 @@
>>   * main.cpp - cam - The libcamera swiss army knife
>>   */
>>  
>> -#include <algorithm>
>> -#include <iomanip>
>>  #include <iostream>
>> -#include <limits.h>
>> -#include <map>
>>  #include <signal.h>
>> -#include <sstream>
>>  #include <string.h>
>>  
>>  #include <libcamera/libcamera.h>
>>  
>> -#include "buffer_writer.h"
>> +#include "capture.h"
>>  #include "event_loop.h"
>> +#include "main.h"
>>  #include "options.h"
>>  
>>  using namespace libcamera;
>>  
>>  OptionsParser::Options options;
>>  std::shared_ptr<Camera> camera;
>> -std::map<Stream *, std::string> streamInfo;
>>  EventLoop *loop;
>> -BufferWriter *writer;
>> -
>> -enum {
>> -	OptCamera = 'c',
>> -	OptCapture = 'C',
>> -	OptFile = 'F',
>> -	OptHelp = 'h',
>> -	OptList = 'l',
>> -	OptStream = 's',
>> -};
>>  
>>  void signalHandler(int signal)
>>  {
>> @@ -85,201 +70,6 @@ static int parseOptions(int argc, char *argv[])
>>  	return 0;
>>  }
>>  
>> -static std::unique_ptr<CameraConfiguration> prepareCameraConfig()
>> -{
>> -	StreamRoles roles;
>> -
>> -	/* If no configuration is provided assume a single video stream. */
>> -	if (!options.isSet(OptStream))
>> -		return camera->generateConfiguration({ StreamRole::VideoRecording });
>> -
>> -	const std::vector<OptionValue> &streamOptions =
>> -		options[OptStream].toArray();
>> -
>> -	/* Use roles and get a default configuration. */
>> -	for (auto const &value : streamOptions) {
>> -		KeyValueParser::Options conf = value.toKeyValues();
>> -
>> -		if (!conf.isSet("role")) {
>> -			roles.push_back(StreamRole::VideoRecording);
>> -		} else if (conf["role"].toString() == "viewfinder") {
>> -			roles.push_back(StreamRole::Viewfinder);
>> -		} else if (conf["role"].toString() == "video") {
>> -			roles.push_back(StreamRole::VideoRecording);
>> -		} else if (conf["role"].toString() == "still") {
>> -			roles.push_back(StreamRole::StillCapture);
>> -		} else {
>> -			std::cerr << "Unknown stream role "
>> -				  << conf["role"].toString() << std::endl;
>> -			return nullptr;
>> -		}
>> -	}
>> -
>> -	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
>> -	if (!config || config->size() != roles.size()) {
>> -		std::cerr << "Failed to get default stream configuration"
>> -			  << std::endl;
>> -		return nullptr;
>> -	}
>> -
>> -	/* Apply configuration explicitly requested. */
>> -	unsigned int i = 0;
>> -	for (auto const &value : streamOptions) {
>> -		KeyValueParser::Options conf = value.toKeyValues();
>> -		StreamConfiguration &cfg = config->at(i++);
>> -
>> -		if (conf.isSet("width"))
>> -			cfg.size.width = conf["width"];
>> -
>> -		if (conf.isSet("height"))
>> -			cfg.size.height = conf["height"];
>> -
>> -		/* TODO: Translate 4CC string to ID. */
>> -		if (conf.isSet("pixelformat"))
>> -			cfg.pixelFormat = conf["pixelformat"];
>> -	}
>> -
>> -	return config;
>> -}
>> -
>> -static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
>> -{
>> -	static uint64_t now, last = 0;
>> -	double fps = 0.0;
>> -
>> -	if (request->status() == Request::RequestCancelled)
>> -		return;
>> -
>> -	struct timespec time;
>> -	clock_gettime(CLOCK_MONOTONIC, &time);
>> -	now = time.tv_sec * 1000 + time.tv_nsec / 1000000;
>> -	fps = now - last;
>> -	fps = last && fps ? 1000.0 / fps : 0.0;
>> -	last = now;
>> -
>> -	std::stringstream info;
>> -	info << "fps: " << std::fixed << std::setprecision(2) << fps;
>> -
>> -	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
>> -		Stream *stream = it->first;
>> -		Buffer *buffer = it->second;
>> -		const std::string &name = streamInfo[stream];
>> -
>> -		info << " " << name
>> -		     << " (" << buffer->index() << ")"
>> -		     << " seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()
>> -		     << " bytesused: " << buffer->bytesused();
>> -
>> -		if (writer)
>> -			writer->write(buffer, name);
>> -	}
>> -
>> -	std::cout << info.str() << std::endl;
>> -
>> -	request = camera->createRequest();
>> -	if (!request) {
>> -		std::cerr << "Can't create request" << std::endl;
>> -		return;
>> -	}
>> -
>> -	request->setBuffers(buffers);
>> -	camera->queueRequest(request);
>> -}
>> -
>> -static int capture()
>> -{
>> -	int ret;
>> -
>> -	std::unique_ptr<CameraConfiguration> config = prepareCameraConfig();
>> -	if (!config) {
>> -		std::cout << "Failed to prepare camera configuration" << std::endl;
>> -		return -EINVAL;
>> -	}
>> -
>> -	ret = camera->configure(config.get());
>> -	if (ret < 0) {
>> -		std::cout << "Failed to configure camera" << std::endl;
>> -		return ret;
>> -	}
>> -
>> -	streamInfo.clear();
>> -
>> -	for (unsigned int index = 0; index < config->size(); ++index) {
>> -		StreamConfiguration &cfg = config->at(index);
>> -		streamInfo[cfg.stream()] = "stream" + std::to_string(index);
>> -	}
>> -
>> -	ret = camera->allocateBuffers();
>> -	if (ret) {
>> -		std::cerr << "Failed to allocate buffers"
>> -			  << std::endl;
>> -		return ret;
>> -	}
>> -
>> -	camera->requestCompleted.connect(requestComplete);
>> -
>> -	/* Identify the stream with the least number of buffers. */
>> -	unsigned int nbuffers = UINT_MAX;
>> -	for (StreamConfiguration &cfg : *config) {
>> -		Stream *stream = cfg.stream();
>> -		nbuffers = std::min(nbuffers, stream->bufferPool().count());
>> -	}
>> -
>> -	/*
>> -	 * TODO: make cam tool smarter to support still capture by for
>> -	 * example pushing a button. For now run all streams all the time.
>> -	 */
>> -
>> -	std::vector<Request *> requests;
>> -	for (unsigned int i = 0; i < nbuffers; i++) {
>> -		Request *request = camera->createRequest();
>> -		if (!request) {
>> -			std::cerr << "Can't create request" << std::endl;
>> -			ret = -ENOMEM;
>> -			goto out;
>> -		}
>> -
>> -		std::map<Stream *, Buffer *> map;
>> -		for (StreamConfiguration &cfg : *config) {
>> -			Stream *stream = cfg.stream();
>> -			map[stream] = &stream->bufferPool().buffers()[i];
>> -		}
>> -
>> -		ret = request->setBuffers(map);
>> -		if (ret < 0) {
>> -			std::cerr << "Can't set buffers for request" << std::endl;
>> -			goto out;
>> -		}
>> -
>> -		requests.push_back(request);
>> -	}
>> -
>> -	ret = camera->start();
>> -	if (ret) {
>> -		std::cout << "Failed to start capture" << std::endl;
>> -		goto out;
>> -	}
>> -
>> -	for (Request *request : requests) {
>> -		ret = camera->queueRequest(request);
>> -		if (ret < 0) {
>> -			std::cerr << "Can't queue request" << std::endl;
>> -			goto out;
>> -		}
>> -	}
>> -
>> -	std::cout << "Capture until user interrupts by SIGINT" << std::endl;
>> -	ret = loop->exec();
>> -
>> -	ret = camera->stop();
>> -	if (ret)
>> -		std::cout << "Failed to stop capture" << std::endl;
>> -out:
>> -	camera->freeBuffers();
>> -
>> -	return ret;
>> -}
>> -
>>  int main(int argc, char **argv)
>>  {
>>  	int ret;
>> @@ -327,26 +117,8 @@ int main(int argc, char **argv)
>>  	}
>>  
>>  	if (options.isSet(OptCapture)) {
>> -		if (!camera) {
>> -			std::cout << "Can't capture without a camera"
>> -				  << std::endl;
>> -			ret = EXIT_FAILURE;
>> -			goto out;
>> -		}
>> -
>> -		if (options.isSet(OptFile)) {
>> -			if (!options[OptFile].toString().empty())
>> -				writer = new BufferWriter(options[OptFile]);
>> -			else
>> -				writer = new BufferWriter();
>> -		}
>> -
>> -		capture();
>> -
>> -		if (options.isSet(OptFile)) {
>> -			delete writer;
>> -			writer = nullptr;
>> -		}
>> +		Capture capture;
>> +		ret = capture.run(camera.get(), loop, options);
> 
> Should we move the event loop to the Capture class ? We may add more
> users of the loop here, but in that case I think loop->exec() should
> stay in main() (which would require splitting Capture::run() into a
> start and stop).
> 
>>  	}
>>  
>>  	if (camera) {
>> diff --git a/src/cam/main.h b/src/cam/main.h
>> new file mode 100644
>> index 0000000000000000..a48bde620dc957f0
>> --- /dev/null
>> +++ b/src/cam/main.h
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * main.h - Cam application
>> + */
>> +#ifndef __CAM_MAIN_H__
>> +#define __CAM_MAIN_H__
>> +
>> +enum {
>> +	OptCamera = 'c',
>> +	OptCapture = 'C',
>> +	OptFile = 'F',
>> +	OptHelp = 'h',
>> +	OptList = 'l',
>> +	OptStream = 's',
>> +};
>> +
>> +#endif /* __CAM_CAPTURE_H__ */
> 
> __CAM_MAIN_H__
> 
>> diff --git a/src/cam/meson.build b/src/cam/meson.build
>> index 851295091d0d5132..6d27b57393584fac 100644
>> --- a/src/cam/meson.build
>> +++ b/src/cam/meson.build
>> @@ -1,5 +1,6 @@
>>  cam_sources = files([
>>      'buffer_writer.cpp',
>> +    'capture.cpp',
>>      'event_loop.cpp',
>>      'main.cpp',
>>      'options.cpp',
>