[libcamera-devel] [PATCH 3/4] cam: Extend request complete handler to deal with multiple streams

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

In the subject line, s/complete/completion/

On Tue, Apr 02, 2019 at 02:54:05AM +0200, Niklas Söderlund wrote:
> The completion handler needs to handle all buffers in the request. Solve
> this by iterating over all buffers in the completed and assign each

s/completed/completed request/ ?

> stream a name as we encounter them. The buffer writer needs to be
> extended to learn about stream names so it can prefix the files it
> writes with it.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/cam/buffer_writer.cpp |  4 ++--
>  src/cam/buffer_writer.h   |  2 +-
>  src/cam/main.cpp          | 41 +++++++++++++++++++++++++++------------
>  3 files changed, 32 insertions(+), 15 deletions(-)
> 
> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
> index 2d2258b4cd1cbbc2..02a9610b8b3caa31 100644
> --- a/src/cam/buffer_writer.cpp
> +++ b/src/cam/buffer_writer.cpp
> @@ -19,13 +19,13 @@ BufferWriter::BufferWriter(const std::string &pattern)
>  {
>  }
>  
> -int BufferWriter::write(libcamera::Buffer *buffer)
> +int BufferWriter::write(libcamera::Buffer *buffer, const std::string &prefix)
>  {
>  	std::string filename;
>  	size_t pos;
>  	int fd, ret = 0;
>  
> -	filename = pattern_;
> +	filename = prefix + "-" + pattern_;

Should the pattern syntax be extended to include a placeholder for the
stream name ?

>  	pos = filename.find_first_of('#');
>  	if (pos != std::string::npos) {
>  		std::stringstream ss;
> diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h
> index 9705773e0e397d45..ebc6c24cd8f8d43a 100644
> --- a/src/cam/buffer_writer.h
> +++ b/src/cam/buffer_writer.h
> @@ -16,7 +16,7 @@ class BufferWriter
>  public:
>  	BufferWriter(const std::string &pattern = "frame-#.bin");
>  
> -	int write(libcamera::Buffer *buffer);
> +	int write(libcamera::Buffer *buffer, const std::string &prefix);

s/prefix/streamName/ ?

>  
>  private:
>  	std::string pattern_;
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index 1bf28ca8eb8c6da7..6aed4073f70d37a2 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -143,28 +143,45 @@ static int prepare_camera_config(std::map<Stream *, StreamConfiguration> *config
>  	return 0;
>  }
>  
> +static std::string streamToName(const Stream *stream)
> +{
> +	static std::map<const Stream *, std::string> names;
> +
> +	if (names.find(stream) == names.end())
> +		names[stream] = std::string("stream") + std::to_string(names.size());
> +
> +	return names[stream];

This will end up creating stream names based on the order of the streams
to buffers map, and there's no clear guarantee it will match the streams
order as specified on the command line. I think it would be best to
create the names map at init time to ensure consistent naming.

I also wonder if the --stream option should support a name parameter.

> +}
> +
>  static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
>  {
>  	static uint64_t last = 0;
> +	double fps = 0.0;
>  
>  	if (request->status() == Request::RequestCancelled)
>  		return;
>  
> -	Buffer *buffer = buffers.begin()->second;
> +	for (auto const &it : buffers) {
> +		Stream *stream = it.first;
> +		Buffer *buffer = it.second;
> +		std::string name = streamToName(stream);
>  
> -	double fps = buffer->timestamp() - last;
> -	fps = last && fps ? 1000000000.0 / fps : 0.0;
> -	last = buffer->timestamp();
> +		if (fps == 0.0) {

How about if (it == buffers.begin()) to emphasize you compute the fps
for the first stream only ? I got a bit confused at first.

> +			fps = buffer->timestamp() - last;
> +			fps = last && fps ? 1000000000.0 / fps : 0.0;
> +			last = buffer->timestamp();
> +		}
>  
> -	std::cout << "seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()
> -		  << " buf: " << buffer->index()
> -		  << " bytesused: " << buffer->bytesused()
> -		  << " timestamp: " << buffer->timestamp()
> -		  << " fps: " << std::fixed << std::setprecision(2) << fps
> -		  << std::endl;
> +		std::cout << name << " seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()

Could we wrap this a bit ?

> +			  << " buf: " << buffer->index()
> +			  << " bytesused: " << buffer->bytesused()
> +			  << " timestamp: " << buffer->timestamp()
> +			  << " fps: " << std::fixed << std::setprecision(2) << fps
> +			  << std::endl;
>  
> -	if (writer)
> -		writer->write(buffer);
> +		if (writer)
> +			writer->write(buffer, name);
> +	}
>  
>  	request = camera->createRequest();
>  	if (!request) {

-- 
Regards,

Laurent Pinchart