[libcamera-devel] [PATCH v2 3/8] cam: Turn BufferWriter into a FrameSink
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Tue Aug 3 09:06:01 CEST 2021
Hi Laurent,
On Fri, Jul 30, 2021 at 04:03:01AM +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>
> ---
> 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)
So this return value signifies whether or not the FrameSink will hold on
to the request, and it's not a success value (?). And if it does hold on
to the request then it needs to emit it in requestReleased.
Should this be documented anywhere, or is the KMSSink later sufficient
as an "example"?
> {
> + 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;
So we're not writing every buffer in each request anymore :D Should this
be pointed out in the commit message?
Other than that, looks good.
Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> }
>
> 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
> + * 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