[libcamera-devel] [PATCH v2 3/8] cam: Turn BufferWriter into a FrameSink
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Aug 3 16:28:59 CEST 2021
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."
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.
Any better ideas? if not ... I'll look the other way.
> }
>
> 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'
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_;
>
More information about the libcamera-devel
mailing list