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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2019-04-02 18:49:02 +0300, Laurent Pinchart wrote:
> 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 ?

It could be done, but maybe it's providing too much customization then 
it's worth? I'm open to this but in a follow up patch, see bellow.

> 
> >  	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 agree and it should be fixed, this will however depend on the issue 
you point out in the roles sereis that it's hard to associate the stream 
returned from streamConfiguration() to the roles requested. Lets fix 
both issues as a series on top.

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

Again in the category if it's feature bloat or not ;-)

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

Good point.

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

-- 
Regards,
Niklas Söderlund