[libcamera-devel] [PATCH v3 4/5] cam: Extend request completion handler to deal with multiple streams

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sat, Apr 06, 2019 at 01:59:28AM +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 request. The streams
> are named automatically streamX, where X is the order of how the stream

s/how/which/

> was passed to configureStream().
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/cam/main.cpp | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index da05612b1c347b31..da5ca3402ce1a823 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -23,6 +23,7 @@ using namespace libcamera;
>  
>  OptionsParser::Options options;
>  std::shared_ptr<Camera> camera;
> +std::map<Stream *, std::string> streamInfo;
>  EventLoop *loop;
>  BufferWriter *writer;

Unrelated to this patch, we should really create a class to hold all
this.

>  
> @@ -87,9 +88,12 @@ static int prepareCameraConfig(CameraConfiguration *config)
>  {
>  	std::vector<StreamUsage> roles;
>  
> +	streamInfo.clear();
> +
>  	/* If no configuration is provided assume a single video stream. */
>  	if (!options.isSet(OptStream)) {
>  		*config = camera->streamConfiguration({ Stream::VideoRecording() });
> +		streamInfo[config->front()] = "stream0";
>  		return 0;
>  	}
>  
> @@ -142,31 +146,42 @@ static int prepareCameraConfig(CameraConfiguration *config)
>  			(*config)[stream].pixelFormat = conf["pixelformat"];
>  	}
>  
> +	for (unsigned int i = 0; i < config->size(); i++)
> +		streamInfo[(*config)[i]] = "stream" + std::to_string(i);

How about using iterators ?

	index = 0;
	for (Stream *stream : *config) {
		streamInfo[stream] = "stream" + std::to_string(index);
		index++;
	}

> +
>  	return 0;
>  }
>  
>  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 it = buffers.begin(); it != buffers.end(); ++it) {
> +		Stream *stream = it->first;
> +		Buffer *buffer = it->second;
> +		std::string name = streamInfo[stream];
>  
> -	double fps = buffer->timestamp() - last;
> -	fps = last && fps ? 1000000000.0 / fps : 0.0;
> -	last = buffer->timestamp();
> +		if (it == buffers.begin()) {
> +			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()
> +			  << " buf: " << buffer->index()
> +			  << " bytesused: " << buffer->bytesused()
> +			  << " timestamp: " << buffer->timestamp()
> +			  << " fps: " << std::fixed << std::setprecision(2) << fps

As the fps is per-request and not per-stream we should ideally print it
once only. One option would be to print a single line per request that
contains per-request information as well as per-stream information for
each stream, but that may be difficult to read. Another option would be
to first print per-request information, and then one line for each
stream. This may be best, otherwise the log

stream0 seq:...
stream1 seq:...

could be understood as either one request containing both streams, or
two requests containing stream0 and stream1 respectively.

I'm tempted to see if we could go for the single line per request
option while keeping it readable enough.

> +			  << std::endl;
>  
> -	if (writer)
> -		writer->write(buffer, "stream0");
> +		if (writer)
> +			writer->write(buffer, name);
> +	}
>  
>  	request = camera->createRequest();
>  	if (!request) {

-- 
Regards,

Laurent Pinchart