[libcamera-devel,27/30] libcamera: Switch to FrameBuffer interface

2375 diff mbox series

Switch to the FrameBuffer interface where all buffers are treated as
external buffers and are allocated outside the camera. Applications
allocating buffers using libcamera are switched to use the
BufferAllocator helper.

A follow up changes to this one will finalize the transition to the new
FrameBuffer interface by removing code that is left unused after this
change.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/camera.h                    |   4 +-
 include/libcamera/request.h                   |  14 +-
 src/android/camera_device.cpp                 |  19 +-
 src/cam/buffer_writer.cpp                     |   9 +-
 src/cam/buffer_writer.h                       |   3 +-
 src/cam/capture.cpp                           |  38 +-
 src/cam/capture.h                             |   4 +-
 src/libcamera/camera.cpp                      |  34 --
 src/libcamera/include/pipeline_handler.h      |   5 +-
 src/libcamera/include/v4l2_videodevice.h      |   9 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp          | 223 +++-------
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 120 +++--
 src/libcamera/pipeline/uvcvideo.cpp           |  20 +-
 src/libcamera/pipeline/vimc.cpp               |  20 +-
 src/libcamera/pipeline_handler.cpp            |   2 +-
 src/libcamera/request.cpp                     |  16 +-
 src/libcamera/v4l2_videodevice.cpp            |  80 +---
 src/qcam/main_window.cpp                      |  47 +-
 src/qcam/main_window.h                        |   6 +-
 test/camera/buffer_import.cpp                 | 415 +++++-------------
 test/camera/capture.cpp                       |  31 +-
 test/camera/statemachine.cpp                  |  12 +-
 test/v4l2_videodevice/buffer_sharing.cpp      |  23 +-
 test/v4l2_videodevice/capture_async.cpp       |  14 +-
 test/v4l2_videodevice/request_buffers.cpp     |  11 +-
 test/v4l2_videodevice/stream_on_off.cpp       |   6 +-
 test/v4l2_videodevice/v4l2_m2mdevice.cpp      |  40 +-
 test/v4l2_videodevice/v4l2_videodevice_test.h |   2 +-
 28 files changed, 415 insertions(+), 812 deletions(-)

Hi Niklas,

Thank you for the patch.

On Wed, Nov 27, 2019 at 12:36:17AM +0100, Niklas Söderlund wrote:
> Switch to the FrameBuffer interface where all buffers are treated as
> external buffers and are allocated outside the camera. Applications
> allocating buffers using libcamera are switched to use the
> BufferAllocator helper.
> 
> A follow up changes to this one will finalize the transition to the new
> FrameBuffer interface by removing code that is left unused after this
> change.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/camera.h                    |   4 +-
>  include/libcamera/request.h                   |  14 +-
>  src/android/camera_device.cpp                 |  19 +-
>  src/cam/buffer_writer.cpp                     |   9 +-
>  src/cam/buffer_writer.h                       |   3 +-
>  src/cam/capture.cpp                           |  38 +-
>  src/cam/capture.h                             |   4 +-
>  src/libcamera/camera.cpp                      |  34 --
>  src/libcamera/include/pipeline_handler.h      |   5 +-
>  src/libcamera/include/v4l2_videodevice.h      |   9 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp          | 223 +++-------
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 120 +++--
>  src/libcamera/pipeline/uvcvideo.cpp           |  20 +-
>  src/libcamera/pipeline/vimc.cpp               |  20 +-
>  src/libcamera/pipeline_handler.cpp            |   2 +-
>  src/libcamera/request.cpp                     |  16 +-
>  src/libcamera/v4l2_videodevice.cpp            |  80 +---
>  src/qcam/main_window.cpp                      |  47 +-
>  src/qcam/main_window.h                        |   6 +-
>  test/camera/buffer_import.cpp                 | 415 +++++-------------
>  test/camera/capture.cpp                       |  31 +-
>  test/camera/statemachine.cpp                  |  12 +-
>  test/v4l2_videodevice/buffer_sharing.cpp      |  23 +-
>  test/v4l2_videodevice/capture_async.cpp       |  14 +-
>  test/v4l2_videodevice/request_buffers.cpp     |  11 +-
>  test/v4l2_videodevice/stream_on_off.cpp       |   6 +-
>  test/v4l2_videodevice/v4l2_m2mdevice.cpp      |  40 +-
>  test/v4l2_videodevice/v4l2_videodevice_test.h |   2 +-
>  28 files changed, 415 insertions(+), 812 deletions(-)

I like the diffstat.

> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index ef6a37bb142c83a6..02d047b9649f53d0 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -19,7 +19,7 @@
>  
>  namespace libcamera {
>  
> -class Buffer;
> +class FrameBuffer;
>  class PipelineHandler;
>  class Request;
>  
> @@ -77,7 +77,7 @@ public:
>  
>  	const std::string &name() const;
>  
> -	Signal<Request *, Buffer *> bufferCompleted;
> +	Signal<Request *, FrameBuffer *> bufferCompleted;
>  	Signal<Request *> requestCompleted;
>  	Signal<Camera *> disconnected;
>  
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index a8708010ec1247a2..02c0ef5f6b028bd4 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -17,9 +17,9 @@
>  
>  namespace libcamera {
>  
> -class Buffer;
>  class Camera;
>  class CameraControlValidator;
> +class FrameBuffer;
>  class Stream;
>  
>  class Request
> @@ -38,9 +38,9 @@ public:
>  
>  	ControlList &controls() { return *controls_; }
>  	ControlList &metadata() { return *metadata_; }
> -	const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }
> -	int addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer);
> -	Buffer *findBuffer(Stream *stream) const;
> +	const std::map<Stream *, FrameBuffer *> &buffers() const { return bufferMap_; }
> +	int addBuffer(Stream *stream, FrameBuffer *buffer);
> +	FrameBuffer *findBuffer(Stream *stream) const;
>  
>  	uint64_t cookie() const { return cookie_; }
>  	Status status() const { return status_; }
> @@ -54,14 +54,14 @@ private:
>  	int prepare();
>  	void complete();
>  
> -	bool completeBuffer(Buffer *buffer);
> +	bool completeBuffer(FrameBuffer *buffer);
>  
>  	Camera *camera_;
>  	CameraControlValidator *validator_;
>  	ControlList *controls_;
>  	ControlList *metadata_;
> -	std::map<Stream *, Buffer *> bufferMap_;
> -	std::unordered_set<Buffer *> pending_;
> +	std::map<Stream *, FrameBuffer *> bufferMap_;
> +	std::unordered_set<FrameBuffer *> pending_;
>  
>  	const uint64_t cookie_;
>  	Status status_;
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 55b29a9a41ab8943..708cf95b0c3c25c2 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -739,13 +739,13 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque
>  	 * and (currently) only supported request buffer.
>  	 */
>  	const buffer_handle_t camera3Handle = *camera3Buffers[0].buffer;
> -	std::array<int, 3> fds = {
> -		camera3Handle->data[0],
> -		camera3Handle->data[1],
> -		camera3Handle->data[2],
> -	};
>  
> -	std::unique_ptr<Buffer> buffer = stream->createBuffer(fds);
> +	std::vector<FrameBuffer::Plane> planes;
> +	planes.push_back({ .fd = camera3Handle->data[0], .length = 0 });
> +	planes.push_back({ .fd = camera3Handle->data[1], .length = 0 });
> +	planes.push_back({ .fd = camera3Handle->data[2], .length = 0 });

OUCH. .length = 0 clearly shows a problem. We don't necessarily need to
fix it as part of this series, but we need a strategy. Either we can get
the camera HAL to supply the length (casting the buffer_handle_t to the
hidden underlying structure ? calling fstat() on the fd ?), or we have
to support length == 0, in which case we may as well drop the length
field completely if we can't rely on it. In the latter case we have to
consider the implications on the API, I think that the BufferAllocator
class would at least need to report the size of the allocated buffers.

> +
> +	FrameBuffer *buffer = new FrameBuffer(planes);

