[libcamera-devel] [PATCH v2 19/25] libcamera: allocator: Add FrameBufferAllocator to help applications allocate buffers

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Dec 30, 2019 at 01:05:04PM +0100, Niklas Söderlund wrote:
> The FrameBuffer interface is based on the idea that all buffers are
> allocated externally to libcamera and are only used by it. This is meant
> to create a simpler API centered around usage of buffers, regardless of
> where they come from.
> 
> 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.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since v1
> - Update commit message
> - Rename class to FrameBufferAllocator
> - Rename source files framebuffer_allocator.{cpp,h}
> - Update include headers
> - Update documentation
> - Check for double allocation for the same stream in allocate()
> - Add interactions with Camera::allocator_ and enforce buffers may only
>   be allocated when the camera is in the configured state.
> ---
>  include/libcamera/camera.h                |   5 +
>  include/libcamera/framebuffer_allocator.h |  47 +++++
>  include/libcamera/meson.build             |   1 +
>  src/libcamera/camera.cpp                  |  15 +-
>  src/libcamera/framebuffer_allocator.cpp   | 213 ++++++++++++++++++++++
>  src/libcamera/meson.build                 |   1 +
>  6 files changed, 281 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/framebuffer_allocator.h
>  create mode 100644 src/libcamera/framebuffer_allocator.cpp
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index ef6a37bb142c83a6..f2827438871189a1 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -20,6 +20,7 @@
>  namespace libcamera {
>  
>  class Buffer;
> +class FrameBufferAllocator;
>  class PipelineHandler;
>  class Request;
>  
> @@ -126,6 +127,10 @@ private:
>  
>  	bool disconnected_;
>  	State state_;
> +
> +	/* Needed to update allocator_ and read state_. */
> +	friend class FrameBufferAllocator;
> +	FrameBufferAllocator *allocator_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
> new file mode 100644
> index 0000000000000000..75952762884e44ac
> --- /dev/null
> +++ b/include/libcamera/framebuffer_allocator.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * framebuffer_allocator.h - FrameBuffer allocator
> + */
> +#ifndef __LIBCAMERA_FRAMEBUFFER_ALLOCATOR_H__
> +#define __LIBCAMERA_FRAMEBUFFER_ALLOCATOR_H__
> +
> +#include <map>
> +#include <memory>
> +#include <unordered_set>
> +#include <vector>
> +
> +namespace libcamera {
> +
> +class Camera;
> +class FrameBuffer;
> +class Stream;
> +
> +class FrameBufferAllocator
> +{
> +public:
> +	static FrameBufferAllocator *create(std::shared_ptr<Camera> camera);
> +
> +	FrameBufferAllocator(const Camera &) = delete;
> +	FrameBufferAllocator &operator=(const Camera &) = delete;
> +
> +	~FrameBufferAllocator();
> +
> +	int allocate(Stream *stream);
> +	int release(Stream *stream);

s/release/free/ ?

> +
> +	const std::unordered_set<Stream *> &streams() const { return streams_; }

This method seems to only be used by Camera::configure() to check if any
buffer has been allocated. How about replacing it with a

	bool allocated() const { return !buffers_.empty(); }

and dropping the streams_ data member ?

> +	const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const;
> +
> +private:
> +	FrameBufferAllocator(std::shared_ptr<Camera> camera);
> +
> +	std::shared_ptr<Camera> camera_;
> +	std::unordered_set<Stream *> streams_;
> +	std::map<Stream *, std::vector<std::unique_ptr<FrameBuffer>>> buffers_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_FRAMEBUFFER_ALLOCATOR_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 543e6773cc5158a0..8db217bb782c1443 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -7,6 +7,7 @@ libcamera_api = files([
>      'event_dispatcher.h',
>      'event_notifier.h',
>      'file_descriptor.h',
> +    'framebuffer_allocator.h',
>      'geometry.h',
>      'logging.h',
>      'object.h',
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index e810fb725d81350d..222c6811d5698f0f 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -9,6 +9,7 @@
>  
>  #include <iomanip>
>  
> +#include <libcamera/framebuffer_allocator.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -405,7 +406,7 @@ const std::string &Camera::name() const
>  
>  Camera::Camera(PipelineHandler *pipe, const std::string &name)
>  	: pipe_(pipe->shared_from_this()), name_(name), disconnected_(false),
> -	  state_(CameraAvailable)
> +	  state_(CameraAvailable), allocator_(nullptr)
>  {
>  }
>  
> @@ -541,6 +542,12 @@ int Camera::release()
>  	if (!stateBetween(CameraAvailable, CameraConfigured))
>  		return -EBUSY;
>  
> +	if (allocator_) {
> +		LOG(Camera, Error)
> +			<< "Allocator must be deleted before camera can be released";

Could you add a

	/*
	 * \todo Try to find a better API that would make this error
	 * impossible
	 */

> +		return -EBUSY;
> +	}
> +
>  	pipe_->unlock();
>  
>  	state_ = CameraAvailable;
> @@ -649,6 +656,12 @@ int Camera::configure(CameraConfiguration *config)
>  	if (!stateBetween(CameraAcquired, CameraConfigured))
>  		return -EACCES;
>  
> +	if (allocator_ && allocator_->streams().size()) {
> +		LOG(Camera, Error)
> +			<< "Allocator must be deleted before camera can be reconfigured";

"Buffers must be freed before the camera can be reconfigured" ?

> +		return -EBUSY;
> +	}
> +
>  	if (config->validate() != CameraConfiguration::Valid) {
>  		LOG(Camera, Error)
>  			<< "Can't configure camera with invalid configuration";
> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> new file mode 100644
> index 0000000000000000..98b649ecf6053790
> --- /dev/null
> +++ b/src/libcamera/framebuffer_allocator.cpp
> @@ -0,0 +1,213 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * framebuffer_allocator.cpp - FrameBuffer allocator
> + */
> +
> +#include <libcamera/framebuffer_allocator.h>
> +
> +#include <errno.h>
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/camera.h>
> +#include <libcamera/stream.h>
> +
> +#include "log.h"
> +#include "pipeline_handler.h"
> +
> +/**
> + * \file framebuffer_allocator.h
> + * \brief FrameBuffer allocator
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Allocator)
> +
> +/**
> + * \class FrameBufferAllocator
> + * \brief FrameBuffer allocator for applications
> + *
> + * 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 framebuffer allocator instance

s/framebuffer/FrameBuffer/

s/ instance// ?

> + * \param[in] camera The camera the allocator serves
> + *

 * A single allocator may be created for a Camera instance.

> + * The caller is responsible for deleting the allocator before the camera is
> + * released.
> + *
> + * \return A pointer to the newly created allocator object or nullptr on error
> + */
> +FrameBufferAllocator *
> +FrameBufferAllocator::create(std::shared_ptr<Camera> camera)
> +{
> +	if (camera->allocator_) {
> +		LOG(Allocator, Error) << "Camera already have an allocator";

s/have/has/

> +		return nullptr;
> +	}
> +
> +	FrameBufferAllocator *allocator = new FrameBufferAllocator(camera);
> +
> +	camera->allocator_ = allocator;
> +
> +	return allocator;

	camera->allocator_ = new FrameBufferAllocator(camera);
	return camera->allocator_;

?

> +}
> +
> +/**
> + * \brief Allocate buffers for a stream with a configuration

s/stream with a configuration/configured stream/

> + * \param[in] stream The stream to allocate buffers for
> + *
> + * Allocate buffers suitable for capturing frames from the \a stream.

 * The Camera shall have been previously configured with Camera::configure() and
 * shall be stopped, and the stream shall be part of the active camera
 * configuration.

> Upon
> + * successful allocation, the allocated buffers can be retrieved with the
> + * buffers() method.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EACCES The camera is not in a state where buffers can be allocated
> + * \retval -EINVAL The allocator do not handle the \a stream

"The \a stream does not belong to the camera"

"... or the stream is not part of the active camera configuration"

and this error should be checked.

> + * \retval -EBUSY Buffers are already allocated for the \a stream
> + */
> +int FrameBufferAllocator::allocate(Stream *stream)
> +{
> +	if (camera_->state_ != Camera::CameraConfigured && camera_->state_ != Camera::CameraPrepared) {

Line wrap ?

> +		LOG(Allocator, Error)
> +			<< "Camera must be in the configured state to allocate buffers";
> +		return -EACCES;
> +	}
> +
> +	auto iter = camera_->streams().find(stream);
> +	if (iter == camera_->streams().end()) {
> +		LOG(Allocator, Error)
> +			<< "Stream does not belong to " << camera_->name();
> +		return -EINVAL;
> +	}
> +
> +	if (buffers_.count(stream)) {
> +		LOG(Allocator, Error) << "Buffers already allocated for stream";
> +		return -EBUSY;
> +	}
> +
> +	unsigned int bufferCount = stream->configuration().bufferCount;
> +	int ret = camera_->pipe_->allocateFrameBuffers(camera_.get(), stream,
> +						       bufferCount,
> +						       &buffers_[stream]);
> +	if (ret)
> +		return ret;
> +
> +	streams_.insert(stream);
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Free buffers previously allocated for a \a stream
> + * \param[in] stream The stream to free buffers for
> + *
> + * Free buffers allocated with allocate().
> + *
> + * This invalidates the buffers returned by buffers().
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EACCES The camera is not in a state where buffers can be freed
> + * \retval -EINVAL The allocator do not handle the \a stream
> + */
> +int FrameBufferAllocator::release(Stream *stream)
> +{
> +	if (camera_->state_ != Camera::CameraConfigured && camera_->state_ != Camera::CameraPrepared) {
> +		LOG(Allocator, Error)
> +			<< "Camera must be in the configured state to free buffers";
> +		return -EACCES;
> +	}
> +
> +	auto iter = buffers_.find(stream);
> +	if (iter == buffers_.end())
> +		return -EINVAL;
> +
> +	std::vector<std::unique_ptr<FrameBuffer>> &buffers = iter->second;
> +
> +	buffers.clear();
> +
> +	camera_->pipe_->freeFrameBuffers(camera_.get(), stream);
> +
> +	buffers_.erase(iter);
> +
> +	streams_.erase(stream);
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn FrameBufferAllocator::streams()
> + * \brief Retrieve the streams the allocator handles
> + * \return The streams the allocator handles
> + */

I won't comment on this as I think the method should be dropped :-)

> +
> +/**
> + * \brief Retrieve the buffers allocated for a \a stream
> + * \param[in] stream The stream to retrive buffers for

s/retrive/retrieve/

or just "The stream"

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

"... or the FrameBufferAllocator instance is destroyed."

Or maybe your proposal is fine, according to
https://en.wikipedia.org/wiki/Coordination_(linguistics)#Mismatch_in_syntactic_category
? Kieran, do you feel like a linguist today ? :-)

> + *
> + * \return The buffers allocated for the \a stream
> + */
> +const std::vector<std::unique_ptr<FrameBuffer>>
> +&FrameBufferAllocator::buffers(Stream *stream) const
> +{
> +	static std::vector<std::unique_ptr<FrameBuffer>> empty;
> +
> +	auto iter = buffers_.find(stream);
> +	if (iter == buffers_.end())
> +		return empty;
> +
> +	return iter->second;
> +}
> +
> +/**
> + * \brief Create a FrameBufferAllocator serving a camera

s/Create/Construct/

> + * \param[in] camera The camera
> + */
> +

No blank line.

> +FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera)
> +	: camera_(camera)
> +{
> +}
> +
> +FrameBufferAllocator::~FrameBufferAllocator()
> +{
> +	for (auto &value : buffers_) {
> +		Stream *stream = value.first;
> +		camera_->pipe_->freeFrameBuffers(camera_.get(), stream);
> +	}
> +
> +	buffers_.clear();
> +
> +	camera_->allocator_ = nullptr;

This should be before allocate() to match the order in the class
definition. I recommend moving both the constructor and the destructor
just after ::create()

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 722c5bc15afe52ef..68d89559b290befd 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -16,6 +16,7 @@ libcamera_sources = files([
>      'event_notifier.cpp',
>      'file_descriptor.cpp',
>      'formats.cpp',
> +    'framebuffer_allocator.cpp',
>      'geometry.cpp',
>      'ipa_context_wrapper.cpp',
>      'ipa_controls.cpp',

-- 
Regards,

Laurent Pinchart