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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2019-04-06 20:23:40 +0300, Laurent Pinchart wrote:
> 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.

I agree, I feel bad when I have to add to this hack. I will try to find 
time to rework this in a follow up patch.

> 
> >  
> > @@ -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++;
> 	}

Sure.

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

I had an attempt of makig this a single line, lets see how it plays out.

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

-- 
Regards,
Niklas Söderlund