[libcamera-devel] [RFC 11/12] libcamera: buffer: Switch to new buffer API
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Nov 1 13:28:57 CET 2019
Hi Niklas,
There's a lot of churn to get through here, so this isn't necessarily a
full review - but just picking out comments as I see them ...
(Ahem, maybe I've spent more time on this now than I expected to at the
beginning)
Also - I'm starting with this patch in the series to see how the new API
is utilised by cam/qcam/tests before looking at earlier patches that
implement it.
On 28/10/2019 02:25, Niklas Söderlund wrote:
> Switch to the new buffer API where all buffers are treated as external
> buffers and are allocated outside the camera. For V4L2 backed cameras a
> helper class BufferAllocator is provided to help applications create
> buffers.
>
> This patch is quiet large as it need to touch most areas of libcamera to
> swap the API. It also restores the buffer sharing test and functionality
> which was broken in the previous change. A follow up change to this one
> will finalize the transition to the new API by removing code that is
> left unused after this change.
This really is a big patch ... and would have benefited from being
reduced to smaller individual commits (I'm certain there are several
small individually compilable chunks in here that could be broken out)
- but I understand this is an RFC with a lot of churn going on in a
small amount of time ...
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
There's a lot in here that I like though!
> ---
> include/ipa/ipa_interface.h | 2 +-
> include/libcamera/buffer.h | 47 +--
> include/libcamera/request.h | 2 +-
> src/cam/buffer_writer.cpp | 7 +-
> src/cam/capture.cpp | 32 ++-
> src/cam/capture.h | 3 +-
> src/ipa/rkisp1/rkisp1.cpp | 14 +-
> src/libcamera/buffer.cpp | 243 +++-------------
> src/libcamera/camera.cpp | 47 +--
> src/libcamera/include/v4l2_videodevice.h | 33 ++-
> src/libcamera/pipeline/ipu3/ipu3.cpp | 199 ++++---------
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 74 ++---
> src/libcamera/pipeline/uvcvideo.cpp | 16 +-
> src/libcamera/pipeline/vimc.cpp | 16 +-
> src/libcamera/request.cpp | 15 +-
> src/libcamera/v4l2_videodevice.cpp | 267 ++++++++++--------
> src/qcam/main_window.cpp | 43 ++-
> src/qcam/main_window.h | 3 +
> test/camera/capture.cpp | 29 +-
> test/camera/statemachine.cpp | 12 +-
> test/v4l2_videodevice/buffer_sharing.cpp | 21 +-
> test/v4l2_videodevice/capture_async.cpp | 12 +-
> test/v4l2_videodevice/request_buffers.cpp | 11 +-
> test/v4l2_videodevice/stream_on_off.cpp | 6 +-
> test/v4l2_videodevice/v4l2_m2mdevice.cpp | 36 +--
> test/v4l2_videodevice/v4l2_videodevice_test.h | 2 +-
> 26 files changed, 435 insertions(+), 757 deletions(-)
>
> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> index 8fd658af5fdb2e6b..efb86a33f6f427ae 100644
> --- a/include/ipa/ipa_interface.h
> +++ b/include/ipa/ipa_interface.h
> @@ -26,7 +26,7 @@ struct IPAStream {
>
> struct IPABuffer {
> unsigned int id;
> - BufferMemory memory;
> + std::vector<std::pair<int, unsigned int>> planes;
> };
>
> struct IPAOperationData {
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index adb642ad5da072d2..3568c2e0cdf8d95b 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -22,11 +22,10 @@ class Stream;
> class Plane final
> {
> public:
> - Plane();
> + Plane(int fd, unsigned int length);
> ~Plane();
>
> - int dmabuf() const { return fd_; }
> - int setDmabuf(int fd, unsigned int length);
> + int fd() const { return fd_; }
>
> void *mem();
> unsigned int length() const { return length_; }
> @@ -67,45 +66,17 @@ private:
> class Buffer final
> {
> public:
> - enum Status {
> - BufferSuccess,
> - BufferError,
> - BufferCancelled,
> - };
> -
> - Buffer(unsigned int index = -1, const Buffer *metadata = nullptr);
> - Buffer(const Buffer &) = delete;
> - Buffer &operator=(const Buffer &) = delete;
> + Buffer(std::vector<std::pair<int, unsigned int>> planes);
> + ~Buffer();
>
> - unsigned int index() const { return index_; }
> - const std::array<int, 3> &dmabufs() const { return dmabuf_; }
> - BufferMemory *mem() { return mem_; }
> -
> - unsigned int bytesused() const { return bytesused_; }
> - uint64_t timestamp() const { return timestamp_; }
> - unsigned int sequence() const { return sequence_; }
> + unsigned int numPlanes() { return planes_.size(); }
> + Plane *plane(unsigned int plane);
>
> - Status status() const { return status_; }
> - Stream *stream() const { return stream_; }
> + std::array<int, 3> fds() const;
> + std::vector<std::pair<int, unsigned int>> description() const;
Ahh C++, when the return type is 4 times as many characters as the
function name ... :-S
> private:
> - friend class Camera;
> - friend class Request;
> - friend class Stream;
> - friend class V4L2VideoDevice;
> -
> - void cancel();
> -
> - unsigned int index_;
> - std::array<int, 3> dmabuf_;
> - BufferMemory *mem_;
> -
> - unsigned int bytesused_;
> - uint64_t timestamp_;
> - unsigned int sequence_;
> -
> - Status status_;
> - Stream *stream_;
> + std::vector<Plane *> planes_;
> };
>
> class BufferInfo
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 88ef7bf03fcfb77b..ac2beab46bea6466 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -40,7 +40,7 @@ public:
> ControlList &metadata() { return *metadata_; }
> const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }
> const BufferInfo &info(Buffer *frame) const { return info_.find(frame)->second; };
> - int addBuffer(std::unique_ptr<Buffer> buffer);
> + int addBuffer(Stream *stream, Buffer *buffer);
> Buffer *findBuffer(Stream *stream) const;
>
> uint64_t cookie() const { return cookie_; }
> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
> index 3ee9e82ba216abb6..c9df851f3ecec056 100644
> --- a/src/cam/buffer_writer.cpp
> +++ b/src/cam/buffer_writer.cpp
> @@ -43,10 +43,9 @@ int BufferWriter::write(Buffer *buffer, const BufferInfo &info,
> if (fd == -1)
> return -errno;
>
> - BufferMemory *mem = buffer->mem();
> - for (Plane &plane : mem->planes()) {
> - void *data = plane.mem();
> - unsigned int length = plane.length();
> + for (unsigned int i = 0; i < buffer->numPlanes(); i++) {
> + void *data = buffer->plane(i)->mem();
> + unsigned int length = buffer->plane(i)->length();
>
> ret = ::write(fd, data, length);
> if (ret < 0) {
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index 251e9f86c86b508d..8bd77dcec2d15f1c 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -57,7 +57,10 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
> writer_ = new BufferWriter();
> }
>
> - ret = capture(loop);
> +
> + BufferAllocator allocator(camera_);
> +
> + ret = capture(loop, allocator);
>
> if (options.isSet(OptFile)) {
> delete writer_;
> @@ -69,14 +72,21 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
> return ret;
> }
>
> -int Capture::capture(EventLoop *loop)
> +int Capture::capture(EventLoop *loop, BufferAllocator &allocator)
> {
> int ret;
>
> /* Identify the stream with the least number of buffers. */
> unsigned int nbuffers = UINT_MAX;
> - for (StreamConfiguration &cfg : *config_)
> - nbuffers = std::min(nbuffers, cfg.bufferCount);
> + for (StreamConfiguration &cfg : *config_) {
> + ret = allocator.allocate(cfg.stream());
> + if (ret < 0) {
> + std::cerr << "Can't allocate buffers" << std::endl;
> + return -ENOMEM;
> + }
> +
> + nbuffers = std::min(nbuffers, static_cast<unsigned int>(ret));
> + }
>
> /*
> * TODO: make cam tool smarter to support still capture by for
> @@ -93,9 +103,9 @@ int Capture::capture(EventLoop *loop)
>
> for (StreamConfiguration &cfg : *config_) {
> Stream *stream = cfg.stream();
> - std::unique_ptr<Buffer> buffer = stream->createBuffer(i);
> + Buffer *buffer = allocator.get(stream)[i];
>
> - ret = request->addBuffer(std::move(buffer));
> + ret = request->addBuffer(stream, buffer);
> if (ret < 0) {
> std::cerr << "Can't set buffer for request"
> << std::endl;
> @@ -179,15 +189,7 @@ void Capture::requestComplete(Request *request)
> for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> Stream *stream = it->first;
> Buffer *buffer = it->second;
> - unsigned int index = buffer->index();
> -
> - std::unique_ptr<Buffer> newBuffer = stream->createBuffer(index);
> - if (!newBuffer) {
> - std::cerr << "Can't create buffer " << index << std::endl;
> - return;
> - }
> -
> - request->addBuffer(std::move(newBuffer));
> + request->addBuffer(stream, buffer);
> }
>
> camera_->queueRequest(request);
> diff --git a/src/cam/capture.h b/src/cam/capture.h
> index c692d48918f2de1d..ee5adad7cbaaae47 100644
> --- a/src/cam/capture.h
> +++ b/src/cam/capture.h
> @@ -10,6 +10,7 @@
> #include <chrono>
> #include <memory>
>
> +#include <libcamera/buffer.h>
> #include <libcamera/camera.h>
> #include <libcamera/request.h>
> #include <libcamera/stream.h>
> @@ -26,7 +27,7 @@ public:
>
> int run(EventLoop *loop, const OptionsParser::Options &options);
> private:
> - int capture(EventLoop *loop);
> + int capture(EventLoop *loop, libcamera::BufferAllocator &allocator);
>
> void requestComplete(libcamera::Request *request);
>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 9a13f5c7df17f335..42d3379ce84d600a 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -47,7 +47,7 @@ private:
> void setControls(unsigned int frame);
> void metadataReady(unsigned int frame, unsigned int aeState);
>
> - std::map<unsigned int, BufferMemory> bufferInfo_;
> + std::map<unsigned int, Buffer *> bufferInfo_;
>
> ControlInfoMap ctrls_;
>
> @@ -101,15 +101,17 @@ void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers)
> {
> for (const IPABuffer &buffer : buffers) {
> - bufferInfo_[buffer.id] = buffer.memory;
> - bufferInfo_[buffer.id].planes()[0].mem();
> + bufferInfo_[buffer.id] = new Buffer(buffer.planes);
> + bufferInfo_[buffer.id]->plane(0)->mem();
This looks weird. I get that the call to ->mem() is probably to perform
any underlying mapping required, but not storing the return value
anywhere looks like someone forgot to assign it.
I see that it was already like that before this patch, but I think it's
worth adding a comment here to make it clear that the address is not
used, and this call is simply to ensure that any required mappings are
performed (if that assumption is correct)
> }
> }
>
> void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
> {
> - for (unsigned int id : ids)
> + for (unsigned int id : ids) {
> + delete bufferInfo_[id];
> bufferInfo_.erase(id);
> + }
> }
>
> void IPARkISP1::processEvent(const IPAOperationData &event)
> @@ -120,7 +122,7 @@ void IPARkISP1::processEvent(const IPAOperationData &event)
> unsigned int bufferId = event.data[1];
>
> const rkisp1_stat_buffer *stats =
> - static_cast<rkisp1_stat_buffer *>(bufferInfo_[bufferId].planes()[0].mem());
> + static_cast<rkisp1_stat_buffer *>(bufferInfo_[bufferId]->plane(0)->mem());
>
> updateStatistics(frame, stats);
> break;
> @@ -130,7 +132,7 @@ void IPARkISP1::processEvent(const IPAOperationData &event)
> unsigned int bufferId = event.data[1];
>
> rkisp1_isp_params_cfg *params =
> - static_cast<rkisp1_isp_params_cfg *>(bufferInfo_[bufferId].planes()[0].mem());
> + static_cast<rkisp1_isp_params_cfg *>(bufferInfo_[bufferId]->plane(0)->mem());
>
> queueRequest(frame, params, event.controls[0]);
> break;
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index fce1ce5e49cbbf42..d00849520bc2b51c 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -48,69 +48,44 @@ LOG_DEFINE_CATEGORY(Buffer)
> * an image may or may not be contiguous.
> */
>
> -Plane::Plane()
> - : fd_(-1), length_(0), mem_(0)
> -{
> -}
> -
> -Plane::~Plane()
> -{
> - munmap();
> -
> - if (fd_ != -1)
> - close(fd_);
> -}
> -
> -/**
> - * \fn Plane::dmabuf()
> - * \brief Get the dmabuf file handle backing the buffer
> - */
> -
> /**
> * \brief Set the dmabuf file handle backing the buffer
> * \param[in] fd The dmabuf file handle
> * \param[in] length The size of the memory region
> *
> - * The \a fd dmabuf file handle is duplicated and stored. The caller may close
> - * the original file handle.
> - *
> - * \return 0 on success or a negative error code otherwise
> + * The \a fd dmabuf file handle is duplicated and stored.
> */
> -int Plane::setDmabuf(int fd, unsigned int length)
> +Plane::Plane(int fd, unsigned int length)
> + : fd_(-1), length_(length), mem_(nullptr)
> {
> if (fd < 0) {
> - LOG(Buffer, Error) << "Invalid dmabuf fd provided";
> - return -EINVAL;
> - }
> -
> - if (fd_ != -1) {
> - close(fd_);
> - fd_ = -1;
> + LOG(Buffer, Fatal) << "Invalid dmabuf fd provided";
> + return;
> }
>
> fd_ = dup(fd);
> +
> if (fd_ == -1) {
> int ret = -errno;
> - LOG(Buffer, Error)
> + LOG(Buffer, Fatal)
Seeing fatal errors in the constructor worries me a little ....
> << "Failed to duplicate dmabuf: " << strerror(-ret);
> - return ret;
> + return;
> }
> +}
>
> - length_ = length;
> +Plane::~Plane()
> +{
> + munmap();
>
> - return 0;
> + if (fd_ != -1)
> + close(fd_);
> }
>
> /**
> - * \brief Map the plane memory data to a CPU accessible address
> - *
> - * The file descriptor to map the memory from must be set by a call to
> - * setDmaBuf() before calling this function.
> - *
> - * \sa setDmaBuf()
> - *
> - * \return 0 on success or a negative error code otherwise
> + * \fn Plane::fd()
> + * \brief Get the dmabuf file handle backing the buffer
> */
> +
Plane::mmap just lost it's documentation ? ...
(Perhaps it became a private function, I'll see later)
> int Plane::mmap()
> {
> void *map;
> @@ -131,13 +106,6 @@ int Plane::mmap()
> return 0;
> }
>
> -/**
> - * \brief Unmap any existing CPU accessible mapping
> - *
> - * Unmap the memory mapped by an earlier call to mmap().
> - *
> - * \return 0 on success or a negative error code otherwise
> - */
That certainly looks intentional so yes, I expect these have become
internal.
> int Plane::munmap()
> {
> int ret = 0;
> @@ -236,166 +204,49 @@ void BufferPool::destroyBuffers()
> * \return A vector containing all the buffers in the pool.
> */
>
> -/**
> - * \class Buffer
> - * \brief A buffer handle and dynamic metadata
> - *
> - * The Buffer class references a buffer memory and associates dynamic metadata
> - * related to the frame contained in the buffer. It allows referencing buffer
> - * memory through a single interface regardless of whether the memory is
> - * allocated internally in libcamera or provided externally through dmabuf.
> - *
> - * Buffer instances are allocated dynamically for a stream through
> - * Stream::createBuffer(), added to a request with Request::addBuffer() and
> - * deleted automatically after the request complete handler returns.
> - */
> -
> -/**
> - * \enum Buffer::Status
> - * Buffer completion status
> - * \var Buffer::BufferSuccess
> - * The buffer has completed with success and contains valid data. All its other
> - * metadata (such as bytesused(), timestamp() or sequence() number) are valid.
> - * \var Buffer::BufferError
> - * The buffer has completed with an error and doesn't contain valid data. Its
> - * other metadata are valid.
> - * \var Buffer::BufferCancelled
> - * The buffer has been cancelled due to capture stop. Its other metadata are
> - * invalid and shall not be used.
> - */
> -
> -/**
> - * \brief Construct a buffer not associated with any stream
> - *
> - * This method constructs an orphaned buffer not associated with any stream. It
> - * is not meant to be called by applications, they should instead create buffers
> - * for a stream with Stream::createBuffer().
> - */
> -Buffer::Buffer(unsigned int index, const Buffer *metadata)
> - : index_(index), dmabuf_({ -1, -1, -1 }),
> - status_(Buffer::BufferSuccess), stream_(nullptr)
> +Buffer::Buffer(std::vector<std::pair<int, unsigned int>> planes)
> {
> - if (metadata) {
> - bytesused_ = metadata->bytesused_;
> - sequence_ = metadata->sequence_;
> - timestamp_ = metadata->timestamp_;
> - } else {
> - bytesused_ = 0;
> - sequence_ = 0;
> - timestamp_ = 0;
> - }
> + for (std::pair<int, unsigned int> plane : planes)
> + planes_.push_back(new Plane(plane.first, plane.second));
I /really/ dislike the non-explicit use of .first, .second types :-(
Mostly because the word 'first' and 'second' are so ambiguous in this
context, it makes readability of the code *really* (*really*) /really/ bad.
I know it's a pain to split this out to (presumably)
for (std::pair<int, unsigned int> plane : planes) {
int fd = plane.first;
unsigned int size = plane.second:
planes_.push_back(new Plane(fd, size));
}
But I think it's so important when using std::pair.
(I totally blame std::pair here)
I would fully expect the compiler to optimise the extra variables out,
so there should be no overhead, but the readability is so much better.
The statement:
"planes_.push_back(new Plane(plane.first, plane.second));"
could far too easily be misinterpreted as 'add the first and second'
plane to planes_ by anyone glancing at the code.
> }
>
> -/**
> - * \fn Buffer::index()
> - * \brief Retrieve the Buffer index
> - * \return The buffer index
> - */
> +Buffer::~Buffer()
> +{
> + for (Plane *plane : planes_)
> + delete plane;
> +}
>
> -/**
> - * \fn Buffer::dmabufs()
> - * \brief Retrieve the dmabuf file descriptors for all buffer planes
> - *
> - * The dmabufs array contains one dmabuf file descriptor per plane. Unused
> - * entries are set to -1.
> - *
> - * \return The dmabuf file descriptors
> - */
> +Plane *Buffer::plane(unsigned int plane)
> +{
> + if (plane > planes_.size()) {
> + LOG(Buffer, Error) << "Plane " << plane << " is out of range";
> + return nullptr;
> + }
>
> -/**
> - * \fn Buffer::mem()
> - * \brief Retrieve the BufferMemory this buffer is associated with
> - *
> - * The association between the buffer and a BufferMemory instance is valid from
> - * the time the request containing this buffer is queued to a camera to the end
> - * of that request's completion handler.
> - *
> - * \return The BufferMemory this buffer is associated with
> - */
> + return planes_[plane];
> +}
>
> -/**
> - * \fn Buffer::bytesused()
> - * \brief Retrieve the number of bytes occupied by the data in the buffer
> - * \return Number of bytes occupied in the buffer
> - */
> +std::array<int, 3> Buffer::fds() const
> +{
> + std::array<int, 3> ret = { -1, -1, -1 };
>
> -/**
> - * \fn Buffer::timestamp()
> - * \brief Retrieve the time when the buffer was processed
> - *
> - * The timestamp is expressed as a number of nanoseconds since the epoch.
> - *
> - * \return Timestamp when the buffer was processed
> - */
> + unsigned int i = 0;
> + for (const Plane *plane : planes_)
> + ret[i++] = plane->fd();
>
> -/**
> - * \fn Buffer::sequence()
> - * \brief Retrieve the buffer sequence number
> - *
> - * The sequence number is a monotonically increasing number assigned to the
> - * buffer processed by the stream. Gaps in the sequence numbers indicate
> - * dropped frames.
> - *
> - * \return Sequence number of the buffer
> - */
> -
> -/**
> - * \fn Buffer::status()
> - * \brief Retrieve the buffer status
> - *
> - * The buffer status reports whether the buffer has completed successfully
> - * (BufferSuccess) or if an error occurred (BufferError).
> - *
> - * \return The buffer status
> - */
> + return ret;
> +}
>
> -/**
> - * \fn Buffer::request()
> - * \brief Retrieve the request this buffer belongs to
> - *
> - * The intended callers of this method are buffer completion handlers that
> - * need to associate a buffer to the request it belongs to.
> - *
> - * A Buffer is associated to a request by Request::prepare() and the
> - * association is valid until the buffer completes. The returned request
> - * pointer is valid only during that interval.
> - *
> - * \return The Request the Buffer belongs to, or nullptr if the buffer is
> - * either completed or not associated with a request
> - * \sa Buffer::setRequest()
> - */
> +std::vector<std::pair<int, unsigned int>> Buffer::description() const
> +{
> + std::vector<std::pair<int, unsigned int>> desc;
>
> -/**
> - * \fn Buffer::stream()
> - * \brief Retrieve the stream this buffer is associated with
> - *
> - * A Buffer is associated to the stream that created it with
> - * Stream::createBuffer() and the association is valid until the buffer is
> - * destroyed. Buffer instances that are created directly are not associated
> - * with any stream.
> - *
> - * \return The Stream the Buffer is associated with, or nullptr if the buffer
> - * is not associated with a stream
> - */
> + for (const Plane *plane : planes_)
> + desc.emplace_back(plane->fd(), plane->length());
>
> -/**
> - * \brief Mark a buffer as cancel by setting its status to BufferCancelled
> - */
> -void Buffer::cancel()
> -{
> - bytesused_ = 0;
> - timestamp_ = 0;
> - sequence_ = 0;
> - status_ = BufferCancelled;
> + return desc;
> }
>
> -/**
> - * \fn Buffer::setRequest()
> - * \brief Set the request this buffer belongs to
> - *
> - * The intended callers are Request::prepare() and Request::completeBuffer().
> - */
> -
> BufferAllocator::BufferAllocator(std::shared_ptr<Camera> camera)
> : camera_(camera)
> {
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index e810fb725d81350d..63944c0199338f39 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -678,12 +678,6 @@ int Camera::configure(CameraConfiguration *config)
>
> stream->configuration_ = cfg;
> activeStreams_.insert(stream);
> -
> - /*
> - * Allocate buffer objects in the pool.
> - * Memory will be allocated and assigned later.
> - */
> - stream->createBuffers(cfg.memoryType, cfg.bufferCount);
> }
>
> state_ = CameraConfigured;
> @@ -739,14 +733,6 @@ int Camera::freeBuffers()
> if (!stateIs(CameraPrepared))
> return -EACCES;
>
> - for (Stream *stream : activeStreams_) {
> - /*
> - * All mappings must be destroyed before buffers can be freed
> - * by the V4L2 device that has allocated them.
> - */
> - stream->destroyBuffers();
> - }
> -
> state_ = CameraConfigured;
>
> return pipe_->freeBuffers(this, activeStreams_);
> @@ -812,24 +798,11 @@ int Camera::queueRequest(Request *request)
>
> for (auto const &it : request->buffers()) {
> Stream *stream = it.first;
> - Buffer *buffer = it.second;
>
> if (activeStreams_.find(stream) == activeStreams_.end()) {
> LOG(Camera, Error) << "Invalid request";
Not caused by this patch, but reading this - it would be really good to
provide more information as to /why/ it's an invalid request to the caller.
> return -EINVAL;
> }
> -
> - if (stream->memoryType() == ExternalMemory) {
> - int index = stream->mapBuffer(buffer);
> - if (index < 0) {
> - LOG(Camera, Error) << "No buffer memory available";
> - return -ENOMEM;
> - }
> -
> - buffer->index_ = index;
> - }
> -
> - buffer->mem_ = &stream->buffers()[buffer->index_];
> }
> int ret = request->prepare();
> @@ -856,15 +829,23 @@ int Camera::queueRequest(Request *request)
> */
> int Camera::start()
> {
> + int ret;
> +
> if (disconnected_)
> return -ENODEV;
>
> if (!stateIs(CameraPrepared))
> return -EACCES;
>
> + for (Stream *stream : streams_) {
> + ret = stream->importBuffers(true);
> + if (ret < 0)
> + return ret;
> + }
> +
> LOG(Camera, Debug) << "Starting capture";
>
> - int ret = pipe_->start(this);
> + ret = pipe_->start(this);
> if (ret)
> return ret;
>
> @@ -899,6 +880,9 @@ int Camera::stop()
>
> pipe_->stop(this);
>
> + for (Stream *stream : streams_)
> + stream->importBuffers(false);
importBuffers (true/false) seems odd here.
Is 'import' really the right verb?
> +
> return 0;
> }
>
> @@ -912,13 +896,6 @@ int Camera::stop()
> */
> void Camera::requestComplete(Request *request)
> {
> - for (auto it : request->buffers()) {
> - Stream *stream = it.first;
> - Buffer *buffer = it.second;
> - if (stream->memoryType() == ExternalMemory)
> - stream->unmapBuffer(buffer);
> - }
> -
> requestCompleted.emit(request);
> delete request;
> }
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 01b90ab4654bfba2..cbbb6cd955bf527f 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -14,7 +14,9 @@
>
> #include <libcamera/buffer.h>
> #include <libcamera/geometry.h>
> +#include <libcamera/request.h>
> #include <libcamera/signal.h>
> +#include <libcamera/stream.h>
>
> #include "formats.h"
> #include "log.h"
> @@ -22,9 +24,6 @@
>
> namespace libcamera {
>
> -class Buffer;
> -class BufferMemory;
> -class BufferPool;
> class EventNotifier;
> class MediaDevice;
> class MediaEntity;
> @@ -161,12 +160,11 @@ public:
> int setFormat(V4L2DeviceFormat *format);
> ImageFormats formats();
>
> - int exportBuffers(BufferPool *pool);
> - int importBuffers(BufferPool *pool);
> + int allocateBuffers(unsigned int count, std::vector<Buffer *> *buffers);
> + int importBuffers(unsigned int count);
> int releaseBuffers();
>
> - int queueBuffer(Buffer *buffer);
> - std::vector<std::unique_ptr<Buffer>> queueAllBuffers();
> + int queueBuffer(Buffer *buffer, const BufferInfo *info = nullptr);
> Signal<Buffer *, const BufferInfo &> bufferReady;
>
> int streamOn();
> @@ -192,8 +190,8 @@ private:
> std::vector<SizeRange> enumSizes(unsigned int pixelFormat);
>
> int requestBuffers(unsigned int count);
> - int createPlane(BufferMemory *buffer, unsigned int index,
> - unsigned int plane, unsigned int length);
> + Buffer *createBuffer(struct v4l2_buffer buf);
> + int expbuf(unsigned int index, unsigned int plane);
expbuf? Even in the private section, shortened function names are a bit
distasteful to me.
Does exp mean 'expand' buffer, 'expose' buffer', export buffer?
I think exportBuffer() would be far more appropriate to the code style.
(edit: Seeing the implementation, waaaay down below - perhaps
exportDMABuffer is more appropriate here.)
> void bufferAvailable(EventNotifier *notifier);
>
> @@ -202,7 +200,7 @@ private:
> enum v4l2_buf_type bufferType_;
> enum v4l2_memory memoryType_;
>
> - BufferPool *bufferPool_;
> + V4L2BufferCache *cache_;
> std::map<unsigned int, Buffer *> queuedBuffers_;
>
> EventNotifier *fdEvent_;
> @@ -227,6 +225,21 @@ private:
> V4L2VideoDevice *capture_;
> };
>
> +class V4L2Stream : public Stream
> +{
> +public:
> + V4L2Stream(V4L2VideoDevice *video, unsigned int bufferCount);
> +
> +protected:
> + int allocateBuffers(std::vector<Buffer *> *buffers) override;
> + int importBuffers(bool enable) override;
> +
> +private:
> + V4L2VideoDevice *video_;
> + unsigned int bufferCount_;
> + bool allocated;
> +};
> +
> } /* namespace libcamera */
>
> #endif /* __LIBCAMERA_V4L2_VIDEODEVICE_H__ */
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 01064ac09859155d..af8e93c04bc40f04 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -43,7 +43,7 @@ public:
> V4L2VideoDevice *dev;
> unsigned int pad;
> std::string name;
> - BufferPool *pool;
> + std::vector<Buffer *> buffers;
> };
>
> ImgUDevice()
> @@ -69,9 +69,8 @@ public:
> int configureOutput(ImgUOutput *output,
> const StreamConfiguration &cfg);
>
> - int importInputBuffers(BufferPool *pool);
> - int importOutputBuffers(ImgUOutput *output, BufferPool *pool);
> - int exportOutputBuffers(ImgUOutput *output, BufferPool *pool);
> + int importInputBuffers(unsigned int count);
> + int exportOutputBuffers(ImgUOutput *output, unsigned int count);
> void freeBuffers();
>
> int start();
> @@ -92,10 +91,6 @@ public:
> ImgUOutput viewfinder_;
> ImgUOutput stat_;
> /* \todo Add param video device for 3A tuning */
> -
> - BufferPool vfPool_;
> - BufferPool statPool_;
> - BufferPool outPool_;
> };
>
> class CIO2Device
> @@ -119,10 +114,10 @@ public:
> int configure(const Size &size,
> V4L2DeviceFormat *outputFormat);
>
> - BufferPool *exportBuffers();
> + int exportBuffers();
> void freeBuffers();
>
> - int start(std::vector<std::unique_ptr<Buffer>> *buffers);
> + int start();
> int stop();
>
> static int mediaBusToFormat(unsigned int code);
> @@ -131,14 +126,17 @@ public:
> V4L2Subdevice *csi2_;
> CameraSensor *sensor_;
>
> - BufferPool pool_;
> +private:
> + std::vector<Buffer *> buffers_;
> };
>
> -class IPU3Stream : public Stream
> +class IPU3Stream : public V4L2Stream
> {
> public:
> - IPU3Stream(ImgUDevice::ImgUOutput *device, const std::string &name)
> - : active_(false), name_(name), device_(device)
> + IPU3Stream(ImgUDevice::ImgUOutput *device, const std::string &name,
> + unsigned int count)
> + : V4L2Stream(device->dev, count), active_(false), name_(name),
> + device_(device)
> {
> }
>
> @@ -170,8 +168,6 @@ public:
>
> IPU3Stream *outStream_;
> IPU3Stream *vfStream_;
> -
> - std::vector<std::unique_ptr<Buffer>> rawBuffers_;
> };
>
> class IPU3CameraConfiguration : public CameraConfiguration
> @@ -296,8 +292,6 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> cfg.size.width &= ~7;
> cfg.size.height &= ~3;
> }
> -
> - cfg.bufferCount = IPU3_BUFFER_COUNT;
> }
>
> CameraConfiguration::Status IPU3CameraConfiguration::validate()
> @@ -635,70 +629,42 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
> const std::set<Stream *> &streams)
> {
> IPU3CameraData *data = cameraData(camera);
> - IPU3Stream *outStream = data->outStream_;
> - IPU3Stream *vfStream = data->vfStream_;
> - CIO2Device *cio2 = &data->cio2_;
> - ImgUDevice *imgu = data->imgu_;
> unsigned int bufferCount;
> int ret;
>
> - /* Share buffers between CIO2 output and ImgU input. */
> - BufferPool *pool = cio2->exportBuffers();
> - if (!pool)
> - return -ENOMEM;
> + ret = data->cio2_.exportBuffers();
> + if (ret < 0)
> + return ret;
>
> - ret = imgu->importInputBuffers(pool);
> - if (ret)
> - goto error;
> + bufferCount = ret;
>
> - /*
> - * Use for the stat's internal pool the same number of buffer as
> - * for the input pool.
> - * \todo To be revised when we'll actually use the stat node.
> - */
> - bufferCount = pool->count();
> - imgu->stat_.pool->createBuffers(bufferCount);
> - ret = imgu->exportOutputBuffers(&imgu->stat_, imgu->stat_.pool);
> + ret = data->imgu_->importInputBuffers(bufferCount);
Which buffers is this now importing? Where are they?
(I'll look for that in the code later, but those are the first questions
that spring to mind seeing this)
If importInputBuffers() knows /which/ buffers to import, then it knows
how many there are ... so bufferCount is redundant... but this feels a
bit horrible to me, The 'data' sharing between two separate objects
(cio2, and imgu) is suddenly very implicit, and not clear.
If it's just because the buffers are accessed directly from the cio2 /
imgu class - and that's what you want - drop the bufferCount to make
everything implicit rather than a mix of implicit/explicit data passing.
Aha - ok - so this doesn't /import/ any buffers into the V4L2 device at
all (so now I'm weary of the naming). Instead, this prepares V4L2 device
to support bufferCount number of buffers as an external allocation, but
with no tie to any particular external buffers at all.
(at which point, I guess the number of bufferCount has become a bit
arbitrary, though still has some meaning?)
I wonder if we should instead do some 'just-in-time' allocation of
buffer objects at the V4L2 side at stream-on time if no one allocated
buffers there before ... but I don't think we need to worry about that now.
> if (ret)
> goto error;
>
> - /* Allocate buffers for each active stream. */
> - for (Stream *s : streams) {
> - IPU3Stream *stream = static_cast<IPU3Stream *>(s);
> - ImgUDevice::ImgUOutput *dev = stream->device_;
> -
> - if (stream->memoryType() == InternalMemory)
> - ret = imgu->exportOutputBuffers(dev, &stream->bufferPool());
> - else
> - ret = imgu->importOutputBuffers(dev, &stream->bufferPool());
> - if (ret)
> - goto error;
> - }
> + ret = data->imgu_->exportOutputBuffers(&data->imgu_->stat_, bufferCount);
> + if (ret < 0)
> + goto error;
>
> /*
> * Allocate buffers also on non-active outputs; use the same number
> * of buffers as the active ones.
> */
> - if (!outStream->active_) {
> - bufferCount = vfStream->configuration().bufferCount;
> - outStream->device_->pool->createBuffers(bufferCount);
> - ret = imgu->exportOutputBuffers(outStream->device_,
> - outStream->device_->pool);
> - if (ret)
> + if (!data->outStream_->active_) {
> + ret = data->imgu_->exportOutputBuffers(data->outStream_->device_,
> + bufferCount);
> + if (ret < 0)
> goto error;
> }
>
> - if (!vfStream->active_) {
> - bufferCount = outStream->configuration().bufferCount;
> - vfStream->device_->pool->createBuffers(bufferCount);
> - ret = imgu->exportOutputBuffers(vfStream->device_,
> - vfStream->device_->pool);
> - if (ret)
> + if (!data->vfStream_->active_) {
> + ret = data->imgu_->exportOutputBuffers(data->vfStream_->device_,
> + bufferCount);
> + if (ret < 0)
> goto error;
> }
>
> return 0;
> -
> error:
> freeBuffers(camera, streams);
>
> @@ -727,7 +693,7 @@ int PipelineHandlerIPU3::start(Camera *camera)
> * Start the ImgU video devices, buffers will be queued to the
> * ImgU output and viewfinder when requests will be queued.
> */
> - ret = cio2->start(&data->rawBuffers_);
> + ret = cio2->start();
> if (ret)
> goto error;
>
> @@ -743,7 +709,6 @@ int PipelineHandlerIPU3::start(Camera *camera)
> error:
> LOG(IPU3, Error) << "Failed to start camera " << camera->name();
>
> - data->rawBuffers_.clear();
> return ret;
> }
>
> @@ -757,8 +722,6 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> if (ret)
> LOG(IPU3, Warning) << "Failed to stop camera "
> << camera->name();
> -
> - data->rawBuffers_.clear();
> }
>
> int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> @@ -880,8 +843,8 @@ int PipelineHandlerIPU3::registerCameras()
> * second.
> */
> data->imgu_ = numCameras ? &imgu1_ : &imgu0_;
> - data->outStream_ = new IPU3Stream(&data->imgu_->output_, "output");
> - data->vfStream_ = new IPU3Stream(&data->imgu_->viewfinder_, "viewfinder");
> + data->outStream_ = new IPU3Stream(&data->imgu_->output_, "output", 4);
> + data->vfStream_ = new IPU3Stream(&data->imgu_->viewfinder_, "viewfinder", 4);
>
> /*
> * Connect video devices' 'bufferReady' signals to their
> @@ -1019,7 +982,6 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>
> output_.pad = PAD_OUTPUT;
> output_.name = "output";
> - output_.pool = &outPool_;
>
> viewfinder_.dev = V4L2VideoDevice::fromEntityName(media,
> name_ + " viewfinder");
> @@ -1029,7 +991,6 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>
> viewfinder_.pad = PAD_VF;
> viewfinder_.name = "viewfinder";
> - viewfinder_.pool = &vfPool_;
>
> stat_.dev = V4L2VideoDevice::fromEntityName(media, name_ + " 3a stat");
> ret = stat_.dev->open();
> @@ -1038,7 +999,6 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>
> stat_.pad = PAD_STAT;
> stat_.name = "stat";
> - stat_.pool = &statPool_;
>
> return 0;
> }
> @@ -1138,65 +1098,25 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
> return 0;
> }
>
> -/**
> - * \brief Import buffers from \a pool into the ImgU input
> - * \param[in] pool The buffer pool to import
> - * \return 0 on success or a negative error code otherwise
> - */
> -int ImgUDevice::importInputBuffers(BufferPool *pool)
> +int ImgUDevice::importInputBuffers(unsigned int count)
> {
> - int ret = input_->importBuffers(pool);
> - if (ret) {
> + int ret = input_->importBuffers(count);
> + if (ret)
> LOG(IPU3, Error) << "Failed to import ImgU input buffers";
> - return ret;
> - }
>
> - return 0;
> + return ret;
> }
>
> -/**
> - * \brief Export buffers from \a output to the provided \a pool
> - * \param[in] output The ImgU output device
> - * \param[in] pool The buffer pool where to export buffers
> - *
> - * Export memory buffers reserved in the video device memory associated with
> - * \a output id to the buffer pool provided as argument.
> - *
> - * \return 0 on success or a negative error code otherwise
> - */
> -int ImgUDevice::exportOutputBuffers(ImgUOutput *output, BufferPool *pool)
> +int ImgUDevice::exportOutputBuffers(ImgUOutput *output, unsigned int count)
> {
> - int ret = output->dev->exportBuffers(pool);
> - if (ret) {
> - LOG(IPU3, Error) << "Failed to export ImgU "
> - << output->name << " buffers";
> - return ret;
> - }
> -
> - return 0;
> -}
> + int ret;
>
> -/**
> - * \brief Reserve buffers in \a output from the provided \a pool
> - * \param[in] output The ImgU output device
> - * \param[in] pool The buffer pool used to reserve buffers in \a output
> - *
> - * Reserve a number of buffers equal to the number of buffers in \a pool
> - * in the \a output device.
> - *
> - * \return 0 on success or a negative error code otherwise
> - */
> -int ImgUDevice::importOutputBuffers(ImgUOutput *output, BufferPool *pool)
> -{
> - int ret = output->dev->importBuffers(pool);
> - if (ret) {
> - LOG(IPU3, Error)
> - << "Failed to import buffer in " << output->name
> - << " ImgU device";
> - return ret;
> - }
> + ret = output->dev->allocateBuffers(count, &output->buffers);
> + if (ret < 0)
> + LOG(IPU3, Error) << "Failed to allocate ImgU "
> + << output->name << " buffers";
>
> - return 0;
> + return ret;
> }
>
> /**
> @@ -1451,38 +1371,33 @@ int CIO2Device::configure(const Size &size,
> return 0;
> }
>
> -/**
> - * \brief Allocate CIO2 memory buffers and export them in a BufferPool
> - *
> - * Allocate memory buffers in the CIO2 video device and export them to
> - * a buffer pool that can be imported by another device.
> - *
> - * \return The buffer pool with export buffers on success or nullptr otherwise
> - */
> -BufferPool *CIO2Device::exportBuffers()
> +int CIO2Device::exportBuffers()
> {
> - pool_.createBuffers(CIO2_BUFFER_COUNT);
> -
> - int ret = output_->exportBuffers(&pool_);
> - if (ret) {
> + int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);
> + if (ret < 0)
> LOG(IPU3, Error) << "Failed to export CIO2 buffers";
> - return nullptr;
> - }
>
> - return &pool_;
> + return ret;
> }
>
> void CIO2Device::freeBuffers()
> {
> + for (Buffer *buffer : buffers_)
> + delete buffer;
> +
> if (output_->releaseBuffers())
> LOG(IPU3, Error) << "Failed to release CIO2 buffers";
> }
>
> -int CIO2Device::start(std::vector<std::unique_ptr<Buffer>> *buffers)
> +int CIO2Device::start()
> {
> - *buffers = output_->queueAllBuffers();
> - if (buffers->empty())
> - return -EINVAL;
> + for (Buffer *buffer : buffers_) {
> + int ret = output_->queueBuffer(buffer);
> + if (ret) {
> + LOG(IPU3, Error) << "Failed to queue CIO2 buffer";
> + return ret;
> + }
> + }
>
> return output_->streamOn();
> }
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 33a058de18b8cf2e..fb224370de19303d 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -31,8 +31,7 @@
> #include "v4l2_subdevice.h"
> #include "v4l2_videodevice.h"
>
> -#define RKISP1_PARAM_BASE 0x100
> -#define RKISP1_STAT_BASE 0x200
> +#define RKISP1_BUFFER_COUNT 4
>
> namespace libcamera {
>
> @@ -153,8 +152,6 @@ public:
> const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
>
> private:
> - static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> -
> /*
> * The RkISP1CameraData instance is guaranteed to be valid as long as the
> * corresponding Camera instance is valid. In order to borrow a
> @@ -213,9 +210,6 @@ private:
> V4L2VideoDevice *param_;
> V4L2VideoDevice *stat_;
>
> - BufferPool paramPool_;
> - BufferPool statPool_;
> -
> std::queue<Buffer *> paramBuffers_;
> std::queue<Buffer *> statBuffers_;
>
> @@ -508,8 +502,6 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> status = Adjusted;
> }
>
> - cfg.bufferCount = RKISP1_BUFFER_COUNT;
> -
> return status;
> }
>
> @@ -660,46 +652,40 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
> const std::set<Stream *> &streams)
> {
> RkISP1CameraData *data = cameraData(camera);
> - Stream *stream = *streams.begin();
> + std::vector<Buffer *> paramBuffers, statBuffers;
> int ret;
>
> - if (stream->memoryType() == InternalMemory)
> - ret = video_->exportBuffers(&stream->bufferPool());
> - else
> - ret = video_->importBuffers(&stream->bufferPool());
> -
> - if (ret)
> - return ret;
>
> - paramPool_.createBuffers(stream->configuration().bufferCount + 1);
> - ret = param_->exportBuffers(¶mPool_);
> - if (ret) {
> - video_->releaseBuffers();
> - return ret;
> - }
> + ret = param_->allocateBuffers(RKISP1_BUFFER_COUNT, ¶mBuffers);
> + if (ret < 0)
> + goto err;
>
> - statPool_.createBuffers(stream->configuration().bufferCount + 1);
> - ret = stat_->exportBuffers(&statPool_);
> - if (ret) {
> - param_->releaseBuffers();
> - video_->releaseBuffers();
> - return ret;
> - }
> + ret = stat_->allocateBuffers(RKISP1_BUFFER_COUNT, &statBuffers);
> + if (ret < 0)
> + goto err;
>
> - for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> - data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
> - .memory = paramPool_.buffers()[i] });
> - paramBuffers_.push(new Buffer(i));
> + for (Buffer *buffer : paramBuffers) {
> + data->ipaBuffers_.push_back({ .id = static_cast<unsigned int>(buffer->plane(0)->fd()),
> + .planes = buffer->description() });
> + paramBuffers_.push(buffer);
> }
>
> - for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> - data->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,
> - .memory = statPool_.buffers()[i] });
> - statBuffers_.push(new Buffer(i));
> + for (Buffer *buffer : statBuffers) {
> + data->ipaBuffers_.push_back({ .id = static_cast<unsigned int>(buffer->plane(0)->fd()),
Can we swap the id member to take an int, rather than an unsigned int to
prevent this casting? I like the idea of treaing the FD as the id for
each buffer, as it should uniquely identify each buffer plane right?
(assuming we still do a dup() for every plane)
Or we could be even more explicit and just call it 'fd' ...
> + .planes = buffer->description() });
> + statBuffers_.push(buffer);
> }
>
> data->ipa_->mapBuffers(data->ipaBuffers_);
>
> + return 0;
> +err:
> + for (Buffer *buffer : paramBuffers)
> + delete buffer;
> +
> + for (Buffer *buffer : statBuffers)
> + delete buffer;
> +
> return ret;
> }
>
> @@ -820,9 +806,11 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)
> if (!info)
> return -ENOENT;
>
> + unsigned int paramid = static_cast<unsigned int>(info->paramBuffer->plane(0)->fd());
> +
> IPAOperationData op;
> op.operation = RKISP1_IPA_EVENT_QUEUE_REQUEST;
> - op.data = { data->frame_, RKISP1_PARAM_BASE | info->paramBuffer->index() };
> + op.data = { data->frame_, paramid };
Does an fd have the same value once it crosses the IPC boundary ?
i.e. is the file descriptor shared as an identical value? or does it get
it's own in that process space, I expect it might change, thus calling
it an ID rather than an FD is probably a good thing.
> op.controls = { request->controls() };
> data->ipa_->processEvent(op);
>
> @@ -874,7 +862,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> std::unique_ptr<RkISP1CameraData> data =
> utils::make_unique<RkISP1CameraData>(this);
>
> - data->stream_ = new Stream();
> + data->stream_ = new V4L2Stream(video_, RKISP1_BUFFER_COUNT);
Interesting, I think I like this wrapping.
Will we hit any issues with RKISP1_BUFFER_COUNT being less than or
greater than the number of buffers used by the other components in the
pipeline? Or is it no longer possible for an application to determine
how many buffers are used.
> ControlInfoMap::Map ctrls;
> ctrls.emplace(std::piecewise_construct,
> @@ -982,9 +970,9 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
> if (!info->paramDequeued)
> return;
>
> - completeRequest(activeCamera_, request);
> -
> data->frameInfo_.destroy(info->frame);
> +
> + completeRequest(activeCamera_, request);
> }
>
> void PipelineHandlerRkISP1::bufferReady(Buffer *buffer, const BufferInfo &info)
> @@ -1023,7 +1011,7 @@ void PipelineHandlerRkISP1::statReady(Buffer *buffer, const BufferInfo &info)
> return;
>
> unsigned int frame = rkinfo->frame;
> - unsigned int statid = RKISP1_STAT_BASE | rkinfo->statBuffer->index();
> + unsigned int statid = static_cast<unsigned int>(rkinfo->statBuffer->plane(0)->fd());
I don't see an explicit reason to not use 'int' here, and remove the casts?
>
> IPAOperationData op;
> op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index ef2e8c9734f844ce..21e594075eefffdc 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -136,8 +136,6 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()
> status = Adjusted;
> }
>
> - cfg.bufferCount = 4;
> -
> return status;
> }
>
> @@ -161,7 +159,6 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
>
> cfg.pixelFormat = formats.pixelformats().front();
> cfg.size = formats.sizes(cfg.pixelFormat).back();
> - cfg.bufferCount = 4;
>
> config->addConfiguration(cfg);
>
> @@ -196,16 +193,7 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> int PipelineHandlerUVC::allocateBuffers(Camera *camera,
> const std::set<Stream *> &streams)
> {
> - UVCCameraData *data = cameraData(camera);
> - Stream *stream = *streams.begin();
> - const StreamConfiguration &cfg = stream->configuration();
> -
> - LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers";
> -
> - if (stream->memoryType() == InternalMemory)
> - return data->video_->exportBuffers(&stream->bufferPool());
> - else
> - return data->video_->importBuffers(&stream->bufferPool());
> + return 0;
> }
>
> int PipelineHandlerUVC::freeBuffers(Camera *camera,
> @@ -331,7 +319,7 @@ int UVCCameraData::init(MediaEntity *entity)
> if (ret)
> return ret;
>
> - stream_ = new Stream();
> + stream_ = new V4L2Stream(video_, 4);
I feel like '4' should be a parameter and come from somewhere else :-D
> video_->bufferReady.connect(this, &UVCCameraData::bufferReady);
>
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index e3eefc49135179f2..a14b7c8834174a5f 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -158,8 +158,6 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()
> status = Adjusted;
> }
>
> - cfg.bufferCount = 4;
> -
> return status;
> }
>
> @@ -190,7 +188,6 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
>
> cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> cfg.size = { 1920, 1080 };
> - cfg.bufferCount = 4;
>
> config->addConfiguration(cfg);
>
> @@ -263,16 +260,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> int PipelineHandlerVimc::allocateBuffers(Camera *camera,
> const std::set<Stream *> &streams)
> {
> - VimcCameraData *data = cameraData(camera);
> - Stream *stream = *streams.begin();
> - const StreamConfiguration &cfg = stream->configuration();
> -
> - LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers";
> -
> - if (stream->memoryType() == InternalMemory)
> - return data->video_->exportBuffers(&stream->bufferPool());
> - else
> - return data->video_->importBuffers(&stream->bufferPool());
> + return 0;
Now a no-op?
So are all buffers allocated at init() time?
> }
>
> int PipelineHandlerVimc::freeBuffers(Camera *camera,
> @@ -419,7 +407,7 @@ int VimcCameraData::init(MediaDevice *media)
> if (video_->open())
> return -ENODEV;
>
> - stream_ = new Stream();
> + stream_ = new V4L2Stream(video_, 4);
>
> video_->bufferReady.connect(this, &VimcCameraData::bufferReady);
>
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 9a0e439a3d05d780..2bda7a57e8d6381e 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -75,11 +75,6 @@ Request::Request(Camera *camera, uint64_t cookie)
>
> Request::~Request()
> {
> - for (auto it : bufferMap_) {
> - Buffer *buffer = it.second;
> - delete buffer;
> - }
> -
> delete metadata_;
> delete controls_;
> delete validator_;
> @@ -125,21 +120,15 @@ Request::~Request()
> * \retval -EEXIST The request already contains a buffer for the stream
> * \retval -EINVAL The buffer does not reference a valid Stream
> */
> -int Request::addBuffer(std::unique_ptr<Buffer> buffer)
> +int Request::addBuffer(Stream *stream, Buffer *buffer)
> {
> - Stream *stream = buffer->stream();
> - if (!stream) {
> - LOG(Request, Error) << "Invalid stream reference";
> - return -EINVAL;
> - }
> -
> auto it = bufferMap_.find(stream);
> if (it != bufferMap_.end()) {
> LOG(Request, Error) << "Buffer already set for stream";
> return -EEXIST;
> }
>
> - bufferMap_[stream] = buffer.release();
> + bufferMap_[stream] = buffer;
>
> return 0;
> }
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 97c6722b8c4c98cf..873452c9608fc9f1 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -19,6 +19,7 @@
>
> #include <libcamera/buffer.h>
> #include <libcamera/event_notifier.h>
> +#include <libcamera/stream.h>
>
> #include "log.h"
> #include "media_device.h"
> @@ -319,7 +320,7 @@ const std::string V4L2DeviceFormat::toString() const
> * \param[in] deviceNode The file-system path to the video device node
> */
> V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> - : V4L2Device(deviceNode), bufferPool_(nullptr), fdEvent_(nullptr)
> + : V4L2Device(deviceNode), cache_(nullptr), fdEvent_(nullptr)
> {
> /*
> * We default to an MMAP based CAPTURE video device, however this will
> @@ -879,27 +880,32 @@ int V4L2VideoDevice::requestBuffers(unsigned int count)
> }
>
> /**
> - * \brief Request buffers to be allocated from the video device and stored in
> - * the buffer pool provided.
> - * \param[out] pool BufferPool to populate with buffers
> + * \brief Operate using buffers allocated from local video device
> + * \param[in] count Number of buffers to allocate
> + * \param[out] buffer Vector to store local buffers
> * \return 0 on success or a negative error code otherwise
> */
> -int V4L2VideoDevice::exportBuffers(BufferPool *pool)
> +int V4L2VideoDevice::allocateBuffers(unsigned int count, std::vector<Buffer *> *buffers)
> {
> unsigned int i;
> int ret;
>
> + if (cache_) {
> + LOG(V4L2, Error) << "Can't export buffers with existing cache";
s/export/allocate/ ?
> + return -EINVAL;
> + }
> +
> + cache_ = new V4L2BufferCache(count);
> +
> memoryType_ = V4L2_MEMORY_MMAP;
>
> - ret = requestBuffers(pool->count());
> - if (ret)
> - return ret;
> + ret = requestBuffers(count);
> + if (ret < 0)
> + goto err_cache;
>
> - /* Map the buffers. */
> - for (i = 0; i < pool->count(); ++i) {
> - struct v4l2_plane planes[VIDEO_MAX_PLANES] = {};
> + for (i = 0; i < count; ++i) {
> struct v4l2_buffer buf = {};
> - BufferMemory &buffer = pool->buffers()[i];
> + struct v4l2_plane planes[VIDEO_MAX_PLANES] = {};
>
> buf.index = i;
> buf.type = bufferType_;
> @@ -912,51 +918,71 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)
> LOG(V4L2, Error)
> << "Unable to query buffer " << i << ": "
> << strerror(-ret);
> - break;
> + goto err_buf;
> }
>
> - if (V4L2_TYPE_IS_MULTIPLANAR(buf.type)) {
> - for (unsigned int p = 0; p < buf.length; ++p) {
> - ret = createPlane(&buffer, i, p,
> - buf.m.planes[p].length);
> - if (ret)
> - break;
> - }
> - } else {
> - ret = createPlane(&buffer, i, 0, buf.length);
> + Buffer *buffer = createBuffer(buf);
Good, I'm glad that part is wrapped up neater.
> + if (!buffer) {
> + LOG(V4L2, Error) << "Unable to create buffer";
> + ret = -EINVAL;
> + goto err_buf;
> }
>
> - if (ret) {
> - LOG(V4L2, Error) << "Failed to create plane";
> - break;
> - }
> + buffers->push_back(buffer);
> + cache_->populate(i, buffer->fds());
> }
>
> - if (ret) {
> - requestBuffers(0);
> - pool->destroyBuffers();
> - return ret;
> + return count;
> +err_buf:
> + requestBuffers(0);
> +
> + for (Buffer *buffer : *buffers)
> + delete buffer;
> +
> + buffers->clear();
> +err_cache:
> + delete cache_;
> + cache_ = nullptr;
> +
> + return ret;
> +}
> +
> +Buffer *V4L2VideoDevice::createBuffer(struct v4l2_buffer buf)
> +{
> + const unsigned int numPlanes = V4L2_TYPE_IS_MULTIPLANAR(buf.type) ? buf.length : 1;
> + std::vector<std::pair<int, unsigned int>> planes;
> + Buffer *frame = nullptr;
> +
> + for (unsigned int plane = 0; plane < numPlanes; plane++) {
> + int fd = expbuf(buf.index, plane);
> + if (fd < 0)
> + goto out;
> +
> + if (V4L2_TYPE_IS_MULTIPLANAR(buf.type))
> + planes.emplace_back(fd, buf.m.planes[plane].length);
> + else
> + planes.emplace_back(fd, buf.length);
> }
>
> - bufferPool_ = pool;
> + if (!planes.empty())
> + frame = new Buffer(planes);
> + else
> + LOG(V4L2, Error) << "Failed to get planes";
> +out:
> + for (std::pair<int, unsigned int> plane : planes)
> + ::close(plane.first);
"Close the plane first?" Before something else?
Or perhaps "Only close the first plane?"
(Sorry, intentionally mis-reading horrible ambiguous std::pair syntax)
> - return 0;
> + return frame;
> }
>
> -int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index,
> - unsigned int planeIndex, unsigned int length)
> +int V4L2VideoDevice::expbuf(unsigned int index, unsigned int plane)
s/expbuf(/exportBuffer(/
Or if you feel 'Buffer' is ambigious against our 'Buffer' object,
s/expbuf(/exportV4L2Buffer(/
or actually :
s/expbuf(/exportDMABuffer(/
> {
> struct v4l2_exportbuffer expbuf = {};
> int ret;
>
> - LOG(V4L2, Debug)
> - << "Buffer " << index
> - << " plane " << planeIndex
> - << ": length=" << length;
> -
> expbuf.type = bufferType_;
> expbuf.index = index;
> - expbuf.plane = planeIndex;
> + expbuf.plane = plane;
> expbuf.flags = O_RDWR;
>
> ret = ioctl(VIDIOC_EXPBUF, &expbuf);
> @@ -966,31 +992,31 @@ int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index,
> return ret;
> }
>
> - buffer->planes().emplace_back();
> - Plane &plane = buffer->planes().back();
> - plane.setDmabuf(expbuf.fd, length);
> - ::close(expbuf.fd);
> -
> - return 0;
> + return expbuf.fd;
> }
>
> /**
> - * \brief Import the externally allocated \a pool of buffers
> - * \param[in] pool BufferPool of buffers to import
> + * \brief Operate using buffers imported from external source
> + * \param[in] count Number of buffers to prepare for
> * \return 0 on success or a negative error code otherwise
> */
> -int V4L2VideoDevice::importBuffers(BufferPool *pool)
> +int V4L2VideoDevice::importBuffers(unsigned int count)
> {
> + /* Only prepare for external buffers if internals not in use. */
/*
* We can only prepare the device to use external buffers when no
* internal buffers have been allocated.
*/
I'm almost wondering if this might happen automatically at stream-on
time if no internal buffer allocation was created... but I think that's
just a future V4L2VideoDevice optimisation.
> + if (cache_)
> + return 0;
Is it an error to try to import buffers when something has already been
imported or allocated ?
> +
> int ret;
>
> memoryType_ = V4L2_MEMORY_DMABUF;
>
> - ret = requestBuffers(pool->count());
> + ret = requestBuffers(count);
> if (ret)
> return ret;
>
> - LOG(V4L2, Debug) << "provided pool of " << pool->count() << " buffers";
> - bufferPool_ = pool;
> + cache_ = new V4L2BufferCache(count);
> +
> + LOG(V4L2, Debug) << "provided for " << count << " external buffers";
>
> return 0;
> }
> @@ -1000,9 +1026,10 @@ int V4L2VideoDevice::importBuffers(BufferPool *pool)
> */
> int V4L2VideoDevice::releaseBuffers()
> {
> - LOG(V4L2, Debug) << "Releasing bufferPool";
> + LOG(V4L2, Debug) << "Releasing buffers";
>
> - bufferPool_ = nullptr;
> + delete cache_;
> + cache_ = nullptr;
>
> return requestBuffers(0);
> }
> @@ -1018,40 +1045,42 @@ int V4L2VideoDevice::releaseBuffers()
> *
> * \return 0 on success or a negative error code otherwise
> */
> -int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> +int V4L2VideoDevice::queueBuffer(Buffer *buffer, const BufferInfo *info)
> {
> - struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
> struct v4l2_buffer buf = {};
> + struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
> int ret;
>
> - buf.index = buffer->index();
> + ASSERT(cache_);
Maybe this?
LOG(V4L2, Fatal) << "No buffer cache available.";
> +
> + buf.index = cache_->pop(buffer->fds());
pop implies that the buffer is removed from the cache object?
(I'll find the cache implementation soon I guess?)
> buf.type = bufferType_;
> buf.memory = memoryType_;
> buf.field = V4L2_FIELD_NONE;
>
> - bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
> - BufferMemory *mem = &bufferPool_->buffers()[buf.index];
> - const std::vector<Plane> &planes = mem->planes();
> + if (V4L2_TYPE_IS_MULTIPLANAR(buf.type)) {
> + buf.length = buffer->numPlanes();
> + buf.m.planes = v4l2Planes;
> + }
>
> if (buf.memory == V4L2_MEMORY_DMABUF) {
Interesting, do we actually support non DMABUF any more?
Does your buffer-cache work without dmabuf?
(I bet we don't have a test anywhere for this :-D)
> - if (multiPlanar) {
> - for (unsigned int p = 0; p < planes.size(); ++p)
> - v4l2Planes[p].m.fd = planes[p].dmabuf();
> - } else {
> - buf.m.fd = planes[0].dmabuf();
> - }
> + if (V4L2_TYPE_IS_MULTIPLANAR(buf.type))
> + for (unsigned int i = 0; i < buffer->numPlanes(); i++)
> + v4l2Planes[i].m.fd = buffer->plane(i)->fd();
> + else
> + buf.m.fd = buffer->plane(0)->fd();
> }
>
> - if (multiPlanar) {
> - buf.length = planes.size();
> - buf.m.planes = v4l2Planes;
> - }
> + if (V4L2_TYPE_IS_OUTPUT(bufferType_) && info) {
> + if (V4L2_TYPE_IS_MULTIPLANAR(buf.type))
V4L2_TYPE_IS_OUTPUT(bufferType_) yet, V4L2_TYPE_IS_MULTIPLANAR(buf.type)?
This is why I wanted to unify the multiPlanar_ variable in the same way
as the bufferType_ has been.
I don't think we can support it changing between API's on a per-buffer
basis can we ?
> + for (unsigned int i = 0; i < buffer->numPlanes(); i++)
> + v4l2Planes[i].bytesused = info->plane(i).bytesused;
> + else
> + buf.bytesused = info->plane(0).bytesused;
>
> - if (V4L2_TYPE_IS_OUTPUT(bufferType_)) {
> - buf.bytesused = buffer->bytesused_;
> - buf.sequence = buffer->sequence_;
> - buf.timestamp.tv_sec = buffer->timestamp_ / 1000000000;
> - buf.timestamp.tv_usec = (buffer->timestamp_ / 1000) % 1000000;
> + buf.sequence = info->sequence();
> + buf.timestamp.tv_sec = info->timestamp() / 1000000000;
> + buf.timestamp.tv_usec = (info->timestamp() / 1000) % 1000000;
> }
>
> LOG(V4L2, Debug) << "Queueing buffer " << buf.index;
> @@ -1072,52 +1101,6 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> return 0;
> }
>
> -/**
> - * \brief Queue all buffers into the video device
> - *
> - * When starting video capture users of the video device often need to queue
> - * all allocated buffers to the device. This helper method simplifies the
> - * implementation of the user by queuing all buffers and returning a vector of
> - * Buffer instances for each queued buffer.
> - *
> - * This method is meant to be used with video capture devices internal to a
> - * pipeline handler, such as ISP statistics capture devices, or raw CSI-2
> - * receivers. For video capture devices facing applications, buffers shall
> - * instead be queued when requests are received, and for video output devices,
> - * buffers shall be queued when frames are ready to be output.
> - *
> - * The caller shall ensure that the returned buffers vector remains valid until
> - * all the queued buffers are dequeued, either during capture, or by stopping
> - * the video device.
> - *
> - * Calling this method on an output device or on a device that has buffers
> - * already queued is an error and will return an empty vector.
> - *
> - * \return A vector of queued buffers, which will be empty if an error occurs
> - */
> -std::vector<std::unique_ptr<Buffer>> V4L2VideoDevice::queueAllBuffers()
> -{
> - int ret;
> -
> - if (!queuedBuffers_.empty())
> - return {};
> -
> - if (V4L2_TYPE_IS_OUTPUT(bufferType_))
> - return {};
> -
> - std::vector<std::unique_ptr<Buffer>> buffers;
> -
> - for (unsigned int i = 0; i < bufferPool_->count(); ++i) {
> - Buffer *buffer = new Buffer(i);
> - buffers.emplace_back(buffer);
> - ret = queueBuffer(buffer);
> - if (ret)
> - return {};
> - }
> -
> - return buffers;
> -}
> -
> /**
> * \brief Slot to handle completed buffer events from the V4L2 video device
> * \param[in] notifier The event notifier
> @@ -1149,6 +1132,7 @@ void V4L2VideoDevice::bufferAvailable(EventNotifier *notifier)
> return;
> }
>
> + cache_->push(buf.index);
A new line would be nice here, I don't think the cache replinishment
should hug this debug line. Or the cache replenish should go below it ...
> LOG(V4L2, Debug) << "Buffer " << buf.index << " is available";
>
> auto it = queuedBuffers_.find(buf.index);
> @@ -1164,7 +1148,6 @@ void V4L2VideoDevice::bufferAvailable(EventNotifier *notifier)
> + buf.timestamp.tv_usec * 1000ULL;
>
> BufferInfo info(status, buf.sequence, timestamp, { { buf.bytesused } });
> - buffer->index_ = buf.index;
>
> /* Notify anyone listening to the device. */
> bufferReady.emit(buffer, info);
> @@ -1216,13 +1199,10 @@ int V4L2VideoDevice::streamOff()
>
> /* Send back all queued buffers. */
> for (auto it : queuedBuffers_) {
> - unsigned int index = it.first;
> Buffer *buffer = it.second;
>
> BufferInfo info(BufferInfo::BufferCancelled, 0, 0, { {} });
>
> - buffer->index_ = index;
> - buffer->cancel();
> bufferReady.emit(buffer, info);
> }
>
> @@ -1353,4 +1333,41 @@ void V4L2M2MDevice::close()
> output_->close();
> }
>
> +V4L2Stream::V4L2Stream(V4L2VideoDevice *video, unsigned int bufferCount)
> + : Stream(), video_(video), bufferCount_(bufferCount),
> + allocated(false)
> +{
> +}
> +
> +int V4L2Stream::allocateBuffers(std::vector<Buffer *> *buffers)
> +{
> + if (buffers) {
> + if (allocated)
> + return -EINVAL;
> +
> + allocated = true;
> + return video_->allocateBuffers(bufferCount_, buffers);
> + } else {
> + if (!allocated)
> + return -EINVAL;
> +
> + return video_->releaseBuffers();
> + }
> +}
> +
> +int V4L2Stream::importBuffers(bool enable)
importBuffers(enable) sounds like mixing API's here
> +{
> + /*
> + * No need to prepare for buffer importing if internal buffers
> + * are allocated.
> + */
> + if (allocated)
> + return 0;
> +
> + if (enable)
> + return video_->importBuffers(bufferCount_);
> + else
> + return video_->releaseBuffers();
I feel like this should be two separate functions with clear naming if
it's really needed ...
> +}
> +
> } /* namespace libcamera */
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 2d7f4ba84fbb6838..d17aec42c82bbd98 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -22,7 +22,7 @@
> using namespace libcamera;
>
> MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> - : options_(options), isCapturing_(false)
> + : options_(options), allocator_(nullptr), isCapturing_(false)
> {
> int ret;
>
> @@ -36,8 +36,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> adjustSize();
>
> ret = openCamera(cm);
> - if (!ret)
> + if (!ret) {
> + allocator_ = new BufferAllocator(camera_);
> ret = startCapture();
> + }
>
> if (ret < 0)
> QTimer::singleShot(0, QCoreApplication::instance(),
> @@ -51,6 +53,8 @@ MainWindow::~MainWindow()
> camera_->release();
> camera_.reset();
> }
> +
> + delete allocator_;
> }
>
> void MainWindow::updateTitle()
> @@ -175,22 +179,24 @@ int MainWindow::startCapture()
> return ret;
> }
>
> + ret = allocator_->allocate(stream);
> + if (ret < 0) {
> + std::cerr << "Failed to allocate internal buffers" << std::endl;
> + return ret;
> + }
> + unsigned int nbuffers = static_cast<unsigned int>(ret);
how about:
unsigned int nbuffers = allocator_->count(stream) ?
(or such)
> +
> std::vector<Request *> requests;
> - for (unsigned int i = 0; i < cfg.bufferCount; ++i) {
> + for (unsigned int i = 0; i < nbuffers; ++i) {
> Request *request = camera_->createRequest();
> - if (!request) {
> + Buffer *buffer = allocator_->get(stream)[i];
> + if (!request || !buffer) {
> std::cerr << "Can't create request" << std::endl;
This is two errors mixed into one ...
I think it would be better to explicitly report if there is !buffer, but
I wonder if this could be better designed not to happen.
Can we iterate around the Buffers created by the allocator directly?
> ret = -ENOMEM;
> goto error;
> }
>
> - std::unique_ptr<Buffer> buffer = stream->createBuffer(i);
> - if (!buffer) {
> - std::cerr << "Can't create buffer " << i << std::endl;
> - goto error;
> - }
> -
> - ret = request->addBuffer(std::move(buffer));
> + ret = request->addBuffer(stream, buffer);
> if (ret < 0) {
> std::cerr << "Can't set buffer for request" << std::endl;
> goto error;
> @@ -281,15 +287,8 @@ void MainWindow::requestComplete(Request *request)
> for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> Stream *stream = it->first;
> Buffer *buffer = it->second;
> - unsigned int index = buffer->index();
> -
> - std::unique_ptr<Buffer> newBuffer = stream->createBuffer(index);
> - if (!newBuffer) {
> - std::cerr << "Can't create buffer " << index << std::endl;
> - return;
> - }
>
> - request->addBuffer(std::move(newBuffer));
> + request->addBuffer(stream, buffer);
> }
>
> camera_->queueRequest(request);
> @@ -297,12 +296,10 @@ void MainWindow::requestComplete(Request *request)
>
> int MainWindow::display(Buffer *buffer, const BufferInfo &info)
> {
> - BufferMemory *mem = buffer->mem();
> - if (mem->planes().size() != 1)
> + if (buffer->numPlanes() != 1)
> return -EINVAL;
>
> - Plane &plane = mem->planes().front();
> - unsigned char *raw = static_cast<unsigned char *>(plane.mem());
> + unsigned char *raw = static_cast<unsigned char *>(buffer->plane(0)->mem());
> viewfinder_->display(raw, info.plane(0).bytesused);
>
> return 0;
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 8cbd72eb0d63cbea..eeb28eaae0dfc135 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -15,6 +15,7 @@
> #include <QObject>
> #include <QTimer>
>
> +#include <libcamera/buffer.h>
> #include <libcamera/camera.h>
> #include <libcamera/camera_manager.h>
> #include <libcamera/stream.h>
> @@ -58,6 +59,8 @@ private:
> const OptionsParser::Options &options_;
>
> std::shared_ptr<Camera> camera_;
> + BufferAllocator *allocator_;
> +
> bool isCapturing_;
> std::unique_ptr<CameraConfiguration> config_;
>
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index 3cf5b798448d01db..8d3d612222a3b21e 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -41,10 +41,9 @@ protected:
> /* Create a new request. */
> Stream *stream = buffers.begin()->first;
> Buffer *buffer = buffers.begin()->second;
> - std::unique_ptr<Buffer> newBuffer = stream->createBuffer(buffer->index());
>
> request = camera_->createRequest();
> - request->addBuffer(std::move(newBuffer));
> + request->addBuffer(stream, buffer);
> camera_->queueRequest(request);
> }
>
> @@ -84,21 +83,25 @@ protected:
> }
>
> Stream *stream = cfg.stream();
> +
> + BufferAllocator allocator(camera_);
> + int ret = allocator.allocate(stream);
> + if (ret < 0)
> + return TestFail;
> +
> + unsigned int nbuffers = static_cast<unsigned int>(ret);
As before, it would be nice to have a call to get the count directly
without having to cast a return value here,
> +
> std::vector<Request *> requests;
> - for (unsigned int i = 0; i < cfg.bufferCount; ++i) {
> + for (unsigned int i = 0; i < nbuffers; ++i) {
And the count might not even be needed if instead we just iterate
directly on the buffers...
> Request *request = camera_->createRequest();
> - if (!request) {
> - cout << "Failed to create request" << endl;
> - return TestFail;
> - }
> + Buffer *buffer = allocator.get(stream)[i];
>
> - std::unique_ptr<Buffer> buffer = stream->createBuffer(i);
> - if (!buffer) {
> - cout << "Failed to create buffer " << i << endl;
> + if (!request || !buffer) {
> + cout << "Failed to create request" << endl;
> return TestFail;
> }
>
> - if (request->addBuffer(std::move(buffer))) {
> + if (request->addBuffer(stream, buffer)) {
> cout << "Failed to associating buffer with request" << endl;
> return TestFail;
> }
> @@ -131,10 +134,10 @@ protected:
> while (timer.isRunning())
> dispatcher->processEvents();
>
> - if (completeRequestsCount_ <= cfg.bufferCount * 2) {
> + if (completeRequestsCount_ <= nbuffers * 2) {
> cout << "Failed to capture enough frames (got "
> << completeRequestsCount_ << " expected at least "
> - << cfg.bufferCount * 2 << ")" << endl;
> + << nbuffers * 2 << ")" << endl;
> return TestFail;
> }
>
> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> index 12d5e0e1d5343f28..a77dec0f8b986634 100644
> --- a/test/camera/statemachine.cpp
> +++ b/test/camera/statemachine.cpp
> @@ -178,6 +178,12 @@ protected:
> if (camera_->allocateBuffers())
> return TestFail;
>
> + /* Use internally allocated buffers. */
> + allocator_ = new BufferAllocator(camera_);
> + Stream *stream = *camera_->streams().begin();
> + if (allocator_->allocate(stream) < 0)
> + return TestFail;
> +
> if (camera_->start())
> return TestFail;
>
> @@ -211,8 +217,7 @@ protected:
> return TestFail;
>
> Stream *stream = *camera_->streams().begin();
> - std::unique_ptr<Buffer> buffer = stream->createBuffer(0);
> - if (request->addBuffer(std::move(buffer)))
> + if (request->addBuffer(stream, allocator_->get(stream)[0]))
> return TestFail;
>
> if (camera_->queueRequest(request))
> @@ -222,6 +227,8 @@ protected:
> if (camera_->stop())
> return TestFail;
>
> + delete allocator_;
> +
> if (camera_->freeBuffers())
> return TestFail;
>
> @@ -278,6 +285,7 @@ protected:
> }
>
> std::unique_ptr<CameraConfiguration> defconf_;
> + BufferAllocator *allocator_;
> };
>
> } /* namespace */
> diff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp
> index 3609c331cf10e056..f0c4ace2f47cc58b 100644
> --- a/test/v4l2_videodevice/buffer_sharing.cpp
> +++ b/test/v4l2_videodevice/buffer_sharing.cpp
> @@ -73,16 +73,14 @@ protected:
> return TestFail;
> }
>
> - pool_.createBuffers(bufferCount);
> -
> - ret = capture_->exportBuffers(&pool_);
> - if (ret) {
> - std::cout << "Failed to export buffers" << std::endl;
> + ret = capture_->allocateBuffers(bufferCount, &buffers_);
> + if (ret < 0) {
> + std::cout << "Failed to allocate buffers" << std::endl;
> return TestFail;
> }
>
> - ret = output_->importBuffers(&pool_);
> - if (ret) {
> + ret = output_->importBuffers(bufferCount);
> + if (ret < 0) {
> std::cout << "Failed to import buffers" << std::endl;
> return TestFail;
> }
> @@ -98,8 +96,7 @@ protected:
> if (info.status() != BufferInfo::BufferSuccess)
> return;
>
> - BufferInfo infocopy = info;
> - output_->queueBuffer(buffer);
> + output_->queueBuffer(buffer, &info);
> framesCaptured_++;
> }
>
> @@ -124,10 +121,8 @@ protected:
> capture_->bufferReady.connect(this, &BufferSharingTest::captureBufferReady);
> output_->bufferReady.connect(this, &BufferSharingTest::outputBufferReady);
>
> - std::vector<std::unique_ptr<Buffer>> buffers;
> - buffers = capture_->queueAllBuffers();
> - if (buffers.empty())
> - return TestFail;
> + for (Buffer *buffer : buffers_)
> + capture_->queueBuffer(buffer);
>
> ret = capture_->streamOn();
> if (ret) {
> diff --git a/test/v4l2_videodevice/capture_async.cpp b/test/v4l2_videodevice/capture_async.cpp
> index 26ea1d17fd901ef8..a12c968f36e937ab 100644
> --- a/test/v4l2_videodevice/capture_async.cpp
> +++ b/test/v4l2_videodevice/capture_async.cpp
> @@ -38,18 +38,14 @@ protected:
> Timer timeout;
> int ret;
>
> - pool_.createBuffers(bufferCount);
> -
> - ret = capture_->exportBuffers(&pool_);
> - if (ret)
> + ret = capture_->allocateBuffers(bufferCount, &buffers_);
> + if (ret < 0)
> return TestFail;
>
> capture_->bufferReady.connect(this, &CaptureAsyncTest::receiveBuffer);
>
> - std::vector<std::unique_ptr<Buffer>> buffers;
> - buffers = capture_->queueAllBuffers();
> - if (buffers.empty())
> - return TestFail;
> + for (Buffer *buffer : buffers_)
> + capture_->queueBuffer(buffer);
>
> ret = capture_->streamOn();
> if (ret)
> diff --git a/test/v4l2_videodevice/request_buffers.cpp b/test/v4l2_videodevice/request_buffers.cpp
> index c4aedf7b3cd61e80..2f8dfe1cafb111df 100644
> --- a/test/v4l2_videodevice/request_buffers.cpp
> +++ b/test/v4l2_videodevice/request_buffers.cpp
> @@ -16,17 +16,10 @@ public:
> protected:
> int run()
> {
> - /*
> - * TODO:
> - * Test invalid requests
> - * Test different buffer allocations
> - */
> const unsigned int bufferCount = 8;
>
> - pool_.createBuffers(bufferCount);
> -
> - int ret = capture_->exportBuffers(&pool_);
> - if (ret)
> + int ret = capture_->allocateBuffers(bufferCount, &buffers_);
> + if (ret != bufferCount)
> return TestFail;
>
> return TestPass;
> diff --git a/test/v4l2_videodevice/stream_on_off.cpp b/test/v4l2_videodevice/stream_on_off.cpp
> index 7664adc4c1f07046..ce48310aa2b7c3a8 100644
> --- a/test/v4l2_videodevice/stream_on_off.cpp
> +++ b/test/v4l2_videodevice/stream_on_off.cpp
> @@ -17,10 +17,8 @@ protected:
> {
> const unsigned int bufferCount = 8;
>
> - pool_.createBuffers(bufferCount);
> -
> - int ret = capture_->exportBuffers(&pool_);
> - if (ret)
> + int ret = capture_->allocateBuffers(bufferCount, &buffers_);
> + if (ret < 0)
> return TestFail;
>
> ret = capture_->streamOn();
> diff --git a/test/v4l2_videodevice/v4l2_m2mdevice.cpp b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> index e6ca90a4604dfcda..def8d82972d754f9 100644
> --- a/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> +++ b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> @@ -112,17 +112,14 @@ protected:
> return TestFail;
> }
>
> - capturePool_.createBuffers(bufferCount);
> - outputPool_.createBuffers(bufferCount);
> -
> - ret = capture->exportBuffers(&capturePool_);
> - if (ret) {
> + ret = capture->allocateBuffers(bufferCount, &captureBuffers_);
> + if (ret < 0) {
> cerr << "Failed to export Capture Buffers" << endl;
> return TestFail;
> }
>
> - ret = output->exportBuffers(&outputPool_);
> - if (ret) {
> + ret = output->allocateBuffers(bufferCount, &outputBuffers_);
> + if (ret < 0) {
> cerr << "Failed to export Output Buffers" << endl;
> return TestFail;
> }
> @@ -130,24 +127,11 @@ protected:
> capture->bufferReady.connect(this, &V4L2M2MDeviceTest::receiveCaptureBuffer);
> output->bufferReady.connect(this, &V4L2M2MDeviceTest::outputBufferComplete);
>
> - std::vector<std::unique_ptr<Buffer>> captureBuffers;
> - captureBuffers = capture->queueAllBuffers();
> - if (captureBuffers.empty()) {
> - cerr << "Failed to queue all Capture Buffers" << endl;
> - return TestFail;
> - }
> + for (Buffer *buffer : captureBuffers_)
> + capture->queueBuffer(buffer);
>
> - /* We can't "queueAllBuffers()" on an output device, so we do it manually */
> - std::vector<std::unique_ptr<Buffer>> outputBuffers;
> - for (unsigned int i = 0; i < outputPool_.count(); ++i) {
> - Buffer *buffer = new Buffer(i);
> - outputBuffers.emplace_back(buffer);
> - ret = output->queueBuffer(buffer);
> - if (ret) {
> - cerr << "Failed to queue output buffer" << i << endl;
> - return TestFail;
> - }
> - }
> + for (Buffer *buffer : outputBuffers_)
> + output->queueBuffer(buffer);
>
> ret = capture->streamOn();
> if (ret) {
> @@ -202,8 +186,8 @@ private:
> std::shared_ptr<MediaDevice> media_;
> V4L2M2MDevice *vim2m_;
>
> - BufferPool capturePool_;
> - BufferPool outputPool_;
> + std::vector<Buffer *> captureBuffers_;
> + std::vector<Buffer *> outputBuffers_;
>
> unsigned int outputFrames_;
> unsigned int captureFrames_;
> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.h b/test/v4l2_videodevice/v4l2_videodevice_test.h
> index 34dd231c6d9d108d..1b6f4b862fbae020 100644
> --- a/test/v4l2_videodevice/v4l2_videodevice_test.h
> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.h
> @@ -41,7 +41,7 @@ protected:
> CameraSensor *sensor_;
> V4L2Subdevice *debayer_;
> V4L2VideoDevice *capture_;
> - BufferPool pool_;
> + std::vector<Buffer *> buffers_;
> };
>
> #endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list