Now that I read this, I wonder what the best API for the FileDescriptor
class would be. The fds that we receive through camera3Handle shall
certainly not be closed by us, but there's also no strict need for us to
duplicate them internally. Would it make sense for FileDescriptor to
have two constructors, one taking an int and one taking an int &&, and
store internally a flag telling whether the fd is owned by the
FileDescriptor class (for the int && case) or borrowed (for the in case)
? FileDescriptor would then only close the fd if it owns it. This would
remove the need to dup(), but the caller would then be responsible to
keep the fd valid and close it itself, which may not be convenient.
Maybe we then need a third constructor that would dup() the fd (and of
course flag ownership of the dup'ed fd in that case), possibly as a flag
for the int constructor ? The rationale for all this is that we should
not only avoid memory allocation for every request, but also, and more
importantly as it is more costly, avoid dup'ing file descriptors for
every request.

>  	if (!buffer) {
>  		LOG(HAL, Error) << "Failed to create buffer";
>  		delete descriptor;
> @@ -754,7 +754,7 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque
>  
>  	Request *request =
>  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> -	request->addBuffer(stream, std::move(buffer));
> +	request->addBuffer(stream, buffer);
>  
>  	int ret = camera_->queueRequest(request);
>  	if (ret) {
> @@ -771,8 +771,8 @@ error:
>  
>  void CameraDevice::requestComplete(Request *request)
>  {
> -	const std::map<Stream *, Buffer *> &buffers = request->buffers();
> -	Buffer *libcameraBuffer = buffers.begin()->second;
> +	const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
> +	FrameBuffer *libcameraBuffer = buffers.begin()->second;

Let's name the variable either frameBuffer or just buffer.

>  	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
>  	std::unique_ptr<CameraMetadata> resultMetadata;
>  
> @@ -825,6 +825,7 @@ void CameraDevice::requestComplete(Request *request)
>  	callbacks_->process_capture_result(callbacks_, &captureResult);
>  
>  	delete descriptor;
> +	delete libcameraBuffer;
>  }
>  
>  void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)
> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
> index b6b40baeee661df6..5cead75a16d0923a 100644
> --- a/src/cam/buffer_writer.cpp
> +++ b/src/cam/buffer_writer.cpp
> @@ -21,7 +21,7 @@ BufferWriter::BufferWriter(const std::string &pattern)
>  {
>  }
>  
> -int BufferWriter::write(Buffer *buffer, const std::string &streamName)
> +int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)
>  {
>  	std::string filename;
>  	size_t pos;
> @@ -42,10 +42,9 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)
>  	if (fd == -1)
>  		return -errno;
>  
> -	BufferMemory *mem = buffer->mem();
> -	for (Dmabuf &dmabuf : mem->planes()) {
> -		void *data = dmabuf.mem();
> -		unsigned int length = dmabuf.length();
> +	for (Dmabuf *dmabuf : buffer->dmabufs()) {
> +		void *data = dmabuf->mem();
> +		unsigned int length = dmabuf->length();

Should we use bytesused from buffer->metadata() ?

>  
>  		ret = ::write(fd, data, length);
>  		if (ret < 0) {
> diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h
> index 7bf785d1e83235ff..5917a7dfb5e28106 100644
> --- a/src/cam/buffer_writer.h
> +++ b/src/cam/buffer_writer.h
> @@ -16,7 +16,8 @@ class BufferWriter
>  public:
>  	BufferWriter(const std::string &pattern = "frame-#.bin");
>  
> -	int write(libcamera::Buffer *buffer, const std::string &streamName);
> +	int write(libcamera::FrameBuffer *buffer,
> +		  const std::string &streamName);
>  
>  private:
>  	std::string pattern_;
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index a4fa88a8d99669bc..154e6e461ee2b96b 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,22 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)
>  	return ret;
>  }
>  
> -int Capture::capture(EventLoop *loop)
> +int Capture::capture(EventLoop *loop, BufferAllocator &allocator)

We tend to use either const references or non-const pointers, I would
pass a *allocator. Or maybe create the allocator in Capture::capture() ?

>  {
>  	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(), cfg);

We seem to always pass the stream from the stream configuration, and I
think we require the camera to be configured with the configuration
before allocating buffers (this needs to be documented). If that's the
case, can't we drop the stream pointer and retrieve it from the
configuration in BufferAllocator::allocate() ?

> +		if (ret < 0) {
> +			std::cerr << "Can't allocate buffers" << std::endl;
> +			return -ENOMEM;
> +		}
> +
> +		unsigned int allocated = allocator.buffers(cfg.stream()).size();
> +		nbuffers = std::min(nbuffers, allocated);
> +	}
>  
>  	/*
>  	 * TODO: make cam tool smarter to support still capture by for
> @@ -93,9 +104,9 @@ int Capture::capture(EventLoop *loop)
>  
>  		for (StreamConfiguration &cfg : *config_) {
>  			Stream *stream = cfg.stream();
> -			std::unique_ptr<Buffer> buffer = stream->createBuffer(i);
> +			FrameBuffer *buffer = allocator.buffers(stream)[i];

You should create a local reference for allocator.buffers(stream) as an
optimization.

>  
> -			ret = request->addBuffer(stream, std::move(buffer));
> +			ret = request->addBuffer(stream, buffer);
>  			if (ret < 0) {
>  				std::cerr << "Can't set buffer for request"
>  					  << std::endl;
> @@ -138,7 +149,7 @@ void Capture::requestComplete(Request *request)
>  	if (request->status() == Request::RequestCancelled)
>  		return;
>  
> -	const std::map<Stream *, Buffer *> &buffers = request->buffers();
> +	const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
>  
>  	std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now();
>  	double fps = std::chrono::duration_cast<std::chrono::milliseconds>(now - last_).count();
> @@ -151,7 +162,7 @@ void Capture::requestComplete(Request *request)
>  
>  	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
>  		Stream *stream = it->first;
> -		Buffer *buffer = it->second;
> +		FrameBuffer *buffer = it->second;
>  		const std::string &name = streamName_[stream];
>  
>  		info << " " << name
> @@ -180,16 +191,9 @@ 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" << std::endl;
> -			return;
> -		}
> +		FrameBuffer *buffer = it->second;
>  
> -		request->addBuffer(stream, std::move(newBuffer));
> +		request->addBuffer(stream, buffer);
>  	}
>  
>  	camera_->queueRequest(request);
> diff --git a/src/cam/capture.h b/src/cam/capture.h
> index c692d48918f2de1d..3a8842fbd2080c2c 100644
> --- a/src/cam/capture.h
> +++ b/src/cam/capture.h
> @@ -10,6 +10,8 @@
>  #include <chrono>
>  #include <memory>
>  
> +#include <libcamera/allocator.h>
> +#include <libcamera/buffer.h>
>  #include <libcamera/camera.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
> @@ -26,7 +28,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/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 6b1b3fb64f8b2c0b..05fdcaab8f918afa 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";
>  			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();
> @@ -923,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;

Next step, reusing requests :-)

>  }
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index 6ce8ea1c8d31b870..e4588f10bac0228c 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -20,12 +20,12 @@
>  
>  namespace libcamera {
>  
> -class Buffer;
>  class Camera;
>  class CameraConfiguration;
>  class CameraManager;
>  class DeviceEnumerator;
>  class DeviceMatch;
> +class FrameBuffer;
>  class MediaDevice;
>  class PipelineHandler;
>  class Request;
> @@ -79,7 +79,8 @@ public:
>  
>  	int queueRequest(Camera *camera, Request *request);
>  
> -	bool completeBuffer(Camera *camera, Request *request, Buffer *buffer);
> +	bool completeBuffer(Camera *camera, Request *request,
> +			    FrameBuffer *buffer);
>  	void completeRequest(Camera *camera, Request *request);
>  
>  	const char *name() const { return name_; }
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 0e94c3f71d0b1208..ced3cb229c509ad2 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -167,9 +167,8 @@ public:
>  	int externalBuffers(unsigned int count);
>  	int releaseBuffers();
>  
> -	int queueBuffer(Buffer *buffer);
> -	std::vector<std::unique_ptr<Buffer>> queueAllBuffers();
> -	Signal<Buffer *> bufferReady;
> +	int queueBuffer(FrameBuffer *buffer);
> +	Signal<FrameBuffer *> bufferReady;
>  
>  	int streamOn();
>  	int streamOff();
> @@ -203,7 +202,7 @@ private:
>  	FrameBuffer *createBuffer(struct v4l2_buffer buf);
>  	int exportDmaBuffer(unsigned int index, unsigned int plane);
>  
> -	Buffer *dequeueBuffer();
> +	FrameBuffer *dequeueBuffer();
>  	void bufferAvailable(EventNotifier *notifier);
>  
>  	V4L2Capability caps_;
> @@ -213,7 +212,7 @@ private:
>  
>  	BufferPool *bufferPool_;
>  	V4L2BufferCache *cache_;
> -	std::map<unsigned int, Buffer *> queuedBuffers_;
> +	std::map<unsigned int, FrameBuffer *> queuedBuffers_;
>  
>  	EventNotifier *fdEvent_;
>  };
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 5080ac6c4d0abe3b..8d9420aea3eb0b21 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -31,6 +31,8 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPU3)
>  
> +class IPU3CameraData;
> +
>  class ImgUDevice
>  {
>  public:
> @@ -44,7 +46,7 @@ public:
>  		V4L2VideoDevice *dev;
>  		unsigned int pad;
>  		std::string name;
> -		BufferPool *pool;
> +		std::vector<FrameBuffer *> buffers;
>  	};
>  
>  	ImgUDevice()
> @@ -70,10 +72,9 @@ public:
>  	int configureOutput(ImgUOutput *output,
>  			    const StreamConfiguration &cfg);
>  
> -	int importInputBuffers(BufferPool *pool);
> -	int importOutputBuffers(ImgUOutput *output, BufferPool *pool);
> -	int exportOutputBuffers(ImgUOutput *output, BufferPool *pool);
> -	void freeBuffers();
> +	int importInputBuffers(unsigned int count);
> +	int exportOutputBuffers(ImgUOutput *output, unsigned int count);
> +	void freeBuffers(IPU3CameraData *data);
>  
>  	int start();
>  	int stop();
> @@ -93,10 +94,6 @@ public:
>  	ImgUOutput viewfinder_;
>  	ImgUOutput stat_;
>  	/* \todo Add param video device for 3A tuning */
> -
> -	BufferPool vfPool_;
> -	BufferPool statPool_;
> -	BufferPool outPool_;
>  };
>  
>  class CIO2Device
> @@ -120,10 +117,10 @@ public:
>  	int configure(const Size &size,
>  		      V4L2DeviceFormat *outputFormat);
>  
> -	BufferPool *exportBuffers();
> +	int exportBuffers();

How about naming this allocateBuffers() to match freeBuffers() ? The
CIO2 always allocates buffers internally and exports them, so there's no
need to differentiate between exporting and importing. The buffers_
vector being private to the CIO2Device class makes me believe this would
be a good rename. Note that it would get in the way of using fences to
pre-queue buffers to the ImgU input, but V4L2 doesn't support fences at
the moment so I don't think we need to care, a bigger refactoring will
likely be needed at that time anyway.

>  	void freeBuffers();
>  
> -	int start(std::vector<std::unique_ptr<Buffer>> *buffers);
> +	int start();
>  	int stop();
>  
>  	static int mediaBusToFormat(unsigned int code);
> @@ -132,7 +129,8 @@ public:
>  	V4L2Subdevice *csi2_;
>  	CameraSensor *sensor_;
>  
> -	BufferPool pool_;
> +private:
> +	std::vector<FrameBuffer *> buffers_;

