[libcamera-devel] [PATCH v2 3/8] cam: Turn BufferWriter into a FrameSink
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Aug 3 18:01:31 CEST 2021
Hi Kieran,
On Tue, Aug 03, 2021 at 04:49:32PM +0100, Kieran Bingham wrote:
> On 03/08/2021 15:40, Laurent Pinchart wrote:
> > 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.
>
> To me, if something has consumed an item - then ... that recipient now
> owns it ... and has perhaps in some form destroyed it.
>
> Or in this instance, it would be the sink's responsibility to deal with
> the request from there on if it has consumed it, so it should be the one
> that is going to requeue it when it has completed (presumably
> asynchronously if it consumed the item/object)
>
>
> It could be that changing the 'verb' from consumeRequest to something
> else will convey the meaning better, and remove my confusion ;-)
It will be processRequest() and requestProcessed in v3.
> >> 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.
>
> I don't think the sink needs to always be forced to emit the
> requestConsumed.
>
> However, I would (later) envisage that there might be multiple sinks
> that could operate on a Request, either in parallel or in a chain.
>
> For example, perhaps there might be a desire to both write out the
> buffer with a FileSink, while simultaneously displaying it on the KMSSink.
>
> (Hrm, you might have mentioned that use case already too recently)
That will be another occasion to rename the function again :-)
> >>> }
> >>>
> >>> 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" ?
>
> Either of those ;-)
>
> >> 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