[libcamera-devel] [PATCH 1/2] cam: capture: Break out capture to a new class
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu May 23 11:32:16 CEST 2019
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 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
--
Kieran
More information about the libcamera-devel
mailing list