[libcamera-devel] [PATCH 1/4] libcamera: Add Buffer and BufferPool classes

[Laurent Pinchart]

Hi Kieran,

On Thu, Jan 31, 2019 at 07:00:31PM +0000, Kieran Bingham wrote:
> On 31/01/2019 18:34, Jacopo Mondi wrote:
> > On Tue, Jan 29, 2019 at 01:53:54PM +0000, Kieran Bingham wrote:
> >> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>
> >> Provide classes that represent frame buffers and pools of frame buffers.
> >>
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>
> >> ---
> >> Kieran
> >>  - Fix checkstyle.py diff
> >> ---
> >>  include/libcamera/buffer.h    |  55 ++++++++++++++++++
> >>  include/libcamera/libcamera.h |   1 +
> >>  include/libcamera/meson.build |   1 +
> >>  src/libcamera/buffer.cpp      | 102 ++++++++++++++++++++++++++++++++++
> >>  src/libcamera/meson.build     |   1 +
> >>  5 files changed, 160 insertions(+)
> >>  create mode 100644 include/libcamera/buffer.h
> >>  create mode 100644 src/libcamera/buffer.cpp

[snip]

> >> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> >> new file mode 100644
> >> index 000000000000..6dfebfc6bb28
> >> --- /dev/null
> >> +++ b/src/libcamera/buffer.cpp

[snip]

> >> +int BufferPool::allocate(unsigned int count)
> >> +{
> >> +	for (unsigned int i = 0; i < count; ++i) {
> >> +		Buffer *buffer = new Buffer();
> >> +		buffers_.push_back(buffer);
> >> +	}
> >> +
> >> +	if (memory_ == Memory::Internal) {
> > 
> > The Internal vs External distinction matches the "exporter" vs
> > "importer" one for the DMABUF use case, or has other purposes?
> > Shouldn't we use then use Importer vs Exporter not to add another
> > term?
> 
> Playing this through - I'm becoming less convinced that we need a
> Memory::Internal.
> 
> I'd like to be able restrict the creation of pools to the 'Device' which
> will own them. And at that point, it can store a pointer of that device.
> 
> Then - if Pool.device == Device Buffers are internal.
> Else Buffers are external.
> 
> 
> I haven't yet seen what benefit setting the memory_ provides - but I'm
> having issues creating the BufferPools anyway. It's a WIP. (see below)

Looking at this from an application point of view, the application needs
a pool of buffers per stream. The pool shall be provided by libcamera,
not by the application, but the memory backing the buffers could be
provided by either libcamera (internal allocation) or the application
(external allocation). We thus need a way for an application to request
a pool with a given number of buffers, and to specify whether buffers
should be allocated by libcamera (in which case the pool will be
associated with a V4L2 device) or will be provided externally (in which
case the pool doesn't need to be associated with a V4L2 device).

You will obviously need, in both cases, to request V4L2 buffers of type
MMAP or DMABUF, but that should be decoupled from the buffers pool
itself. Please keep in mind that the buffers pool class will be used
internally to share buffers between devices (for instance between the
IPU3 CIO2 and ImgU), where you will have a single buffers pool but two
V4L2 buffers queues. The pool is thus distinct from the queues.

This being said, when an application requests a buffers pool with
internal allocation, the allocation of buffers will be delegated to the
V4L2 device, so you may certainly implement a V4L2BufferPool class for
this case.

> >> +		int ret = allocateMemory();
> > 
> > This is left to subclasses, which I do expect to use MMAP or DMABUF,
> > and each subclass will have its own specific fields. I wonder if it is
> > worth to provide a base class like Buffer which is not pure virtual.
> > 
> > In example, importer vs exporter does not apply to mmap buffers, am I
> > right? Also, why would a DMABUF implementation expose a mmap() and
> > unmap() method?
> > 
> > Have you considered having Buffer as pure virtual, and use it keep the
> > vector<Buffers *> in the pool, but move most of the details and
> > implementations in subclasses?
> 
> I think that's sort of the way that my current branch is heading. I'm
> trying to get the V4L2Device to create the objects directly. Having some
> issues with casting and storage though.
> 
> I have a V4L2Buffer which allows instantiation of the Buffer object from
> the V4L2 device.
> 
> But if I create a V4L2BufferPool - I'm hitting issues with how to
> express a vector of Buffer objects - that are also (or rather are in
> fact) V4L2Buffer objects.
> 
> 
> In summary - I believe I agree with you here. The interface at Buffer.h
> should be just that. The interface exposed to the application, with as
> generic implementation as possible.
> 
> >> +		if (ret < 0)
> >> +			return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +void BufferPool::free()
> >> +{
> >> +	for (Buffer *buffer : buffers_)
> >> +		delete buffer;
> >> +
> >> +	buffers_.clear();
> > 
> > I think buffers_ will be destroyed anyway, so you could drop this.
> > 
> 
> This function is named free(). It's not a destructor. So at this stage,
> I think this .clear() needs to be kept.
> 
> Granted, it's only use is in the destructor - but if someone were to
> call free() directly - it should clear() the vector.
> 
> >> +}
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index f9f25c0ecf15..b5b25965fd12 100644
> >> --- a/src/libcamera/meson.build
> >> +++ b/src/libcamera/meson.build
> >> @@ -1,4 +1,5 @@
> >>  libcamera_sources = files([
> >> +    'buffer.cpp',
> >>      'camera.cpp',
> >>      'camera_manager.cpp',
> >>      'device_enumerator.cpp',

-- 
Regards,

Laurent Pinchart