[libcamera-devel] [PATCH 1/2] libcamera: framebuffer: Enable attaching additional data to FrameBuffer

[Jacopo Mondi]

Hi Hiro,

On Thu, Oct 28, 2021 at 04:30:37PM +0900, Hirokazu Honda wrote:
> We cannot have a subclass of FrameBuffer because it is marked as final.
> This adds a FrameBuffer constructor with FrameBuffer::Private. So we
> can attach some additional resources with FrameBuffer through a
> customized FrameBuffer::Private class.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/framebuffer.h          |  2 ++
>  include/libcamera/internal/framebuffer.h |  1 +
>  src/libcamera/framebuffer.cpp            | 17 +++++++++++++++--
>  3 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> index 7f2f176a..02bf1533 100644
> --- a/include/libcamera/framebuffer.h
> +++ b/include/libcamera/framebuffer.h
> @@ -57,6 +57,8 @@ public:
>  		unsigned int length;
>  	};
>
> +	FrameBuffer(std::unique_ptr<Private> d,
> +		    const std::vector<Plane> &planes, unsigned int cookie = 0);

I wonder if it's right to make this available from a public
header. We have this pattern in use for Camera, which cannot however be
constructed from the libcamera public API, while FrameBuffer can.

Applications cannot extend FrameBuffer::Private though, so only
components with access to the internal headers can, so I guess this is
fine!


>  	FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);

Could you align the declaration order in the .cpp and .h files ?

Alternatively, we can decide FrameBuffer can potentially store some
PrivateData in its FrameBuffer::Private in the case we ever get to
provide an allocator based on dmabuf heaps. But that's maybe for
later.

>
>  	const std::vector<Plane> &planes() const { return planes_; }
> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> index cd33c295..5ab9b3b2 100644
> --- a/include/libcamera/internal/framebuffer.h
> +++ b/include/libcamera/internal/framebuffer.h
> @@ -19,6 +19,7 @@ class FrameBuffer::Private : public Extensible::Private
>
>  public:
>  	Private();
> +	virtual ~Private() = default;

Recently I think we're pushing to move the '= default' declarations to
the .cpp files to avoid inlining then in the headers.

Overall I think that's good!

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

>
>  	void setRequest(Request *request) { request_ = request; }
>  	bool isContiguous() const { return isContiguous_; }
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index d44a98ba..26587ff3 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -213,8 +213,21 @@ FrameBuffer::Private::Private()
>   * \param[in] cookie Cookie
>   */
>  FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> -	: Extensible(std::make_unique<Private>()), planes_(planes),
> -	  cookie_(cookie)
> +	: FrameBuffer(std::make_unique<Private>(), planes, cookie)
> +{
> +}
> +
> +/**
> + * \brief Construct a FrameBuffer with an extensible private class and an array
> + * of planes
> + * \param[in] d The extensible private class
> + * \param[in] planes The frame memory planes
> + * \param[in] cookie Cookie
> + */
> +FrameBuffer::FrameBuffer(std::unique_ptr<Private> d,
> +			 const std::vector<Plane> &planes,
> +			 unsigned int cookie)
> +	: Extensible(std::move(d)), planes_(planes), cookie_(cookie)
>  {
>  	metadata_.planes_.resize(planes_.size());
>
> --
> 2.33.1.1089.g2158813163f-goog
>