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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Jan 10, 2020 at 08:38:00PM +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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> * Changes since v2
> - Replace FrameBufferAllocator::streams() with FrameBufferAllocator::allocated()
> - Add todo to see if we can find a better API for Camera::relase() with a active FrameBufferAllocator
> - Touch up documentation
> - Make sure stream to be allocated is part of the cameras active configuration not just the camera
> 
> * 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 |  45 +++++
>  include/libcamera/meson.build             |   1 +
>  src/libcamera/camera.cpp                  |  19 +-
>  src/libcamera/framebuffer_allocator.cpp   | 219 ++++++++++++++++++++++
>  src/libcamera/meson.build                 |   1 +
>  6 files changed, 289 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..2fd5870b15c69087 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 to read state_ and activeStreams_. */
> +	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..42812253ada94d94
> --- /dev/null
> +++ b/include/libcamera/framebuffer_allocator.h
> @@ -0,0 +1,45 @@
> +/* 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 <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 free(Stream *stream);
> +
> +	bool allocated() const { return !buffers_.empty(); }
> +	const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const;
> +
> +private:
> +	FrameBufferAllocator(std::shared_ptr<Camera> camera);
> +
> +	std::shared_ptr<Camera> camera_;
> +	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..91f5ef7771782eba 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,16 @@ int Camera::release()
>  	if (!stateBetween(CameraAvailable, CameraConfigured))
>  		return -EBUSY;
>  
> +	if (allocator_) {
> +		/*
> +		 * \todo Try to find a better API that would make this error
> +		 * impossible.
> +		 */
> +		LOG(Camera, Error)
> +			<< "Buffers must be freed before the camera can be reconfigured";
> +		return -EBUSY;
> +	}
> +
>  	pipe_->unlock();
>  
>  	state_ = CameraAvailable;
> @@ -649,6 +660,12 @@ int Camera::configure(CameraConfiguration *config)
>  	if (!stateBetween(CameraAcquired, CameraConfigured))
>  		return -EACCES;
>  
> +	if (allocator_ && allocator_->allocated()) {
> +		LOG(Camera, Error)
> +			<< "Allocator must be deleted before 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..b0246b6b530b6997
> --- /dev/null
> +++ b/src/libcamera/framebuffer_allocator.cpp
> @@ -0,0 +1,219 @@
> +/* 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 free(), 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
> + * \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 has an allocator";
> +		return nullptr;
> +	}
> +
> +	camera->allocator_ = new FrameBufferAllocator(camera);
> +	return camera->allocator_;
> +}
> +
> +/**
> + * \brief Construct a FrameBufferAllocator serving a camera
> + * \param[in] camera The camera
> + */
> +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;
> +}
> +
> +/**
> + * \brief Allocate buffers for a 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 \a stream does not belong to the camera or the stream is
> + * not part of the active camera configuration
> + * \retval -EBUSY Buffers are already allocated for the \a stream
> + */
> +int FrameBufferAllocator::allocate(Stream *stream)
> +{
> +	if (camera_->state_ != Camera::CameraConfigured &&
> +	    camera_->state_ != Camera::CameraPrepared) {
> +		LOG(Allocator, Error)
> +			<< "Camera must be in the configured state to allocate buffers";
> +		return -EACCES;
> +	}
> +
> +	auto iterCamera = camera_->streams().find(stream);
> +	if (iterCamera == camera_->streams().end()) {

	if (camera_->streams().find(stream) == camera_->streams().end()) {

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

	if (camera_->activeStreams_.find(stream) == camera_->activeStreams_.end()) {

> +		LOG(Allocator, Error)
> +			<< "Stream is not part of " << camera_->name()
> +			<< " active configuration";
> +		return -EINVAL;
> +	}
> +
> +	if (buffers_.count(stream)) {
> +		LOG(Allocator, Error) << "Buffers already allocated for stream";
> +		return -EBUSY;
> +	}
> +
> +	int ret = camera_->pipe_->exportFrameBuffers(camera_.get(), stream,
> +						     &buffers_[stream]);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Free buffers previously allocated for a \a stream
> + * \param[in] stream The stream
> + *
> + * 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::free(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);
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn FrameBufferAllocator::allocated()
> + * \brief Retrieve the streams the allocator handles

You can drop this first brief, it's a leftover.

> + * \brief Check if the allocator have allocated buffers for any stream

s/have/has/

> + * \return True if the allocator have allocated buffers for one or more

s/have/has/

and keep my Reviewed-by.

> + * streams, false otherwise
> + */
> +
> +/**
> + * \brief Retrieve the buffers allocated for a \a stream
> + * \param[in] stream The stream to retrive buffers for
> + *
> + * This method shall only be called after successfully allocating buffers for
> + * \a stream with allocate(). The returned buffers are valid until free() is
> + * called for the same stream or the FrameBufferAllocator instance is destroyed.
> + *
> + * \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;
> +}
> +
> +} /* 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