[libcamera-devel] [PATCH 25/30] libcamera: allocator: Add BufferAllocator to help applications allocate buffers

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Dec 02, 2019 at 02:16:24PM +0100, Jacopo Mondi wrote:
> On Wed, Nov 27, 2019 at 12:36:15AM +0100, Niklas Söderlund wrote:
> > The FrameBuffer interface is based on the idea that all buffers are
> > allocated outside libcamera and are only used by it. This is done to

s/outside/externally to/

> > make the API simpler and so that both internal and external buffers are
> > handled in the same way.

"This is meant to create a simpler API centered around usage of buffers,
regardless of where they come from."

> > As V4L2 do not provide a way to allocate buffers without the help of a
> > video device add an application facing helper which hide this. This
> > allow applications to allocate buffers and then use them with cameras.

"Linux however lacks a centralized allocator at the moment, and not all
users of libcamera are expected to use another device that could provide
suitable buffers for the camera. This patch thus adds a helper class to
allocate buffers internally in libcamera, in a way that matches the
needs of the FrameBuffer-based API."

> 
> I'm sorry but I really fail to see why we need this abstraction.
> 
> The BufferAllocator
> - is Constructed with a camera
> - allocate buffers from Streams with a StreamConfiguration
> - maintain a list of allocated buffers, indexed by stream
> 
> The usage I see in cam is
> 
> allocator = new Allocator(Camera);
> for (streamConfig : camera->config())
>         allocator.allocate(streamConfig->stream(),
>                            streamConfig);
> 
> for (nbufs : numBuffers)
>         request = new Request()
> 
>         for (streamConfig : camera->config())
>                 buffer = allocator.buffer(streamConfig->stream())[nbufs];
>                 request->addBuffer(streamConfig->stream(), buffer);
> 
> My first question would be
> - if allocator.allocate takes a Stream and a config (the stream could
>   be accessed from the config, but that's a different issue) and just
>   calls 'stream.allocateBuffers(config)' what prevents application from
>   calling stream.allocateBuffers() themselves ?

Stream::allocateBuffers() is an internal API. The idea behind the buffer
allocator and the rest of the buffer rework is to create a single API
centered around buffer usage, but this isn't possible due to the lack of
a suitable centralized allocator at the moment. We thus need to provide
a way for applications to allocate buffers internally in the camera, and
we have decided to do so through a single helper class to avoid exposing
any other allocation-related API to applications. The helper could then
be dropped later when a centralized allocator will be available, or
converted to use the centralized allocator transparently.

> - The whole point of allocator is to maintain a map to Stream to
>   vectors of Buffers ? So can't that list live in the Stream itself ?

The main point of the allocator is to handle as much of the accounting
needed for buffer allocation internally to avoid spreading it through
libcamera. We can't achieve 100% of this goal as we still need to
allocate the buffers on V4L2 video nodes.

>   Can't the map live in the camera if we want to ? They need to be put
>   together when adding a buffer-stream pair to a request, and the
>   whole dance of allocation and buffer access which all needs a Stream
>   to work seems like quite a complex and API for applications.
>   The above portion of code could be simplified to
> 
> for (stream : camera->streams()) {
>         StreamConfiguration *config = camera->streamConfig(stream);
>                                                 ^- this is easy to implement
>         stream->allocate(config);
> 
> for (nbufs : numBuffers)
>         request = new Request()
> 
>         for (streamConfig : camera->config())
>                 buffer = streamConfig->stream().buffer(i);
>                 request->addBuffer(streamConfig->stream(), buffer);
> 
> Isn't this less convoluted ?
> 
> How do we expect the buffer importing to look like, as I am missing
> how this series handles it ?
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  include/libcamera/allocator.h |  39 ++++++++++
> >  include/libcamera/meson.build |   1 +
> >  include/libcamera/stream.h    |   1 +
> >  src/libcamera/allocator.cpp   | 141 ++++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build     |   1 +
> >  5 files changed, 183 insertions(+)
> >  create mode 100644 include/libcamera/allocator.h
> >  create mode 100644 src/libcamera/allocator.cpp
> >
> > diff --git a/include/libcamera/allocator.h b/include/libcamera/allocator.h
> > new file mode 100644
> > index 0000000000000000..36fce38491b5f3ef
> > --- /dev/null
> > +++ b/include/libcamera/allocator.h
> > @@ -0,0 +1,39 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * allocator.h - Buffer allocator

buffer_allocator.h ? Or maybe even better framebuffer_allocator.h ? Same
for the .cpp file.

s/Buffer/FrameBuffer/

> > + */
> > +#ifndef __LIBCAMERA_ALLOCATOR_H__

And __LIBCAMERA_BUFFER_ALLOCATOR_H__ (or FRAMEBUFFER as appropriate) ?

> > +#define __LIBCAMERA_ALLOCATOR_H__
> > +
> > +#include <map>
> > +#include <memory>
> > +#include <vector>
> > +
> > +namespace libcamera {
> > +
> > +class Camera;
> > +class FrameBuffer;
> > +class Stream;
> > +struct StreamConfiguration;
> > +
> > +class BufferAllocator

FrameBufferAllocator ?

> > +{
> > +public:
> > +	BufferAllocator(std::shared_ptr<Camera> camera);
> > +	~BufferAllocator();
> > +
> > +	int allocate(Stream *stream, const StreamConfiguration &config);
> > +	void release(Stream *stream);
> > +
> > +	const std::vector<FrameBuffer *> &buffers(Stream *stream) const;
> > +
> > +private:
> > +	std::shared_ptr<Camera> camera_;
> > +	std::map<Stream *, std::vector<FrameBuffer *>> buffers_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_ALLOCATOR_H__ */
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 99abf06099407c1f..0d0ba2248fd8a679 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -1,4 +1,5 @@
> >  libcamera_api = files([
> > +    'allocator.h',
> >      'bound_method.h',
> >      'buffer.h',
> >      'camera.h',
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > index a3c692c347340382..8e653408fd54cf74 100644
> > --- a/include/libcamera/stream.h
> > +++ b/include/libcamera/stream.h
> > @@ -85,6 +85,7 @@ public:
> >  	MemoryType memoryType() const { return memoryType_; }
> >
> >  protected:
> > +	friend class BufferAllocator; /* To allocate and release buffers. */
> >  	friend class Camera;
> >
> >  	virtual int allocateBuffers(const StreamConfiguration &config,
> > diff --git a/src/libcamera/allocator.cpp b/src/libcamera/allocator.cpp
> > new file mode 100644
> > index 0000000000000000..8b517c809c05cbcd
> > --- /dev/null
> > +++ b/src/libcamera/allocator.cpp
> > @@ -0,0 +1,141 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * allocator.cpp - Buffer allocator

s/Buffer/FrameBuffer/ (and similar changes below where appropriate)

> > + */
> > +
> > +#include <libcamera/allocator.h>
> > +
> > +#include <errno.h>
> > +#include <string.h>
> > +#include <sys/mman.h>
> > +#include <unistd.h>

I think you only need errno.h.

> > +#include <libcamera/camera.h>
> > +#include <libcamera/stream.h>

You should also include <libcamera/buffer.h>.

> > +#include "log.h"
> > +
> > +/**
> > + * \file allocator.h
> > + * \brief Buffer allocator
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(Allocator)
> > +
> > +/**
> > + * \class BufferAllocator
> > + * \brief Buffer allocator for applications
> > + *
> > + * All buffers are to be treated as if they where allocated outside of the
> > + * camera. In some situations however the only source applications can allocate
> > + * buffers from is the camera. The BufferAllocator is the interface applications
> > + * shall use in these situations.
> > + *
> > + * The buffer allocator creates buffers using resources from a camera and/or
> > + * a stream. The buffers allocated this way are owned by the application and it
> > + * is responsible for their lifetime management. But just as buffers allocated
> > + * external from cameras and streams it's not valid to free a buffer while it's
> > + * queue to a camera.
> > + *
> > + * All buffers allocator are automatically freed when the allocator object is
> > + * deleted. It is the applications responsibility to make sure this do not
> > + * happen while one or more of the buffers are queued to a camera.
> > + *
> > + * If buffers are allocated outside the scope of libcamera by the application it
> > + * do not need to interact with the buffer allocator.

 * The libcamera API is designed to consume buffers provided by applications as
 * FrameBuffer instances. This makes libcamera a user of buffers exported by
 * other devices (such as displays or video encoders), or allocated from an
 * external allocator (such as ION on Android platforms). In some situations,
 * applications do not have any mean to allocate or get hold of suitable
 * buffers, for instance when no other device is involved, on Linux platforms
 * that lack a centralized allocator. The FrameBufferAllocator class provides a
 * buffer allocator that can be used in these situations.
 *
 * Applications create a framebuffer allocator for a Camera, and use it to
 * allocate buffers for streams of a CameraConfiguration with allocate(). They
 * control which streams to allocate buffers for, and can thus use external
 * buffers for a subset of the streams if desired.
 *
 * Buffers are deleted for a stream with release(), and destroying the allocator
 * automatically deletes all allocated buffers. Applications own the buffers
 * allocated by the FrameBufferAllocator and are responsible for ensuring the
 * buffers are not deleted while they are in use (part of a Request that has
 * been queued and hasn't completed yet).
 *
 * Usage of the FrameBufferAllocator is optional, if all buffers for a camera
 * are provided externally applications shall not use this class.

> > + */
> > +
> > +/**
> > + * \brief Create a BufferAllocator serving a camera
> > + * \param[in] camera Camera the allocator shall serve

s/Camera/The camera/

and maybe just

 * \param[in] camera The camera

> > + */
> > +
> > +BufferAllocator::BufferAllocator(std::shared_ptr<Camera> camera)
> > +	: camera_(camera)
> > +{
> > +}
> > +
> > +BufferAllocator::~BufferAllocator()
> > +{
> > +	for (auto &value : buffers_) {
> > +		Stream *stream = value.first;
> > +		std::vector<FrameBuffer *> &buffers = value.second;
> > +
> > +		for (FrameBuffer *buffer : buffers)
> > +			delete buffer;
> > +
> > +		buffers.clear();
> > +
> > +		stream->releaseBuffers();
> > +	}
> > +
> > +	buffers_.clear();
> > +}
> > +
> > +/**
> > + * \brief Allocate buffers from a stream

 * \brief Allocate buffers for a stream with a configuration

> > + * \param[in] stream The stream to allocate buffers from

s/from/for/

> > + * \param[in] config The configuration described the buffers to be allocated

The configuration doesn't describe the buffers to be allocated. Unless
someone has a better proposal, I would just go for

 * \param[in] config The stream configuration

> > + *
> > + * Allocate buffers matching exactly what is described in \a config. If buffers
> > + * matching \a config can't be allocated an error is returned and no buffers are
> > + * allocated.

How about

 * Allocate buffers suitable for capturing frames from the \a stream configured
 * as described by \a config. The configuration shall be valid for the stream as
 * checked CameraConfiguration::validate().
 *
 * Upon successful allocation, the allocated buffers can be retrieved with the
 * buffers() method.

I would drop the second sentence.

Doesn't the camera need to have been configured too ? I think the
documentation needs to be expanded to explain when allocating buffers is
allowed. I also wonder what happens when reconfiguring a camera for
which buffers have been allocated. Is this supported, and if so, in what
conditions ? If you explain how this works I can help expanding the
documentation.

> > + *
> > + * \return 0 on success or a negative error code otherwise

How about enumerating the error codes ? I think we should return -EINVAL
if the stream doesn't belong to the camera of the configuration isn't
valid, -ENOMEM if the buffers can't be allocated and -EBUSY if they are
already allocated. But this would require guaranteeing that
Stream::allocateBuffers() only return those codes. I think it's doable,
but should be documented.

> > + */
> > +int BufferAllocator::allocate(Stream *stream, const StreamConfiguration &config)
> > +{
> > +	auto iter = camera_->streams().find(stream);
> > +	if (iter != camera_->streams().end())
> > +		return stream->allocateBuffers(config, &buffers_[stream]);
> > +
> > +	LOG(Allocator, Error) << "Stream do not belong to " << camera_->name();

s/do/does/

> > +	return -EINVAL;

I think you should handle the errors as they are detected.

	auto iter = camera_->streams().find(stream);
	if (iter == camera_->streams().end()) {
		LOG(Allocator, Error)
			<< "Stream does not belong to " << camera_->name();
		return -EINVAL;
	}

	return stream->allocateBuffers(config, &buffers_[stream]);

Should you also protect against double allocation with

	if (buffers_.count(stream)) {
		LOG(Allocator, Error) << "Buffers already allocated for stream";
		return -EBUSY;
	}

> > +}
> > +
> > +/**
> > + * \brief Free allocated buffers

 * \brief Free buffers previously allocated for a \a stream

> > + * \param[in] stream The stream to free buffers for
> > + *
> > + * Free buffers allocated with allocate().

Add "This invalidates the buffers returned by buffers()."

> > + */
> > +void BufferAllocator::release(Stream *stream)
> > +{
> > +	auto iter = buffers_.find(stream);
> > +	if (iter == buffers_.end())
> > +		return;
> > +
> > +	std::vector<FrameBuffer *> &buffers = iter->second;
> > +
> > +	for (FrameBuffer *buffer : buffers)
> > +		delete buffer;
> > +
> > +	buffers.clear();
> > +
> > +	stream->releaseBuffers();
> > +
> > +	buffers_.erase(iter);
> > +}
> > +
> > +/**
> > + * \brief Retrive array of allocated buffers

 * \brief Retrieve the buffers allocated for a \a stream

> > + * \param[in] stream The stream to retrive buffers for

s/retrive/retrieve/

 *
 * This method shall only be called after successfully allocating buffers for
 * \a stream with allocate(). The returned buffers are valid until the earlier
 * of a call to release() for the same stream or the destruction of the
 * FrameBufferPool instance.
 *

> > + *
> > + * \return Array of buffers

 * \return The buffers allocated for the \a stream

> > + */
> > +const std::vector<FrameBuffer *> &BufferAllocator::buffers(Stream *stream) const
> > +{
> > +	static std::vector<FrameBuffer *> empty;
> > +
> > +	auto iter = buffers_.find(stream);
> > +	if (iter == buffers_.end())
> > +		return empty;
> > +
> > +	return iter->second;
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index c4f965bd7413b37e..b7041e003fdb1d49 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -1,4 +1,5 @@
> >  libcamera_sources = files([
> > +    'allocator.cpp',
> >      'bound_method.cpp',
> >      'buffer.cpp',
> >      'byte_stream_buffer.cpp',

-- 
Regards,

Laurent Pinchart