Would a vector of unique pointers simplify CIO2Device::freeBuffers() ?

>  };
>  
>  class IPU3Stream : public V4L2Stream
> @@ -163,17 +161,15 @@ public:
>  		delete vfStream_;
>  	}
>  
> -	void imguOutputBufferReady(Buffer *buffer);
> -	void imguInputBufferReady(Buffer *buffer);
> -	void cio2BufferReady(Buffer *buffer);
> +	void imguOutputBufferReady(FrameBuffer *buffer);
> +	void imguInputBufferReady(FrameBuffer *buffer);
> +	void cio2BufferReady(FrameBuffer *buffer);
>  
>  	CIO2Device cio2_;
>  	ImgUDevice *imgu_;
>  
>  	IPU3Stream *outStream_;
>  	IPU3Stream *vfStream_;
> -
> -	std::vector<std::unique_ptr<Buffer>> rawBuffers_;
>  };
>  
>  class IPU3CameraConfiguration : public CameraConfiguration
> @@ -637,70 +633,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.
> -	 */

Could you please keep the comments ? They're important.

> -	bufferCount = pool->count();
> -	imgu->stat_.pool->createBuffers(bufferCount);
> -	ret = imgu->exportOutputBuffers(&imgu->stat_, imgu->stat_.pool);
> +	ret = data->imgu_->importInputBuffers(bufferCount);
>  	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);
>  
> @@ -713,7 +681,7 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera,
>  	IPU3CameraData *data = cameraData(camera);
>  
>  	data->cio2_.freeBuffers();
> -	data->imgu_->freeBuffers();
> +	data->imgu_->freeBuffers(data);
>  
>  	return 0;
>  }
> @@ -729,7 +697,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;
>  
> @@ -745,7 +713,6 @@ int PipelineHandlerIPU3::start(Camera *camera)
>  error:
>  	LOG(IPU3, Error) << "Failed to start camera " << camera->name();
>  
> -	data->rawBuffers_.clear();
>  	return ret;
>  }
>  
> @@ -759,8 +726,6 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>  	if (ret)
>  		LOG(IPU3, Warning) << "Failed to stop camera "
>  				   << camera->name();
> -
> -	data->rawBuffers_.clear();
>  }
>  
>  int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request)
> @@ -769,7 +734,7 @@ int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request)
>  
>  	for (auto it : request->buffers()) {
>  		IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
> -		Buffer *buffer = it.second;
> +		FrameBuffer *buffer = it.second;
>  
>  		int ret = stream->device_->dev->queueBuffer(buffer);
>  		if (ret < 0)
> @@ -930,7 +895,7 @@ int PipelineHandlerIPU3::registerCameras()
>   * Buffers completed from the ImgU input are immediately queued back to the
>   * CIO2 unit to continue frame capture.
>   */
> -void IPU3CameraData::imguInputBufferReady(Buffer *buffer)
> +void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer)
>  {
>  	/* \todo Handle buffer failures when state is set to BufferError. */
>  	if (buffer->info().status() == BufferInfo::BufferCancelled)
> @@ -945,7 +910,7 @@ void IPU3CameraData::imguInputBufferReady(Buffer *buffer)
>   *
>   * Buffers completed from the ImgU output are directed to the application.
>   */
> -void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
> +void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
>  {
>  	Request *request = buffer->request();
>  
> @@ -964,7 +929,7 @@ void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
>   * Buffers completed from the CIO2 are immediately queued to the ImgU unit
>   * for further processing.
>   */
> -void IPU3CameraData::cio2BufferReady(Buffer *buffer)
> +void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>  {
>  	/* \todo Handle buffer failures when state is set to BufferError. */
>  	if (buffer->info().status() == BufferInfo::BufferCancelled)
> @@ -1020,7 +985,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");
> @@ -1030,7 +994,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();
> @@ -1039,7 +1002,6 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>  
>  	stat_.pad = PAD_STAT;
>  	stat_.name = "stat";
> -	stat_.pool = &statPool_;
>  
>  	return 0;
>  }
> @@ -1139,85 +1101,49 @@ 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)

This method doesn't import buffers, so I think it should be renamed.

>  {
> -	int ret = input_->importBuffers(pool);
> -	if (ret) {
> +	int ret = input_->externalBuffers(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
> - */

Let's not lose all the documentation, even if the method is internal :-)
I was about to make a proposal on how to keep this, but given that the
methods will likely need to be refactored for the reasons explained
below, I expect the documentation will change anyway.

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

I think you're leaking this. A vector of unique pointers would help
making ownership clearer.

> +	if (ret < 0)
> +		LOG(IPU3, Error) << "Failed to allocate ImgU "
> +				 << output->name << " buffers";
>  
> -	return 0;
> +	return ret;
>  }

How about moving this method to the ImgUOutput class ? It doesn't access
any field of ImgUDevice, I think it would make the code more readably,
and you could just call it allocateBuffers there.

>  
>  /**
>   * \brief Release buffers for all the ImgU video devices
>   */
> -void ImgUDevice::freeBuffers()
> +void ImgUDevice::freeBuffers(IPU3CameraData *data)
>  {
>  	int ret;
>  
> -	ret = output_.dev->releaseBuffers();
> -	if (ret)
> -		LOG(IPU3, Error) << "Failed to release ImgU output buffers";
> +	if (!data->outStream_->active_) {
> +		ret = output_.dev->releaseBuffers();
> +		if (ret)
> +			LOG(IPU3, Error) << "Failed to release ImgU output buffers";
> +	}

I would like to simplify this method and avoid passing data as an
argument, to keep the symmetry with the other buffer-related methods. 
How about simplifying this by either tracking whether buffers have been
allocated in struct ImgUOutput, or making V4L2VideoDevice::releaseBuffers()
a no-op when no buffers have been allocated (and updating its
documentation to say so explicitly) and calling it unconditionally ? And
maybe adding an ImgUOutput::freeBuffers() to keep symmetry with the
proposed ImgUOutput::allocateBuffers() above ?

Some more logic may be needed as I suspect the import and export cases
need to be treated differently, and given my comments on the Stream
interface for buffer allocation, I think you'll have to rework this
anyway. The code is quite convoluted here to be frank, it's hard to
track which buffers are allocated and free by who, and I think that's at
least partly the consequence of the automatic handling in
V4L2Stream::start() and V4L2Stream::stop(). I would much rather see this
being done explicitly in pipeline handlers, it would be a bit more work,
but I think the result would be much easier to read and validate.

>  
>  	ret = stat_.dev->releaseBuffers();
>  	if (ret)
>  		LOG(IPU3, Error) << "Failed to release ImgU stat buffers";
>  
> -	ret = viewfinder_.dev->releaseBuffers();
> -	if (ret)
> -		LOG(IPU3, Error) << "Failed to release ImgU viewfinder buffers";
> +	if (!data->vfStream_->active_) {
> +		ret = viewfinder_.dev->releaseBuffers();
> +		if (ret)
> +			LOG(IPU3, Error) << "Failed to release ImgU viewfinder buffers";
> +	}
>  
>  	ret = input_->releaseBuffers();
>  	if (ret)
> @@ -1452,38 +1378,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
> - */

/**
 * \brief Allocate frame buffers for the CIO2 output
 *
 * Allocate frame buffers in the CIO2 video device to be used to capture frames
 * from the CIO2 output. The buffers are stored in the CIO2Device::buffers_
 * vector.
 *
 * \return 0 on success or a negative error code if allocation fails
 */

> -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 (FrameBuffer *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 (FrameBuffer *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 ca3d92c7ad637c3a..96e863f0208fa748 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -32,8 +32,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 {
>  
> @@ -52,9 +51,9 @@ struct RkISP1FrameInfo {
>  	unsigned int frame;
>  	Request *request;
>  
> -	Buffer *paramBuffer;
> -	Buffer *statBuffer;
> -	Buffer *videoBuffer;
> +	FrameBuffer *paramBuffer;
> +	FrameBuffer *statBuffer;
> +	FrameBuffer *videoBuffer;
>  
>  	bool paramFilled;
>  	bool paramDequeued;
> @@ -70,7 +69,7 @@ public:
>  	int destroy(unsigned int frame);
>  
>  	RkISP1FrameInfo *find(unsigned int frame);
> -	RkISP1FrameInfo *find(Buffer *buffer);
> +	RkISP1FrameInfo *find(FrameBuffer *buffer);
>  	RkISP1FrameInfo *find(Request *request);
>  
>  private:
> @@ -89,7 +88,7 @@ public:
>  		setDelay(QueueBuffers, -1, 10);
>  	}
>  
> -	void bufferReady(Buffer *buffer)
> +	void bufferReady(FrameBuffer *buffer)
>  	{
>  		/*
>  		 * Calculate SOE by taking the end of DMA set by the kernel and applying
> @@ -154,8 +153,6 @@ public:
>  	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
>  
>  private:
> -	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> -

Any reason for this ? 

>  	/*
>  	 * The RkISP1CameraData instance is guaranteed to be valid as long as the
>  	 * corresponding Camera instance is valid. In order to borrow a
> @@ -203,9 +200,9 @@ private:
>  	int initLinks();
>  	int createCamera(MediaEntity *sensor);
>  	void tryCompleteRequest(Request *request);
> -	void bufferReady(Buffer *buffer);
> -	void paramReady(Buffer *buffer);
> -	void statReady(Buffer *buffer);
> +	void bufferReady(FrameBuffer *buffer);
> +	void paramReady(FrameBuffer *buffer);
> +	void statReady(FrameBuffer *buffer);
>  
>  	MediaDevice *media_;
>  	V4L2Subdevice *dphy_;
> @@ -214,11 +211,8 @@ private:
>  	V4L2VideoDevice *param_;
>  	V4L2VideoDevice *stat_;
>  
> -	BufferPool paramPool_;
> -	BufferPool statPool_;
> -
> -	std::queue<Buffer *> paramBuffers_;
> -	std::queue<Buffer *> statBuffers_;
> +	std::queue<FrameBuffer *> paramBuffers_;
> +	std::queue<FrameBuffer *> statBuffers_;

As for the IPU3 pipeline handler, wouldn't a queue of unique pointers
help ? Please also take into account the other comments made for the
IPU3 pipeline handler, some of them apply here.

>  
>  	Camera *activeCamera_;
>  };
> @@ -234,15 +228,15 @@ RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stre
>  		LOG(RkISP1, Error) << "Parameters buffer underrun";
>  		return nullptr;
>  	}
> -	Buffer *paramBuffer = pipe_->paramBuffers_.front();
> +	FrameBuffer *paramBuffer = pipe_->paramBuffers_.front();
>  
>  	if (pipe_->statBuffers_.empty()) {
>  		LOG(RkISP1, Error) << "Statisitc buffer underrun";
>  		return nullptr;
>  	}
> -	Buffer *statBuffer = pipe_->statBuffers_.front();
> +	FrameBuffer *statBuffer = pipe_->statBuffers_.front();
>  
> -	Buffer *videoBuffer = request->findBuffer(stream);
> +	FrameBuffer *videoBuffer = request->findBuffer(stream);
>  	if (!videoBuffer) {
>  		LOG(RkISP1, Error)
>  			<< "Attempt to queue request with invalid stream";
> @@ -295,7 +289,7 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)
>  	return nullptr;
>  }
>  
> -RkISP1FrameInfo *RkISP1Frames::find(Buffer *buffer)
> +RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)
>  {
>  	for (auto &itInfo : frameInfo_) {
>  		RkISP1FrameInfo *info = itInfo.second;
> @@ -661,57 +655,42 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
>  					   const std::set<Stream *> &streams)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
> -	Stream *stream = *streams.begin();
> +	std::vector<FrameBuffer *> paramBuffers, statBuffers;
> +	unsigned int count = 1;
>  	int ret;
>  
> -	if (stream->memoryType() == InternalMemory)
> -		ret = video_->exportBuffers(&stream->bufferPool());
> -	else
> -		ret = video_->importBuffers(&stream->bufferPool());
>  
> -	if (ret)
> -		return ret;
> +	ret = param_->allocateBuffers(RKISP1_BUFFER_COUNT, &paramBuffers);
> +	if (ret < 0)
> +		goto err;
>  
> -	paramPool_.createBuffers(stream->configuration().bufferCount + 1);
> -	ret = param_->exportBuffers(&paramPool_);
> -	if (ret) {
> -		video_->releaseBuffers();
> -		return ret;
> -	}
> +	ret = stat_->allocateBuffers(RKISP1_BUFFER_COUNT, &statBuffers);
> +	if (ret < 0)
> +		goto err;
>  
> -	statPool_.createBuffers(stream->configuration().bufferCount + 1);
> -	ret = stat_->exportBuffers(&statPool_);
> -	if (ret) {
> -		param_->releaseBuffers();
> -		video_->releaseBuffers();
> -		return ret;
> +	for (FrameBuffer *buffer : paramBuffers) {
> +		buffer->setCookie(count++);
> +		data->ipaBuffers_.push_back({ .id = buffer->cookie(),
> +					      .planes = buffer->planes() });

I think I'm OK with this use of the cookie API, but it should be
documented as being meant for internal purpose, and not for
applications. It should ideally be unaccessible from applications
completely, but a friend statement won't help. We can possibly address
this later, let's add a todo statement in the FrameBuffer class for this
(but if you already have an idea on how to do so in an easy way, there's
no need to wait :-)), 

> +		paramBuffers_.push(buffer);
>  	}
>  
> -	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> -		std::vector<FrameBuffer::Plane> planes;
> -		planes.push_back({
> -			.fd = paramPool_.buffers()[i].planes()[0].fd(),
> -			.length = paramPool_.buffers()[i].planes()[0].length(),
> -		});
> -
> -		data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
> -					      .planes = planes });
> -		paramBuffers_.push(new Buffer(i));
> +	for (FrameBuffer *buffer : statBuffers) {
> +		buffer->setCookie(count++);
> +		data->ipaBuffers_.push_back({ .id = buffer->cookie(),
> +					      .planes = buffer->planes() });
> +		statBuffers_.push(buffer);
>  	}
>  
> -	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> -		std::vector<FrameBuffer::Plane> planes;
> -		planes.push_back({
> -			.fd = statPool_.buffers()[i].planes()[0].fd(),
> -			.length = statPool_.buffers()[i].planes()[0].length(),
> -		});
> +	data->ipa_->mapBuffers(data->ipaBuffers_);
>  
> -		data->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,
> -					      .planes = planes });
> -		statBuffers_.push(new Buffer(i));
> -	}
> +	return 0;
> +err:
> +	for (FrameBuffer *buffer : paramBuffers)
> +		delete buffer;
>  
> -	data->ipa_->mapBuffers(data->ipaBuffers_);
> +	for (FrameBuffer *buffer : statBuffers)
> +		delete buffer;

I really think V4L2VideoDevice::allocateBuffers() needs to take a vector
of std::unique_ptr<FrameBuffer> :-)

>  
>  	return ret;
>  }
> @@ -744,9 +723,6 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera,
>  	if (stat_->releaseBuffers())
>  		LOG(RkISP1, Error) << "Failed to release stat buffers";
>  
> -	if (video_->releaseBuffers())
> -		LOG(RkISP1, Error) << "Failed to release video buffers";
> -
>  	return 0;
>  }
>  
> @@ -834,9 +810,11 @@ int PipelineHandlerRkISP1::queueRequestHardware(Camera *camera,
>  	if (!info)
>  		return -ENOENT;
>  
> +	unsigned int paramid = info->paramBuffer->cookie();
> +
>  	IPAOperationData op;
>  	op.operation = RKISP1_IPA_EVENT_QUEUE_REQUEST;
> -	op.data = { data->frame_, RKISP1_PARAM_BASE | info->paramBuffer->index() };
> +	op.data = { data->frame_, paramid };

One option to avoid the need for the cookie API would be to transition
IPAInterface to use FrameBuffer * instead of ids. We could then perform
the conversion to and from ids in the wrappers and proxy, with the
mapping being kept there. IPAOperationData would need a new vector of
FrameBuffer *. This may not be the best solution, and would go on top of
this series anyway.

>  	op.controls = { request->controls() };
>  	data->ipa_->processEvent(op);
>  
> @@ -996,12 +974,12 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
>  	if (!info->paramDequeued)
>  		return;
>  
> -	completeRequest(activeCamera_, request);
> -
>  	data->frameInfo_.destroy(info->frame);
> +
> +	completeRequest(activeCamera_, request);

Why is completeRequest() moved after the destruction of the frame info ?

>  }
>  
> -void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)
> +void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>  {
>  	ASSERT(activeCamera_);
>  	RkISP1CameraData *data = cameraData(activeCamera_);
> @@ -1016,7 +994,7 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)
>  	tryCompleteRequest(request);
>  }
>  
> -void PipelineHandlerRkISP1::paramReady(Buffer *buffer)
> +void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
>  {
>  	ASSERT(activeCamera_);
>  	RkISP1CameraData *data = cameraData(activeCamera_);
> @@ -1027,7 +1005,7 @@ void PipelineHandlerRkISP1::paramReady(Buffer *buffer)
>  	tryCompleteRequest(info->request);
>  }
>  
> -void PipelineHandlerRkISP1::statReady(Buffer *buffer)
> +void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>  {
>  	ASSERT(activeCamera_);
>  	RkISP1CameraData *data = cameraData(activeCamera_);
> @@ -1037,7 +1015,7 @@ void PipelineHandlerRkISP1::statReady(Buffer *buffer)
>  		return;
>  
>  	unsigned int frame = info->frame;
> -	unsigned int statid = RKISP1_STAT_BASE | info->statBuffer->index();
> +	unsigned int statid = info->statBuffer->cookie();
>  
>  	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 9cc90bf454cb4392..c275b9f0f75429bd 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -42,7 +42,7 @@ public:
>  	}
>  
>  	int init(MediaEntity *entity);
> -	void bufferReady(Buffer *buffer);
> +	void bufferReady(FrameBuffer *buffer);
>  
>  	V4L2VideoDevice *video_;
>  	Stream *stream_;
> @@ -196,23 +196,13 @@ 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,
>  				    const std::set<Stream *> &streams)
>  {
> -	UVCCameraData *data = cameraData(camera);
> -	return data->video_->releaseBuffers();
> +	return 0;
>  }
>  
>  int PipelineHandlerUVC::start(Camera *camera)
> @@ -266,7 +256,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
>  int PipelineHandlerUVC::queueRequestHardware(Camera *camera, Request *request)
>  {
>  	UVCCameraData *data = cameraData(camera);
> -	Buffer *buffer = request->findBuffer(data->stream_);
> +	FrameBuffer *buffer = request->findBuffer(data->stream_);
>  	if (!buffer) {
>  		LOG(UVC, Error)
>  			<< "Attempt to queue request with invalid stream";
> @@ -373,7 +363,7 @@ int UVCCameraData::init(MediaEntity *entity)
>  	return 0;
>  }
>  
> -void UVCCameraData::bufferReady(Buffer *buffer)
> +void UVCCameraData::bufferReady(FrameBuffer *buffer)
>  {
>  	Request *request = buffer->request();
>  
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index b1222bd21fb629a0..64793788c752c791 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -57,7 +57,7 @@ public:
>  	}
>  
>  	int init(MediaDevice *media);
> -	void bufferReady(Buffer *buffer);
> +	void bufferReady(FrameBuffer *buffer);
>  
>  	CameraSensor *sensor_;
>  	V4L2Subdevice *debayer_;
> @@ -264,23 +264,13 @@ 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;
>  }
>  
>  int PipelineHandlerVimc::freeBuffers(Camera *camera,
>  				     const std::set<Stream *> &streams)
>  {
> -	VimcCameraData *data = cameraData(camera);
> -	return data->video_->releaseBuffers();
> +	return 0;
>  }
>  
>  int PipelineHandlerVimc::start(Camera *camera)
> @@ -328,7 +318,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
>  int PipelineHandlerVimc::queueRequestHardware(Camera *camera, Request *request)
>  {
>  	VimcCameraData *data = cameraData(camera);
> -	Buffer *buffer = request->findBuffer(data->stream_);
> +	FrameBuffer *buffer = request->findBuffer(data->stream_);
>  	if (!buffer) {
>  		LOG(VIMC, Error)
>  			<< "Attempt to queue request with invalid stream";
> @@ -459,7 +449,7 @@ int VimcCameraData::init(MediaDevice *media)
>  	return 0;
>  }
>  
> -void VimcCameraData::bufferReady(Buffer *buffer)
> +void VimcCameraData::bufferReady(FrameBuffer *buffer)
>  {
>  	Request *request = buffer->request();
>  
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index c9e348b98da7b736..b107e23258dee692 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -398,7 +398,7 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request)
>   * otherwise
>   */
>  bool PipelineHandler::completeBuffer(Camera *camera, Request *request,
> -				     Buffer *buffer)
> +				     FrameBuffer *buffer)
>  {
>  	camera->bufferCompleted.emit(request, buffer);
>  	return request->completeBuffer(buffer);
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 7593bf9dfa546401..a4b27e1cb08a7641 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_;
> @@ -126,7 +121,7 @@ Request::~Request()
>   * \retval -EEXIST The request already contains a buffer for the stream
>   * \retval -EINVAL The buffer does not reference a valid Stream
>   */

The documentation of this method, as well as Request::buffers(), still
mentions the Buffer class, it should be updated to mention FrameBuffer.
Furthermore the documentation needs to be updated to explain the new
ownership rules for buffers, as it currently states

"Ownership of the buffer is passed to the request. It will be deleted
when the request is destroyed after completing."

Please don't just remove the paragraph but explain the new rules :-)
Could you please go through the documentation of the Request class to
make sure nothing else is outdated ?

> -int Request::addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer)
> +int Request::addBuffer(Stream *stream, FrameBuffer *buffer)
>  {
>  	if (!stream) {
>  		LOG(Request, Error) << "Invalid stream reference";
> @@ -139,7 +134,7 @@ int Request::addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer)
>  		return -EEXIST;
>  	}
>  
> -	bufferMap_[stream] = buffer.release();
> +	bufferMap_[stream] = buffer;
>  
>  	return 0;
>  }
> @@ -159,7 +154,7 @@ int Request::addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer)
>   * \return The buffer associated with the stream, or nullptr if the stream is
>   * not part of this request
>   */
> -Buffer *Request::findBuffer(Stream *stream) const
> +FrameBuffer *Request::findBuffer(Stream *stream) const
>  {
>  	auto it = bufferMap_.find(stream);
>  	if (it == bufferMap_.end())
> @@ -219,8 +214,9 @@ int Request::prepare()
>  	}
>  
>  	for (auto const &pair : bufferMap_) {
> -		Buffer *buffer = pair.second;
> +		FrameBuffer *buffer = pair.second;
>  		buffer->request_ = this;
> +

I don't think a blank line is needed, but that's up to you.

>  		pending_.insert(buffer);
>  	}
>  
> @@ -253,7 +249,7 @@ void Request::complete()
>   * \return True if all buffers contained in the request have completed, false
>   * otherwise
>   */
> -bool Request::completeBuffer(Buffer *buffer)
> +bool Request::completeBuffer(FrameBuffer *buffer)
>  {
>  	int ret = pending_.erase(buffer);
>  	ASSERT(ret == 1);
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index f5810956b2040ce6..a29ed697468a7f59 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1191,9 +1191,11 @@ int V4L2VideoDevice::externalBuffers(unsigned int count)
>   */
>  int V4L2VideoDevice::releaseBuffers()
>  {
> -	LOG(V4L2, Debug) << "Releasing bufferPool";
> +	LOG(V4L2, Debug) << "Releasing buffers";
>  
>  	bufferPool_ = nullptr;
> +	delete cache_;
> +	cache_ = nullptr;
>  
>  	return requestBuffers(0);
>  }
> @@ -1209,27 +1211,26 @@ int V4L2VideoDevice::releaseBuffers()

You should explain here that the V4L2 buffer is automatically picked
using the cache (with slightly more details than this :-)).

>   *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> +int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>  {
>  	struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
>  	struct v4l2_buffer buf = {};
>  	int ret;
>  
> -	buf.index = buffer->index();
> +	buf.index = cache_->fetch(buffer);
>  	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<Dmabuf> &dmabufs = mem->planes();
> +	const std::vector<Dmabuf *> &dmabufs = buffer->dmabufs();
>  
>  	if (buf.memory == V4L2_MEMORY_DMABUF) {
>  		if (multiPlanar) {
>  			for (unsigned int p = 0; p < dmabufs.size(); ++p)
> -				v4l2Planes[p].m.fd = dmabufs[p].fd();
> +				v4l2Planes[p].m.fd = dmabufs[p]->fd();
>  		} else {
> -			buf.m.fd = dmabufs[0].fd();
> +			buf.m.fd = dmabufs[0]->fd();
>  		}
>  	}
>  
> @@ -1275,52 +1276,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 Dequeue the next available buffer from the video device
>   *
> @@ -1329,7 +1284,7 @@ std::vector<std::unique_ptr<Buffer>> V4L2VideoDevice::queueAllBuffers()
>   *
>   * \return A pointer to the dequeued buffer on success, or nullptr otherwise
>   */
> -Buffer *V4L2VideoDevice::dequeueBuffer()
> +FrameBuffer *V4L2VideoDevice::dequeueBuffer()
>  {
>  	struct v4l2_buffer buf = {};
>  	struct v4l2_plane planes[VIDEO_MAX_PLANES] = {};
> @@ -1352,15 +1307,15 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
>  
>  	LOG(V4L2, Debug) << "Buffer " << buf.index << " is available";
>  
> +	cache_->put(buf.index);
> +
>  	auto it = queuedBuffers_.find(buf.index);
> -	Buffer *buffer = it->second;
> +	FrameBuffer *buffer = it->second;
>  	queuedBuffers_.erase(it);
>  
>  	if (queuedBuffers_.empty())
>  		fdEvent_->setEnabled(false);
>  
> -	buffer->index_ = buf.index;
> -
>  	BufferInfo::Status status = buf.flags & V4L2_BUF_FLAG_ERROR
>  		? BufferInfo::BufferError : BufferInfo::BufferSuccess;
>  	uint64_t timestamp = buf.timestamp.tv_sec * 1000000000ULL
> @@ -1383,7 +1338,7 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
>   */

The documentation of this function also mentions Buffer.

>  void V4L2VideoDevice::bufferAvailable(EventNotifier *notifier)
>  {
> -	Buffer *buffer = dequeueBuffer();
> +	FrameBuffer *buffer = dequeueBuffer();
>  	if (!buffer)
>  		return;
>  
> @@ -1418,7 +1373,7 @@ int V4L2VideoDevice::streamOn()
>   * \brief Stop the video stream
>   *
>   * Buffers that are still queued when the video stream is stopped are
> - * immediately dequeued with their status set to Buffer::BufferError,
> + * immediately dequeued with their status set to BufferInfo::BufferCancelled,
>   * and the bufferReady signal is emitted for them. The order in which those
>   * buffers are dequeued is not specified.
>   *
> @@ -1437,11 +1392,10 @@ int V4L2VideoDevice::streamOff()
>  
>  	/* Send back all queued buffers. */
>  	for (auto it : queuedBuffers_) {
> -		unsigned int index = it.first;
> -		Buffer *buffer = it.second;
> +		FrameBuffer *buffer = it.second;
> +
> +		buffer->info_.update(BufferInfo::BufferCancelled, 0, 0, { {} });
>  
> -		buffer->index_ = index;
> -		buffer->cancel();
>  		bufferReady.emit(buffer);
>  	}
>  
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 771020112f09b1ef..9650bd970f89aa41 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,8 +179,14 @@ int MainWindow::startCapture()
>  		return ret;
>  	}
>  
> +	ret = allocator_->allocate(stream, cfg);
> +	if (ret < 0) {
> +		std::cerr << "Failed to allocate internal buffers" << std::endl;

Maybe s/internal buffers/capture buffers/ ? From the point of view of a
user of que qcam application, "internal buffers" could be confusing.

> +		return ret;
> +	}
> +
>  	std::vector<Request *> requests;
> -	for (unsigned int i = 0; i < cfg.bufferCount; ++i) {
> +	for (FrameBuffer *buffer : allocator_->buffers(stream)) {
>  		Request *request = camera_->createRequest();
>  		if (!request) {
>  			std::cerr << "Can't create request" << std::endl;
> @@ -184,13 +194,7 @@ int MainWindow::startCapture()
>  			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(stream, std::move(buffer));
> +		ret = request->addBuffer(stream, buffer);
>  		if (ret < 0) {
>  			std::cerr << "Can't set buffer for request" << std::endl;
>  			goto error;
> @@ -253,11 +257,11 @@ void MainWindow::requestComplete(Request *request)
>  	if (request->status() == Request::RequestCancelled)
>  		return;
>  
> -	const std::map<Stream *, Buffer *> &buffers = request->buffers();
> +	const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
>  
>  	framesCaptured_++;
>  
> -	Buffer *buffer = buffers.begin()->second;
> +	FrameBuffer *buffer = buffers.begin()->second;
>  	const BufferInfo &info = buffer->info();
>  
>  	double fps = info.timestamp() - lastBufferTime_;
> @@ -280,29 +284,20 @@ 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" << std::endl;
> -			return;
> -		}
> +		FrameBuffer *buffer = it->second;
>  
> -		request->addBuffer(stream, std::move(newBuffer));
> +		request->addBuffer(stream, buffer);
>  	}
>  
>  	camera_->queueRequest(request);
>  }
>  
> -int MainWindow::display(Buffer *buffer)
> +int MainWindow::display(FrameBuffer *buffer)
>  {
> -	BufferMemory *mem = buffer->mem();
> -	if (mem->planes().size() != 1)
> +	if (buffer->planes().size() != 1)
>  		return -EINVAL;
>  
> -	Dmabuf &dmabuf = mem->planes().front();
> -	unsigned char *raw = static_cast<unsigned char *>(dmabuf.mem());
> +	unsigned char *raw = static_cast<unsigned char *>(buffer->dmabufs()[0]->mem());
>  	viewfinder_->display(raw, buffer->info().planes()[0].bytesused);
>  
>  	return 0;
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 0786e915ec512255..b38aa2ac959f44aa 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -14,6 +14,8 @@
>  #include <QObject>
>  #include <QTimer>
>  
> +#include <libcamera/allocator.h>
> +#include <libcamera/buffer.h>
>  #include <libcamera/camera.h>
>  #include <libcamera/camera_manager.h>
>  #include <libcamera/stream.h>
> @@ -49,7 +51,7 @@ private:
>  	void stopCapture();
>  
>  	void requestComplete(Request *request);
> -	int display(Buffer *buffer);
> +	int display(FrameBuffer *buffer);
>  
>  	QString title_;
>  	QTimer titleTimer_;
> @@ -57,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/buffer_import.cpp b/test/camera/buffer_import.cpp
> index 5dbaed9255d3d60c..c98afeb9597e21c2 100644
> --- a/test/camera/buffer_import.cpp
> +++ b/test/camera/buffer_import.cpp
> @@ -21,35 +21,48 @@
>  #include "test.h"
>  
>  using namespace libcamera;
> +using namespace std;

This patch is big enough, let's not include this change. I'm actually
thinking that tests should move away from using namespace std.

>  
> -/* Keep SINK_BUFFER_COUNT > CAMERA_BUFFER_COUNT + 1 */
> -static constexpr unsigned int SINK_BUFFER_COUNT = 8;
> -static constexpr unsigned int CAMERA_BUFFER_COUNT = 4;
> +namespace {

Why the anonymous namespace here ?

>  
> -class FrameSink

Let's add a small comment.

/* A provider of external buffers, suitable for import by a Camera. */

> +class BufferSource
>  {
>  public:
> -	FrameSink()
> +	BufferSource()
>  		: video_(nullptr)
>  	{
>  	}
>  
> -	int init()
> +	~BufferSource()
> +	{
> +		if (video_) {
> +			video_->releaseBuffers();
> +			video_->close();
> +		}
> +
> +		delete video_;
> +		video_ = nullptr;
> +
> +		if (media_)
> +			media_->release();
> +	}
> +
> +	int allocate(const StreamConfiguration &config)
>  	{
>  		int ret;
>  
>  		/* Locate and open the video device. */
> -		std::string videoDeviceName = "vivid-000-vid-out";
> +		string videoDeviceName = "vivid-000-vid-out";
>  
> -		std::unique_ptr<DeviceEnumerator> enumerator =
> +		unique_ptr<DeviceEnumerator> enumerator =
>  			DeviceEnumerator::create();
>  		if (!enumerator) {
> -			std::cout << "Failed to create device enumerator" << std::endl;
> +			cout << "Failed to create device enumerator" << std::endl;
>  			return TestFail;
>  		}
>  
>  		if (enumerator->enumerate()) {
> -			std::cout << "Failed to enumerate media devices" << std::endl;
> +			cout << "Failed to enumerate media devices" << std::endl;
>  			return TestFail;
>  		}
>  
> @@ -58,13 +71,13 @@ public:
>  
>  		media_ = enumerator->search(dm);
>  		if (!media_) {
> -			std::cout << "No vivid output device available" << std::endl;
> +			cout << "No vivid output device available" << std::endl;
>  			return TestSkip;
>  		}
>  
>  		video_ = V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName);
>  		if (!video_) {
> -			std::cout << "Unable to open " << videoDeviceName << std::endl;
> +			cout << "Unable to open " << videoDeviceName << std::endl;
>  			return TestFail;
>  		}
>  
> @@ -72,366 +85,178 @@ public:
>  			return TestFail;
>  
>  		/* Configure the format. */
> -		ret = video_->getFormat(&format_);
> +		V4L2DeviceFormat format;
> +		ret = video_->getFormat(&format);
>  		if (ret) {
> -			std::cout << "Failed to get format on output device" << std::endl;
> +			cout << "Failed to get format on output device" << std::endl;
>  			return ret;
>  		}
>  
> -		format_.size.width = 1920;
> -		format_.size.height = 1080;
> -		format_.fourcc = V4L2_PIX_FMT_RGB24;
> -		format_.planesCount = 1;
> -		format_.planes[0].size = 1920 * 1080 * 3;
> -		format_.planes[0].bpl = 1920 * 3;
> -
> -		if (video_->setFormat(&format_)) {
> -			cleanup();
> +		format.size = config.size;
> +		format.fourcc = V4L2VideoDevice::toV4L2Fourcc(config.pixelFormat, false);

Isn't it a problem not to set planesCount, planes.size and planes.bpl ?
I would at least set planesCount to 1 and initialize the other two
fields to 0.

> +		if (video_->setFormat(&format))
>  			return TestFail;
> -		}
>  
> -		/* Export the buffers to a pool. */
> -		pool_.createBuffers(SINK_BUFFER_COUNT);
> -		ret = video_->exportBuffers(&pool_);
> -		if (ret) {
> -			std::cout << "Failed to export buffers" << std::endl;
> -			cleanup();
> -			return TestFail;
> -		}
> -
> -		/* Only use the first CAMERA_BUFFER_COUNT buffers to start with. */
> -		availableBuffers_.resize(CAMERA_BUFFER_COUNT);
> -		std::iota(availableBuffers_.begin(), availableBuffers_.end(), 0);
> -
> -		/* Connect the buffer ready signal. */
> -		video_->bufferReady.connect(this, &FrameSink::bufferComplete);
> -
> -		return TestPass;
> +		return video_->allocateBuffers(config.bufferCount, &buffers_);
>  	}
>  
> -	void cleanup()
> -	{
> -		if (video_) {
> -			video_->streamOff();
> -			video_->releaseBuffers();
> -			video_->close();
> -
> -			delete video_;
> -			video_ = nullptr;
> -		}
> -
> -		if (media_)
> -			media_->release();
> -	}
> -
> -	int start()
> -	{
> -		requestsCount_ = 0;
> -		done_ = false;
> -
> -		int ret = video_->streamOn();
> -		if (ret < 0)
> -			return ret;
> -
> -		/* Queue all the initial requests. */
> -		for (unsigned int index = 0; index < CAMERA_BUFFER_COUNT; ++index)
> -			queueRequest(index);
> -
> -		return 0;
> -	}
> -
> -	int stop()
> -	{
> -		return video_->streamOff();
> -	}
> -
> -	void requestComplete(uint64_t cookie, const Buffer *metadata)
> -	{
> -		unsigned int index = cookie;
> -
> -		Buffer *buffer = new Buffer(index, metadata);
> -		int ret = video_->queueBuffer(buffer);
> -		if (ret < 0)
> -			std::cout << "Failed to queue buffer to sink" << std::endl;
> -	}
> -
> -	bool done() const { return done_; }
> -
> -	PixelFormat format() const
> -	{
> -		return video_->toPixelFormat(format_.fourcc);
> -	}
> -
> -	const Size &size() const
> -	{
> -		return format_.size;
> -	}
> -
> -	Signal<uint64_t, int> requestReady;
> +	const vector<FrameBuffer *> &buffers() { return buffers_; };
>  
>  private:
> -	void queueRequest(unsigned int index)
> -	{
> -		auto it = std::find(availableBuffers_.begin(),
> -				    availableBuffers_.end(), index);
> -		availableBuffers_.erase(it);
> -
> -		uint64_t cookie = index;
> -		BufferMemory &mem = pool_.buffers()[index];
> -		int dmabuf = mem.planes()[0].fd();
> -
> -		requestReady.emit(cookie, dmabuf);
> -
> -		requestsCount_++;
> -	}
> -
> -	void bufferComplete(Buffer *buffer)
> -	{
> -		availableBuffers_.push_back(buffer->index());
> -
> -		/*
> -		 * Pick the buffer for the next request among the available
> -		 * buffers.
> -		 *
> -		 * For the first 20 frames, select the buffer that has just
> -		 * completed to keep the mapping of dmabuf fds to buffers
> -		 * unchanged in the camera.
> -		 *
> -		 * For the next 20 frames, cycle randomly over the available
> -		 * buffers. The mapping should still be kept unchanged, as the
> -		 * camera should map using the cached fds.
> -		 *
> -		 * For the last 20 frames, cycles through all buffers, which
> -		 * should trash the mappings.
> -		 */
> -		unsigned int index = buffer->index();
> -		delete buffer;
> -
> -		std::cout << "Completed buffer, request=" << requestsCount_
> -			  << ", available buffers=" << availableBuffers_.size()
> -			  << std::endl;
> -
> -		if (requestsCount_ >= 60) {
> -			if (availableBuffers_.size() == SINK_BUFFER_COUNT)
> -				done_ = true;
> -			return;
> -		}
> -
> -		if (requestsCount_ == 40) {
> -			/* Add the remaining of the buffers. */
> -			for (unsigned int i = CAMERA_BUFFER_COUNT;
> -			     i < SINK_BUFFER_COUNT; ++i)
> -				availableBuffers_.push_back(i);
> -		}
> -
> -		if (requestsCount_ >= 20) {
> -			/*
> -			 * Wait until we have enough buffers to make this
> -			 * meaningful. Preferably half of the camera buffers,
> -			 * but no less than 2 in any case.
> -			 */
> -			const unsigned int min_pool_size =
> -				std::min(CAMERA_BUFFER_COUNT / 2, 2U);
> -			if (availableBuffers_.size() < min_pool_size)
> -				return;
> -
> -			/* Pick a buffer at random. */
> -			unsigned int pos = random_() % availableBuffers_.size();
> -			index = availableBuffers_[pos];
> -		}
> -
> -		queueRequest(index);
> -	}

If I understand this properly, the BufferSource class is now only used
as a source of buffers, and you don't cycle buffers back and forth
between the camera and the exporter. We're thus losing the ability to
trash the V4L2 buffer cache, which I think we should test. Isn't there a
way to keep this ?

> -
> -	std::shared_ptr<MediaDevice> media_;
> +	shared_ptr<MediaDevice> media_;
>  	V4L2VideoDevice *video_;
> -	BufferPool pool_;
> -	V4L2DeviceFormat format_;
> -
> -	unsigned int requestsCount_;
> -	std::vector<int> availableBuffers_;
> -	std::random_device random_;
> -
> -	bool done_;
> +	vector<FrameBuffer *> buffers_;
>  };
>  
> -class BufferImportTest : public CameraTest, public Test
> +class BufferImport : public CameraTest, public Test

Test classes should be named with a Test suffix. The other camera tests
don't follow the rule, they should be fixed.

>  {
>  public:
> -	BufferImportTest()
> +	BufferImport()
>  		: CameraTest("VIMC Sensor B")
>  	{
>  	}
>  
> -	void queueRequest(uint64_t cookie, int dmabuf)
> +protected:
> +	unsigned int completeBuffersCount_;
> +	unsigned int completeRequestsCount_;

How about moving private members (variables and methods) to the end in a
private section ? Variables should also go after methods.

> +
> +	void bufferComplete(Request *request, FrameBuffer *buffer)
>  	{
> -		Request *request = camera_->createRequest(cookie);
> +		if (buffer->info().status() != BufferInfo::BufferSuccess)
> +			return;
>  
> -		std::unique_ptr<Buffer> buffer = stream_->createBuffer({ dmabuf, -1, -1 });
> -		request->addBuffer(stream_, move(buffer));
> -		camera_->queueRequest(request);
> +		completeBuffersCount_++;
>  	}
>  
> -protected:
> -	void bufferComplete(Request *request, Buffer *buffer)
> +	void requestComplete(Request *request)
>  	{
> -		if (buffer->info().status() != BufferInfo::BufferSuccess)
> +		if (request->status() != Request::RequestComplete)
>  			return;
>  
> -		unsigned int index = buffer->index();
> -		int dmabuf = buffer->dmabufs()[0];
> +		const map<Stream *, FrameBuffer *> &buffers = request->buffers();
>  
> -		/* Record dmabuf to index remappings. */
> -		bool remapped = false;
> -		if (bufferMappings_.find(index) != bufferMappings_.end()) {
> -			if (bufferMappings_[index] != dmabuf)
> -				remapped = true;
> -		}
> +		completeRequestsCount_++;
>  
> -		std::cout << "Completed request " << framesCaptured_
> -			  << ": dmabuf fd " << dmabuf
> -			  << " -> index " << index
> -			  << " (" << (remapped ? 'R' : '-') << ")"
> -			  << std::endl;
> +		/* Create a new request. */
> +		Stream *stream = buffers.begin()->first;
> +		FrameBuffer *buffer = buffers.begin()->second;
>  
> -		if (remapped)
> -			bufferRemappings_.push_back(framesCaptured_);
> +		request = camera_->createRequest();
> +		request->addBuffer(stream, buffer);
> +		camera_->queueRequest(request);
> +	}
>  
> -		bufferMappings_[index] = dmabuf;
> -		framesCaptured_++;
> +	int init() override
> +	{
> +		if (status_ != TestPass)
> +			return status_;
>  
> -		sink_.requestComplete(request->cookie(), buffer);
> +		config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> +		if (!config_ || config_->size() != 1) {
> +			cout << "Failed to generate default configuration" << endl;
> +			return TestFail;
> +		}
>  
> -		if (framesCaptured_ == 60)
> -			sink_.stop();
> +		return TestPass;
>  	}
>  
> -	int initCamera()
> +	int run() override
>  	{
> -		if (camera_->acquire()) {
> -			std::cout << "Failed to acquire the camera" << std::endl;
> -			return TestFail;
> -		}
> +		StreamConfiguration &cfg = config_->at(0);
>  
> -		/*
> -		 * Configure the Stream to work with externally allocated
> -		 * buffers by setting the memoryType to ExternalMemory.
> -		 */

This is hard to review as it's a rewrite of the test, and removing all
comments without adding new ones don't help. Please add comments where
appropriate, and explain how the buffer import test is modified in the
commit message. Ideally this should have been split to a separate
commit, but I understand it will break bisection (although I'm not sure
any issue could be meaningfully bisected to the middle of this series
:-)).

> -		std::unique_ptr<CameraConfiguration> config;
> -		config = camera_->generateConfiguration({ StreamRole::VideoRecording });
> -		if (!config || config->size() != 1) {
> -			std::cout << "Failed to generate configuration" << std::endl;
> +		if (camera_->acquire()) {
> +			cout << "Failed to acquire the camera" << endl;
>  			return TestFail;
>  		}
>  
> -		StreamConfiguration &cfg = config->at(0);
> -		cfg.size = sink_.size();
> -		cfg.pixelFormat = sink_.format();
> -		cfg.bufferCount = CAMERA_BUFFER_COUNT;
> -		cfg.memoryType = ExternalMemory;
> -
> -		if (camera_->configure(config.get())) {
> -			std::cout << "Failed to set configuration" << std::endl;
> +		if (camera_->configure(config_.get())) {
> +			cout << "Failed to set default configuration" << endl;
>  			return TestFail;
>  		}
>  
> -		stream_ = cfg.stream();
> -
> -		/* Allocate buffers. */
>  		if (camera_->allocateBuffers()) {
> -			std::cout << "Failed to allocate buffers" << std::endl;
> +			cout << "Failed to allocate buffers" << endl;
>  			return TestFail;
>  		}
>  
> -		/* Connect the buffer completed signal. */
> -		camera_->bufferCompleted.connect(this, &BufferImportTest::bufferComplete);
> +		Stream *stream = cfg.stream();
>  
> -		return TestPass;
> -	}
> +		BufferSource source;
> +		int ret = source.allocate(cfg);
> +		if (ret < 0)
> +			return TestFail;
>  
> -	int init()
> -	{
> -		if (status_ != TestPass)
> -			return status_;
> +		vector<Request *> requests;
> +		for (FrameBuffer *buffer : source.buffers()) {
> +			Request *request = camera_->createRequest();
> +			if (!request) {
> +				cout << "Failed to create request" << endl;
> +				return TestFail;
> +			}
>  
> -		int ret = sink_.init();
> -		if (ret != TestPass) {
> -			cleanup();
> -			return ret;
> -		}
> +			if (request->addBuffer(stream, buffer)) {
> +				cout << "Failed to associating buffer with request" << endl;
> +				return TestFail;
> +			}
>  
> -		ret = initCamera();
> -		if (ret != TestPass) {
> -			cleanup();
> -			return ret;
> +			requests.push_back(request);
>  		}
>  
> -		sink_.requestReady.connect(this, &BufferImportTest::queueRequest);
> -		return TestPass;
> -	}
> -
> -	int run()
> -	{
> -		int ret;
> +		completeRequestsCount_ = 0;
> +		completeBuffersCount_ = 0;
>  
> -		framesCaptured_ = 0;
> +		camera_->bufferCompleted.connect(this, &BufferImport::bufferComplete);
> +		camera_->requestCompleted.connect(this, &BufferImport::requestComplete);
>  
>  		if (camera_->start()) {
> -			std::cout << "Failed to start camera" << std::endl;
> +			cout << "Failed to start camera" << endl;
>  			return TestFail;
>  		}
>  
> -		ret = sink_.start();
> -		if (ret < 0) {
> -			std::cout << "Failed to start sink" << std::endl;
> -			return TestFail;
> +		for (Request *request : requests) {
> +			if (camera_->queueRequest(request)) {
> +				cout << "Failed to queue request" << endl;
> +				return TestFail;
> +			}
>  		}
>  
>  		EventDispatcher *dispatcher = cm_->eventDispatcher();
>  
>  		Timer timer;
> -		timer.start(5000);
> -		while (timer.isRunning() && !sink_.done())
> +		timer.start(1000);
> +		while (timer.isRunning())
>  			dispatcher->processEvents();
>  
> -		std::cout << framesCaptured_ << " frames captured, "
> -			  << bufferRemappings_.size() << " buffers remapped"
> -			  << std::endl;
> +		unsigned int nbuffers = source.buffers().size();
>  
> -		if (framesCaptured_ < 60) {
> -			std::cout << "Too few frames captured" << std::endl;
> +		if (completeRequestsCount_ <= nbuffers * 2) {
> +			cout << "Failed to capture enough frames (got "
> +			     << completeRequestsCount_ << " expected at least "
> +			     << nbuffers * 2 << ")" << endl;
>  			return TestFail;
>  		}
>  
> -		if (bufferRemappings_.empty()) {
> -			std::cout << "No buffer remappings" << std::endl;
> +		if (completeRequestsCount_ != completeBuffersCount_) {
> +			cout << "Number of completed buffers and requests differ" << endl;
>  			return TestFail;
>  		}
>  
> -		if (bufferRemappings_[0] < 40) {
> -			std::cout << "Early buffer remapping" << std::endl;
> +		if (camera_->stop()) {
> +			cout << "Failed to stop camera" << endl;
>  			return TestFail;
>  		}
>  
> -		return TestPass;
> -	}
> -
> -	void cleanup()
> -	{
> -		sink_.cleanup();
> +		if (camera_->freeBuffers()) {
> +			cout << "Failed to free buffers" << endl;
> +			return TestFail;
> +		}
>  
> -		camera_->stop();
> -		camera_->freeBuffers();
> +		return TestPass;
>  	}
>  
> -private:
> -	Stream *stream_;
> -
> -	std::map<unsigned int, int> bufferMappings_;
> -	std::vector<unsigned int> bufferRemappings_;
> -	unsigned int framesCaptured_;
> -
> -	FrameSink sink_;

Shouldn't this be private ?

> +	unique_ptr<CameraConfiguration> config_;
>  };
>  
> -TEST_REGISTER(BufferImportTest);
> +} /* namespace */
> +
> +TEST_REGISTER(BufferImport);
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index 8307ea2629801679..ee6d0a1bc1fa839d 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -26,7 +26,7 @@ protected:
>  	unsigned int completeBuffersCount_;
>  	unsigned int completeRequestsCount_;
>  
> -	void bufferComplete(Request *request, Buffer *buffer)
> +	void bufferComplete(Request *request, FrameBuffer *buffer)
>  	{
>  		if (buffer->info().status() != BufferInfo::BufferSuccess)
>  			return;
> @@ -39,17 +39,16 @@ protected:
>  		if (request->status() != Request::RequestComplete)
>  			return;
>  
> -		const std::map<Stream *, Buffer *> &buffers = request->buffers();
> +		const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
>  
>  		completeRequestsCount_++;
>  
>  		/* Create a new request. */
>  		Stream *stream = buffers.begin()->first;
> -		Buffer *buffer = buffers.begin()->second;
> -		std::unique_ptr<Buffer> newBuffer = stream->createBuffer(buffer->index());
> +		FrameBuffer *buffer = buffers.begin()->second;
>  
>  		request = camera_->createRequest();
> -		request->addBuffer(stream, std::move(newBuffer));
> +		request->addBuffer(stream, buffer);
>  		camera_->queueRequest(request);
>  	}
>  
> @@ -87,21 +86,21 @@ protected:
>  		}
>  
>  		Stream *stream = cfg.stream();
> +
> +		BufferAllocator allocator(camera_);
> +		int ret = allocator.allocate(stream, cfg);
> +		if (ret < 0)
> +			return TestFail;
> +
>  		std::vector<Request *> requests;
> -		for (unsigned int i = 0; i < cfg.bufferCount; ++i) {
> +		for (FrameBuffer *buffer : allocator.buffers(stream)) {
>  			Request *request = camera_->createRequest();
>  			if (!request) {
>  				cout << "Failed to create request" << endl;
>  				return TestFail;
>  			}
>  
> -			std::unique_ptr<Buffer> buffer = stream->createBuffer(i);
> -			if (!buffer) {
> -				cout << "Failed to create buffer " << i << endl;
> -				return TestFail;
> -			}
> -
> -			if (request->addBuffer(stream, std::move(buffer))) {
> +			if (request->addBuffer(stream, buffer)) {
>  				cout << "Failed to associating buffer with request" << endl;
>  				return TestFail;
>  			}
> @@ -134,10 +133,12 @@ protected:
>  		while (timer.isRunning())
>  			dispatcher->processEvents();
>  
> -		if (completeRequestsCount_ <= cfg.bufferCount * 2) {
> +		unsigned int nbuffers = allocator.buffers(stream).size();
> +
> +		if (completeRequestsCount_ <= nbuffers * 2) {

This is an interesting change, and ideally it shouldn't be needed.
Shouldn't the allocator guarantee that it will allocate the number of
buffers specified in the stream configuration ? Otherwise we'll have two
sources for the same piece of information, without a clear rule to tell
which one to trust. This is a potential issue in the API and needs to be
addressed (at least recorded in a \todo item, with some idea on how to
fix it, to make sure it won't require major changes in the API).

>  			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 f627b8f37422350e..e9468d806f977a63 100644
> --- a/test/camera/statemachine.cpp
> +++ b/test/camera/statemachine.cpp
> @@ -185,6 +185,12 @@ protected:
>  		if (camera_->allocateBuffers())
>  			return TestFail;
>  
> +		/* Use internally allocated buffers. */
> +		allocator_ = new BufferAllocator(camera_);
> +		Stream *stream = *camera_->streams().begin();
> +		if (allocator_->allocate(stream, defconf_->at(0)) < 0)
> +			return TestFail;

You're leaking allocator_, and allocating it in testConfigured() and
deleting it in testRuning() is a hack. Let's allocate it in init() and
delete it in cleanup(). Please run this test in valgrind to ensure there
is no memory leak or use after free.

> +
>  		if (camera_->start())
>  			return TestFail;
>  
> @@ -218,8 +224,7 @@ protected:
>  			return TestFail;
>  
>  		Stream *stream = *camera_->streams().begin();
> -		std::unique_ptr<Buffer> buffer = stream->createBuffer(0);
> -		if (request->addBuffer(stream, std::move(buffer)))
> +		if (request->addBuffer(stream, allocator_->buffers(stream)[0]))
>  			return TestFail;
>  
>  		if (camera_->queueRequest(request))
> @@ -229,6 +234,8 @@ protected:
>  		if (camera_->stop())
>  			return TestFail;
>  
> +		delete allocator_;
> +
>  		if (camera_->freeBuffers())
>  			return TestFail;
>  
> @@ -283,6 +290,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 6b71caef111693d6..1b6ff756ac53a96a 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_->externalBuffers(bufferCount);
> +		if (ret < 0) {
>  			std::cout << "Failed to import buffers" << std::endl;
>  			return TestFail;
>  		}
> @@ -90,7 +88,7 @@ protected:
>  		return 0;
>  	}
>  
> -	void captureBufferReady(Buffer *buffer)
> +	void captureBufferReady(FrameBuffer *buffer)
>  	{
>  		const BufferInfo &info = buffer->info();
>  
> @@ -101,10 +99,11 @@ protected:
>  			return;
>  
>  		output_->queueBuffer(buffer);
> +

Not necessarily needed.

>  		framesCaptured_++;
>  	}
>  
> -	void outputBufferReady(Buffer *buffer)
> +	void outputBufferReady(FrameBuffer *buffer)
>  	{
>  		const BufferInfo &info = buffer->info();
>  
> @@ -127,10 +126,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 (FrameBuffer *buffer : buffers_)
> +			capture_->queueBuffer(buffer);

Shouldn't you check the return value of queueBuffer() ? Same in the
other tests below. You could add a queueAllBuffers() helper method to
V4L2VideoDeviceTest() to wrap the loop and perform the error checking, I
think it would help.

>  
>  		ret = capture_->streamOn();
>  		if (ret) {
> diff --git a/test/v4l2_videodevice/capture_async.cpp b/test/v4l2_videodevice/capture_async.cpp
> index 0bc0067c50909c9d..78ac34c4799e9e5a 100644
> --- a/test/v4l2_videodevice/capture_async.cpp
> +++ b/test/v4l2_videodevice/capture_async.cpp
> @@ -20,7 +20,7 @@ public:
>  	CaptureAsyncTest()
>  		: V4L2VideoDeviceTest("vimc", "Raw Capture 0"), frames(0) {}
>  
> -	void receiveBuffer(Buffer *buffer)
> +	void receiveBuffer(FrameBuffer *buffer)
>  	{
>  		std::cout << "Received buffer" << std::endl;
>  		frames++;
> @@ -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 (FrameBuffer *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
> -		 */

Are we doing this now ?

>  		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 442bcac5dc49cc59..2dd4b9440b4d4d71 100644
> --- a/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> +++ b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> @@ -29,7 +29,7 @@ public:
>  	{
>  	}
>  
> -	void outputBufferComplete(Buffer *buffer)
> +	void outputBufferComplete(FrameBuffer *buffer)
>  	{
>  		cout << "Received output buffer" << endl;
>  
> @@ -39,7 +39,7 @@ public:
>  		vim2m_->output()->queueBuffer(buffer);
>  	}
>  
> -	void receiveCaptureBuffer(Buffer *buffer)
> +	void receiveCaptureBuffer(FrameBuffer *buffer)
>  	{
>  		cout << "Received capture buffer" << endl;
>  
> @@ -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 (FrameBuffer *buffer : captureBuffers_)
> +			capture->queueBuffer(buffer);

You need to check for errors here too, as well as below.

>  
> -		/* 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 (FrameBuffer *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<FrameBuffer *> captureBuffers_;
> +	std::vector<FrameBuffer *> outputBuffers_;

This is leaked. The good news is that turning it into a vector of unique
pointers will help :-)

>  
>  	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..02800c2a16f7b0f7 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<FrameBuffer *> buffers_;

Same here.

>  };
>  
>  #endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */