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

[Kieran Bingham]

Quoting Hirokazu Honda (2021-11-01 07:16:51)
> 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.
> 

Wow, this is interesting. Extensible was, as I understood it used to
protect libcamera internals from being accessed by applications (here
Android is an application of course).

But this patch, lets applications create their own FrameBuffers
- where the private data is not accessible to libcamera, so it's the
complete inverse!

I wonder if that was the intention of the 'cookie' though, as a user
settable private data ... But maybe that's still needed to be separate
from this so that the allocator can track private data, while the usage
of the buffer is stored in the cookie?



> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/framebuffer.h          |  3 +++
>  include/libcamera/internal/framebuffer.h |  1 +
>  src/libcamera/framebuffer.cpp            | 26 ++++++++++++++++++++++--
>  3 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> index 7f2f176a..98ef9d5d 100644
> --- a/include/libcamera/framebuffer.h
> +++ b/include/libcamera/framebuffer.h
> @@ -58,6 +58,9 @@ public:
>         };
>  
>         FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);
> +       FrameBuffer(std::unique_ptr<Private> d,

This bit feels a bit wierd, so I'm suspecting Laurent will have an
opinion here.

But overall, I don't see why applications (or rather, external
FrameBufferAllocators) can't do this - it won't affect internal
libcamera usage... so I'm going to throw this on here.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +                   const std::vector<Plane> &planes, unsigned int cookie = 0);
> +
>  
>         const std::vector<Plane> &planes() const { return planes_; }
>         Request *request() const;
> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> index cd33c295..3cced5b1 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();
>  
>         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..34e6cd4d 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -116,6 +116,15 @@ FrameBuffer::Private::Private()
>  {
>  }
>  
> +/**
> + * \fn FrameBuffer::Private::~Private()
> + * \brief FrameBuffer::Private destructor
> + *
> + */
> +FrameBuffer::Private::~Private()
> +{
> +}
> +
>  /**
>   * \fn FrameBuffer::Private::setRequest()
>   * \brief Set the request this buffer belongs to
> @@ -213,8 +222,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
>