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

14433 diff mbox series

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@chromium.org>
Reviewed-by: Jacopo Mondi <jacopo@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(-)

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@chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo@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@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
>

Hi Kieran

On Mon, Nov 08, 2021 at 11:19:51PM +0000, Kieran Bingham wrote:
> 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 had the same question.. but then I realized that
FrameBuffer::Private is not accessible to applications so they cannot
subclass it. Also, as Hiro pointed out, the same pattern is in use in
the Camera class.

>
> 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@chromium.org>
> > Reviewed-by: Jacopo Mondi <jacopo@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@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
> >

Hello,

On Tue, Nov 09, 2021 at 10:02:01AM +0100, Jacopo Mondi wrote:
> On Mon, Nov 08, 2021 at 11:19:51PM +0000, Kieran Bingham wrote:
> > 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 had the same question.. but then I realized that
> FrameBuffer::Private is not accessible to applications so they cannot
> subclass it. Also, as Hiro pointed out, the same pattern is in use in
> the Camera class.

That's right. The Extensible class pattern was designed to match derived
classes on the Extensible and Private side, but it turned out to be
useful to only derive on the Private side (this was enabled by commit
33dd4fab9d39 ("libcamera: base: class: Don't pass Extensible pointer to
Private constructor")).

To allow user-defined FrameBufferAllocator implementations, we'll need
more than this.

> > 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@chromium.org>
> > > Reviewed-by: Jacopo Mondi <jacopo@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.

The alternative is to not make the FrameBuffer class final, which would
be more intrusive. We'll probably get there later, but I think this is
fine for now, even if it would be nice not to expose a constructor that
applications can't use.

> > 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@ideasonboard.com>
> >
> > > +                   const std::vector<Plane> &planes, unsigned int cookie = 0);
> > > +
> > >

Extra blank line.

> > >         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()

This line isn't needed, it's implicit when the documentation block is
right above the function it documents.

> > > + * \brief FrameBuffer::Private destructor
> > > + *

Extra blank line.

> > > + */
> > > +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());
> > >