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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Mar 01, 2021 at 10:02:08AM +0100, Jacopo Mondi wrote:
> On Sun, Feb 28, 2021 at 09:10:21PM +0200, Laurent Pinchart wrote:
> > 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,

Agreed.

> so using a pointer to the actual implementation seems the less risky
> way.

That I'm not sure about :-) You can store any amount of custom data in a
derived class, so a base abstract class for the interface plus derived
classes that implement that interface could work fine too.

An implementation based on Extensible could be fine too, please just
don't rule the base + derived option out yet in your investigations.

> I'll see how Extensible looks like and send a v2.
> 
> > > > 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