[libcamera-devel,v2,3/8] cam: Turn BufferWriter into a FrameSink

13148 diff mbox series

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@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(-)

Hi Laurent,

Thanks for your work.

On 2021-07-30 04:03:01 +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@ideasonboard.com>

I really like the end result of this patch, the patch itself touches 
many different topics and could be split up to ease review, you know 
this is my pet peeve ;-)

Review is still not too harsh and I like the end result so no need to 
split it for my benefit,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
> 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;
>  	}
>  
>  	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_;
> -- 
> Regards,
> 
> Laurent Pinchart
>

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@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@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_;

Hi Paul,

On Tue, Aug 03, 2021 at 04:06:01PM +0900, paul.elder@ideasonboard.com wrote:
> 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@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"?

Documentation is always good. I'll add a comment block in
frame_sink.cpp.

> >  {
> > +	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?

Not quite, we should instead fix the BufferWriter :-) Good catch, I'll
fix this.

> Other than that, looks good.
> 
> Reviewed-by: Paul Elder <paul.elder@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_;

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@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@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_;
>

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@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.

> 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.

> >  	}
> >  
> >  	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" ?

> Reviewed-by: Kieran Bingham <kieran.bingham@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_;

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@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@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_;
>

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@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@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_;

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@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@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_;

Hi Umang,

On Wed, Aug 04, 2021 at 02:05:56PM +0530, Umang Jain wrote:
> 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@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?

Oops. There should be a

	ret = -errno;

just before this line. I'll fix it.

> Rest looks good:
> Reviewed-by: Umang Jain <umang.jain@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_;