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

[Laurent Pinchart]

Hi Kieran,

On Wed, Apr 03, 2019 at 11:24:27AM +0700, Kieran Bingham wrote:
> On 03/04/2019 02:12, 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 and assign
> > each 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 |  5 +++--
> >  src/cam/buffer_writer.h   |  2 +-
> >  src/cam/main.cpp          | 42 ++++++++++++++++++++++++++++-----------
> >  3 files changed, 34 insertions(+), 15 deletions(-)
> > 
> > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
> > index 2d2258b4cd1cbbc2..b82ffaded688226a 100644
> > --- a/src/cam/buffer_writer.cpp
> > +++ b/src/cam/buffer_writer.cpp
> > @@ -19,13 +19,14 @@ BufferWriter::BufferWriter(const std::string &pattern)
> >  {
> >  }
> >  
> > -int BufferWriter::write(libcamera::Buffer *buffer)
> > +int BufferWriter::write(libcamera::Buffer *buffer,
> > +			const std::string &streamName)
> >  {
> >  	std::string filename;
> >  	size_t pos;
> >  	int fd, ret = 0;
> >  
> > -	filename = pattern_;
> > +	filename = streamName + "-" + pattern_;
> >  	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..7bf785d1e83235ff 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 &streamName);
> >  
> >  private:
> >  	std::string pattern_;
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index b47bda21cbb7f220..9af7907a3d937c28 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -145,28 +145,46 @@ static int prepareCameraConfig(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];
> > +}
> > +
> >  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 = streamToName(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();
> > +		}
> 
> 
> Should we track FPS on a per stream basis? Here you are really
> calculating Requests (with at least one buffer) per second, which might
> be a perfectly reasonable thing to calculate, but what if (as a simple
> extreme) I queued up requests to alternate between two streams A and B.
> 
> --------
> R:1 A
> R:2    B
> R:3 A
> R:4    B
> --------
> 
> Is that 4 frames per interval? or two?

I think the most interesting FPS value relates to the frame rate of the
sensor, so that should be tracked at the request level, not the stream
level. It could however make sense to count the number of frames
captured for every stream and add the counts to the log message.

> It might be worth splitting the calculation out to a function and
> storing the result on a per stream basis without using static locals.

We should create a top-level class for the cam application to store the
existing global variables.

> >  
> > -	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
> > +			  << std::endl;
> >  
> > -	if (writer)
> > -		writer->write(buffer);
> > +		if (writer)
> > +			writer->write(buffer, name);
> > +	}
> >  
> >  	request = camera->createRequest();
> >  	if (!request) {

-- 
Regards,

Laurent Pinchart