[libcamera-devel] [PATCH v2 3/8] cam: Turn BufferWriter into a FrameSink
Umang Jain
umang.jain at ideasonboard.com
Wed Aug 4 10:35:56 CEST 2021
Hi Laurent,
Thank you for the patch.
On 7/30/21 6:33 AM, 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)
I'm still processing what the return value signifies here. In one of the
other mails, you mentioned to insert a comment/doc about it, so I'll
wait for that in further versions.
> {
> + 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;
What exactly is ret here? I see it's initialized to '0' but never again
set till here. Am I missing something?
Rest looks good:
Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> + 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;
> }
>
> 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