[libcamera-devel,RFC,04/17] libcamera: frame_buffer: Document the FrameBuffer::Private class

13090 diff mbox series

The FrameBuffer::Private class is exposed to pipeline handlers, and is
thus part of the internal libcamera API. As such, it should be
documented.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/framebuffer.cpp | 10 ++++++++++
 1 file changed, 10 insertions(+)

Hi Laurent,

Thanks for your patch.

On 2021-07-23 07:00:23 +0300, Laurent Pinchart wrote:
> The FrameBuffer::Private class is exposed to pipeline handlers, and is
> thus part of the internal libcamera API. As such, it should be
> documented.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/libcamera/framebuffer.cpp | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index 48d14b31f68d..a59e93fbdf91 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -100,6 +100,16 @@ LOG_DEFINE_CATEGORY(Buffer)
>   * \brief Array of per-plane metadata
>   */
>  
> +/**
> + * \class FrameBuffer::Private
> + * \brief Base class for FrameBuffer private data
> + *
> + * The FrameBuffer::Private class stores all private data associated with a
> + * framebuffer. It implements the d-pointer design pattern to hide core
> + * FrameBuffer data from the public API, and exposes utility functions to
> + * pipeline handlers.
> + */
> +
>  FrameBuffer::Private::Private()
>  	: request_(nullptr)
>  {
> -- 
> Regards,
> 
> Laurent Pinchart
>

Hi Laurent

On Fri, Jul 23, 2021 at 07:00:23AM +0300, Laurent Pinchart wrote:
> The FrameBuffer::Private class is exposed to pipeline handlers, and is
> thus part of the internal libcamera API. As such, it should be
> documented.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/framebuffer.cpp | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index 48d14b31f68d..a59e93fbdf91 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -100,6 +100,16 @@ LOG_DEFINE_CATEGORY(Buffer)
>   * \brief Array of per-plane metadata
>   */
>
> +/**
> + * \class FrameBuffer::Private
> + * \brief Base class for FrameBuffer private data
> + *
> + * The FrameBuffer::Private class stores all private data associated with a
> + * framebuffer. It implements the d-pointer design pattern to hide core
> + * FrameBuffer data from the public API, and exposes utility functions to
> + * pipeline handlers.

A single function, actually, but they can grow.

Anyway, that's what's interesting and a bit scary about this pattern.
It is meant to allow changes to the internal API not to be reflected
on the public one, but it also create a 'private' interface available
to selected components, in our case the internal ones that have access
to the libcamera/internal headers.

Anyway...

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

thanks
   j

> + */
> +
>  FrameBuffer::Private::Private()
>  	: request_(nullptr)
>  {
> --
> Regards,
>
> Laurent Pinchart
>

Hi Jacopo,

On Thu, Jul 29, 2021 at 10:33:57PM +0200, Jacopo Mondi wrote:
> On Fri, Jul 23, 2021 at 07:00:23AM +0300, Laurent Pinchart wrote:
> > The FrameBuffer::Private class is exposed to pipeline handlers, and is
> > thus part of the internal libcamera API. As such, it should be
> > documented.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/framebuffer.cpp | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > index 48d14b31f68d..a59e93fbdf91 100644
> > --- a/src/libcamera/framebuffer.cpp
> > +++ b/src/libcamera/framebuffer.cpp
> > @@ -100,6 +100,16 @@ LOG_DEFINE_CATEGORY(Buffer)
> >   * \brief Array of per-plane metadata
> >   */
> >
> > +/**
> > + * \class FrameBuffer::Private
> > + * \brief Base class for FrameBuffer private data
> > + *
> > + * The FrameBuffer::Private class stores all private data associated with a
> > + * framebuffer. It implements the d-pointer design pattern to hide core
> > + * FrameBuffer data from the public API, and exposes utility functions to
> > + * pipeline handlers.
> 
> A single function, actually, but they can grow.
> 
> Anyway, that's what's interesting and a bit scary about this pattern.
> It is meant to allow changes to the internal API not to be reflected
> on the public one, but it also create a 'private' interface available
> to selected components, in our case the internal ones that have access
> to the libcamera/internal headers.

Absolutely, that's what I really like about it :-) It helps with ABI
compatibility, but also with removing functions from the public API.

> Anyway...
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > + */
> > +
> >  FrameBuffer::Private::Private()
> >  	: request_(nullptr)
> >  {