[libcamera-devel] [PATCH v2 3/8] cam: Turn BufferWriter into a FrameSink
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Aug 3 16:40:19 CEST 2021
Hi Kieran,
On Tue, Aug 03, 2021 at 03:28:59PM +0100, Kieran Bingham wrote:
> On 30/07/2021 02:03, 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>
> > ---
> > Changes since v1:
> >
> > - Print message if the file can't be opened
> > ---
> > src/cam/buffer_writer.cpp | 33 +++++++++++++++++---
> > src/cam/buffer_writer.h | 14 ++++++---
> > src/cam/camera_session.cpp | 64 ++++++++++++++++++++++++++++++++------
> > src/cam/camera_session.h | 6 ++--
> > 4 files changed, 96 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
> > index a7648a92fc92..2cf8644e843d 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,8 +60,11 @@ void BufferWriter::mapBuffer(FrameBuffer *buffer)
> > }
> > }
> >
> > -int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)
> > +bool BufferWriter::consumeRequest(Request *request)
> > {
> > + const Stream *stream = request->buffers().begin()->first;
> > + FrameBuffer *buffer = request->buffers().begin()->second;
> > +
> > std::string filename;
> > size_t pos;
> > int fd, ret = 0;
> > @@ -58,7 +78,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());
> > }
> > @@ -66,8 +86,11 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)
> > fd = open(filename.c_str(), O_CREAT | O_WRONLY |
> > (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;
> > + if (fd == -1) {
> > + std::cerr << "failed to open file " << filename << ": "
> > + << strerror(-ret) << std::endl;
> > + return true;
> > + }
> >
> > for (unsigned int i = 0; i < buffer->planes().size(); ++i) {
> > const FrameBuffer::Plane &plane = buffer->planes()[i];
> > @@ -97,5 +120,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 7626de42d369..955bc2713f4c 100644
> > --- a/src/cam/buffer_writer.h
> > +++ b/src/cam/buffer_writer.h
> > @@ -10,20 +10,24 @@
> > #include <map>
> > #include <string>
> >
> > -#include <libcamera/framebuffer.h>
> > +#include <libcamera/stream.h>
> >
> > -class BufferWriter
> > +#include "frame_sink.h"
> > +
> > +class BufferWriter : public FrameSink
> > {
> > public:
> > BufferWriter(const std::string &pattern = "");
> > ~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 consumeRequest(libcamera::Request *request) 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/camera_session.cpp b/src/cam/camera_session.cpp
> > index 0d49fc1ade83..465c8e24190e 100644
> > --- a/src/cam/camera_session.cpp
> > +++ b/src/cam/camera_session.cpp
> > @@ -13,6 +13,7 @@
> > #include <libcamera/control_ids.h>
> > #include <libcamera/property_ids.h>
> >
> > +#include "buffer_writer.h"
> > #include "camera_session.h"
> > #include "event_loop.h"
> > #include "main.h"
> > @@ -162,9 +163,20 @@ int CameraSession::start()
> >
> > if (options_.isSet(OptFile)) {
> > if (!options_[OptFile].toString().empty())
> > - writer_ = std::make_unique<BufferWriter>(options_[OptFile]);
> > + sink_ = std::make_unique<BufferWriter>(options_[OptFile]);
> > else
> > - writer_ = std::make_unique<BufferWriter>();
> > + sink_ = std::make_unique<BufferWriter>();
> > + }
> > +
> > + if (sink_) {
> > + ret = sink_->configure(*config_);
> > + if (ret < 0) {
> > + std::cout << "Failed to configure frame sink"
> > + << std::endl;
> > + return ret;
> > + }
> > +
> > + sink_->requestReleased.connect(this, &CameraSession::sinkRelease);
> > }
> >
> > allocator_ = std::make_unique<FrameBufferAllocator>(camera_);
> > @@ -178,7 +190,13 @@ void CameraSession::stop()
> > if (ret)
> > std::cout << "Failed to stop capture" << std::endl;
> >
> > - writer_.reset();
> > + if (sink_) {
> > + ret = sink_->stop();
> > + if (ret)
> > + std::cout << "Failed to stop frame sink" << std::endl;
> > + }
> > +
> > + sink_.reset();
> >
> > requests_.clear();
> >
> > @@ -227,16 +245,26 @@ int CameraSession::startCapture()
> > return ret;
> > }
> >
> > - if (writer_)
> > - writer_->mapBuffer(buffer.get());
> > + if (sink_)
> > + sink_->mapBuffer(buffer.get());
> > }
> >
> > requests_.push_back(std::move(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;
> > }
> >
> > @@ -245,6 +273,8 @@ int CameraSession::startCapture()
> > if (ret < 0) {
> > std::cerr << "Can't queue request" << std::endl;
> > camera_->stop();
> > + if (sink_)
> > + sink_->stop();
> > return ret;
> > }
> > }
> > @@ -296,6 +326,8 @@ void CameraSession::processRequest(Request *request)
> > fps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;
> > last_ = ts;
> >
> > + bool requeue = true;
> > +
> > std::stringstream info;
> > info << ts / 1000000000 << "."
> > << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000
> > @@ -304,11 +336,10 @@ void CameraSession::processRequest(Request *request)
> > for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> > const 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: ";
> >
> > @@ -318,9 +349,11 @@ void CameraSession::processRequest(Request *request)
> > if (++nplane < metadata.planes.size())
> > info << "/";
> > }
> > + }
> >
> > - if (writer_)
> > - writer_->write(buffer, name);
> > + if (sink_) {
> > + if (!sink_->consumeRequest(request))
> > + requeue = false;
>
> This reads counter intuitively.
>
> "If the request was not consumed, don't requeue it."
That's what it does, if the request wasn't consumed synchronously, don't
requeue it.
> I wonder if we can we 'move' the request into the consumeRequest?
>
> Although then we have to 'return' the request if it's not consumed...
> and I'm not sure if that's better anyway.
Yes it's not very nice.
> Any better ideas? if not ... I'll look the other way.
> > + if (!sink_->consumeRequest(request))
> > + requeue = false;
We could define an enum with two values. I'm sure we would bikeshed the
enum and enumerator names though :-)
Another option would be always have the sink emit the requestConsumed
signal. It would require more intrusive changes in
CameraSession::processRequest() though, to correctly handle the
synchronous case.
> > }
> >
> > std::cout << info.str() << std::endl;
> > @@ -340,6 +373,19 @@ void CameraSession::processRequest(Request *request)
> > return;
> > }
> >
> > + /*
> > + * If the frame sink holds on the request, we'll requeue it later in the
>
> 'holds on to the'
"on to" or "onto" ?
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > + * complete handler.
> > + */
> > + if (!requeue)
> > + return;
> > +
> > + request->reuse(Request::ReuseBuffers);
> > + camera_->queueRequest(request);
> > +}
> > +
> > +void CameraSession::sinkRelease(Request *request)
> > +{
> > request->reuse(Request::ReuseBuffers);
> > queueRequest(request);
> > }
> > diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h
> > index b0f50e7f998e..2ccc71977a99 100644
> > --- a/src/cam/camera_session.h
> > +++ b/src/cam/camera_session.h
> > @@ -21,9 +21,10 @@
> > #include <libcamera/request.h>
> > #include <libcamera/stream.h>
> >
> > -#include "buffer_writer.h"
> > #include "options.h"
> >
> > +class FrameSink;
> > +
> > class CameraSession
> > {
> > public:
> > @@ -53,13 +54,14 @@ private:
> > int queueRequest(libcamera::Request *request);
> > void requestComplete(libcamera::Request *request);
> > void processRequest(libcamera::Request *request);
> > + void sinkRelease(libcamera::Request *request);
> >
> > const OptionsParser::Options &options_;
> > std::shared_ptr<libcamera::Camera> camera_;
> > std::unique_ptr<libcamera::CameraConfiguration> config_;
> >
> > std::map<const libcamera::Stream *, std::string> streamName_;
> > - std::unique_ptr<BufferWriter> writer_;
> > + std::unique_ptr<FrameSink> sink_;
> > unsigned int cameraIndex_;
> >
> > uint64_t last_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list