[libcamera-devel] [PATCH 1/2] cam: capture: Break out capture to a new class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu May 23 11:29:35 CEST 2019
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 at 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',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list