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

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2019-04-03 16:05:41 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Wed, Apr 03, 2019 at 03:12:34AM +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 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);
> 
> As I've run into this issue before, and I solved it in a quite hackish
> way, how would you here deal with the following scenario:
> 
> 1) Create a request
> 2) Add <Stream *viewfinder, Buffer *buffer> to the request
> 3) int x = 0;
> 4) Queue the request;
> 5) Handle the completed request;
> 6) Re-add the just completed <Stream *viewfinder, Buffer *buffer> pair
> 7) if (x < 10) then x++; goto 4); else goto 8)
> 8) Add a <Stream *stillcapture, Buffer *buffer> to the request
> 9) Handle the completed request, now with 2 streams
> 10) Requeue viewfinder only
> 
> All this procedure just to say: how would an application recognize
> what stream is for which usage? Should they keep track of that or do
> we plan to provide a way to make Streams identifiable? (eg. by adding
> the StreamUsage to the Stream)

This is a good question and on outstanding issue we need to solve really 
soon. Currently there is no way to make a stream identifiable.

> 
> Thanks
>   j
> 
> >
> > -	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
> > +			  << std::endl;
> >
> > -	if (writer)
> > -		writer->write(buffer);
> > +		if (writer)
> > +			writer->write(buffer, name);
> > +	}
> >
> >  	request = camera->createRequest();
> >  	if (!request) {
> > --
> > 2.21.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel



-- 
Regards,
Niklas Söderlund