[{"id":3389,"web_url":"https://patchwork.libcamera.org/comment/3389/","msgid":"<20200108041701.GE10933@pendragon.ideasonboard.com>","date":"2020-01-08T04:17:01","subject":"Re: [libcamera-devel] [PATCH v2 20/25] libcamera: Switch to\n\tFrameBuffer interface","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Mon, Dec 30, 2019 at 01:05:05PM +0100, Niklas Söderlund wrote:\n> Switch to the FrameBuffer interface where all buffers are treated as\n> external buffers and are allocated outside the camera. Applications\n> allocating buffers using libcamera are switched to use the\n> FrameBufferAllocator helper.\n> \n> A follow up changes to this one will finalize the transition to the new\n\ns/A follow up/Follow-up/\n\n> FrameBuffer interface by removing code that is left unused after this\n> change.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  include/libcamera/camera.h               |   4 +-\n>  include/libcamera/request.h              |  14 +--\n>  src/android/camera_device.cpp            |  29 ++++--\n>  src/cam/buffer_writer.cpp                |   5 +-\n>  src/cam/buffer_writer.h                  |   3 +-\n>  src/cam/capture.cpp                      |  42 ++++----\n>  src/cam/capture.h                        |   5 +-\n>  src/libcamera/camera.cpp                 |  49 +++------\n>  src/libcamera/include/pipeline_handler.h |   5 +-\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 124 +++++++----------------\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  41 ++------\n>  src/libcamera/pipeline/uvcvideo.cpp      |  22 ++--\n>  src/libcamera/pipeline/vimc.cpp          |  22 ++--\n>  src/libcamera/pipeline_handler.cpp       |   2 +-\n>  src/libcamera/request.cpp                |  29 +++---\n>  src/qcam/main_window.cpp                 |  44 ++++----\n>  src/qcam/main_window.h                   |   6 +-\n>  test/camera/buffer_import.cpp            |  19 ++--\n>  test/camera/capture.cpp                  |  38 ++++---\n>  test/camera/statemachine.cpp             |  12 ++-\n>  20 files changed, 209 insertions(+), 306 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index f2827438871189a1..20f48faed62da7a7 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -19,7 +19,7 @@\n>  \n>  namespace libcamera {\n>  \n> -class Buffer;\n> +class FrameBuffer;\n>  class FrameBufferAllocator;\n>  class PipelineHandler;\n>  class Request;\n> @@ -78,7 +78,7 @@ public:\n>  \n>  \tconst std::string &name() const;\n>  \n> -\tSignal<Request *, Buffer *> bufferCompleted;\n> +\tSignal<Request *, FrameBuffer *> bufferCompleted;\n>  \tSignal<Request *> requestCompleted;\n>  \tSignal<Camera *> disconnected;\n>  \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index a8708010ec1247a2..02c0ef5f6b028bd4 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -17,9 +17,9 @@\n>  \n>  namespace libcamera {\n>  \n> -class Buffer;\n>  class Camera;\n>  class CameraControlValidator;\n> +class FrameBuffer;\n>  class Stream;\n>  \n>  class Request\n> @@ -38,9 +38,9 @@ public:\n>  \n>  \tControlList &controls() { return *controls_; }\n>  \tControlList &metadata() { return *metadata_; }\n> -\tconst std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }\n> -\tint addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer);\n> -\tBuffer *findBuffer(Stream *stream) const;\n> +\tconst std::map<Stream *, FrameBuffer *> &buffers() const { return bufferMap_; }\n> +\tint addBuffer(Stream *stream, FrameBuffer *buffer);\n> +\tFrameBuffer *findBuffer(Stream *stream) const;\n>  \n>  \tuint64_t cookie() const { return cookie_; }\n>  \tStatus status() const { return status_; }\n> @@ -54,14 +54,14 @@ private:\n>  \tint prepare();\n>  \tvoid complete();\n>  \n> -\tbool completeBuffer(Buffer *buffer);\n> +\tbool completeBuffer(FrameBuffer *buffer);\n>  \n>  \tCamera *camera_;\n>  \tCameraControlValidator *validator_;\n>  \tControlList *controls_;\n>  \tControlList *metadata_;\n> -\tstd::map<Stream *, Buffer *> bufferMap_;\n> -\tstd::unordered_set<Buffer *> pending_;\n> +\tstd::map<Stream *, FrameBuffer *> bufferMap_;\n> +\tstd::unordered_set<FrameBuffer *> pending_;\n>  \n>  \tconst uint64_t cookie_;\n>  \tStatus status_;\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ebe91ea8af4f6436..21417aba19135de5 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -739,13 +739,19 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque\n>  \t * and (currently) only supported request buffer.\n>  \t */\n>  \tconst buffer_handle_t camera3Handle = *camera3Buffers[0].buffer;\n> -\tstd::array<int, 3> fds = {\n> -\t\tcamera3Handle->data[0],\n> -\t\tcamera3Handle->data[1],\n> -\t\tcamera3Handle->data[2],\n> -\t};\n>  \n> -\tstd::unique_ptr<Buffer> buffer = stream->createBuffer(fds);\n> +\tstd::vector<FrameBuffer::Plane> planes;\n> +\tfor (int i = 0; i < 3; i++) {\n> +\t\tplanes[i].fd = FileDescriptor(camera3Handle->data[i]);\n\nYou haven't tested this, have you ? :-) planes is empty at this point.\n\n> +\t\t/*\n> +\t\t * Setting length to zero here is OK as the length is only used\n> +\t\t * to map the memory of the plane. Libcamera do not need to poke\n> +\t\t * at the memory content queued by the HAL.\n> +\t\t */\n> +\t\tplanes[i].length = 0;\n> +\t}\n> +\n> +\tFrameBuffer *buffer = new FrameBuffer(planes);\n>  \tif (!buffer) {\n>  \t\tLOG(HAL, Error) << \"Failed to create buffer\";\n>  \t\tdelete descriptor;\n> @@ -754,7 +760,7 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque\n>  \n>  \tRequest *request =\n>  \t\tcamera_->createRequest(reinterpret_cast<uint64_t>(descriptor));\n> -\trequest->addBuffer(stream, std::move(buffer));\n> +\trequest->addBuffer(stream, buffer);\n>  \n>  \tint ret = camera_->queueRequest(request);\n>  \tif (ret) {\n> @@ -771,8 +777,8 @@ error:\n>  \n>  void CameraDevice::requestComplete(Request *request)\n>  {\n> -\tconst std::map<Stream *, Buffer *> &buffers = request->buffers();\n> -\tBuffer *libcameraBuffer = buffers.begin()->second;\n> +\tconst std::map<Stream *, FrameBuffer *> &buffers = request->buffers();\n> +\tFrameBuffer *buffer = buffers.begin()->second;\n>  \tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n>  \tstd::unique_ptr<CameraMetadata> resultMetadata;\n>  \n> @@ -803,11 +809,11 @@ void CameraDevice::requestComplete(Request *request)\n>  \n>  \tif (status == CAMERA3_BUFFER_STATUS_OK) {\n>  \t\tnotifyShutter(descriptor->frameNumber,\n> -\t\t\t      libcameraBuffer->metadata().timestamp);\n> +\t\t\t      buffer->metadata().timestamp);\n>  \n>  \t\tcaptureResult.partial_result = 1;\n>  \t\tresultMetadata = getResultMetadata(descriptor->frameNumber,\n> -\t\t\t\t\t\t   libcameraBuffer->metadata().timestamp);\n> +\t\t\t\t\t\t   buffer->metadata().timestamp);\n>  \t\tcaptureResult.result = resultMetadata->get();\n>  \t}\n>  \n> @@ -825,6 +831,7 @@ void CameraDevice::requestComplete(Request *request)\n>  \tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>  \n>  \tdelete descriptor;\n> +\tdelete buffer;\n>  }\n>  \n>  void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)\n> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> index 7c58a1f50829f290..320ae3332a4db4a6 100644\n> --- a/src/cam/buffer_writer.cpp\n> +++ b/src/cam/buffer_writer.cpp\n> @@ -22,7 +22,7 @@ BufferWriter::BufferWriter(const std::string &pattern)\n>  {\n>  }\n>  \n> -int BufferWriter::write(Buffer *buffer, const std::string &streamName)\n> +int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)\n>  {\n>  \tstd::string filename;\n>  \tsize_t pos;\n> @@ -43,8 +43,7 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)\n>  \tif (fd == -1)\n>  \t\treturn -errno;\n>  \n> -\tBufferMemory *mem = buffer->mem();\n> -\tfor (const FrameBuffer::Plane &plane : mem->planes()) {\n> +\tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n>  \t\tvoid *data = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,\n>  \t\t\t\t  plane.fd.fd(), 0);\n>  \t\tunsigned int length = plane.length;\n> diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h\n> index 7bf785d1e83235ff..5917a7dfb5e28106 100644\n> --- a/src/cam/buffer_writer.h\n> +++ b/src/cam/buffer_writer.h\n> @@ -16,7 +16,8 @@ class BufferWriter\n>  public:\n>  \tBufferWriter(const std::string &pattern = \"frame-#.bin\");\n>  \n> -\tint write(libcamera::Buffer *buffer, const std::string &streamName);\n> +\tint write(libcamera::FrameBuffer *buffer,\n> +\t\t  const std::string &streamName);\n>  \n>  private:\n>  \tstd::string pattern_;\n> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> index da942f56118983bd..dd078eb0ae4a2c62 100644\n> --- a/src/cam/capture.cpp\n> +++ b/src/cam/capture.cpp\n> @@ -57,7 +57,10 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)\n>  \t\t\twriter_ = new BufferWriter();\n>  \t}\n>  \n> -\tret = capture(loop);\n> +\n> +\tFrameBufferAllocator *allocator = FrameBufferAllocator::create(camera_);\n> +\n> +\tret = capture(loop, allocator);\n>  \n>  \tif (options.isSet(OptFile)) {\n>  \t\tdelete writer_;\n> @@ -66,17 +69,27 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)\n>  \n>  \tcamera_->freeBuffers();\n>  \n> +\tdelete allocator;\n> +\n>  \treturn ret;\n>  }\n>  \n> -int Capture::capture(EventLoop *loop)\n> +int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)\n>  {\n>  \tint ret;\n>  \n>  \t/* Identify the stream with the least number of buffers. */\n>  \tunsigned int nbuffers = UINT_MAX;\n> -\tfor (StreamConfiguration &cfg : *config_)\n> -\t\tnbuffers = std::min(nbuffers, cfg.bufferCount);\n> +\tfor (StreamConfiguration &cfg : *config_) {\n> +\t\tret = allocator->allocate(cfg.stream());\n> +\t\tif (ret < 0) {\n> +\t\t\tstd::cerr << \"Can't allocate buffers\" << std::endl;\n> +\t\t\treturn -ENOMEM;\n> +\t\t}\n> +\n> +\t\tunsigned int allocated = allocator->buffers(cfg.stream()).size();\n> +\t\tnbuffers = std::min(nbuffers, allocated);\n> +\t}\n>  \n>  \t/*\n>  \t * TODO: make cam tool smarter to support still capture by for\n> @@ -93,9 +106,11 @@ int Capture::capture(EventLoop *loop)\n>  \n>  \t\tfor (StreamConfiguration &cfg : *config_) {\n>  \t\t\tStream *stream = cfg.stream();\n> -\t\t\tstd::unique_ptr<Buffer> buffer = stream->createBuffer(i);\n> +\t\t\tconst std::vector<std::unique_ptr<FrameBuffer>> &buffers =\n> +\t\t\t\tallocator->buffers(stream);\n> +\t\t\tconst std::unique_ptr<FrameBuffer> &buffer = buffers[i];\n>  \n> -\t\t\tret = request->addBuffer(stream, std::move(buffer));\n> +\t\t\tret = request->addBuffer(stream, buffer.get());\n>  \t\t\tif (ret < 0) {\n>  \t\t\t\tstd::cerr << \"Can't set buffer for request\"\n>  \t\t\t\t\t  << std::endl;\n> @@ -138,7 +153,7 @@ void Capture::requestComplete(Request *request)\n>  \tif (request->status() == Request::RequestCancelled)\n>  \t\treturn;\n>  \n> -\tconst std::map<Stream *, Buffer *> &buffers = request->buffers();\n> +\tconst std::map<Stream *, FrameBuffer *> &buffers = request->buffers();\n>  \n>  \tstd::chrono::steady_clock::time_point now = std::chrono::steady_clock::now();\n>  \tdouble fps = std::chrono::duration_cast<std::chrono::milliseconds>(now - last_).count();\n> @@ -151,7 +166,7 @@ void Capture::requestComplete(Request *request)\n>  \n>  \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n>  \t\tStream *stream = it->first;\n> -\t\tBuffer *buffer = it->second;\n> +\t\tFrameBuffer *buffer = it->second;\n>  \t\tconst std::string &name = streamName_[stream];\n>  \n>  \t\tconst FrameMetadata &metadata = buffer->metadata();\n> @@ -185,16 +200,9 @@ void Capture::requestComplete(Request *request)\n>  \n>  \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n>  \t\tStream *stream = it->first;\n> -\t\tBuffer *buffer = it->second;\n> -\t\tunsigned int index = buffer->index();\n> -\n> -\t\tstd::unique_ptr<Buffer> newBuffer = stream->createBuffer(index);\n> -\t\tif (!newBuffer) {\n> -\t\t\tstd::cerr << \"Can't create buffer\" << std::endl;\n> -\t\t\treturn;\n> -\t\t}\n> +\t\tFrameBuffer *buffer = it->second;\n>  \n> -\t\trequest->addBuffer(stream, std::move(newBuffer));\n> +\t\trequest->addBuffer(stream, buffer);\n>  \t}\n>  \n>  \tcamera_->queueRequest(request);\n> diff --git a/src/cam/capture.h b/src/cam/capture.h\n> index c692d48918f2de1d..9bca5661070efcf5 100644\n> --- a/src/cam/capture.h\n> +++ b/src/cam/capture.h\n> @@ -10,7 +10,9 @@\n>  #include <chrono>\n>  #include <memory>\n>  \n> +#include <libcamera/buffer.h>\n>  #include <libcamera/camera.h>\n> +#include <libcamera/framebuffer_allocator.h>\n>  #include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n>  \n> @@ -26,7 +28,8 @@ public:\n>  \n>  \tint run(EventLoop *loop, const OptionsParser::Options &options);\n>  private:\n> -\tint capture(EventLoop *loop);\n> +\tint capture(EventLoop *loop,\n> +\t\t    libcamera::FrameBufferAllocator *allocator);\n>  \n>  \tvoid requestComplete(libcamera::Request *request);\n>  \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 222c6811d5698f0f..5ea663ef85720f2e 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -691,12 +691,6 @@ int Camera::configure(CameraConfiguration *config)\n>  \n>  \t\tstream->configuration_ = cfg;\n>  \t\tactiveStreams_.insert(stream);\n> -\n> -\t\t/*\n> -\t\t * Allocate buffer objects in the pool.\n> -\t\t * Memory will be allocated and assigned later.\n> -\t\t */\n> -\t\tstream->createBuffers(cfg.memoryType, cfg.bufferCount);\n>  \t}\n>  \n>  \tstate_ = CameraConfigured;\n> @@ -752,14 +746,6 @@ int Camera::freeBuffers()\n>  \tif (!stateIs(CameraPrepared))\n>  \t\treturn -EACCES;\n>  \n> -\tfor (Stream *stream : activeStreams_) {\n> -\t\t/*\n> -\t\t * All mappings must be destroyed before buffers can be freed\n> -\t\t * by the V4L2 device that has allocated them.\n> -\t\t */\n> -\t\tstream->destroyBuffers();\n> -\t}\n> -\n>  \tstate_ = CameraConfigured;\n>  \n>  \treturn pipe_->freeBuffers(this, activeStreams_);\n> @@ -825,24 +811,11 @@ int Camera::queueRequest(Request *request)\n>  \n>  \tfor (auto const &it : request->buffers()) {\n>  \t\tStream *stream = it.first;\n> -\t\tBuffer *buffer = it.second;\n>  \n>  \t\tif (activeStreams_.find(stream) == activeStreams_.end()) {\n>  \t\t\tLOG(Camera, Error) << \"Invalid request\";\n>  \t\t\treturn -EINVAL;\n>  \t\t}\n> -\n> -\t\tif (stream->memoryType() == ExternalMemory) {\n> -\t\t\tint index = stream->mapBuffer(buffer);\n> -\t\t\tif (index < 0) {\n> -\t\t\t\tLOG(Camera, Error) << \"No buffer memory available\";\n> -\t\t\t\treturn -ENOMEM;\n> -\t\t\t}\n> -\n> -\t\t\tbuffer->index_ = index;\n> -\t\t}\n> -\n> -\t\tbuffer->mem_ = &stream->buffers()[buffer->index_];\n>  \t}\n>  \n>  \tint ret = request->prepare();\n> @@ -877,6 +850,14 @@ int Camera::start()\n>  \n>  \tLOG(Camera, Debug) << \"Starting capture\";\n>  \n> +\tfor (Stream * stream : activeStreams_) {\n> +\t\tif (allocator_ && !allocator_->buffers(stream).empty())\n> +\t\t\tcontinue;\n> +\n> +\t\tunsigned int bufferCount = stream->configuration_.bufferCount;\n> +\t\tpipe_->importFrameBuffers(this, stream, bufferCount);\n\nMaybe ditch the bufferCount local variable ? And actually, how about\nditching the bufferCount parameter to importFrameBuffers() ? The\npipeline handlers can get it from stream->configuration_.bufferCount\nthemselves.\n\n> +\t}\n> +\n>  \tint ret = pipe_->start(this);\n>  \tif (ret)\n>  \t\treturn ret;\n> @@ -912,6 +893,13 @@ int Camera::stop()\n>  \n>  \tpipe_->stop(this);\n>  \n> +\tfor (Stream * stream : activeStreams_) {\n> +\t\tif (allocator_ && !allocator_->buffers(stream).empty())\n> +\t\t\tcontinue;\n> +\n> +\t\tpipe_->freeFrameBuffers(this, stream);\n> +\t}\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -925,13 +913,6 @@ int Camera::stop()\n>   */\n>  void Camera::requestComplete(Request *request)\n>  {\n> -\tfor (auto it : request->buffers()) {\n> -\t\tStream *stream = it.first;\n> -\t\tBuffer *buffer = it.second;\n> -\t\tif (stream->memoryType() == ExternalMemory)\n> -\t\t\tstream->unmapBuffer(buffer);\n> -\t}\n> -\n>  \trequestCompleted.emit(request);\n>  \tdelete request;\n>  }\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 520d3fccaaa130cc..20b866c4a511387a 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -20,12 +20,12 @@\n>  \n>  namespace libcamera {\n>  \n> -class Buffer;\n>  class Camera;\n>  class CameraConfiguration;\n>  class CameraManager;\n>  class DeviceEnumerator;\n>  class DeviceMatch;\n> +class FrameBuffer;\n>  class MediaDevice;\n>  class PipelineHandler;\n>  class Request;\n> @@ -86,7 +86,8 @@ public:\n>  \n>  \tint queueRequest(Camera *camera, Request *request);\n>  \n> -\tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n> +\tbool completeBuffer(Camera *camera, Request *request,\n> +\t\t\t    FrameBuffer *buffer);\n>  \tvoid completeRequest(Camera *camera, Request *request);\n>  \n>  \tconst char *name() const { return name_; }\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index f9bddcc88523301f..2f3ba2bf10f2f177 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -31,6 +31,8 @@ namespace libcamera {\n>  \n>  LOG_DEFINE_CATEGORY(IPU3)\n>  \n> +class IPU3CameraData;\n> +\n>  class ImgUDevice\n>  {\n>  public:\n> @@ -44,7 +46,6 @@ public:\n>  \t\tV4L2VideoDevice *dev;\n>  \t\tunsigned int pad;\n>  \t\tstd::string name;\n> -\t\tBufferPool *pool;\n>  \t\tstd::vector<std::unique_ptr<FrameBuffer>> buffers;\n>  \t};\n>  \n> @@ -71,9 +72,7 @@ public:\n>  \tint configureOutput(ImgUOutput *output,\n>  \t\t\t    const StreamConfiguration &cfg);\n>  \n> -\tint importOutputBuffers(ImgUOutput *output, BufferPool *pool);\n> -\tint exportOutputBuffers(ImgUOutput *output, BufferPool *pool);\n> -\tvoid freeBuffers();\n> +\tvoid freeBuffers(IPU3CameraData *data);\n>  \n>  \tint start();\n>  \tint stop();\n> @@ -93,9 +92,6 @@ public:\n>  \tImgUOutput viewfinder_;\n>  \tImgUOutput stat_;\n>  \t/* \\todo Add param video device for 3A tuning */\n> -\n> -\tBufferPool vfPool_;\n> -\tBufferPool outPool_;\n>  };\n>  \n>  class CIO2Device\n> @@ -156,7 +152,7 @@ public:\n>  \t{\n>  \t}\n>  \n> -\tvoid imguOutputBufferReady(Buffer *buffer);\n> +\tvoid imguOutputBufferReady(FrameBuffer *buffer);\n>  \tvoid imguInputBufferReady(FrameBuffer *buffer);\n>  \tvoid cio2BufferReady(FrameBuffer *buffer);\n>  \n> @@ -693,39 +689,30 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n\nHmmmm... allocateFrameBuffers() and importFrameBuffers() are supposed to\nallocate other internal resources for the streams, but we still call\nPipelineHandlerIPU3::allocateBuffers() from\nPipelineHandlerIPU3::start(). There's something not quite right here :-S\nI may accept to fix this on top of this series, but we're getting very\nclose to a point where I would consider this as a blocker, as it has\nimplications on the API. More work is clearly needed, the\nallocateFrameBuffers(), importFrameBuffers() and start() operations\nprobably need to be revisited.\n\n>  \t\tgoto error;\n>  \t}\n>  \n> -\t/* Allocate buffers for each active stream. */\n> -\tfor (Stream *s : streams) {\n> -\t\tIPU3Stream *stream = static_cast<IPU3Stream *>(s);\n> -\t\tImgUDevice::ImgUOutput *dev = stream->device_;\n> -\n> -\t\tif (stream->memoryType() == InternalMemory)\n> -\t\t\tret = imgu->exportOutputBuffers(dev, &stream->bufferPool());\n> -\t\telse\n> -\t\t\tret = imgu->importOutputBuffers(dev, &stream->bufferPool());\n> -\t\tif (ret)\n> -\t\t\tgoto error;\n> -\t}\n> -\n>  \t/*\n>  \t * Allocate buffers also on non-active outputs; use the same number\n>  \t * of buffers as the active ones.\n>  \t */\n>  \tif (!outStream->active_) {\n> -\t\tbufferCount = vfStream->configuration().bufferCount;\n> -\t\toutStream->device_->pool->createBuffers(bufferCount);\n> -\t\tret = imgu->exportOutputBuffers(outStream->device_,\n> -\t\t\t\t\t\toutStream->device_->pool);\n> -\t\tif (ret)\n> +\t\tImgUDevice::ImgUOutput *output = outStream->device_;\n> +\n> +\t\tret = output->dev->exportBuffers(bufferCount, &output->buffers);\n> +\t\tif (ret < 0) {\n> +\t\t\tLOG(IPU3, Error) << \"Failed to allocate ImgU \"\n> +\t\t\t\t\t << output->name << \" buffers\";\n>  \t\t\tgoto error;\n> +\t\t}\n>  \t}\n>  \n>  \tif (!vfStream->active_) {\n> -\t\tbufferCount = outStream->configuration().bufferCount;\n> -\t\tvfStream->device_->pool->createBuffers(bufferCount);\n> -\t\tret = imgu->exportOutputBuffers(vfStream->device_,\n> -\t\t\t\t\t\tvfStream->device_->pool);\n> -\t\tif (ret)\n> +\t\tImgUDevice::ImgUOutput *output = vfStream->device_;\n> +\n> +\t\tret = output->dev->exportBuffers(bufferCount, &output->buffers);\n> +\t\tif (ret < 0) {\n> +\t\t\tLOG(IPU3, Error) << \"Failed to allocate ImgU \"\n> +\t\t\t\t\t << output->name << \" buffers\";\n>  \t\t\tgoto error;\n> +\t\t}\n>  \t}\n>  \n>  \treturn 0;\n> @@ -742,7 +729,7 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera,\n>  \tIPU3CameraData *data = cameraData(camera);\n>  \n>  \tdata->cio2_.freeBuffers();\n> -\tdata->imgu_->freeBuffers();\n> +\tdata->imgu_->freeBuffers(data);\n>  \n>  \treturn 0;\n>  }\n> @@ -795,7 +782,7 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n>  \n>  \tfor (auto it : request->buffers()) {\n>  \t\tIPU3Stream *stream = static_cast<IPU3Stream *>(it.first);\n> -\t\tBuffer *buffer = it.second;\n> +\t\tFrameBuffer *buffer = it.second;\n>  \n>  \t\tint ret = stream->device_->dev->queueBuffer(buffer);\n>  \t\tif (ret < 0)\n> @@ -922,9 +909,9 @@ int PipelineHandlerIPU3::registerCameras()\n>  \t\t\t\t\t&IPU3CameraData::cio2BufferReady);\n>  \t\tdata->imgu_->input_->frameBufferReady.connect(data.get(),\n>  \t\t\t\t\t&IPU3CameraData::imguInputBufferReady);\n> -\t\tdata->imgu_->output_.dev->bufferReady.connect(data.get(),\n> +\t\tdata->imgu_->output_.dev->frameBufferReady.connect(data.get(),\n>  \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n> -\t\tdata->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),\n> +\t\tdata->imgu_->viewfinder_.dev->frameBufferReady.connect(data.get(),\n>  \t\t\t\t\t&IPU3CameraData::imguOutputBufferReady);\n>  \n>  \t\t/* Create and register the Camera instance. */\n> @@ -973,7 +960,7 @@ void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer)\n>   *\n>   * Buffers completed from the ImgU output are directed to the application.\n>   */\n> -void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> +void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>  {\n>  \tRequest *request = buffer->request();\n>  \n> @@ -1048,7 +1035,6 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n>  \n>  \toutput_.pad = PAD_OUTPUT;\n>  \toutput_.name = \"output\";\n> -\toutput_.pool = &outPool_;\n>  \n>  \tviewfinder_.dev = V4L2VideoDevice::fromEntityName(media,\n>  \t\t\t\t\t\t\t  name_ + \" viewfinder\");\n> @@ -1058,7 +1044,6 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n>  \n>  \tviewfinder_.pad = PAD_VF;\n>  \tviewfinder_.name = \"viewfinder\";\n> -\tviewfinder_.pool = &vfPool_;\n>  \n>  \tstat_.dev = V4L2VideoDevice::fromEntityName(media, name_ + \" 3a stat\");\n>  \tret = stat_.dev->open();\n> @@ -1166,69 +1151,28 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n>  \treturn 0;\n>  }\n>  \n> -/**\n> - * \\brief Export buffers from \\a output to the provided \\a pool\n> - * \\param[in] output The ImgU output device\n> - * \\param[in] pool The buffer pool where to export buffers\n> - *\n> - * Export memory buffers reserved in the video device memory associated with\n> - * \\a output id to the buffer pool provided as argument.\n> - *\n> - * \\return 0 on success or a negative error code otherwise\n> - */\n> -int ImgUDevice::exportOutputBuffers(ImgUOutput *output, BufferPool *pool)\n> -{\n> -\tint ret = output->dev->exportBuffers(pool);\n> -\tif (ret) {\n> -\t\tLOG(IPU3, Error) << \"Failed to export ImgU \"\n> -\t\t\t\t << output->name << \" buffers\";\n> -\t\treturn ret;\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> -\n> -/**\n> - * \\brief Reserve buffers in \\a output from the provided \\a pool\n> - * \\param[in] output The ImgU output device\n> - * \\param[in] pool The buffer pool used to reserve buffers in \\a output\n> - *\n> - * Reserve a number of buffers equal to the number of buffers in \\a pool\n> - * in the \\a output device.\n> - *\n> - * \\return 0 on success or a negative error code otherwise\n> - */\n> -int ImgUDevice::importOutputBuffers(ImgUOutput *output, BufferPool *pool)\n> -{\n> -\tint ret = output->dev->importBuffers(pool);\n> -\tif (ret) {\n> -\t\tLOG(IPU3, Error)\n> -\t\t\t<< \"Failed to import buffer in \" << output->name\n> -\t\t\t<< \" ImgU device\";\n> -\t\treturn ret;\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> -\n>  /**\n>   * \\brief Release buffers for all the ImgU video devices\n>   */\n> -void ImgUDevice::freeBuffers()\n> +void ImgUDevice::freeBuffers(IPU3CameraData *data)\n>  {\n>  \tint ret;\n>  \n> -\tret = output_.dev->releaseBuffers();\n> -\tif (ret)\n> -\t\tLOG(IPU3, Error) << \"Failed to release ImgU output buffers\";\n> +\tif (!data->outStream_.active_) {\n\nThis makes me even more convinced that we're not there yet. It's too\ncomplicated for pipeline handlers.\n\n> +\t\tret = output_.dev->releaseBuffers();\n> +\t\tif (ret)\n> +\t\t\tLOG(IPU3, Error) << \"Failed to release ImgU output buffers\";\n> +\t}\n>  \n>  \tret = stat_.dev->releaseBuffers();\n>  \tif (ret)\n>  \t\tLOG(IPU3, Error) << \"Failed to release ImgU stat buffers\";\n>  \n> -\tret = viewfinder_.dev->releaseBuffers();\n> -\tif (ret)\n> -\t\tLOG(IPU3, Error) << \"Failed to release ImgU viewfinder buffers\";\n> +\tif (!data->vfStream_.active_) {\n> +\t\tret = viewfinder_.dev->releaseBuffers();\n> +\t\tif (ret)\n> +\t\t\tLOG(IPU3, Error) << \"Failed to release ImgU viewfinder buffers\";\n> +\t}\n>  \n>  \tret = input_->releaseBuffers();\n>  \tif (ret)\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index e77925f6f9deff08..cc9c9ff961e948dc 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -51,7 +51,7 @@ struct RkISP1FrameInfo {\n>  \n>  \tFrameBuffer *paramBuffer;\n>  \tFrameBuffer *statBuffer;\n> -\tBuffer *videoBuffer;\n> +\tFrameBuffer *videoBuffer;\n>  \n>  \tbool paramFilled;\n>  \tbool paramDequeued;\n> @@ -67,7 +67,6 @@ public:\n>  \tint destroy(unsigned int frame);\n>  \n>  \tRkISP1FrameInfo *find(unsigned int frame);\n> -\tRkISP1FrameInfo *find(Buffer *buffer);\n>  \tRkISP1FrameInfo *find(FrameBuffer *buffer);\n>  \tRkISP1FrameInfo *find(Request *request);\n>  \n> @@ -87,7 +86,7 @@ public:\n>  \t\tsetDelay(QueueBuffers, -1, 10);\n>  \t}\n>  \n> -\tvoid bufferReady(Buffer *buffer)\n> +\tvoid bufferReady(FrameBuffer *buffer)\n>  \t{\n>  \t\t/*\n>  \t\t * Calculate SOE by taking the end of DMA set by the kernel and applying\n> @@ -208,7 +207,7 @@ private:\n>  \tint initLinks();\n>  \tint createCamera(MediaEntity *sensor);\n>  \tvoid tryCompleteRequest(Request *request);\n> -\tvoid bufferReady(Buffer *buffer);\n> +\tvoid bufferReady(FrameBuffer *buffer);\n>  \tvoid paramReady(FrameBuffer *buffer);\n>  \tvoid statReady(FrameBuffer *buffer);\n>  \n> @@ -246,7 +245,7 @@ RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stre\n>  \t}\n>  \tFrameBuffer *statBuffer = pipe_->statBuffers_.front();\n>  \n> -\tBuffer *videoBuffer = request->findBuffer(stream);\n> +\tFrameBuffer *videoBuffer = request->findBuffer(stream);\n>  \tif (!videoBuffer) {\n>  \t\tLOG(RkISP1, Error)\n>  \t\t\t<< \"Attempt to queue request with invalid stream\";\n> @@ -299,26 +298,14 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n>  \treturn nullptr;\n>  }\n>  \n> -RkISP1FrameInfo *RkISP1Frames::find(Buffer *buffer)\n> -{\n> -\tfor (auto &itInfo : frameInfo_) {\n> -\t\tRkISP1FrameInfo *info = itInfo.second;\n> -\n> -\t\tif (info->videoBuffer == buffer)\n> -\t\t\treturn info;\n> -\t}\n> -\n> -\tLOG(RkISP1, Error) << \"Can't locate info from buffer\";\n> -\treturn nullptr;\n> -}\n> -\n>  RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n>  {\n>  \tfor (auto &itInfo : frameInfo_) {\n>  \t\tRkISP1FrameInfo *info = itInfo.second;\n>  \n>  \t\tif (info->paramBuffer == buffer ||\n> -\t\t    info->statBuffer == buffer)\n> +\t\t    info->statBuffer == buffer ||\n> +\t\t    info->videoBuffer == buffer)\n>  \t\t\treturn info;\n>  \t}\n>  \n> @@ -696,17 +683,8 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n>  {\n>  \tRkISP1CameraData *data = cameraData(camera);\n>  \tunsigned int count = 1;\n> -\tStream *stream = *streams.begin();\n>  \tint ret;\n>  \n> -\tif (stream->memoryType() == InternalMemory)\n> -\t\tret = video_->exportBuffers(&stream->bufferPool());\n> -\telse\n> -\t\tret = video_->importBuffers(&stream->bufferPool());\n> -\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n>  \tunsigned int maxBuffers = 0;\n>  \tfor (const Stream *s : camera->streams())\n>  \t\tmaxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n> @@ -772,9 +750,6 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera,\n>  \tif (stat_->releaseBuffers())\n>  \t\tLOG(RkISP1, Error) << \"Failed to release stat buffers\";\n>  \n> -\tif (video_->releaseBuffers())\n> -\t\tLOG(RkISP1, Error) << \"Failed to release video buffers\";\n> -\n>  \treturn 0;\n>  }\n>  \n> @@ -980,7 +955,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>  \tif (param_->open() < 0)\n>  \t\treturn false;\n>  \n> -\tvideo_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n> +\tvideo_->frameBufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);\n>  \tstat_->frameBufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n>  \tparam_->frameBufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n>  \n> @@ -1029,7 +1004,7 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)\n>  \tcompleteRequest(activeCamera_, request);\n>  }\n>  \n> -void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> +void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n>  {\n>  \tASSERT(activeCamera_);\n>  \tRkISP1CameraData *data = cameraData(activeCamera_);\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 655c5e6d96a696d6..c29bad707b464bcd 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -41,7 +41,7 @@ public:\n>  \t}\n>  \n>  \tint init(MediaEntity *entity);\n> -\tvoid bufferReady(Buffer *buffer);\n> +\tvoid bufferReady(FrameBuffer *buffer);\n>  \n>  \tV4L2VideoDevice *video_;\n>  \tStream stream_;\n> @@ -226,23 +226,13 @@ void PipelineHandlerUVC::freeFrameBuffers(Camera *camera, Stream *stream)\n>  int PipelineHandlerUVC::allocateBuffers(Camera *camera,\n>  \t\t\t\t\tconst std::set<Stream *> &streams)\n>  {\n> -\tUVCCameraData *data = cameraData(camera);\n> -\tStream *stream = *streams.begin();\n> -\tconst StreamConfiguration &cfg = stream->configuration();\n> -\n> -\tLOG(UVC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n> -\n> -\tif (stream->memoryType() == InternalMemory)\n> -\t\treturn data->video_->exportBuffers(&stream->bufferPool());\n> -\telse\n> -\t\treturn data->video_->importBuffers(&stream->bufferPool());\n> +\treturn 0;\n>  }\n>  \n>  int PipelineHandlerUVC::freeBuffers(Camera *camera,\n>  \t\t\t\t    const std::set<Stream *> &streams)\n>  {\n> -\tUVCCameraData *data = cameraData(camera);\n> -\treturn data->video_->releaseBuffers();\n> +\treturn 0;\n>  }\n>  \n>  int PipelineHandlerUVC::start(Camera *camera)\n> @@ -296,7 +286,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n>  int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)\n>  {\n>  \tUVCCameraData *data = cameraData(camera);\n> -\tBuffer *buffer = request->findBuffer(&data->stream_);\n> +\tFrameBuffer *buffer = request->findBuffer(&data->stream_);\n>  \tif (!buffer) {\n>  \t\tLOG(UVC, Error)\n>  \t\t\t<< \"Attempt to queue request with invalid stream\";\n> @@ -361,7 +351,7 @@ int UVCCameraData::init(MediaEntity *entity)\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tvideo_->bufferReady.connect(this, &UVCCameraData::bufferReady);\n> +\tvideo_->frameBufferReady.connect(this, &UVCCameraData::bufferReady);\n>  \n>  \t/* Initialise the supported controls. */\n>  \tconst ControlInfoMap &controls = video_->controls();\n> @@ -401,7 +391,7 @@ int UVCCameraData::init(MediaEntity *entity)\n>  \treturn 0;\n>  }\n>  \n> -void UVCCameraData::bufferReady(Buffer *buffer)\n> +void UVCCameraData::bufferReady(FrameBuffer *buffer)\n>  {\n>  \tRequest *request = buffer->request();\n>  \n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 02235ffb0515982f..33fec6520240b328 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -55,7 +55,7 @@ public:\n>  \t}\n>  \n>  \tint init(MediaDevice *media);\n> -\tvoid bufferReady(Buffer *buffer);\n> +\tvoid bufferReady(FrameBuffer *buffer);\n>  \n>  \tCameraSensor *sensor_;\n>  \tV4L2Subdevice *debayer_;\n> @@ -293,23 +293,13 @@ void PipelineHandlerVimc::freeFrameBuffers(Camera *camera, Stream *stream)\n>  int PipelineHandlerVimc::allocateBuffers(Camera *camera,\n>  \t\t\t\t\t const std::set<Stream *> &streams)\n>  {\n> -\tVimcCameraData *data = cameraData(camera);\n> -\tStream *stream = *streams.begin();\n> -\tconst StreamConfiguration &cfg = stream->configuration();\n> -\n> -\tLOG(VIMC, Debug) << \"Requesting \" << cfg.bufferCount << \" buffers\";\n> -\n> -\tif (stream->memoryType() == InternalMemory)\n> -\t\treturn data->video_->exportBuffers(&stream->bufferPool());\n> -\telse\n> -\t\treturn data->video_->importBuffers(&stream->bufferPool());\n> +\treturn 0;\n>  }\n>  \n>  int PipelineHandlerVimc::freeBuffers(Camera *camera,\n>  \t\t\t\t     const std::set<Stream *> &streams)\n>  {\n> -\tVimcCameraData *data = cameraData(camera);\n> -\treturn data->video_->releaseBuffers();\n> +\treturn 0;\n>  }\n>  \n>  int PipelineHandlerVimc::start(Camera *camera)\n> @@ -357,7 +347,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n>  int PipelineHandlerVimc::queueRequestDevice(Camera *camera, Request *request)\n>  {\n>  \tVimcCameraData *data = cameraData(camera);\n> -\tBuffer *buffer = request->findBuffer(&data->stream_);\n> +\tFrameBuffer *buffer = request->findBuffer(&data->stream_);\n>  \tif (!buffer) {\n>  \t\tLOG(VIMC, Error)\n>  \t\t\t<< \"Attempt to queue request with invalid stream\";\n> @@ -449,7 +439,7 @@ int VimcCameraData::init(MediaDevice *media)\n>  \tif (video_->open())\n>  \t\treturn -ENODEV;\n>  \n> -\tvideo_->bufferReady.connect(this, &VimcCameraData::bufferReady);\n> +\tvideo_->frameBufferReady.connect(this, &VimcCameraData::bufferReady);\n>  \n>  \traw_ = new V4L2VideoDevice(media->getEntityByName(\"Raw Capture 1\"));\n>  \tif (raw_->open())\n> @@ -486,7 +476,7 @@ int VimcCameraData::init(MediaDevice *media)\n>  \treturn 0;\n>  }\n>  \n> -void VimcCameraData::bufferReady(Buffer *buffer)\n> +void VimcCameraData::bufferReady(FrameBuffer *buffer)\n>  {\n>  \tRequest *request = buffer->request();\n>  \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index fb69ca8e97216e6c..11286d31d4e54213 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -460,7 +460,7 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request)\n>   * otherwise\n>   */\n>  bool PipelineHandler::completeBuffer(Camera *camera, Request *request,\n> -\t\t\t\t     Buffer *buffer)\n> +\t\t\t\t     FrameBuffer *buffer)\n>  {\n>  \tcamera->bufferCompleted.emit(request, buffer);\n>  \treturn request->completeBuffer(buffer);\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index b17a6c2278361f00..4ac0d9684e5be801 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -75,11 +75,6 @@ Request::Request(Camera *camera, uint64_t cookie)\n>  \n>  Request::~Request()\n>  {\n> -\tfor (auto it : bufferMap_) {\n> -\t\tBuffer *buffer = it.second;\n> -\t\tdelete buffer;\n> -\t}\n> -\n>  \tdelete metadata_;\n>  \tdelete controls_;\n>  \tdelete validator_;\n> @@ -106,18 +101,18 @@ Request::~Request()\n>   * \\brief Retrieve the request's streams to buffers map\n>   *\n>   * Return a reference to the map that associates each Stream part of the\n> - * request to the Buffer the Stream output should be directed to.\n> + * request to the FrameBuffer the Stream output should be directed to.\n>   *\n> - * \\return The map of Stream to Buffer\n> + * \\return The map of Stream to FrameBuffer\n>   */\n>  \n>  /**\n> - * \\brief Store a Buffer with its associated Stream in the Request\n> + * \\brief Store a FrameBuffer with its associated Stream in the Request\n\ns/Store/Add/\ns/in the/to the/\n\n>   * \\param[in] stream The stream the buffer belongs to\n> - * \\param[in] buffer The Buffer to store in the request\n\ns/to store in/to add to/\n\n> + * \\param[in] buffer The FrameBuffer to store in the request\n\nDitto.\n\n>   *\n> - * Ownership of the buffer is passed to the request. It will be deleted when\n> - * the request is destroyed after completing.\n> + * Ownership of the buffer is passed to the request. Owner will be returned to\n> + * the user when Request complete callback is called.\n\nThis isn't accurate, the application still owns the buffer.\n\n * A reference to the buffer is stored in the request. The caller is responsible\n * for ensuring that the buffer will remain valid until the request complete\n * callback is called.\n\n>   *\n>   * A request can only contain one buffer per stream. If a buffer has already\n>   * been added to the request for the same stream, this method returns -EEXIST.\n> @@ -126,7 +121,7 @@ Request::~Request()\n>   * \\retval -EEXIST The request already contains a buffer for the stream\n>   * \\retval -EINVAL The buffer does not reference a valid Stream\n>   */\n> -int Request::addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer)\n> +int Request::addBuffer(Stream *stream, FrameBuffer *buffer)\n>  {\n>  \tif (!stream) {\n>  \t\tLOG(Request, Error) << \"Invalid stream reference\";\n> @@ -135,11 +130,11 @@ int Request::addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer)\n>  \n>  \tauto it = bufferMap_.find(stream);\n>  \tif (it != bufferMap_.end()) {\n> -\t\tLOG(Request, Error) << \"Buffer already set for stream\";\n> +\t\tLOG(Request, Error) << \"FrameBuffer already set for stream\";\n>  \t\treturn -EEXIST;\n>  \t}\n>  \n> -\tbufferMap_[stream] = buffer.release();\n> +\tbufferMap_[stream] = buffer;\n>  \n>  \treturn 0;\n>  }\n> @@ -159,7 +154,7 @@ int Request::addBuffer(Stream *stream, std::unique_ptr<Buffer> buffer)\n>   * \\return The buffer associated with the stream, or nullptr if the stream is\n>   * not part of this request\n>   */\n> -Buffer *Request::findBuffer(Stream *stream) const\n> +FrameBuffer *Request::findBuffer(Stream *stream) const\n>  {\n>  \tauto it = bufferMap_.find(stream);\n>  \tif (it == bufferMap_.end())\n> @@ -219,7 +214,7 @@ int Request::prepare()\n>  \t}\n>  \n>  \tfor (auto const &pair : bufferMap_) {\n> -\t\tBuffer *buffer = pair.second;\n> +\t\tFrameBuffer *buffer = pair.second;\n>  \t\tbuffer->request_ = this;\n>  \t\tpending_.insert(buffer);\n>  \t}\n> @@ -253,7 +248,7 @@ void Request::complete()\n>   * \\return True if all buffers contained in the request have completed, false\n>   * otherwise\n>   */\n> -bool Request::completeBuffer(Buffer *buffer)\n> +bool Request::completeBuffer(FrameBuffer *buffer)\n>  {\n>  \tint ret = pending_.erase(buffer);\n>  \tASSERT(ret == 1);\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 47793702b9aa0ee9..e53ed2801291f2c5 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -23,7 +23,7 @@\n>  using namespace libcamera;\n>  \n>  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n> -\t: options_(options), isCapturing_(false)\n> +\t: options_(options), allocator_(nullptr), isCapturing_(false)\n>  {\n>  \tint ret;\n>  \n> @@ -37,8 +37,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)\n>  \tadjustSize();\n>  \n>  \tret = openCamera(cm);\n> -\tif (!ret)\n> +\tif (!ret) {\n> +\t\tallocator_ = FrameBufferAllocator::create(camera_);\n>  \t\tret = startCapture();\n> +\t}\n>  \n>  \tif (ret < 0)\n>  \t\tQTimer::singleShot(0, QCoreApplication::instance(),\n> @@ -49,6 +51,7 @@ MainWindow::~MainWindow()\n>  {\n>  \tif (camera_) {\n>  \t\tstopCapture();\n> +\t\tdelete allocator_;\n>  \t\tcamera_->release();\n>  \t\tcamera_.reset();\n>  \t}\n> @@ -176,8 +179,14 @@ int MainWindow::startCapture()\n>  \t\treturn ret;\n>  \t}\n>  \n> +\tret = allocator_->allocate(stream);\n> +\tif (ret < 0) {\n> +\t\tstd::cerr << \"Failed to allocate capture buffers\" << std::endl;\n> +\t\treturn ret;\n> +\t}\n> +\n>  \tstd::vector<Request *> requests;\n> -\tfor (unsigned int i = 0; i < cfg.bufferCount; ++i) {\n> +\tfor (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {\n>  \t\tRequest *request = camera_->createRequest();\n>  \t\tif (!request) {\n>  \t\t\tstd::cerr << \"Can't create request\" << std::endl;\n> @@ -185,13 +194,7 @@ int MainWindow::startCapture()\n>  \t\t\tgoto error;\n>  \t\t}\n>  \n> -\t\tstd::unique_ptr<Buffer> buffer = stream->createBuffer(i);\n> -\t\tif (!buffer) {\n> -\t\t\tstd::cerr << \"Can't create buffer \" << i << std::endl;\n> -\t\t\tgoto error;\n> -\t\t}\n> -\n> -\t\tret = request->addBuffer(stream, std::move(buffer));\n> +\t\tret = request->addBuffer(stream, buffer.get());\n>  \t\tif (ret < 0) {\n>  \t\t\tstd::cerr << \"Can't set buffer for request\" << std::endl;\n>  \t\t\tgoto error;\n> @@ -254,11 +257,11 @@ void MainWindow::requestComplete(Request *request)\n>  \tif (request->status() == Request::RequestCancelled)\n>  \t\treturn;\n>  \n> -\tconst std::map<Stream *, Buffer *> &buffers = request->buffers();\n> +\tconst std::map<Stream *, FrameBuffer *> &buffers = request->buffers();\n>  \n>  \tframesCaptured_++;\n>  \n> -\tBuffer *buffer = buffers.begin()->second;\n> +\tFrameBuffer *buffer = buffers.begin()->second;\n>  \tconst FrameMetadata &metadata = buffer->metadata();\n>  \n>  \tdouble fps = metadata.timestamp - lastBufferTime_;\n> @@ -281,28 +284,21 @@ void MainWindow::requestComplete(Request *request)\n>  \n>  \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n>  \t\tStream *stream = it->first;\n> -\t\tBuffer *buffer = it->second;\n> -\t\tunsigned int index = buffer->index();\n> -\n> -\t\tstd::unique_ptr<Buffer> newBuffer = stream->createBuffer(index);\n> -\t\tif (!newBuffer) {\n> -\t\t\tstd::cerr << \"Can't create buffer\" << std::endl;\n> -\t\t\treturn;\n> -\t\t}\n> +\t\tFrameBuffer *buffer = it->second;\n>  \n> -\t\trequest->addBuffer(stream, std::move(newBuffer));\n> +\t\trequest->addBuffer(stream, buffer);\n>  \t}\n>  \n>  \tcamera_->queueRequest(request);\n>  }\n>  \n> -int MainWindow::display(Buffer *buffer)\n> +int MainWindow::display(FrameBuffer *buffer)\n>  {\n> -\tif (buffer->mem()->planes().size() != 1)\n> +\tif (buffer->planes().size() != 1)\n>  \t\treturn -EINVAL;\n>  \n>  \t/* \\todo: Once the FrameBuffer is done cache mapped memory. */\n> -\tconst FrameBuffer::Plane &plane = buffer->mem()->planes().front();\n> +\tconst FrameBuffer::Plane &plane = buffer->planes().front();\n>  \tvoid *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,\n>  \t\t\t    plane.fd.fd(), 0);\n>  \n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index 0786e915ec512255..05cde4ceab5f7ea1 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -14,8 +14,10 @@\n>  #include <QObject>\n>  #include <QTimer>\n>  \n> +#include <libcamera/buffer.h>\n>  #include <libcamera/camera.h>\n>  #include <libcamera/camera_manager.h>\n> +#include <libcamera/framebuffer_allocator.h>\n>  #include <libcamera/stream.h>\n>  \n>  #include \"../cam/options.h\"\n> @@ -49,7 +51,7 @@ private:\n>  \tvoid stopCapture();\n>  \n>  \tvoid requestComplete(Request *request);\n> -\tint display(Buffer *buffer);\n> +\tint display(FrameBuffer *buffer);\n>  \n>  \tQString title_;\n>  \tQTimer titleTimer_;\n> @@ -57,6 +59,8 @@ private:\n>  \tconst OptionsParser::Options &options_;\n>  \n>  \tstd::shared_ptr<Camera> camera_;\n> +\tFrameBufferAllocator *allocator_;\n> +\n>  \tbool isCapturing_;\n>  \tstd::unique_ptr<CameraConfiguration> config_;\n>  \n> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> index 6559a89ce914a183..63e5fb100e49f958 100644\n> --- a/test/camera/buffer_import.cpp\n> +++ b/test/camera/buffer_import.cpp\n> @@ -117,7 +117,7 @@ public:\n>  \t}\n>  \n>  protected:\n> -\tvoid bufferComplete(Request *request, Buffer *buffer)\n> +\tvoid bufferComplete(Request *request, FrameBuffer *buffer)\n>  \t{\n>  \t\tif (buffer->metadata().status != FrameMetadata::FrameSuccess)\n>  \t\t\treturn;\n> @@ -130,17 +130,16 @@ protected:\n>  \t\tif (request->status() != Request::RequestComplete)\n>  \t\t\treturn;\n>  \n> -\t\tconst std::map<Stream *, Buffer *> &buffers = request->buffers();\n> +\t\tconst std::map<Stream *, FrameBuffer *> &buffers = request->buffers();\n>  \n>  \t\tcompleteRequestsCount_++;\n>  \n>  \t\t/* Create a new request. */\n>  \t\tStream *stream = buffers.begin()->first;\n> -\t\tint dmabuf = buffers.begin()->second->dmabufs()[0];\n> -\t\tstd::unique_ptr<Buffer> buffer = stream->createBuffer({ dmabuf, -1, -1 });\n> +\t\tFrameBuffer *buffer = buffers.begin()->second;\n>  \n>  \t\trequest = camera_->createRequest();\n> -\t\trequest->addBuffer(stream, std::move(buffer));\n> +\t\trequest->addBuffer(stream, buffer);\n>  \t\tcamera_->queueRequest(request);\n>  \t}\n>  \n> @@ -155,9 +154,6 @@ protected:\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> -\t\tStreamConfiguration &cfg = config_->at(0);\n> -\t\tcfg.memoryType = ExternalMemory;\n> -\n>  \t\treturn TestPass;\n>  \t}\n>  \n> @@ -188,17 +184,14 @@ protected:\n>  \t\t\treturn TestFail;\n>  \n>  \t\tstd::vector<Request *> requests;\n> -\t\tfor (const std::unique_ptr<FrameBuffer> &framebuffer : source.buffers()) {\n> -\t\t\tint dmabuf = framebuffer->planes()[0].fd.fd();\n> -\n> +\t\tfor (const std::unique_ptr<FrameBuffer> &buffer : source.buffers()) {\n>  \t\t\tRequest *request = camera_->createRequest();\n>  \t\t\tif (!request) {\n>  \t\t\t\tstd::cout << \"Failed to create request\" << std::endl;\n>  \t\t\t\treturn TestFail;\n>  \t\t\t}\n>  \n> -\t\t\tstd::unique_ptr<Buffer> buffer = stream->createBuffer({ dmabuf, -1, -1 });\n> -\t\t\tif (request->addBuffer(stream, std::move(buffer))) {\n> +\t\t\tif (request->addBuffer(stream, buffer.get())) {\n>  \t\t\t\tstd::cout << \"Failed to associating buffer with request\" << std::endl;\n>  \t\t\t\treturn TestFail;\n>  \t\t\t}\n> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> index 0d9ffc476650f414..de879ee4eb1420a6 100644\n> --- a/test/camera/capture.cpp\n> +++ b/test/camera/capture.cpp\n> @@ -26,7 +26,7 @@ protected:\n>  \tunsigned int completeBuffersCount_;\n>  \tunsigned int completeRequestsCount_;\n>  \n> -\tvoid bufferComplete(Request *request, Buffer *buffer)\n> +\tvoid bufferComplete(Request *request, FrameBuffer *buffer)\n>  \t{\n>  \t\tif (buffer->metadata().status != FrameMetadata::FrameSuccess)\n>  \t\t\treturn;\n> @@ -39,17 +39,16 @@ protected:\n>  \t\tif (request->status() != Request::RequestComplete)\n>  \t\t\treturn;\n>  \n> -\t\tconst std::map<Stream *, Buffer *> &buffers = request->buffers();\n> +\t\tconst std::map<Stream *, FrameBuffer *> &buffers = request->buffers();\n>  \n>  \t\tcompleteRequestsCount_++;\n>  \n>  \t\t/* Create a new request. */\n>  \t\tStream *stream = buffers.begin()->first;\n> -\t\tBuffer *buffer = buffers.begin()->second;\n> -\t\tstd::unique_ptr<Buffer> newBuffer = stream->createBuffer(buffer->index());\n> +\t\tFrameBuffer *buffer = buffers.begin()->second;\n>  \n>  \t\trequest = camera_->createRequest();\n> -\t\trequest->addBuffer(stream, std::move(newBuffer));\n> +\t\trequest->addBuffer(stream, buffer);\n>  \t\tcamera_->queueRequest(request);\n>  \t}\n>  \n> @@ -64,9 +63,16 @@ protected:\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> +\t\tallocator_ = FrameBufferAllocator::create(camera_);\n> +\n>  \t\treturn TestPass;\n>  \t}\n>  \n> +\tvoid cleanup() override\n> +\t{\n> +\t\tdelete allocator_;\n> +\t}\n> +\n>  \tint run() override\n>  \t{\n>  \t\tStreamConfiguration &cfg = config_->at(0);\n> @@ -87,21 +93,20 @@ protected:\n>  \t\t}\n>  \n>  \t\tStream *stream = cfg.stream();\n> +\n> +\t\tint ret = allocator_->allocate(stream);\n> +\t\tif (ret < 0)\n> +\t\t\treturn TestFail;\n> +\n>  \t\tstd::vector<Request *> requests;\n> -\t\tfor (unsigned int i = 0; i < cfg.bufferCount; ++i) {\n> +\t\tfor (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {\n>  \t\t\tRequest *request = camera_->createRequest();\n>  \t\t\tif (!request) {\n>  \t\t\t\tcout << \"Failed to create request\" << endl;\n>  \t\t\t\treturn TestFail;\n>  \t\t\t}\n>  \n> -\t\t\tstd::unique_ptr<Buffer> buffer = stream->createBuffer(i);\n> -\t\t\tif (!buffer) {\n> -\t\t\t\tcout << \"Failed to create buffer \" << i << endl;\n> -\t\t\t\treturn TestFail;\n> -\t\t\t}\n> -\n> -\t\t\tif (request->addBuffer(stream, std::move(buffer))) {\n> +\t\t\tif (request->addBuffer(stream, buffer.get())) {\n>  \t\t\t\tcout << \"Failed to associating buffer with request\" << endl;\n>  \t\t\t\treturn TestFail;\n>  \t\t\t}\n> @@ -134,10 +139,12 @@ protected:\n>  \t\twhile (timer.isRunning())\n>  \t\t\tdispatcher->processEvents();\n>  \n> -\t\tif (completeRequestsCount_ <= cfg.bufferCount * 2) {\n> +\t\tunsigned int nbuffers = allocator_->buffers(stream).size();\n> +\n> +\t\tif (completeRequestsCount_ <= nbuffers * 2) {\n>  \t\t\tcout << \"Failed to capture enough frames (got \"\n>  \t\t\t     << completeRequestsCount_ << \" expected at least \"\n> -\t\t\t     << cfg.bufferCount * 2 << \")\" << endl;\n> +\t\t\t     << nbuffers * 2 << \")\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> @@ -160,6 +167,7 @@ protected:\n>  \t}\n>  \n>  \tstd::unique_ptr<CameraConfiguration> config_;\n> +\tFrameBufferAllocator *allocator_;\n>  };\n>  \n>  } /* namespace */\n> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> index f627b8f37422350e..f3a7ca7c32a5ec97 100644\n> --- a/test/camera/statemachine.cpp\n> +++ b/test/camera/statemachine.cpp\n> @@ -185,6 +185,12 @@ protected:\n>  \t\tif (camera_->allocateBuffers())\n>  \t\t\treturn TestFail;\n>  \n> +\t\t/* Use internally allocated buffers. */\n> +\t\tallocator_ = FrameBufferAllocator::create(camera_);\n> +\t\tStream *stream = *camera_->streams().begin();\n> +\t\tif (allocator_->allocate(stream) < 0)\n> +\t\t\treturn TestFail;\n> +\n>  \t\tif (camera_->start())\n>  \t\t\treturn TestFail;\n>  \n> @@ -218,8 +224,7 @@ protected:\n>  \t\t\treturn TestFail;\n>  \n>  \t\tStream *stream = *camera_->streams().begin();\n> -\t\tstd::unique_ptr<Buffer> buffer = stream->createBuffer(0);\n> -\t\tif (request->addBuffer(stream, std::move(buffer)))\n> +\t\tif (request->addBuffer(stream, allocator_->buffers(stream)[0].get()))\n>  \t\t\treturn TestFail;\n>  \n>  \t\tif (camera_->queueRequest(request))\n> @@ -229,6 +234,8 @@ protected:\n>  \t\tif (camera_->stop())\n>  \t\t\treturn TestFail;\n>  \n> +\t\tdelete allocator_;\n> +\n>  \t\tif (camera_->freeBuffers())\n>  \t\t\treturn TestFail;\n>  \n> @@ -283,6 +290,7 @@ protected:\n>  \t}\n>  \n>  \tstd::unique_ptr<CameraConfiguration> defconf_;\n> +\tFrameBufferAllocator *allocator_;\n>  };\n>  \n>  } /* namespace */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 53CC16045D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  8 Jan 2020 05:17:13 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 845E352F;\n\tWed,  8 Jan 2020 05:17:12 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578457032;\n\tbh=WucSC0eDmzBE51jh8XelEsS9t6/skaoWqEUIehcoIBs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kgWXq2kfhk7BBusHT8SlWo7MUBVYjCTasc6Z6/Q+zFLMz27oVMcEgLo5JCDZsWJ56\n\tXv3fSdZfN8OzzovVpAHy6RzgB2GTYlvnEidc/e+pnFTelM3yb6Nc3Vbc2HiVgQWKaG\n\tVZ+G47t7EVJ6dLGBelPZkqy69DoubciQfNALRU3g=","Date":"Wed, 8 Jan 2020 06:17:01 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200108041701.GE10933@pendragon.ideasonboard.com>","References":"<20191230120510.938333-1-niklas.soderlund@ragnatech.se>\n\t<20191230120510.938333-21-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191230120510.938333-21-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 20/25] libcamera: Switch to\n\tFrameBuffer interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 08 Jan 2020 04:17:13 -0000"}}]