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

2572 diff mbox series

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@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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

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@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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',