[{"id":2258,"web_url":"https://patchwork.libcamera.org/comment/2258/","msgid":"<20190714104242.GD31102@wyvern>","date":"2019-07-14T10:42:42","subject":"Re: [libcamera-devel] [PATCH v2 06/16] libcamera: buffer: Split\n\tmemory information to BufferMemory","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nOn 2019-07-13 20:23:41 +0300, Laurent Pinchart wrote:\n> The Buffer class is a large beast the stores information about the\n> buffer memory, dynamic metadata related to the frame stored in the\n> buffer, and buffer reference data (in the index). In order to implement\n> buffer import we will need to extend this with dmabuf file descriptors,\n> making usage of the class even more complex.\n> \n> Refactor the Buffer class by splitting the buffer memory information to\n> a BufferMemory class, and repurposing the Buffer class to reference a\n> buffer and to store dynamic metadata. The BufferMemory class becomes a\n> long term storage, valid and stable from the time buffer memory is\n> allocated to the time it is freed. The Buffer class, on the other hand,\n> becomes transient, is created on demand when an application requires a\n> buffer, is given to a request, and is deleted when the request\n> completes.\n> \n> Buffer and BufferMemory don't need to be copied, so their copy\n> constructor and assignment operators are deleted.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nI really like the split, nice work.\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  include/libcamera/buffer.h               |  55 +++++---\n>  include/libcamera/request.h              |   4 +-\n>  include/libcamera/stream.h               |   4 +\n>  src/cam/buffer_writer.cpp                |   4 +-\n>  src/cam/buffer_writer.h                  |   3 +-\n>  src/cam/capture.cpp                      |  35 +++--\n>  src/libcamera/buffer.cpp                 | 164 ++++++++++++++---------\n>  src/libcamera/include/v4l2_videodevice.h |   8 +-\n>  src/libcamera/request.cpp                |  35 +++--\n>  src/libcamera/stream.cpp                 |  24 ++++\n>  src/libcamera/v4l2_videodevice.cpp       |  40 +++---\n>  src/qcam/main_window.cpp                 |  40 ++++--\n>  src/qcam/main_window.h                   |   2 +-\n>  test/camera/capture.cpp                  |  19 ++-\n>  test/camera/statemachine.cpp             |   6 +-\n>  15 files changed, 298 insertions(+), 145 deletions(-)\n> \n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 260a62e9e77e..f5ba6207bcef 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -14,6 +14,7 @@ namespace libcamera {\n>  \n>  class BufferPool;\n>  class Request;\n> +class Stream;\n>  \n>  class Plane final\n>  {\n> @@ -36,6 +37,30 @@ private:\n>  \tvoid *mem_;\n>  };\n>  \n> +class BufferMemory final\n> +{\n> +public:\n> +\tstd::vector<Plane> &planes() { return planes_; }\n> +\n> +private:\n> +\tstd::vector<Plane> planes_;\n> +};\n> +\n> +class BufferPool final\n> +{\n> +public:\n> +\t~BufferPool();\n> +\n> +\tvoid createBuffers(unsigned int count);\n> +\tvoid destroyBuffers();\n> +\n> +\tunsigned int count() const { return buffers_.size(); }\n> +\tstd::vector<BufferMemory> &buffers() { return buffers_; }\n> +\n> +private:\n> +\tstd::vector<BufferMemory> buffers_;\n> +};\n> +\n>  class Buffer final\n>  {\n>  public:\n> @@ -45,20 +70,24 @@ public:\n>  \t\tBufferCancelled,\n>  \t};\n>  \n> -\tBuffer();\n> +\tBuffer(unsigned int index = -1, const Buffer *metadata = nullptr);\n> +\tBuffer(const Buffer &) = delete;\n> +\tBuffer &operator=(const Buffer &) = delete;\n>  \n>  \tunsigned int index() const { return index_; }\n> +\n>  \tunsigned int bytesused() const { return bytesused_; }\n>  \tuint64_t timestamp() const { return timestamp_; }\n>  \tunsigned int sequence() const { return sequence_; }\n> +\n>  \tStatus status() const { return status_; }\n> -\tstd::vector<Plane> &planes() { return planes_; }\n>  \tRequest *request() const { return request_; }\n> +\tStream *stream() const { return stream_; }\n>  \n>  private:\n> -\tfriend class BufferPool;\n>  \tfriend class PipelineHandler;\n>  \tfriend class Request;\n> +\tfriend class Stream;\n>  \tfriend class V4L2VideoDevice;\n>  \n>  \tvoid cancel();\n> @@ -66,28 +95,14 @@ private:\n>  \tvoid setRequest(Request *request) { request_ = request; }\n>  \n>  \tunsigned int index_;\n> +\n>  \tunsigned int bytesused_;\n>  \tuint64_t timestamp_;\n>  \tunsigned int sequence_;\n> +\n>  \tStatus status_;\n> -\n> -\tstd::vector<Plane> planes_;\n>  \tRequest *request_;\n> -};\n> -\n> -class BufferPool final\n> -{\n> -public:\n> -\t~BufferPool();\n> -\n> -\tvoid createBuffers(unsigned int count);\n> -\tvoid destroyBuffers();\n> -\n> -\tunsigned int count() const { return buffers_.size(); }\n> -\tstd::vector<Buffer> &buffers() { return buffers_; }\n> -\n> -private:\n> -\tstd::vector<Buffer> buffers_;\n> +\tStream *stream_;\n>  };\n>  \n>  } /* namespace libcamera */\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index dd165bc21c03..7a60be617645 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -8,6 +8,7 @@\n>  #define __LIBCAMERA_REQUEST_H__\n>  \n>  #include <map>\n> +#include <memory>\n>  #include <stdint.h>\n>  #include <unordered_set>\n>  \n> @@ -33,10 +34,11 @@ public:\n>  \tRequest(Camera *camera, uint64_t cookie = 0);\n>  \tRequest(const Request &) = delete;\n>  \tRequest &operator=(const Request &) = delete;\n> +\t~Request();\n>  \n>  \tControlList &controls() { return controls_; }\n>  \tconst std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }\n> -\tint setBuffers(const std::map<Stream *, Buffer *> &streamMap);\n> +\tint addBuffer(std::unique_ptr<Buffer> buffer);\n>  \tBuffer *findBuffer(Stream *stream) const;\n>  \n>  \tuint64_t cookie() const { return cookie_; }\n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index 5b4fea324ce4..f595a630eb4a 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -8,6 +8,7 @@\n>  #define __LIBCAMERA_STREAM_H__\n>  \n>  #include <map>\n> +#include <memory>\n>  #include <string>\n>  #include <vector>\n>  \n> @@ -66,6 +67,9 @@ class Stream\n>  {\n>  public:\n>  \tStream();\n> +\n> +\tstd::unique_ptr<Buffer> createBuffer(unsigned int index);\n> +\n>  \tBufferPool &bufferPool() { return bufferPool_; }\n>  \tconst StreamConfiguration &configuration() const { return configuration_; }\n>  \n> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> index e0374ffcb319..b7f2ed4f2d2d 100644\n> --- a/src/cam/buffer_writer.cpp\n> +++ b/src/cam/buffer_writer.cpp\n> @@ -19,7 +19,7 @@ BufferWriter::BufferWriter(const std::string &pattern)\n>  {\n>  }\n>  \n> -int BufferWriter::write(libcamera::Buffer *buffer,\n> +int BufferWriter::write(libcamera::Buffer *buffer, libcamera::BufferMemory *mem,\n>  \t\t\tconst std::string &streamName)\n>  {\n>  \tstd::string filename;\n> @@ -41,7 +41,7 @@ int BufferWriter::write(libcamera::Buffer *buffer,\n>  \tif (fd == -1)\n>  \t\treturn -errno;\n>  \n> -\tfor (libcamera::Plane &plane : buffer->planes()) {\n> +\tfor (libcamera::Plane &plane : mem->planes()) {\n>  \t\tvoid *data = plane.mem();\n>  \t\tunsigned int length = plane.length();\n>  \n> diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h\n> index 7bf785d1e832..9bea205fac1d 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::Buffer *buffer, libcamera::BufferMemory *mem,\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 6b842d73390d..1cf59063c93b 100644\n> --- a/src/cam/capture.cpp\n> +++ b/src/cam/capture.cpp\n> @@ -95,13 +95,14 @@ int Capture::capture(EventLoop *loop)\n>  \t\tstd::map<Stream *, Buffer *> map;\n>  \t\tfor (StreamConfiguration &cfg : *config_) {\n>  \t\t\tStream *stream = cfg.stream();\n> -\t\t\tmap[stream] = &stream->bufferPool().buffers()[i];\n> -\t\t}\n> +\t\t\tstd::unique_ptr<Buffer> buffer = stream->createBuffer(i);\n>  \n> -\t\tret = request->setBuffers(map);\n> -\t\tif (ret < 0) {\n> -\t\t\tstd::cerr << \"Can't set buffers for request\" << std::endl;\n> -\t\t\treturn ret;\n> +\t\t\tret = request->addBuffer(std::move(buffer));\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> +\t\t\t\treturn ret;\n> +\t\t\t}\n>  \t\t}\n>  \n>  \t\trequests.push_back(request);\n> @@ -155,6 +156,7 @@ void Capture::requestComplete(Request *request, const std::map<Stream *, Buffer\n>  \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n>  \t\tStream *stream = it->first;\n>  \t\tBuffer *buffer = it->second;\n> +\t\tBufferMemory *mem = &stream->bufferPool().buffers()[buffer->index()];\n>  \t\tconst std::string &name = streamName_[stream];\n>  \n>  \t\tinfo << \" \" << name\n> @@ -163,17 +165,34 @@ void Capture::requestComplete(Request *request, const std::map<Stream *, Buffer\n>  \t\t     << \" bytesused: \" << buffer->bytesused();\n>  \n>  \t\tif (writer_)\n> -\t\t\twriter_->write(buffer, name);\n> +\t\t\twriter_->write(buffer, mem, name);\n>  \t}\n>  \n>  \tstd::cout << info.str() << std::endl;\n>  \n> +\t/*\n> +\t * Create a new request and populate it with one buffer for each\n> +\t * stream.\n> +\t */\n>  \trequest = camera_->createRequest();\n>  \tif (!request) {\n>  \t\tstd::cerr << \"Can't create request\" << std::endl;\n>  \t\treturn;\n>  \t}\n>  \n> -\trequest->setBuffers(buffers);\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 \" << index << std::endl;\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\trequest->addBuffer(std::move(newBuffer));\n> +\t}\n> +\n>  \tcamera_->queueRequest(request);\n>  }\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index c0a020942f5c..ecbf25246a55 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -173,12 +173,78 @@ void *Plane::mem()\n>   */\n>  \n>  /**\n> - * \\class Buffer\n> + * \\class BufferMemory\n>   * \\brief A memory buffer to store an image\n>   *\n> - * The Buffer class represents the memory buffers used to store a\n> - * full frame image, which may contain multiple separate memory Plane\n> - * objects if the image format is multi-planar.\n> + * The BufferMemory class represents the memory buffers used to store full frame\n> + * images, which may contain multiple separate memory Plane objects if the\n> + * image format is multi-planar.\n> + */\n> +\n> +/**\n> + * \\fn BufferMemory::planes()\n> + * \\brief Retrieve the planes within the buffer\n> + * \\return A reference to a vector holding all Planes within the buffer\n> + */\n> +\n> +/**\n> + * \\class BufferPool\n> + * \\brief A pool of buffers\n> + *\n> + * The BufferPool class groups together a collection of Buffers to store frames.\n> + * The buffers must be exported by a device before they can be imported into\n> + * another device for further use.\n> + */\n> +\n> +BufferPool::~BufferPool()\n> +{\n> +\tdestroyBuffers();\n> +}\n> +\n> +/**\n> + * \\brief Create buffers in the Pool\n> + * \\param[in] count The number of buffers to create\n> + */\n> +void BufferPool::createBuffers(unsigned int count)\n> +{\n> +\tbuffers_.resize(count);\n> +}\n> +\n> +/**\n> + * \\brief Release all buffers from pool\n> + *\n> + * If no buffers have been created or if buffers have already been released no\n> + * operation is performed.\n> + */\n> +void BufferPool::destroyBuffers()\n> +{\n> +\tbuffers_.resize(0);\n> +}\n> +\n> +/**\n> + * \\fn BufferPool::count()\n> + * \\brief Retrieve the number of buffers contained within the pool\n> + * \\return The number of buffers contained in the pool\n> + */\n> +\n> +/**\n> + * \\fn BufferPool::buffers()\n> + * \\brief Retrieve all the buffers in the pool\n> + * \\return A vector containing all the buffers in the pool.\n> + */\n> +\n> +/**\n> + * \\class Buffer\n> + * \\brief A buffer handle and dynamic metadata\n> + *\n> + * The Buffer class references a buffer memory and associates dynamic metadata\n> + * related to the frame contained in the buffer. It allows referencing buffer\n> + * memory through a single interface regardless of whether the memory is\n> + * allocated internally in libcamera or provided externally through dmabuf.\n> + *\n> + * Buffer instances are allocated dynamically for a stream through\n> + * Stream::createBuffer(), added to a request with Request::addBuffer() and\n> + * deleted automatically after the request complete handler returns.\n>   */\n>  \n>  /**\n> @@ -195,9 +261,26 @@ void *Plane::mem()\n>   * invalid and shall not be used.\n>   */\n>  \n> -Buffer::Buffer()\n> -\t: index_(-1), request_(nullptr)\n> +/**\n> + * \\brief Construct a buffer not associated with any stream\n> + *\n> + * This method constructs an orphaned buffer not associated with any stream. It\n> + * is not meant to be called by applications, they should instead create buffers\n> + * for a stream with Stream::createBuffer().\n> + */\n> +Buffer::Buffer(unsigned int index, const Buffer *metadata)\n> +\t: index_(index), status_(Buffer::BufferSuccess), request_(nullptr),\n> +\t  stream_(nullptr)\n>  {\n> +\tif (metadata) {\n> +\t\tbytesused_ = metadata->bytesused_;\n> +\t\tsequence_ = metadata->sequence_;\n> +\t\ttimestamp_ = metadata->timestamp_;\n> +\t} else {\n> +\t\tbytesused_ = 0;\n> +\t\tsequence_ = 0;\n> +\t\ttimestamp_ = 0;\n> +\t}\n>  }\n>  \n>  /**\n> @@ -206,12 +289,6 @@ Buffer::Buffer()\n>   * \\return The buffer index\n>   */\n>  \n> -/**\n> - * \\fn Buffer::planes()\n> - * \\brief Retrieve the planes within the buffer\n> - * \\return A reference to a vector holding all Planes within the buffer\n> - */\n> -\n>  /**\n>   * \\fn Buffer::bytesused()\n>   * \\brief Retrieve the number of bytes occupied by the data in the buffer\n> @@ -264,6 +341,19 @@ Buffer::Buffer()\n>   * \\sa Buffer::setRequest()\n>   */\n>  \n> +/**\n> + * \\fn Buffer::stream()\n> + * \\brief Retrieve the stream this buffer is associated with\n> + *\n> + * A Buffer is associated to the stream that created it with\n> + * Stream::createBuffer() and the association is valid until the buffer is\n> + * destroyed. Buffer instances that are created directly are not associated\n> + * with any stream.\n> + *\n> + * \\return The Stream the Buffer is associated with, or nullptr if the buffer\n> + * is not associated with a stream\n> + */\n> +\n>  /**\n>   * \\brief Mark a buffer as cancel by setting its status to BufferCancelled\n>   */\n> @@ -282,54 +372,4 @@ void Buffer::cancel()\n>   * The intended callers are Request::prepare() and Request::completeBuffer().\n>   */\n>  \n> -/**\n> - * \\class BufferPool\n> - * \\brief A pool of buffers\n> - *\n> - * The BufferPool class groups together a collection of Buffers to store frames.\n> - * The buffers must be exported by a device before they can be imported into\n> - * another device for further use.\n> - */\n> -\n> -BufferPool::~BufferPool()\n> -{\n> -\tdestroyBuffers();\n> -}\n> -\n> -/**\n> - * \\brief Create buffers in the Pool\n> - * \\param[in] count The number of buffers to create\n> - */\n> -void BufferPool::createBuffers(unsigned int count)\n> -{\n> -\tunsigned int index = 0;\n> -\n> -\tbuffers_.resize(count);\n> -\tfor (Buffer &buffer : buffers_)\n> -\t\tbuffer.index_ = index++;\n> -}\n> -\n> -/**\n> - * \\brief Release all buffers from pool\n> - *\n> - * If no buffers have been created or if buffers have already been released no\n> - * operation is performed.\n> - */\n> -void BufferPool::destroyBuffers()\n> -{\n> -\tbuffers_.resize(0);\n> -}\n> -\n> -/**\n> - * \\fn BufferPool::count()\n> - * \\brief Retrieve the number of buffers contained within the pool\n> - * \\return The number of buffers contained in the pool\n> - */\n> -\n> -/**\n> - * \\fn BufferPool::buffers()\n> - * \\brief Retrieve all the buffers in the pool\n> - * \\return A vector containing all the buffers in the pool.\n> - */\n> -\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> index f948a41fb8c1..8238a39b0da4 100644\n> --- a/src/libcamera/include/v4l2_videodevice.h\n> +++ b/src/libcamera/include/v4l2_videodevice.h\n> @@ -7,7 +7,6 @@\n>  #ifndef __LIBCAMERA_V4L2_VIDEODEVICE_H__\n>  #define __LIBCAMERA_V4L2_VIDEODEVICE_H__\n>  \n> -#include <atomic>\n>  #include <string>\n>  #include <vector>\n>  \n> @@ -23,6 +22,7 @@\n>  namespace libcamera {\n>  \n>  class Buffer;\n> +class BufferMemory;\n>  class BufferPool;\n>  class EventNotifier;\n>  class MediaDevice;\n> @@ -165,8 +165,8 @@ private:\n>  \tstd::vector<SizeRange> enumSizes(unsigned int pixelFormat);\n>  \n>  \tint requestBuffers(unsigned int count);\n> -\tint createPlane(Buffer *buffer, unsigned int plane,\n> -\t\t\tunsigned int length);\n> +\tint createPlane(BufferMemory *buffer, unsigned int index,\n> +\t\t\tunsigned int plane, unsigned int length);\n>  \n>  \tBuffer *dequeueBuffer();\n>  \tvoid bufferAvailable(EventNotifier *notifier);\n> @@ -177,7 +177,7 @@ private:\n>  \tenum v4l2_memory memoryType_;\n>  \n>  \tBufferPool *bufferPool_;\n> -\tstd::atomic<unsigned int> queuedBuffersCount_;\n> +\tstd::map<unsigned int, Buffer *> queuedBuffers_;\n>  \n>  \tEventNotifier *fdEvent_;\n>  };\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 8cf41a43a80e..45e7133febb0 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -60,6 +60,14 @@ Request::Request(Camera *camera, uint64_t cookie)\n>  {\n>  }\n>  \n> +Request::~Request()\n> +{\n> +\tfor (auto it : bufferMap_) {\n> +\t\tBuffer *buffer = it.second;\n> +\t\tdelete buffer;\n> +\t}\n> +}\n> +\n>  /**\n>   * \\fn Request::controls()\n>   * \\brief Retrieve the request's ControlList\n> @@ -87,19 +95,30 @@ Request::Request(Camera *camera, uint64_t cookie)\n>   */\n>  \n>  /**\n> - * \\brief Set the streams to capture with associated buffers\n> - * \\param[in] streamMap The map of streams to buffers\n> + * \\brief Store a Buffer with its associated Stream in the Request\n> + * \\param[in] buffer The Buffer to store in the request\n> + *\n> + * Ownership of the buffer is passed to the request. It will be deleted when\n> + * the request is destroyed after completing.\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> + *\n>   * \\return 0 on success or a negative error code otherwise\n> - * \\retval -EBUSY Buffers have already been set\n> + * \\retval -EEXIST The request already contains a buffer for the stream\n>   */\n> -int Request::setBuffers(const std::map<Stream *, Buffer *> &streamMap)\n> +int Request::addBuffer(std::unique_ptr<Buffer> buffer)\n>  {\n> -\tif (!bufferMap_.empty()) {\n> -\t\tLOG(Request, Error) << \"Buffers already set\";\n> -\t\treturn -EBUSY;\n> +\tStream *stream = buffer->stream();\n> +\n> +\tauto it = bufferMap_.find(stream);\n> +\tif (it != bufferMap_.end()) {\n> +\t\tLOG(Request, Error) << \"Buffer already set for stream\";\n> +\t\treturn -EEXIST;\n>  \t}\n>  \n> -\tbufferMap_ = streamMap;\n> +\tbufferMap_[stream] = buffer.release();\n> +\n>  \treturn 0;\n>  }\n>  \n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index d8e87c62281c..6d80646b55cd 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -407,6 +407,30 @@ Stream::Stream()\n>  {\n>  }\n>  \n> +/**\n> + * \\brief Create a Buffer instance\n> + * \\param[in] index The desired buffer index\n> + *\n> + * This method creates a Buffer instance that references a BufferMemory from\n> + * the stream's buffers pool by its \\a index. The index shall be lower than the\n> + * number of buffers in the pool.\n> + *\n> + * \\return A newly created Buffer on success or nullptr otherwise\n> + */\n> +std::unique_ptr<Buffer> Stream::createBuffer(unsigned int index)\n> +{\n> +\tif (index >= bufferPool_.count()) {\n> +\t\tLOG(Stream, Error) << \"Invalid buffer index \" << index;\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tBuffer *buffer = new Buffer();\n> +\tbuffer->index_ = index;\n> +\tbuffer->stream_ = this;\n> +\n> +\treturn std::unique_ptr<Buffer>(buffer);\n> +}\n> +\n>  /**\n>   * \\fn Stream::bufferPool()\n>   * \\brief Retrieve the buffer pool for the stream\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index af5ba803de45..65b4098abc05 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -268,8 +268,7 @@ const std::string V4L2DeviceFormat::toString() const\n>   * \\param[in] deviceNode The file-system path to the video device node\n>   */\n>  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> -\t: V4L2Device(deviceNode), bufferPool_(nullptr),\n> -\t  queuedBuffersCount_(0), fdEvent_(nullptr)\n> +\t: V4L2Device(deviceNode), bufferPool_(nullptr), fdEvent_(nullptr)\n>  {\n>  \t/*\n>  \t * We default to an MMAP based CAPTURE video device, however this will\n> @@ -764,7 +763,7 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)\n>  \tfor (i = 0; i < pool->count(); ++i) {\n>  \t\tstruct v4l2_plane planes[VIDEO_MAX_PLANES] = {};\n>  \t\tstruct v4l2_buffer buf = {};\n> -\t\tBuffer &buffer = pool->buffers()[i];\n> +\t\tBufferMemory &buffer = pool->buffers()[i];\n>  \n>  \t\tbuf.index = i;\n>  \t\tbuf.type = bufferType_;\n> @@ -782,13 +781,13 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)\n>  \n>  \t\tif (V4L2_TYPE_IS_MULTIPLANAR(buf.type)) {\n>  \t\t\tfor (unsigned int p = 0; p < buf.length; ++p) {\n> -\t\t\t\tret = createPlane(&buffer, p,\n> +\t\t\t\tret = createPlane(&buffer, i, p,\n>  \t\t\t\t\t\t  buf.m.planes[p].length);\n>  \t\t\t\tif (ret)\n>  \t\t\t\t\tbreak;\n>  \t\t\t}\n>  \t\t} else {\n> -\t\t\tret = createPlane(&buffer, 0, buf.length);\n> +\t\t\tret = createPlane(&buffer, i, 0, buf.length);\n>  \t\t}\n>  \n>  \t\tif (ret) {\n> @@ -808,19 +807,19 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)\n>  \treturn 0;\n>  }\n>  \n> -int V4L2VideoDevice::createPlane(Buffer *buffer, unsigned int planeIndex,\n> -\t\t\t\t unsigned int length)\n> +int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index,\n> +\t\t\t\t unsigned int planeIndex, unsigned int length)\n>  {\n>  \tstruct v4l2_exportbuffer expbuf = {};\n>  \tint ret;\n>  \n>  \tLOG(V4L2, Debug)\n> -\t\t<< \"Buffer \" << buffer->index()\n> +\t\t<< \"Buffer \" << index\n>  \t\t<< \" plane \" << planeIndex\n>  \t\t<< \": length=\" << length;\n>  \n>  \texpbuf.type = bufferType_;\n> -\texpbuf.index = buffer->index();\n> +\texpbuf.index = index;\n>  \texpbuf.plane = planeIndex;\n>  \texpbuf.flags = O_RDWR;\n>  \n> @@ -904,7 +903,8 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n>  \tbuf.field = V4L2_FIELD_NONE;\n>  \n>  \tbool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> -\tconst std::vector<Plane> &planes = buffer->planes();\n> +\tBufferMemory *mem = &bufferPool_->buffers()[buf.index];\n> +\tconst std::vector<Plane> &planes = mem->planes();\n>  \n>  \tif (buf.memory == V4L2_MEMORY_DMABUF) {\n>  \t\tif (multiPlanar) {\n> @@ -937,9 +937,11 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n>  \t\treturn ret;\n>  \t}\n>  \n> -\tif (queuedBuffersCount_++ == 0)\n> +\tif (queuedBuffers_.empty())\n>  \t\tfdEvent_->setEnabled(true);\n>  \n> +\tqueuedBuffers_[buf.index] = buffer;\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -971,7 +973,7 @@ int V4L2VideoDevice::queueAllBuffers(std::vector<std::unique_ptr<Buffer>> *buffe\n>  {\n>  \tint ret;\n>  \n> -\tif (queuedBuffersCount_)\n> +\tif (!queuedBuffers_.empty())\n>  \t\treturn -EINVAL;\n>  \n>  \tif (V4L2_TYPE_IS_OUTPUT(bufferType_))\n> @@ -980,8 +982,7 @@ int V4L2VideoDevice::queueAllBuffers(std::vector<std::unique_ptr<Buffer>> *buffe\n>  \tbuffers->clear();\n>  \n>  \tfor (unsigned int i = 0; i < bufferPool_->count(); ++i) {\n> -\t\tBuffer *buffer = new Buffer();\n> -\t\tbuffer->index_ = i;\n> +\t\tBuffer *buffer = new Buffer(i);\n>  \t\tbuffers->emplace_back(buffer);\n>  \t\tret = queueBuffer(buffer);\n>  \t\tif (ret)\n> @@ -1022,11 +1023,14 @@ Buffer *V4L2VideoDevice::dequeueBuffer()\n>  \n>  \tASSERT(buf.index < bufferPool_->count());\n>  \n> -\tif (--queuedBuffersCount_ == 0)\n> +\tauto it = queuedBuffers_.find(buf.index);\n> +\tBuffer *buffer = it->second;\n> +\tqueuedBuffers_.erase(it);\n> +\n> +\tif (queuedBuffers_.empty())\n>  \t\tfdEvent_->setEnabled(false);\n>  \n> -\tBuffer *buffer = &bufferPool_->buffers()[buf.index];\n> -\n> +\tbuffer->index_ = buf.index;\n>  \tbuffer->bytesused_ = buf.bytesused;\n>  \tbuffer->timestamp_ = buf.timestamp.tv_sec * 1000000000ULL\n>  \t\t\t   + buf.timestamp.tv_usec * 1000ULL;\n> @@ -1101,7 +1105,7 @@ int V4L2VideoDevice::streamOff()\n>  \t\treturn ret;\n>  \t}\n>  \n> -\tqueuedBuffersCount_ = 0;\n> +\tqueuedBuffers_.clear();\n>  \tfdEvent_->setEnabled(false);\n>  \n>  \treturn 0;\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 907d2423ed15..6ecf30e33bcf 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -142,8 +142,7 @@ int MainWindow::startCapture()\n>  \n>  \tBufferPool &pool = stream->bufferPool();\n>  \tstd::vector<Request *> requests;\n> -\n> -\tfor (Buffer &buffer : pool.buffers()) {\n> +\tfor (unsigned int i = 0; i < pool.count(); ++i) {\n>  \t\tRequest *request = camera_->createRequest();\n>  \t\tif (!request) {\n>  \t\t\tstd::cerr << \"Can't create request\" << std::endl;\n> @@ -151,11 +150,15 @@ int MainWindow::startCapture()\n>  \t\t\tgoto error;\n>  \t\t}\n>  \n> -\t\tstd::map<Stream *, Buffer *> map;\n> -\t\tmap[stream] = &buffer;\n> -\t\tret = request->setBuffers(map);\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(std::move(buffer));\n>  \t\tif (ret < 0) {\n> -\t\t\tstd::cerr << \"Can't set buffers for request\" << std::endl;\n> +\t\t\tstd::cerr << \"Can't set buffer for request\" << std::endl;\n>  \t\t\tgoto error;\n>  \t\t}\n>  \n> @@ -219,6 +222,7 @@ void MainWindow::requestComplete(Request *request,\n>  \n>  \tframesCaptured_++;\n>  \n> +\tStream *stream = buffers.begin()->first;\n>  \tBuffer *buffer = buffers.begin()->second;\n>  \n>  \tdouble fps = buffer->timestamp() - lastBufferTime_;\n> @@ -232,7 +236,8 @@ void MainWindow::requestComplete(Request *request,\n>  \t\t  << \" fps: \" << std::fixed << std::setprecision(2) << fps\n>  \t\t  << std::endl;\n>  \n> -\tdisplay(buffer);\n> +\tBufferMemory *mem = &stream->bufferPool().buffers()[buffer->index()];\n> +\tdisplay(buffer, mem);\n>  \n>  \trequest = camera_->createRequest();\n>  \tif (!request) {\n> @@ -240,16 +245,29 @@ void MainWindow::requestComplete(Request *request,\n>  \t\treturn;\n>  \t}\n>  \n> -\trequest->setBuffers(buffers);\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 \" << index << std::endl;\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\trequest->addBuffer(std::move(newBuffer));\n> +\t}\n> +\n>  \tcamera_->queueRequest(request);\n>  }\n>  \n> -int MainWindow::display(Buffer *buffer)\n> +int MainWindow::display(Buffer *buffer, BufferMemory *mem)\n>  {\n> -\tif (buffer->planes().size() != 1)\n> +\tif (mem->planes().size() != 1)\n>  \t\treturn -EINVAL;\n>  \n> -\tPlane &plane = buffer->planes().front();\n> +\tPlane &plane = mem->planes().front();\n>  \tunsigned char *raw = static_cast<unsigned char *>(plane.mem());\n>  \tviewfinder_->display(raw, buffer->bytesused());\n>  \n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index f58cb6a65b2b..b4f6f747e16e 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -48,7 +48,7 @@ private:\n>  \n>  \tvoid requestComplete(Request *request,\n>  \t\t\t     const std::map<Stream *, Buffer *> &buffers);\n> -\tint display(Buffer *buffer);\n> +\tint display(Buffer *buffer, BufferMemory *mem);\n>  \n>  \tQString title_;\n>  \tQTimer titleTimer_;\n> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> index 7ce247cc482d..ff1cbd6cbac0 100644\n> --- a/test/camera/capture.cpp\n> +++ b/test/camera/capture.cpp\n> @@ -34,9 +34,13 @@ protected:\n>  \n>  \t\tcompleteRequestsCount_++;\n>  \n> -\t\t/* Reuse the buffers for a new request. */\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> +\n>  \t\trequest = camera_->createRequest();\n> -\t\trequest->setBuffers(buffers);\n> +\t\trequest->addBuffer(std::move(newBuffer));\n>  \t\tcamera_->queueRequest(request);\n>  \t}\n>  \n> @@ -78,15 +82,20 @@ protected:\n>  \t\tStream *stream = cfg.stream();\n>  \t\tBufferPool &pool = stream->bufferPool();\n>  \t\tstd::vector<Request *> requests;\n> -\t\tfor (Buffer &buffer : pool.buffers()) {\n> +\t\tfor (unsigned int i = 0; i < pool.count(); ++i) {\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::map<Stream *, Buffer *> map = { { stream, &buffer } };\n> -\t\t\tif (request->setBuffers(map)) {\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(std::move(buffer))) {\n>  \t\t\t\tcout << \"Failed to associating buffer with request\" << endl;\n>  \t\t\t\treturn TestFail;\n>  \t\t\t}\n> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> index 84d2a6fab5f0..12d5e0e1d534 100644\n> --- a/test/camera/statemachine.cpp\n> +++ b/test/camera/statemachine.cpp\n> @@ -211,10 +211,8 @@ protected:\n>  \t\t\treturn TestFail;\n>  \n>  \t\tStream *stream = *camera_->streams().begin();\n> -\t\tBufferPool &pool = stream->bufferPool();\n> -\t\tBuffer &buffer = pool.buffers().front();\n> -\t\tstd::map<Stream *, Buffer *> map = { { stream, &buffer } };\n> -\t\tif (request->setBuffers(map))\n> +\t\tstd::unique_ptr<Buffer> buffer = stream->createBuffer(0);\n> +\t\tif (request->addBuffer(std::move(buffer)))\n>  \t\t\treturn TestFail;\n>  \n>  \t\tif (camera_->queueRequest(request))\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-pf1-x443.google.com (mail-pf1-x443.google.com\n\t[IPv6:2607:f8b0:4864:20::443])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3BD861572\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 14 Jul 2019 12:42:49 +0200 (CEST)","by mail-pf1-x443.google.com with SMTP id y15so6151804pfn.5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 14 Jul 2019 03:42:49 -0700 (PDT)","from localhost (softbank126159224182.bbtec.net. [126.159.224.182])\n\tby smtp.gmail.com with ESMTPSA id\n\te10sm13986045pfi.173.2019.07.14.03.42.45\n\t(version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256);\n\tSun, 14 Jul 2019 03:42:46 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=gMfrRAy0lYJvRjtq4l0FqMhYAyphbTobhm87/24nVk8=;\n\tb=icBJaJ2NVmo9X9kgCuYh5Bb3x2E7RXgWcYuAhVsJ/I9dQFxizNcyt6kGtyE9/ErzLH\n\tj3EyduZlYXAh1MEhB+epR+7zJrxk4TGrPMZnQSm7Xp+oXFVBnZbI9oSpE0/VR3aw59Su\n\tBafvM5hvMuZwoZO0U0pEumQbx/8+pHSD0pq+OmElY9cqTjlIKTEIrumoGfGOzwn12F8n\n\tho6+pxfshsYgq0jX07sVSi1h5+8hboYgn0TcMMxPc7HHrUKC3PTUxgAxUrF29MdriMJK\n\tFn9Da5rywK94bhdfBL+cvtkY7WvQYOa8fSqgdde/5OcVu4ukE8hm7MfLQRt2g2X84HYI\n\tnEvA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=gMfrRAy0lYJvRjtq4l0FqMhYAyphbTobhm87/24nVk8=;\n\tb=YzxDYi4mABA/dqUbfK44ndoZhd5Dk1YbPUBZ6Zn02qCxgU917SyTQGiNKvPVZDtSmV\n\tWhGSbshu4O7edvIy5XnK9kKbe+JxBhnk6C8qy2D+fWGUbOSyvG8T5sQVVIMbvcWdlTlJ\n\tdFU/9zHj0GUCHOJkfEwm1hGOC21tyNR1BNfaNYwDL9vReMi9EPx3yYdMpb5vyh1F23Lp\n\tuAiAYu3+DNkf1aBIbKkAqB6wWxplHr/67fCF73YHPOV1IXorL6DlD9owgXlODt00vKVT\n\tA2ZNloKtiL3YQfQiF7NXy8xyAqwIN5vBELeGz25bzeaYlezztAQdwDibIBqd7EGrQ9N/\n\t4r+Q==","X-Gm-Message-State":"APjAAAVu4S+Byga4CJoaqpHeBsrTDrFOyJMIS+URWxMHgXwywudcZd2D\n\tDviB8yGV8L7aA1Sie1bdCVk=","X-Google-Smtp-Source":"APXvYqxFiJ56QvFVEa9Thu/Bigl2i3ZOU3D67S6cB/F3bSc+D0HlQ6nfCwNlDJaDL9XPCxw8KHx6mA==","X-Received":"by 2002:a65:5b8e:: with SMTP id\n\ti14mr20746964pgr.188.1563100967681; \n\tSun, 14 Jul 2019 03:42:47 -0700 (PDT)","Date":"Sun, 14 Jul 2019 19:42:42 +0900","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190714104242.GD31102@wyvern>","References":"<20190713172351.25452-1-laurent.pinchart@ideasonboard.com>\n\t<20190713172351.25452-7-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190713172351.25452-7-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 06/16] libcamera: buffer: Split\n\tmemory information to BufferMemory","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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":"Sun, 14 Jul 2019 10:42:50 -0000"}}]