[libcamera-devel] [PATCH 3/8] cam: Turn BufferWriter into a FrameSink
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue May 19 16:59:28 CEST 2020
Hi Laurent,
On 2020-05-19 17:43:32 +0300, Laurent Pinchart wrote:
> 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 ;-)
Sorry, I will pay less attention in the future ;-P
>
> > 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 ?
I don't think this needs to be solved the perfect way now, but I do
think we need something where adding a 3rd sink that supports more then
one stream and needs to hold onto buffers do not break, but they might
not need to function in the most efficient way.
As we already discussed that for now cam would only support a single
sink would it make sens to rework the interface to deal with a Request
instead of a FrameBuffer? That way the logic you have here could still
be used while allowing new sinks to be added which would be less
fragile.
That being said, I do agree with you that deepening on what end gaols we
have for cam we will likely need to rework how requests are constructed.
We still have the whole concept of request reuse and request templates
to bash out :-)
I'm however not convinced I like the idea of making cam an interactive
console tool. Things like push spacebar to save frame to disk while
viewing the viewfinder on the framebuffer I think maybe is another
application, but I might be wrong.
>
> > > + }
> > > }
> > >
> > > 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list