[libcamera-devel] [PATCH v2 3/8] cam: Turn BufferWriter into a FrameSink
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Aug 3 17:49:32 CEST 2021
Hi Laurent,
On 03/08/2021 15:40, Laurent Pinchart wrote:
> 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.
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 ;-)
>> 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)
>>> }
>>>
>>> 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_;
>
More information about the libcamera-devel
mailing list