[libcamera-devel] [RFC 08/12] libcamera: buffer: Add a buffer allocator

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Nov 15, 2019 at 04:11:56PM +0100, Jacopo Mondi wrote:
> On Mon, Oct 28, 2019 at 03:25:21AM +0100, Niklas Söderlund wrote:
> > From an applications point of view buffers shall not be allocated
> > directly by a camera or stream. Instead they shall be allocated
> > externally and used by a camera.
> >
> > To model this behavior add a buffer allocator that is exposed to
> > applications. How the allocator creates buffers is pipeline specific and

s/pipeline specific/pipeline-specific/

> > handled by sub-classing the stream class.
> 
> I would dare to question the whole allocator idea here... I mean, the
> allocator basically uses the streams' virtual allocateBuffer, whose
> importBuffers counterpart is the invoked at Camera::start().

I like the allocator, but I would dare to question the commit message
:-) I know what this is about as we've discussed it during the last code
camp, but it's not very clear here. Documentation will help, and I think
you could then rewrite the commit message with some excerpts of the
documentation. It should explain the design idea behind this (which is,
in a nutshell, that the libcamera API should be designed based on
externally-allocated buffers, with a helper allocator to provide buffers
to applications that can't allocate them externally).

> We still need to call Camera::allocateBuffers() for internal buffers
> allocation, which makes the 'buffer allocation' operations quite a
> few...
> 
> I wonder if, as the allocator really need to be exposed to
> applications..
> 
> I assume for the internally allocated buffer it could be simply
> replaced by calling Stream::allocate() on each Stream part of the
> camera configuration.
> 
> I still fail to find how externally allocated buffers are imported
> now, will they go through the buffer allocator as well ?
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  include/libcamera/buffer.h | 17 ++++++++++++++
> >  include/libcamera/stream.h |  1 +
> >  src/libcamera/buffer.cpp   | 48 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 66 insertions(+)
> >
> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> > index c626f669040b3c04..adb642ad5da072d2 100644
> > --- a/include/libcamera/buffer.h
> > +++ b/include/libcamera/buffer.h
> > @@ -8,11 +8,14 @@
> >  #define __LIBCAMERA_BUFFER_H__
> >
> >  #include <array>
> > +#include <map>
> > +#include <memory>
> >  #include <stdint.h>
> >  #include <vector>
> >
> >  namespace libcamera {
> >
> > +class Camera;
> >  class Request;
> >  class Stream;
> >
> > @@ -139,6 +142,20 @@ private:
> >  	std::vector<PlaneInfo> planes_;
> >  };
> >
> > +class BufferAllocator
> > +{
> > +public:
> > +	BufferAllocator(std::shared_ptr<Camera> camera);
> > +	~BufferAllocator();
> > +
> > +	int allocate(Stream *stream);

I like the buffer allocator concept, but I wonder if the API is right.
You rely on allocate() being called after Camera::configure() to let the
pipeline handler determine the buffers sizes. Isn't this a bit too
limiting ? Shouldn't we let the application request a specific size
instead ? This would require usage of VIDIOC_CREATE_BUFS in
V4L2VideoDevice. Do you think it's overkill, and applications that need
to allocate buffers with larger sizes should then allocate them
externally ?

The lack of free() makes the API unbalanced. It feels a bit fragile, but
I'd have to see how this class is used to judge if that's fine.

> > +	std::vector<Buffer *> get(Stream *stream);

Should this return a const std::vector<Buffer *> & ? Or do you envision
the caller would need a writable copy anyway ?

> > +
> > +private:
> > +	std::shared_ptr<Camera> camera_;
> > +	std::map<Stream *, std::vector<Buffer *>> allocated_;

s/allocated_/buffers_/ ?

> > +};
> > +

This should be moved to a separate file to show that it's really an
optional helper and not part of the core buffer API.

> >  } /* namespace libcamera */
> >
> >  #endif /* __LIBCAMERA_BUFFER_H__ */
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > index dac4831cfa1a9b1d..b051341511a7ab7c 100644
> > --- a/include/libcamera/stream.h
> > +++ b/include/libcamera/stream.h
> > @@ -84,6 +84,7 @@ public:
> >  	MemoryType memoryType() const { return memoryType_; }
> >
> >  protected:
> > +	friend class BufferAllocator;

Does this belong to a separate patch ?

> >  	friend class Camera;
> >
> >  	virtual int allocateBuffers(std::vector<Buffer *> *buffers) { return -EINVAL; }
> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> > index 10b16a862393b536..fce1ce5e49cbbf42 100644
> > --- a/src/libcamera/buffer.cpp
> > +++ b/src/libcamera/buffer.cpp
> > @@ -12,6 +12,9 @@
> >  #include <sys/mman.h>
> >  #include <unistd.h>
> >
> > +#include <libcamera/camera.h>
> > +#include <libcamera/stream.h>
> > +
> >  #include "log.h"
> >
> >  /**
> > @@ -393,4 +396,49 @@ void Buffer::cancel()
> >   * The intended callers are Request::prepare() and Request::completeBuffer().
> >   */
> >
> > +BufferAllocator::BufferAllocator(std::shared_ptr<Camera> camera)
> > +	: camera_(camera)
> > +{
> > +}
> > +
> > +BufferAllocator::~BufferAllocator()
> > +{
> > +	for (auto &it : allocated_) {

It just occurred to me that, technically speaking, this isn't an
iterator, so the name "it" isn't the best. The variable's type is
decltype(allocated_)::value_type, which resolves to

std::pair<const Stream *, std::vector<Buffer *>>

Maybe this should be renamed to streamBuffers ? Or possibly value ? Or
another short name we could use through the libcamera code base in
similar cases (val ? mapval ?) ? Or maybe this is overkill :-)

> > +		Stream *stream = it.first;
> > +		std::vector<Buffer *> &buffers = it.second;
> > +
> > +		for (Buffer *buffer : buffers)
> > +			delete buffer;
> > +
> > +		buffers.clear();
> > +
> > +		stream->allocateBuffers(nullptr);

Wouldn't Stream::freeBuffers() be a nicer name ?

> > +	}
> > +}
> > +
> > +int BufferAllocator::allocate(Stream *stream)
> > +{
> > +	bool found = false;
> > +
> > +	for (const Stream *s : camera_->streams()) {
> > +		if (stream == s) {
> > +			found = true;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!found)
> > +		return -EINVAL;

Should we log a message here ? Should we store a pointer to the camera
in the Stream class to facilitate this kind of error checking ?

> > +
> > +	return stream->allocateBuffers(&allocated_[stream]);
> > +}
> > +
> > +std::vector<Buffer *> BufferAllocator::get(Stream *stream)
> > +{
> > +	if (allocated_.find(stream) == allocated_.end())
> > +		return {};
> > +
> > +	return allocated_[stream];

This results in a double lookup.

	auto iter = allocated_.find(stream);
	if (iter == allocated_.end())
		return {};

	return iter->second;

> > +}
> > +
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart