[libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a FrameSink
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 19 16:43:32 CEST 2020
Hi Laurent,
On Tue, May 19, 2020 at 04:38:14PM +0200, Niklas Söderlund wrote:
> On 2020-05-19 06:25:00 +0300, Laurent Pinchart wrote:
> > Make the BufferWriter class inherit from FrameSink, and use the
> > FrameSink API to manage it. This makes the code more generic, and will
> > allow usage of other sinks.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/cam/buffer_writer.cpp | 25 ++++++++++---
> > src/cam/buffer_writer.h | 13 ++++---
> > src/cam/capture.cpp | 76 ++++++++++++++++++++++++++++++++-------
> > src/cam/capture.h | 6 ++--
> > 4 files changed, 97 insertions(+), 23 deletions(-)
> >
> > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
> > index c5a5eb46224a..2bec4b132155 100644
> > --- a/src/cam/buffer_writer.cpp
> > +++ b/src/cam/buffer_writer.cpp
> > @@ -13,6 +13,8 @@
> > #include <sys/mman.h>
> > #include <unistd.h>
> >
> > +#include <libcamera/camera.h>
> > +
> > #include "buffer_writer.h"
> >
> > using namespace libcamera;
> > @@ -32,6 +34,21 @@ BufferWriter::~BufferWriter()
> > mappedBuffers_.clear();
> > }
> >
> > +int BufferWriter::configure(const libcamera::CameraConfiguration &config)
> > +{
> > + int ret = FrameSink::configure(config);
> > + if (ret < 0)
> > + return ret;
> > +
> > + streamNames_.clear();
> > + for (unsigned int index = 0; index < config.size(); ++index) {
> > + const StreamConfiguration &cfg = config.at(index);
> > + streamNames_[cfg.stream()] = "stream" + std::to_string(index);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > void BufferWriter::mapBuffer(FrameBuffer *buffer)
> > {
> > for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > @@ -43,7 +60,7 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)
> > }
> > }
> >
> > -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)
> > +bool BufferWriter::consumeBuffer(const Stream *stream, FrameBuffer *buffer)
> > {
> > std::string filename;
> > size_t pos;
> > @@ -53,7 +70,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)
> > pos = filename.find_first_of('#');
> > if (pos != std::string::npos) {
> > std::stringstream ss;
> > - ss << streamName << "-" << std::setw(6)
> > + ss << streamNames_[stream] << "-" << std::setw(6)
> > << std::setfill('0') << buffer->metadata().sequence;
> > filename.replace(pos, 1, ss.str());
> > }
> > @@ -62,7 +79,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)
> > (pos == std::string::npos ? O_APPEND : O_TRUNC),
> > S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);
> > if (fd == -1)
> > - return -errno;
> > + return true;
>
> Would it make sens to log an error here as the error no longer is
> propagated to the caller?
Good point, I'll do so.
> >
> > for (const FrameBuffer::Plane &plane : buffer->planes()) {
> > void *data = mappedBuffers_[plane.fd.fd()].first;
> > @@ -84,5 +101,5 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)
> >
> > close(fd);
> >
> > - return ret;
> > + return true;
> > }
> > diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h
> > index 47e26103e13e..5a5b176f73d8 100644
> > --- a/src/cam/buffer_writer.h
> > +++ b/src/cam/buffer_writer.h
> > @@ -12,18 +12,23 @@
> >
> > #include <libcamera/buffer.h>
> >
> > -class BufferWriter
> > +#include "frame_sink.h"
> > +
> > +class BufferWriter : public FrameSink
> > {
> > public:
> > BufferWriter(const std::string &pattern = "frame-#.bin");
> > ~BufferWriter();
> >
> > - void mapBuffer(libcamera::FrameBuffer *buffer);
> > + int configure(const libcamera::CameraConfiguration &config) override;
> >
> > - int write(libcamera::FrameBuffer *buffer,
> > - const std::string &streamName);
> > + void mapBuffer(libcamera::FrameBuffer *buffer) override;
> > +
> > + bool consumeBuffer(const libcamera::Stream *stream,
> > + libcamera::FrameBuffer *buffer) override;
> >
> > private:
> > + std::map<const libcamera::Stream *, std::string> streamNames_;
> > std::string pattern_;
> > std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
> > };
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index b7e06bcc9463..7fc9cba48892 100644
> > --- a/src/cam/capture.cpp
> > +++ b/src/cam/capture.cpp
> > @@ -11,6 +11,7 @@
> > #include <limits.h>
> > #include <sstream>
> >
> > +#include "buffer_writer.h"
> > #include "capture.h"
> > #include "main.h"
> >
> > @@ -18,7 +19,7 @@ using namespace libcamera;
> >
> > Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config,
> > const StreamRoles &roles)
> > - : camera_(camera), config_(config), roles_(roles), writer_(nullptr)
> > + : camera_(camera), config_(config), roles_(roles), sink_(nullptr)
> > {
> > }
> >
> > @@ -47,20 +48,28 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
> >
> > if (options.isSet(OptFile)) {
> > if (!options[OptFile].toString().empty())
> > - writer_ = new BufferWriter(options[OptFile]);
> > + sink_ = new BufferWriter(options[OptFile]);
> > else
> > - writer_ = new BufferWriter();
> > + sink_ = new BufferWriter();
> > }
> >
> > + if (sink_) {
> > + ret = sink_->configure(*config_);
> > + if (ret < 0) {
> > + std::cout << "Failed to configure frame sink"
> > + << std::endl;
> > + return ret;
> > + }
> > +
> > + sink_->bufferReleased.connect(this, &Capture::sinkRelease);
> > + }
> >
> > FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);
> >
> > ret = capture(loop, allocator);
> >
> > - if (options.isSet(OptFile)) {
> > - delete writer_;
> > - writer_ = nullptr;
> > - }
> > + delete sink_;
> > + sink_ = nullptr;
> >
> > delete allocator;
> >
> > @@ -110,16 +119,26 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)
> > return ret;
> > }
> >
> > - if (writer_)
> > - writer_->mapBuffer(buffer.get());
> > + if (sink_)
> > + sink_->mapBuffer(buffer.get());
> > }
> >
> > requests.push_back(request);
> > }
> >
> > + if (sink_) {
> > + ret = sink_->start();
> > + if (ret) {
> > + std::cout << "Failed to start frame sink" << std::endl;
> > + return ret;
> > + }
> > + }
> > +
> > ret = camera_->start();
> > if (ret) {
> > std::cout << "Failed to start capture" << std::endl;
> > + if (sink_)
> > + sink_->stop();
> > return ret;
> > }
> >
> > @@ -128,6 +147,8 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)
> > if (ret < 0) {
> > std::cerr << "Can't queue request" << std::endl;
> > camera_->stop();
> > + if (sink_)
> > + sink_->stop();
> > return ret;
> > }
> > }
> > @@ -141,6 +162,12 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)
> > if (ret)
> > std::cout << "Failed to stop capture" << std::endl;
> >
> > + if (sink_) {
> > + ret = sink_->stop();
> > + if (ret)
> > + std::cout << "Failed to stop frame sink" << std::endl;
> > + }
> > +
> > return ret;
> > }
> >
> > @@ -157,17 +184,18 @@ void Capture::requestComplete(Request *request)
> > ? 1000.0 / fps : 0.0;
> > last_ = now;
> >
> > + bool requeue = true;
> > +
> > std::stringstream info;
> > info << "fps: " << std::fixed << std::setprecision(2) << fps;
> >
> > for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> > Stream *stream = it->first;
> > FrameBuffer *buffer = it->second;
> > - const std::string &name = streamName_[stream];
> >
> > const FrameMetadata &metadata = buffer->metadata();
> >
> > - info << " " << name
> > + info << " " << streamName_[stream]
> > << " seq: " << std::setw(6) << std::setfill('0') << metadata.sequence
> > << " bytesused: ";
> >
> > @@ -178,12 +206,21 @@ void Capture::requestComplete(Request *request)
> > info << "/";
> > }
> >
> > - if (writer_)
> > - writer_->write(buffer, name);
> > + if (sink_) {
> > + if (!sink_->consumeBuffer(stream, buffer))
> > + requeue = false;
>
> This will only work for requests that contains one buffer. In this
> change it will works as BufferWriter::consumeBuffer() returns true and
> the while request is requeued as the buffer writer does not need to hold
> onto the buffers. I know that the new sink added later in this sereis
> only supports one stream so it returning false and holding onto the
> buffer will also work with this logic.
Damn, you noticed ;-)
> Would it instead make sens to rework this so that the sink always would
> need to signal that it's done with the buffer(s) instead of requing the
> request from the requestComplete() callback? That way the behavior the
> sinks would be the same disregarding for how long it needs to hold on to
> the buffer.
I think this mechanism is indeed a bit fragile, and needs to be
reworked. It works in the two cases we support (file sink and kms sink),
but isn't future-proof. That being said, fixing it properly isn't
trivial. We can only requeue the request when all buffers have been
processed (or, rather, maybe we'll need to requeue a request with other
buffers instead). We can't unconditionally queue a new request in the
bufferReleased callback of the sink, as that would only work for a
single stream.
In addition to that, I think the cam application needs to be reworked to
move processing of buffers to its main thread. The file sink, in
particular, is too expensive to run in the request completion callback,
which runs in the camera manager thread. I think it could make sense to
address the two issues together. Do you think it needs to be done as
part of this series ? Or could I just add a \todo to explain the issue
and make sure we don't forget about it ?
> > + }
> > }
> >
> > std::cout << info.str() << std::endl;
> >
> > + /*
> > + * If the frame sink holds on the buffer, we'll requeue it later in the
> > + * complete handler.
> > + */
> > + if (!requeue)
> > + return;
> > +
> > /*
> > * Create a new request and populate it with one buffer for each
> > * stream.
> > @@ -203,3 +240,16 @@ void Capture::requestComplete(Request *request)
> >
> > camera_->queueRequest(request);
> > }
> > +
> > +void Capture::sinkRelease(libcamera::FrameBuffer *buffer)
> > +{
> > + Request *request = camera_->createRequest();
> > + if (!request) {
> > + std::cerr << "Can't create request" << std::endl;
> > + return;
> > + }
> > +
> > + request->addBuffer(config_->at(0).stream(), buffer);
> > +
> > + camera_->queueRequest(request);
> > +}
> > diff --git a/src/cam/capture.h b/src/cam/capture.h
> > index c0e697b831fb..3be1d98e4d3e 100644
> > --- a/src/cam/capture.h
> > +++ b/src/cam/capture.h
> > @@ -16,10 +16,11 @@
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> >
> > -#include "buffer_writer.h"
> > #include "event_loop.h"
> > #include "options.h"
> >
> > +class FrameSink;
> > +
> > class Capture
> > {
> > public:
> > @@ -33,13 +34,14 @@ private:
> > libcamera::FrameBufferAllocator *allocator);
> >
> > void requestComplete(libcamera::Request *request);
> > + void sinkRelease(libcamera::FrameBuffer *buffer);
> >
> > std::shared_ptr<libcamera::Camera> camera_;
> > libcamera::CameraConfiguration *config_;
> > libcamera::StreamRoles roles_;
> >
> > std::map<libcamera::Stream *, std::string> streamName_;
> > - BufferWriter *writer_;
> > + FrameSink *sink_;
> > std::chrono::steady_clock::time_point last_;
> > };
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list