[libcamera-devel] [PATCH 07/12] android: camera_buffer: Implement PIMPL pattern

[Laurent Pinchart]

Hi Jacopo,

On Sun, Feb 28, 2021 at 08:36:23PM +0200, Laurent Pinchart wrote:
> On Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote:
> > In order to prepare to support more memory backends, make the
> > CameraBuffer class implement the PIMPL (pointer-to-implementation)
> > pattern.
> 
> Is this the right design pattern for the job ? The pimpl/d-pointer
> pattern is used to hide the implementation from the interface of the
> class, mostly to help maintaining ABI compatibility. In this specific
> case, what you need is likely just an overload, with a base class
> defining virtual methods.
> 
> Another option, if you want to simplify the implementation (I'm actually
> not entirely sure it would be simpler) is to define the interface in
> camera_buffer.h, and multiple implementations in different .cpp files,
> all named CameraBuffer. The .cpp file corresponding to the platform
> would then be selected at compilation time. The drawback is that you
> then would need to duplicate all functions in all implementations, even
> the ones that could be shared.

After reviewing the rest of the series I see that you can't expose the
backend-specific private members in CameraBuffer, which explains why
you're using this design pattern.

You could keep using this pattern, with the CameraBufferImpl only
storing the data, not duplicating every function. In that case I'd
inherit from libcamera::Extensible instead of creating a manual
implementation.

The other option is to use a base class and multiple derived classes as
mentioned above. We've discussed this previously and I agreed that we
could just select which file to compile as a simple implementation, but
I didn't realize then that we would need private data. I'm wondering
whether the implementation wouldn't be simpler with inheritance than
with private data. Sorry for not thinking about it in the beginning.

> > Define the CameraBuffer class interface whose actual implementation is
> > delegated to an inner CameraBufferImpl class.
> > 
> > Temporary maintain libcamera::MappedBuffer as the CameraBuffer base
> > class to maintain compatibility of the CameraStream::process() interface
> > that requires a MappedBuffer * as second argument and will be converted
> > to use a CameraBuffer in the next patch.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_buffer.h               | 12 ++++
> >  src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++-
> >  2 files changed, 91 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> > index 0590cd84652b..2a91e6a3c9c1 100644
> > --- a/src/android/camera_buffer.h
> > +++ b/src/android/camera_buffer.h
> > @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer
> >  public:
> >  	CameraBuffer(buffer_handle_t camera3Buffer, int flags);
> >  	~CameraBuffer();
> > +
> > +	bool isValid() const;
> > +
> > +	unsigned int numPlanes() const;
> > +	ssize_t planeSize(unsigned int plane) const;
> > +
> > +	const uint8_t *plane(unsigned int plane) const;
> > +	uint8_t *plane(unsigned int plane);
> > +
> > +private:
> > +	class CameraBufferImpl;
> > +	CameraBufferImpl *impl_;
> >  };
> >  
> >  #endif /* __ANDROID_CAMERA_BUFFER_H__ */
> > diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp
> > index 807304a9e42d..10a43a61bd4d 100644
> > --- a/src/android/mm/android_generic_buffer.cpp
> > +++ b/src/android/mm/android_generic_buffer.cpp
> > @@ -13,7 +13,21 @@ using namespace libcamera;
> >  
> >  LOG_DECLARE_CATEGORY(HAL)
> >  
> > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)
> > +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer
> > +{
> > +public:
> > +	CameraBufferImpl(buffer_handle_t camera3Buffer, int flags);
> > +	~CameraBufferImpl();
> > +
> > +	unsigned int numPlanes() const;
> > +	ssize_t planeSize(unsigned int plane) const;
> > +
> > +	const uint8_t *plane(unsigned int plane) const;
> > +	uint8_t *plane(unsigned int plane);
> > +};
> > +
> > +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer,
> > +						 int flags)
> >  {
> >  	maps_.reserve(camera3Buffer->numFds);
> >  	error_ = 0;
> > @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)
> >  	}
> >  }
> >  
> > +CameraBuffer::CameraBufferImpl::~CameraBufferImpl()
> > +{
> > +}
> > +
> > +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const
> > +{
> > +	return maps_.size();
> > +}
> > +
> > +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const
> > +{
> > +	if (plane >= maps_.size())
> > +		return -EINVAL;
> > +
> > +	return maps_[plane].size();
> > +}
> > +
> > +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const
> > +{
> > +	if (plane >= maps_.size())
> > +		return nullptr;
> > +
> > +	return maps_[plane].data();
> > +}
> > +
> > +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane)
> > +{
> > +	if (plane >= maps_.size())
> > +		return nullptr;
> > +
> > +	return maps_[plane].data();
> > +}
> > +
> > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)
> > +	: impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags))
> > +{
> > +}
> > +
> >  CameraBuffer::~CameraBuffer()
> >  {
> > +	delete impl_;
> > +}
> > +
> > +bool CameraBuffer::isValid() const
> > +{
> > +	return impl_->isValid();
> > +}
> > +
> > +unsigned int CameraBuffer::numPlanes() const
> > +{
> > +	return impl_->numPlanes();
> > +}
> > +
> > +ssize_t CameraBuffer::planeSize(unsigned int plane) const
> > +{
> > +	return impl_->planeSize(plane);
> > +}
> > +
> > +const uint8_t *CameraBuffer::plane(unsigned int plane) const
> > +{
> > +	return impl_->plane(plane);
> > +}
> > +
> > +uint8_t *CameraBuffer::plane(unsigned int plane)
> > +{
> > +	return impl_->plane(plane);
> >  }

-- 
Regards,

Laurent Pinchart