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

[Jacopo Mondi]

Hi Laurent,

On Mon, Mar 01, 2021 at 11:24:22AM +0200, Laurent Pinchart wrote:
> 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.

Yes, but then what instantiating the right derived class using a
compile-time option would require a factory or a chain of #ifdef

However, if we compile in a single dervide class at the time, we can
instantiate a CameraBuffer unconditionally ? I'll try this one as
well, it might work and be simpler than using Extensible.

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