[libcamera-devel] [PATCH 1/4] libcamera: Add Buffer and BufferPool classes
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Jan 31 20:00:31 CET 2019
Hi Jacopo,
On 31/01/2019 18:34, Jacopo Mondi wrote:
> Hi Kieran,
> thanks for the patch
>
> 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
>>
>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
>> new file mode 100644
>> index 000000000000..97c8025d9e77
>> --- /dev/null
>> +++ b/include/libcamera/buffer.h
>> @@ -0,0 +1,55 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2018, Google Inc.
>
> 2019 ?
Laurent created this patch, in 2018. I've just been adding on top of his
patch. (except for one small issue I must have seen with checkstyle).
I guess I can update the year too ...
>> + *
>> + * buffer.h - Buffer handling
>> + */
>> +#ifndef __LIBCAMERA_BUFFER_H__
>> +#define __LIBCAMERA_BUFFER_H__
>> +
>> +#include <vector>
>> +
>> +namespace libcamera {
>> +
>> +class Buffer
>> +{
>> +public:
>> + Buffer();
>> + ~Buffer();
>> +
>> + int dmabuf() const { return fd_; };
>> + int setDmabuf(int fd);
>> +
>> + int mmap();
>> + void munmap();
>> +
>> +private:
>> + int fd_;
>> +};
>> +
>> +class BufferPool
>> +{
>> +public:
>> + enum Memory {
>> + Internal,
>> + External,
>> + };
>> +
>> + BufferPool(Memory memory);
>> + virtual ~BufferPool();
>> +
>> + int allocate(unsigned int count);
>> + void free();
>> +
>> + unsigned int count() const { return buffers_.size(); };
>> +
>> +private:
>> + virtual int allocateMemory() = 0;
>> +
>> + BufferPool::Memory memory_;
>> + std::vector<Buffer *> buffers_;
>> +};
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_BUFFER_H__ */
>> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h
>> index c0511cf6d662..6619e556ebbd 100644
>> --- a/include/libcamera/libcamera.h
>> +++ b/include/libcamera/libcamera.h
>> @@ -7,6 +7,7 @@
>> #ifndef __LIBCAMERA_LIBCAMERA_H__
>> #define __LIBCAMERA_LIBCAMERA_H__
>>
>> +#include <libcamera/buffer.h>
>> #include <libcamera/camera.h>
>> #include <libcamera/camera_manager.h>
>> #include <libcamera/event_dispatcher.h>
>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
>> index d7cb55ba4a49..8fcbbf7892ea 100644
>> --- a/include/libcamera/meson.build
>> +++ b/include/libcamera/meson.build
>> @@ -1,4 +1,5 @@
>> libcamera_api = files([
>> + 'buffer.h',
>> 'camera.h',
>> 'camera_manager.h',
>> 'event_dispatcher.h',
>> 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
>> @@ -0,0 +1,102 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2018, Google Inc.
>
> 2019 ?
>
>> + *
>> + * buffer.cpp - Buffer handling
>> + */
>> +
>> +#include <errno.h>
>> +#include <unistd.h>
>> +
>> +#include <libcamera/buffer.h>
>> +
>> +/**
>> + * \file buffer.h
>> + * \brief Buffer handling
>> + */
>> +
>> +namespace libcamera {
>> +
>> +/**
>> + * \class Buffer
>> + * \brief A memory buffer to store a frame
>> + *
>> + * The Buffer class represents a memory buffer used to store a frame.
>> + */
>> +Buffer::Buffer()
>> + : fd_(-1)
>> +{
>> +}
>> +
>> +Buffer::~Buffer()
>> +{
>> + if (fd_ != -1)
>> + close(fd_);
>> +}
>> +
>> +/**
>> + * \fn Buffer::dmabuf()
>> + * \brief Get the dmabuf file handle backing the buffer
>> + */
>> +
>> +/**
>> + * \brief Set the dmabuf file handle backing the buffer
>> + *
>> + * The \a fd dmabuf file handle is duplicated and stored.
>> + */
>> +int Buffer::setDmabuf(int fd)
>> +{
>> + if (fd_ != -1) {
>> + close(fd_);
>> + fd_ = -1;
>> + }
>> +
>> + if (fd != -1)
>> + return 0;
>
> I didn't get this... Shouldn't we exit if (fd == -1) ?
ah yes - I spotted this before - I thought I fixed it.
Looks like I must have dropped/lost the fix.
I'll update now so it doesn't get lost.
>
>> +
>> + fd_ = dup(fd);
>> + if (fd_ == -1)
>
> A more approriate error handling sequence might be considered here.
>
>> + return -errno;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * \class BufferPool
>> + * \brief A pool of buffers
>> + */
>> +BufferPool::BufferPool(BufferPool::Memory memory)
>> + : memory_(memory)
>> +{
>> +}
>> +
>> +BufferPool::~BufferPool()
>> +{
>> + free();
>> +}
>> +
>> +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)
>> + 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.
> Thanks
> j
>
>> +}
>> +
>> +} /* 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',
>> --
>> 2.19.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list