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

[Jacopo Mondi]

Hi Laurent,

On Sun, Feb 28, 2021 at 09:10:21PM +0200, Laurent Pinchart wrote:
> 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.

That's exactly the reason why I went in that directin

>
> 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.

I didn't realize that in the beginning as well.

For the cros backend we could get away by getting an instance of the
cros::CameraBufferManager through the static singleton constructor in
every function, by changing the interface and adding the current
buffer as parameter, but that would make the whole hierachy stateless,
something I am not sure we can guarantee for all the backends we'll
have to implement. I would so refrain from tying our hands, so using a
pointer to the actual implementation seems the less risky way.

I'll see how Extensible looks like and send a v2.

Thanks for review
   j
>
> > > 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