[{"id":3259,"web_url":"https://patchwork.libcamera.org/comment/3259/","msgid":"<20191215152046.GE4889@pendragon.ideasonboard.com>","date":"2019-12-15T15:20:46","subject":"Re: [libcamera-devel] [PATCH 27/30] 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 Wed, Nov 27, 2019 at 12:36:17AM +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> BufferAllocator helper.\n> \n> A follow up changes to this one will finalize the transition to the new\n> FrameBuffer interface by removing code that is left unused after this\n> change.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/camera.h | 4 +-\n> include/libcamera/request.h | 14 +-\n> src/android/camera_device.cpp | 19 +-\n> src/cam/buffer_writer.cpp | 9 +-\n> src/cam/buffer_writer.h | 3 +-\n> src/cam/capture.cpp | 38 +-\n> src/cam/capture.h | 4 +-\n> src/libcamera/camera.cpp | 34 --\n> src/libcamera/include/pipeline_handler.h | 5 +-\n> src/libcamera/include/v4l2_videodevice.h | 9 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 223 +++-------\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 120 +++--\n> src/libcamera/pipeline/uvcvideo.cpp | 20 +-\n> src/libcamera/pipeline/vimc.cpp | 20 +-\n> src/libcamera/pipeline_handler.cpp | 2 +-\n> src/libcamera/request.cpp | 16 +-\n> src/libcamera/v4l2_videodevice.cpp | 80 +---\n> src/qcam/main_window.cpp | 47 +-\n> src/qcam/main_window.h | 6 +-\n> test/camera/buffer_import.cpp | 415 +++++-------------\n> test/camera/capture.cpp | 31 +-\n> test/camera/statemachine.cpp | 12 +-\n> test/v4l2_videodevice/buffer_sharing.cpp | 23 +-\n> test/v4l2_videodevice/capture_async.cpp | 14 +-\n> test/v4l2_videodevice/request_buffers.cpp | 11 +-\n> test/v4l2_videodevice/stream_on_off.cpp | 6 +-\n> test/v4l2_videodevice/v4l2_m2mdevice.cpp | 40 +-\n> test/v4l2_videodevice/v4l2_videodevice_test.h | 2 +-\n> 28 files changed, 415 insertions(+), 812 deletions(-)\n\nI like the diffstat.\n\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index ef6a37bb142c83a6..02d047b9649f53d0 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 PipelineHandler;\n> class Request;\n> \n> @@ -77,7 +77,7 @@ public:\n> \n> \tconst std::string &name() const;\n> \n> -\tSignal bufferCompleted;\n> +\tSignal bufferCompleted;\n> \tSignal requestCompleted;\n> \tSignal 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 &buffers() const { return bufferMap_; }\n> -\tint addBuffer(Stream *stream, std::unique_ptr buffer);\n> -\tBuffer *findBuffer(Stream *stream) const;\n> +\tconst std::map &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 bufferMap_;\n> -\tstd::unordered_set pending_;\n> +\tstd::map bufferMap_;\n> +\tstd::unordered_set 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 55b29a9a41ab8943..708cf95b0c3c25c2 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -739,13 +739,13 @@ 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 fds = {\n> -\t\tcamera3Handle->data[0],\n> -\t\tcamera3Handle->data[1],\n> -\t\tcamera3Handle->data[2],\n> -\t};\n> \n> -\tstd::unique_ptr buffer = stream->createBuffer(fds);\n> +\tstd::vector planes;\n> +\tplanes.push_back({ .fd = camera3Handle->data[0], .length = 0 });\n> +\tplanes.push_back({ .fd = camera3Handle->data[1], .length = 0 });\n> +\tplanes.push_back({ .fd = camera3Handle->data[2], .length = 0 });\n\nOUCH. .length = 0 clearly shows a problem. We don't necessarily need to\nfix it as part of this series, but we need a strategy. Either we can get\nthe camera HAL to supply the length (casting the buffer_handle_t to the\nhidden underlying structure ? calling fstat() on the fd ?), or we have\nto support length == 0, in which case we may as well drop the length\nfield completely if we can't rely on it. In the latter case we have to\nconsider the implications on the API, I think that the BufferAllocator\nclass would at least need to report the size of the allocated buffers.\n\n> +\n> +\tFrameBuffer *buffer = new FrameBuffer(planes);\n\nNow that I read this, I wonder what the best API for the FileDescriptor\nclass would be. The fds that we receive through camera3Handle shall\ncertainly not be closed by us, but there's also no strict need for us to\nduplicate them internally. Would it make sense for FileDescriptor to\nhave two constructors, one taking an int and one taking an int &&, and\nstore internally a flag telling whether the fd is owned by the\nFileDescriptor class (for the int && case) or borrowed (for the in case)\n? FileDescriptor would then only close the fd if it owns it. This would\nremove the need to dup(), but the caller would then be responsible to\nkeep the fd valid and close it itself, which may not be convenient.\nMaybe we then need a third constructor that would dup() the fd (and of\ncourse flag ownership of the dup'ed fd in that case), possibly as a flag\nfor the int constructor ? The rationale for all this is that we should\nnot only avoid memory allocation for every request, but also, and more\nimportantly as it is more costly, avoid dup'ing file descriptors for\nevery request.\n\n> \tif (!buffer) {\n> \t\tLOG(HAL, Error) << \"Failed to create buffer\";\n> \t\tdelete descriptor;\n> @@ -754,7 +754,7 @@ void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reque\n> \n> \tRequest *request =\n> \t\tcamera_->createRequest(reinterpret_cast(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 +771,8 @@ error:\n> \n> void CameraDevice::requestComplete(Request *request)\n> {\n> -\tconst std::map &buffers = request->buffers();\n> -\tBuffer *libcameraBuffer = buffers.begin()->second;\n> +\tconst std::map &buffers = request->buffers();\n> +\tFrameBuffer *libcameraBuffer = buffers.begin()->second;\n\nLet's name the variable either frameBuffer or just buffer.\n\n> \tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> \tstd::unique_ptr resultMetadata;\n> \n> @@ -825,6 +825,7 @@ void CameraDevice::requestComplete(Request *request)\n> \tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> \n> \tdelete descriptor;\n> +\tdelete libcameraBuffer;\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 b6b40baeee661df6..5cead75a16d0923a 100644\n> --- a/src/cam/buffer_writer.cpp\n> +++ b/src/cam/buffer_writer.cpp\n> @@ -21,7 +21,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> @@ -42,10 +42,9 @@ 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 (Dmabuf &dmabuf : mem->planes()) {\n> -\t\tvoid *data = dmabuf.mem();\n> -\t\tunsigned int length = dmabuf.length();\n> +\tfor (Dmabuf *dmabuf : buffer->dmabufs()) {\n> +\t\tvoid *data = dmabuf->mem();\n> +\t\tunsigned int length = dmabuf->length();\n\nShould we use bytesused from buffer->metadata() ?\n\n> \n> \t\tret = ::write(fd, data, length);\n> \t\tif (ret < 0) {\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 a4fa88a8d99669bc..154e6e461ee2b96b 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> +\tBufferAllocator allocator(camera_);\n> +\n> +\tret = capture(loop, allocator);\n> \n> \tif (options.isSet(OptFile)) {\n> \t\tdelete writer_;\n> @@ -69,14 +72,22 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)\n> \treturn ret;\n> }\n> \n> -int Capture::capture(EventLoop *loop)\n> +int Capture::capture(EventLoop *loop, BufferAllocator &allocator)\n\nWe tend to use either const references or non-const pointers, I would\npass a *allocator. Or maybe create the allocator in Capture::capture() ?\n\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(), cfg);\n\nWe seem to always pass the stream from the stream configuration, and I\nthink we require the camera to be configured with the configuration\nbefore allocating buffers (this needs to be documented). If that's the\ncase, can't we drop the stream pointer and retrieve it from the\nconfiguration in BufferAllocator::allocate() ?\n\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 +104,9 @@ 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 = stream->createBuffer(i);\n> +\t\t\tFrameBuffer *buffer = allocator.buffers(stream)[i];\n\nYou should create a local reference for allocator.buffers(stream) as an\noptimization.\n\n> \n> -\t\t\tret = request->addBuffer(stream, std::move(buffer));\n> +\t\t\tret = request->addBuffer(stream, 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> @@ -138,7 +149,7 @@ void Capture::requestComplete(Request *request)\n> \tif (request->status() == Request::RequestCancelled)\n> \t\treturn;\n> \n> -\tconst std::map &buffers = request->buffers();\n> +\tconst std::map &buffers = request->buffers();\n> \n> \tstd::chrono::steady_clock::time_point now = std::chrono::steady_clock::now();\n> \tdouble fps = std::chrono::duration_cast(now - last_).count();\n> @@ -151,7 +162,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\tinfo << \" \" << name\n> @@ -180,16 +191,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 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..3a8842fbd2080c2c 100644\n> --- a/src/cam/capture.h\n> +++ b/src/cam/capture.h\n> @@ -10,6 +10,8 @@\n> #include \n> #include \n> \n> +#include \n> +#include \n> #include \n> #include \n> #include \n> @@ -26,7 +28,7 @@ public:\n> \n> \tint run(EventLoop *loop, const OptionsParser::Options &options);\n> private:\n> -\tint capture(EventLoop *loop);\n> +\tint capture(EventLoop *loop, libcamera::BufferAllocator &allocator);\n> \n> \tvoid requestComplete(libcamera::Request *request);\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 6b1b3fb64f8b2c0b..05fdcaab8f918afa 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -678,12 +678,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> @@ -739,14 +733,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> @@ -812,24 +798,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> @@ -923,13 +896,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\nNext step, reusing requests :-)\n\n> }\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 6ce8ea1c8d31b870..e4588f10bac0228c 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> @@ -79,7 +79,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/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> index 0e94c3f71d0b1208..ced3cb229c509ad2 100644\n> --- a/src/libcamera/include/v4l2_videodevice.h\n> +++ b/src/libcamera/include/v4l2_videodevice.h\n> @@ -167,9 +167,8 @@ public:\n> \tint externalBuffers(unsigned int count);\n> \tint releaseBuffers();\n> \n> -\tint queueBuffer(Buffer *buffer);\n> -\tstd::vector> queueAllBuffers();\n> -\tSignal bufferReady;\n> +\tint queueBuffer(FrameBuffer *buffer);\n> +\tSignal bufferReady;\n> \n> \tint streamOn();\n> \tint streamOff();\n> @@ -203,7 +202,7 @@ private:\n> \tFrameBuffer *createBuffer(struct v4l2_buffer buf);\n> \tint exportDmaBuffer(unsigned int index, unsigned int plane);\n> \n> -\tBuffer *dequeueBuffer();\n> +\tFrameBuffer *dequeueBuffer();\n> \tvoid bufferAvailable(EventNotifier *notifier);\n> \n> \tV4L2Capability caps_;\n> @@ -213,7 +212,7 @@ private:\n> \n> \tBufferPool *bufferPool_;\n> \tV4L2BufferCache *cache_;\n> -\tstd::map queuedBuffers_;\n> +\tstd::map queuedBuffers_;\n> \n> \tEventNotifier *fdEvent_;\n> };\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 5080ac6c4d0abe3b..8d9420aea3eb0b21 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,7 @@ public:\n> \t\tV4L2VideoDevice *dev;\n> \t\tunsigned int pad;\n> \t\tstd::string name;\n> -\t\tBufferPool *pool;\n> +\t\tstd::vector buffers;\n> \t};\n> \n> \tImgUDevice()\n> @@ -70,10 +72,9 @@ public:\n> \tint configureOutput(ImgUOutput *output,\n> \t\t\t const StreamConfiguration &cfg);\n> \n> -\tint importInputBuffers(BufferPool *pool);\n> -\tint importOutputBuffers(ImgUOutput *output, BufferPool *pool);\n> -\tint exportOutputBuffers(ImgUOutput *output, BufferPool *pool);\n> -\tvoid freeBuffers();\n> +\tint importInputBuffers(unsigned int count);\n> +\tint exportOutputBuffers(ImgUOutput *output, unsigned int count);\n> +\tvoid freeBuffers(IPU3CameraData *data);\n> \n> \tint start();\n> \tint stop();\n> @@ -93,10 +94,6 @@ public:\n> \tImgUOutput viewfinder_;\n> \tImgUOutput stat_;\n> \t/* \\todo Add param video device for 3A tuning */\n> -\n> -\tBufferPool vfPool_;\n> -\tBufferPool statPool_;\n> -\tBufferPool outPool_;\n> };\n> \n> class CIO2Device\n> @@ -120,10 +117,10 @@ public:\n> \tint configure(const Size &size,\n> \t\t V4L2DeviceFormat *outputFormat);\n> \n> -\tBufferPool *exportBuffers();\n> +\tint exportBuffers();\n\nHow about naming this allocateBuffers() to match freeBuffers() ? The\nCIO2 always allocates buffers internally and exports them, so there's no\nneed to differentiate between exporting and importing. The buffers_\nvector being private to the CIO2Device class makes me believe this would\nbe a good rename. Note that it would get in the way of using fences to\npre-queue buffers to the ImgU input, but V4L2 doesn't support fences at\nthe moment so I don't think we need to care, a bigger refactoring will\nlikely be needed at that time anyway.\n\n> \tvoid freeBuffers();\n> \n> -\tint start(std::vector> *buffers);\n> +\tint start();\n> \tint stop();\n> \n> \tstatic int mediaBusToFormat(unsigned int code);\n> @@ -132,7 +129,8 @@ public:\n> \tV4L2Subdevice *csi2_;\n> \tCameraSensor *sensor_;\n> \n> -\tBufferPool pool_;\n> +private:\n> +\tstd::vector buffers_;\n\nWould a vector of unique pointers simplify CIO2Device::freeBuffers() ?\n\n> };\n> \n> class IPU3Stream : public V4L2Stream\n> @@ -163,17 +161,15 @@ public:\n> \t\tdelete vfStream_;\n> \t}\n> \n> -\tvoid imguOutputBufferReady(Buffer *buffer);\n> -\tvoid imguInputBufferReady(Buffer *buffer);\n> -\tvoid cio2BufferReady(Buffer *buffer);\n> +\tvoid imguOutputBufferReady(FrameBuffer *buffer);\n> +\tvoid imguInputBufferReady(FrameBuffer *buffer);\n> +\tvoid cio2BufferReady(FrameBuffer *buffer);\n> \n> \tCIO2Device cio2_;\n> \tImgUDevice *imgu_;\n> \n> \tIPU3Stream *outStream_;\n> \tIPU3Stream *vfStream_;\n> -\n> -\tstd::vector> rawBuffers_;\n> };\n> \n> class IPU3CameraConfiguration : public CameraConfiguration\n> @@ -637,70 +633,42 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n> \t\t\t\t\t const std::set &streams)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tIPU3Stream *outStream = data->outStream_;\n> -\tIPU3Stream *vfStream = data->vfStream_;\n> -\tCIO2Device *cio2 = &data->cio2_;\n> -\tImgUDevice *imgu = data->imgu_;\n> \tunsigned int bufferCount;\n> \tint ret;\n> \n> -\t/* Share buffers between CIO2 output and ImgU input. */\n> -\tBufferPool *pool = cio2->exportBuffers();\n> -\tif (!pool)\n> -\t\treturn -ENOMEM;\n> +\tret = data->cio2_.exportBuffers();\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> \n> -\tret = imgu->importInputBuffers(pool);\n> -\tif (ret)\n> -\t\tgoto error;\n> +\tbufferCount = ret;\n> \n> -\t/*\n> -\t * Use for the stat's internal pool the same number of buffer as\n> -\t * for the input pool.\n> -\t * \\todo To be revised when we'll actually use the stat node.\n> -\t */\n\nCould you please keep the comments ? They're important.\n\n> -\tbufferCount = pool->count();\n> -\timgu->stat_.pool->createBuffers(bufferCount);\n> -\tret = imgu->exportOutputBuffers(&imgu->stat_, imgu->stat_.pool);\n> +\tret = data->imgu_->importInputBuffers(bufferCount);\n> \tif (ret)\n> \t\tgoto error;\n> \n> -\t/* Allocate buffers for each active stream. */\n> -\tfor (Stream *s : streams) {\n> -\t\tIPU3Stream *stream = static_cast(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> +\tret = data->imgu_->exportOutputBuffers(&data->imgu_->stat_, bufferCount);\n> +\tif (ret < 0)\n> +\t\tgoto error;\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> +\tif (!data->outStream_->active_) {\n> +\t\tret = data->imgu_->exportOutputBuffers(data->outStream_->device_,\n> +\t\t\t\t\t\t bufferCount);\n> +\t\tif (ret < 0)\n> \t\t\tgoto error;\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> +\tif (!data->vfStream_->active_) {\n> +\t\tret = data->imgu_->exportOutputBuffers(data->vfStream_->device_,\n> +\t\t\t\t\t\t bufferCount);\n> +\t\tif (ret < 0)\n> \t\t\tgoto error;\n> \t}\n> \n> \treturn 0;\n> -\n> error:\n> \tfreeBuffers(camera, streams);\n> \n> @@ -713,7 +681,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> @@ -729,7 +697,7 @@ int PipelineHandlerIPU3::start(Camera *camera)\n> \t * Start the ImgU video devices, buffers will be queued to the\n> \t * ImgU output and viewfinder when requests will be queued.\n> \t */\n> -\tret = cio2->start(&data->rawBuffers_);\n> +\tret = cio2->start();\n> \tif (ret)\n> \t\tgoto error;\n> \n> @@ -745,7 +713,6 @@ int PipelineHandlerIPU3::start(Camera *camera)\n> error:\n> \tLOG(IPU3, Error) << \"Failed to start camera \" << camera->name();\n> \n> -\tdata->rawBuffers_.clear();\n> \treturn ret;\n> }\n> \n> @@ -759,8 +726,6 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \tif (ret)\n> \t\tLOG(IPU3, Warning) << \"Failed to stop camera \"\n> \t\t\t\t << camera->name();\n> -\n> -\tdata->rawBuffers_.clear();\n> }\n> \n> int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request)\n> @@ -769,7 +734,7 @@ int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request)\n> \n> \tfor (auto it : request->buffers()) {\n> \t\tIPU3Stream *stream = static_cast(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> @@ -930,7 +895,7 @@ int PipelineHandlerIPU3::registerCameras()\n> * Buffers completed from the ImgU input are immediately queued back to the\n> * CIO2 unit to continue frame capture.\n> */\n> -void IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> +void IPU3CameraData::imguInputBufferReady(FrameBuffer *buffer)\n> {\n> \t/* \\todo Handle buffer failures when state is set to BufferError. */\n> \tif (buffer->info().status() == BufferInfo::BufferCancelled)\n> @@ -945,7 +910,7 @@ void IPU3CameraData::imguInputBufferReady(Buffer *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> @@ -964,7 +929,7 @@ void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> * Buffers completed from the CIO2 are immediately queued to the ImgU unit\n> * for further processing.\n> */\n> -void IPU3CameraData::cio2BufferReady(Buffer *buffer)\n> +void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> {\n> \t/* \\todo Handle buffer failures when state is set to BufferError. */\n> \tif (buffer->info().status() == BufferInfo::BufferCancelled)\n> @@ -1020,7 +985,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> @@ -1030,7 +994,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> @@ -1039,7 +1002,6 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n> \n> \tstat_.pad = PAD_STAT;\n> \tstat_.name = \"stat\";\n> -\tstat_.pool = &statPool_;\n> \n> \treturn 0;\n> }\n> @@ -1139,85 +1101,49 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> \treturn 0;\n> }\n> \n> -/**\n> - * \\brief Import buffers from \\a pool into the ImgU input\n> - * \\param[in] pool The buffer pool to import\n> - * \\return 0 on success or a negative error code otherwise\n> - */\n> -int ImgUDevice::importInputBuffers(BufferPool *pool)\n> +int ImgUDevice::importInputBuffers(unsigned int count)\n\nThis method doesn't import buffers, so I think it should be renamed.\n\n> {\n> -\tint ret = input_->importBuffers(pool);\n> -\tif (ret) {\n> +\tint ret = input_->externalBuffers(count);\n> +\tif (ret)\n> \t\tLOG(IPU3, Error) << \"Failed to import ImgU input buffers\";\n> -\t\treturn ret;\n> -\t}\n> \n> -\treturn 0;\n> +\treturn ret;\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\nLet's not lose all the documentation, even if the method is internal :-)\nI was about to make a proposal on how to keep this, but given that the\nmethods will likely need to be refactored for the reasons explained\nbelow, I expect the documentation will change anyway.\n\n> -int ImgUDevice::exportOutputBuffers(ImgUOutput *output, BufferPool *pool)\n> +int ImgUDevice::exportOutputBuffers(ImgUOutput *output, unsigned int count)\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> +\tint ret;\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> +\tret = output->dev->allocateBuffers(count, &output->buffers);\n\nI think you're leaking this. A vector of unique pointers would help\nmaking ownership clearer.\n\n> +\tif (ret < 0)\n> +\t\tLOG(IPU3, Error) << \"Failed to allocate ImgU \"\n> +\t\t\t\t << output->name << \" buffers\";\n> \n> -\treturn 0;\n> +\treturn ret;\n> }\n\nHow about moving this method to the ImgUOutput class ? It doesn't access\nany field of ImgUDevice, I think it would make the code more readably,\nand you could just call it allocateBuffers there.\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> +\t\tret = output_.dev->releaseBuffers();\n> +\t\tif (ret)\n> +\t\t\tLOG(IPU3, Error) << \"Failed to release ImgU output buffers\";\n> +\t}\n\nI would like to simplify this method and avoid passing data as an\nargument, to keep the symmetry with the other buffer-related methods. \nHow about simplifying this by either tracking whether buffers have been\nallocated in struct ImgUOutput, or making V4L2VideoDevice::releaseBuffers()\na no-op when no buffers have been allocated (and updating its\ndocumentation to say so explicitly) and calling it unconditionally ? And\nmaybe adding an ImgUOutput::freeBuffers() to keep symmetry with the\nproposed ImgUOutput::allocateBuffers() above ?\n\nSome more logic may be needed as I suspect the import and export cases\nneed to be treated differently, and given my comments on the Stream\ninterface for buffer allocation, I think you'll have to rework this\nanyway. The code is quite convoluted here to be frank, it's hard to\ntrack which buffers are allocated and free by who, and I think that's at\nleast partly the consequence of the automatic handling in\nV4L2Stream::start() and V4L2Stream::stop(). I would much rather see this\nbeing done explicitly in pipeline handlers, it would be a bit more work,\nbut I think the result would be much easier to read and validate.\n\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> @@ -1452,38 +1378,33 @@ int CIO2Device::configure(const Size &size,\n> \treturn 0;\n> }\n> \n> -/**\n> - * \\brief Allocate CIO2 memory buffers and export them in a BufferPool\n> - *\n> - * Allocate memory buffers in the CIO2 video device and export them to\n> - * a buffer pool that can be imported by another device.\n> - *\n> - * \\return The buffer pool with export buffers on success or nullptr otherwise\n> - */\n\n/**\n * \\brief Allocate frame buffers for the CIO2 output\n *\n * Allocate frame buffers in the CIO2 video device to be used to capture frames\n * from the CIO2 output. The buffers are stored in the CIO2Device::buffers_\n * vector.\n *\n * \\return 0 on success or a negative error code if allocation fails\n */\n\n> -BufferPool *CIO2Device::exportBuffers()\n> +int CIO2Device::exportBuffers()\n> {\n> -\tpool_.createBuffers(CIO2_BUFFER_COUNT);\n> -\n> -\tint ret = output_->exportBuffers(&pool_);\n> -\tif (ret) {\n> +\tint ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);\n> +\tif (ret < 0)\n> \t\tLOG(IPU3, Error) << \"Failed to export CIO2 buffers\";\n> -\t\treturn nullptr;\n> -\t}\n> \n> -\treturn &pool_;\n> +\treturn ret;\n> }\n> \n> void CIO2Device::freeBuffers()\n> {\n> +\tfor (FrameBuffer *buffer : buffers_)\n> +\t\tdelete buffer;\n> +\n> \tif (output_->releaseBuffers())\n> \t\tLOG(IPU3, Error) << \"Failed to release CIO2 buffers\";\n> }\n> \n> -int CIO2Device::start(std::vector> *buffers)\n> +int CIO2Device::start()\n> {\n> -\t*buffers = output_->queueAllBuffers();\n> -\tif (buffers->empty())\n> -\t\treturn -EINVAL;\n> +\tfor (FrameBuffer *buffer : buffers_) {\n> +\t\tint ret = output_->queueBuffer(buffer);\n> +\t\tif (ret) {\n> +\t\t\tLOG(IPU3, Error) << \"Failed to queue CIO2 buffer\";\n> +\t\t\treturn ret;\n> +\t\t}\n> +\t}\n> \n> \treturn output_->streamOn();\n> }\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index ca3d92c7ad637c3a..96e863f0208fa748 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -32,8 +32,7 @@\n> #include \"v4l2_subdevice.h\"\n> #include \"v4l2_videodevice.h\"\n> \n> -#define RKISP1_PARAM_BASE 0x100\n> -#define RKISP1_STAT_BASE 0x200\n> +#define RKISP1_BUFFER_COUNT 4\n> \n> namespace libcamera {\n> \n> @@ -52,9 +51,9 @@ struct RkISP1FrameInfo {\n> \tunsigned int frame;\n> \tRequest *request;\n> \n> -\tBuffer *paramBuffer;\n> -\tBuffer *statBuffer;\n> -\tBuffer *videoBuffer;\n> +\tFrameBuffer *paramBuffer;\n> +\tFrameBuffer *statBuffer;\n> +\tFrameBuffer *videoBuffer;\n> \n> \tbool paramFilled;\n> \tbool paramDequeued;\n> @@ -70,7 +69,7 @@ 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> private:\n> @@ -89,7 +88,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> @@ -154,8 +153,6 @@ public:\n> \tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> \n> private:\n> -\tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> -\n\nAny reason for this ? \n\n> \t/*\n> \t * The RkISP1CameraData instance is guaranteed to be valid as long as the\n> \t * corresponding Camera instance is valid. In order to borrow a\n> @@ -203,9 +200,9 @@ private:\n> \tint initLinks();\n> \tint createCamera(MediaEntity *sensor);\n> \tvoid tryCompleteRequest(Request *request);\n> -\tvoid bufferReady(Buffer *buffer);\n> -\tvoid paramReady(Buffer *buffer);\n> -\tvoid statReady(Buffer *buffer);\n> +\tvoid bufferReady(FrameBuffer *buffer);\n> +\tvoid paramReady(FrameBuffer *buffer);\n> +\tvoid statReady(FrameBuffer *buffer);\n> \n> \tMediaDevice *media_;\n> \tV4L2Subdevice *dphy_;\n> @@ -214,11 +211,8 @@ private:\n> \tV4L2VideoDevice *param_;\n> \tV4L2VideoDevice *stat_;\n> \n> -\tBufferPool paramPool_;\n> -\tBufferPool statPool_;\n> -\n> -\tstd::queue paramBuffers_;\n> -\tstd::queue statBuffers_;\n> +\tstd::queue paramBuffers_;\n> +\tstd::queue statBuffers_;\n\nAs for the IPU3 pipeline handler, wouldn't a queue of unique pointers\nhelp ? Please also take into account the other comments made for the\nIPU3 pipeline handler, some of them apply here.\n\n> \n> \tCamera *activeCamera_;\n> };\n> @@ -234,15 +228,15 @@ RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stre\n> \t\tLOG(RkISP1, Error) << \"Parameters buffer underrun\";\n> \t\treturn nullptr;\n> \t}\n> -\tBuffer *paramBuffer = pipe_->paramBuffers_.front();\n> +\tFrameBuffer *paramBuffer = pipe_->paramBuffers_.front();\n> \n> \tif (pipe_->statBuffers_.empty()) {\n> \t\tLOG(RkISP1, Error) << \"Statisitc buffer underrun\";\n> \t\treturn nullptr;\n> \t}\n> -\tBuffer *statBuffer = pipe_->statBuffers_.front();\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> @@ -295,7 +289,7 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> \treturn nullptr;\n> }\n> \n> -RkISP1FrameInfo *RkISP1Frames::find(Buffer *buffer)\n> +RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> {\n> \tfor (auto &itInfo : frameInfo_) {\n> \t\tRkISP1FrameInfo *info = itInfo.second;\n> @@ -661,57 +655,42 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,\n> \t\t\t\t\t const std::set &streams)\n> {\n> \tRkISP1CameraData *data = cameraData(camera);\n> -\tStream *stream = *streams.begin();\n> +\tstd::vector paramBuffers, statBuffers;\n> +\tunsigned int count = 1;\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> +\tret = param_->allocateBuffers(RKISP1_BUFFER_COUNT, ¶mBuffers);\n> +\tif (ret < 0)\n> +\t\tgoto err;\n> \n> -\tparamPool_.createBuffers(stream->configuration().bufferCount + 1);\n> -\tret = param_->exportBuffers(¶mPool_);\n> -\tif (ret) {\n> -\t\tvideo_->releaseBuffers();\n> -\t\treturn ret;\n> -\t}\n> +\tret = stat_->allocateBuffers(RKISP1_BUFFER_COUNT, &statBuffers);\n> +\tif (ret < 0)\n> +\t\tgoto err;\n> \n> -\tstatPool_.createBuffers(stream->configuration().bufferCount + 1);\n> -\tret = stat_->exportBuffers(&statPool_);\n> -\tif (ret) {\n> -\t\tparam_->releaseBuffers();\n> -\t\tvideo_->releaseBuffers();\n> -\t\treturn ret;\n> +\tfor (FrameBuffer *buffer : paramBuffers) {\n> +\t\tbuffer->setCookie(count++);\n> +\t\tdata->ipaBuffers_.push_back({ .id = buffer->cookie(),\n> +\t\t\t\t\t .planes = buffer->planes() });\n\nI think I'm OK with this use of the cookie API, but it should be\ndocumented as being meant for internal purpose, and not for\napplications. It should ideally be unaccessible from applications\ncompletely, but a friend statement won't help. We can possibly address\nthis later, let's add a todo statement in the FrameBuffer class for this\n(but if you already have an idea on how to do so in an easy way, there's\nno need to wait :-)), \n\n> +\t\tparamBuffers_.push(buffer);\n> \t}\n> \n> -\tfor (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {\n> -\t\tstd::vector planes;\n> -\t\tplanes.push_back({\n> -\t\t\t.fd = paramPool_.buffers()[i].planes()[0].fd(),\n> -\t\t\t.length = paramPool_.buffers()[i].planes()[0].length(),\n> -\t\t});\n> -\n> -\t\tdata->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,\n> -\t\t\t\t\t .planes = planes });\n> -\t\tparamBuffers_.push(new Buffer(i));\n> +\tfor (FrameBuffer *buffer : statBuffers) {\n> +\t\tbuffer->setCookie(count++);\n> +\t\tdata->ipaBuffers_.push_back({ .id = buffer->cookie(),\n> +\t\t\t\t\t .planes = buffer->planes() });\n> +\t\tstatBuffers_.push(buffer);\n> \t}\n> \n> -\tfor (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {\n> -\t\tstd::vector planes;\n> -\t\tplanes.push_back({\n> -\t\t\t.fd = statPool_.buffers()[i].planes()[0].fd(),\n> -\t\t\t.length = statPool_.buffers()[i].planes()[0].length(),\n> -\t\t});\n> +\tdata->ipa_->mapBuffers(data->ipaBuffers_);\n> \n> -\t\tdata->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,\n> -\t\t\t\t\t .planes = planes });\n> -\t\tstatBuffers_.push(new Buffer(i));\n> -\t}\n> +\treturn 0;\n> +err:\n> +\tfor (FrameBuffer *buffer : paramBuffers)\n> +\t\tdelete buffer;\n> \n> -\tdata->ipa_->mapBuffers(data->ipaBuffers_);\n> +\tfor (FrameBuffer *buffer : statBuffers)\n> +\t\tdelete buffer;\n\nI really think V4L2VideoDevice::allocateBuffers() needs to take a vector\nof std::unique_ptr :-)\n\n> \n> \treturn ret;\n> }\n> @@ -744,9 +723,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> @@ -834,9 +810,11 @@ int PipelineHandlerRkISP1::queueRequestHardware(Camera *camera,\n> \tif (!info)\n> \t\treturn -ENOENT;\n> \n> +\tunsigned int paramid = info->paramBuffer->cookie();\n> +\n> \tIPAOperationData op;\n> \top.operation = RKISP1_IPA_EVENT_QUEUE_REQUEST;\n> -\top.data = { data->frame_, RKISP1_PARAM_BASE | info->paramBuffer->index() };\n> +\top.data = { data->frame_, paramid };\n\nOne option to avoid the need for the cookie API would be to transition\nIPAInterface to use FrameBuffer * instead of ids. We could then perform\nthe conversion to and from ids in the wrappers and proxy, with the\nmapping being kept there. IPAOperationData would need a new vector of\nFrameBuffer *. This may not be the best solution, and would go on top of\nthis series anyway.\n\n> \top.controls = { request->controls() };\n> \tdata->ipa_->processEvent(op);\n> \n> @@ -996,12 +974,12 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)\n> \tif (!info->paramDequeued)\n> \t\treturn;\n> \n> -\tcompleteRequest(activeCamera_, request);\n> -\n> \tdata->frameInfo_.destroy(info->frame);\n> +\n> +\tcompleteRequest(activeCamera_, request);\n\nWhy is completeRequest() moved after the destruction of the frame info ?\n\n> }\n> \n> -void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> +void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> {\n> \tASSERT(activeCamera_);\n> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> @@ -1016,7 +994,7 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> \ttryCompleteRequest(request);\n> }\n> \n> -void PipelineHandlerRkISP1::paramReady(Buffer *buffer)\n> +void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n> {\n> \tASSERT(activeCamera_);\n> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> @@ -1027,7 +1005,7 @@ void PipelineHandlerRkISP1::paramReady(Buffer *buffer)\n> \ttryCompleteRequest(info->request);\n> }\n> \n> -void PipelineHandlerRkISP1::statReady(Buffer *buffer)\n> +void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> {\n> \tASSERT(activeCamera_);\n> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> @@ -1037,7 +1015,7 @@ void PipelineHandlerRkISP1::statReady(Buffer *buffer)\n> \t\treturn;\n> \n> \tunsigned int frame = info->frame;\n> -\tunsigned int statid = RKISP1_STAT_BASE | info->statBuffer->index();\n> +\tunsigned int statid = info->statBuffer->cookie();\n> \n> \tIPAOperationData op;\n> \top.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 9cc90bf454cb4392..c275b9f0f75429bd 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -42,7 +42,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> @@ -196,23 +196,13 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n> int PipelineHandlerUVC::allocateBuffers(Camera *camera,\n> \t\t\t\t\tconst std::set &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 &streams)\n> {\n> -\tUVCCameraData *data = cameraData(camera);\n> -\treturn data->video_->releaseBuffers();\n> +\treturn 0;\n> }\n> \n> int PipelineHandlerUVC::start(Camera *camera)\n> @@ -266,7 +256,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> int PipelineHandlerUVC::queueRequestHardware(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> @@ -373,7 +363,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 b1222bd21fb629a0..64793788c752c791 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -57,7 +57,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> @@ -264,23 +264,13 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n> int PipelineHandlerVimc::allocateBuffers(Camera *camera,\n> \t\t\t\t\t const std::set &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 &streams)\n> {\n> -\tVimcCameraData *data = cameraData(camera);\n> -\treturn data->video_->releaseBuffers();\n> +\treturn 0;\n> }\n> \n> int PipelineHandlerVimc::start(Camera *camera)\n> @@ -328,7 +318,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> int PipelineHandlerVimc::queueRequestHardware(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> @@ -459,7 +449,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 c9e348b98da7b736..b107e23258dee692 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -398,7 +398,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 7593bf9dfa546401..a4b27e1cb08a7641 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> @@ -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\nThe documentation of this method, as well as Request::buffers(), still\nmentions the Buffer class, it should be updated to mention FrameBuffer.\nFurthermore the documentation needs to be updated to explain the new\nownership rules for buffers, as it currently states\n\n\"Ownership of the buffer is passed to the request. It will be deleted\nwhen the request is destroyed after completing.\"\n\nPlease don't just remove the paragraph but explain the new rules :-)\nCould you please go through the documentation of the Request class to\nmake sure nothing else is outdated ?\n\n> -int Request::addBuffer(Stream *stream, std::unique_ptr buffer)\n> +int Request::addBuffer(Stream *stream, FrameBuffer *buffer)\n> {\n> \tif (!stream) {\n> \t\tLOG(Request, Error) << \"Invalid stream reference\";\n> @@ -139,7 +134,7 @@ int Request::addBuffer(Stream *stream, std::unique_ptr buffer)\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)\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,8 +214,9 @@ 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> +\n\nI don't think a blank line is needed, but that's up to you.\n\n> \t\tpending_.insert(buffer);\n> \t}\n> \n> @@ -253,7 +249,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/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index f5810956b2040ce6..a29ed697468a7f59 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1191,9 +1191,11 @@ int V4L2VideoDevice::externalBuffers(unsigned int count)\n> */\n> int V4L2VideoDevice::releaseBuffers()\n> {\n> -\tLOG(V4L2, Debug) << \"Releasing bufferPool\";\n> +\tLOG(V4L2, Debug) << \"Releasing buffers\";\n> \n> \tbufferPool_ = nullptr;\n> +\tdelete cache_;\n> +\tcache_ = nullptr;\n> \n> \treturn requestBuffers(0);\n> }\n> @@ -1209,27 +1211,26 @@ int V4L2VideoDevice::releaseBuffers()\n\nYou should explain here that the V4L2 buffer is automatically picked\nusing the cache (with slightly more details than this :-)).\n\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n> +int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> {\n> \tstruct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};\n> \tstruct v4l2_buffer buf = {};\n> \tint ret;\n> \n> -\tbuf.index = buffer->index();\n> +\tbuf.index = cache_->fetch(buffer);\n> \tbuf.type = bufferType_;\n> \tbuf.memory = memoryType_;\n> \tbuf.field = V4L2_FIELD_NONE;\n> \n> \tbool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);\n> -\tBufferMemory *mem = &bufferPool_->buffers()[buf.index];\n> -\tconst std::vector &dmabufs = mem->planes();\n> +\tconst std::vector &dmabufs = buffer->dmabufs();\n> \n> \tif (buf.memory == V4L2_MEMORY_DMABUF) {\n> \t\tif (multiPlanar) {\n> \t\t\tfor (unsigned int p = 0; p < dmabufs.size(); ++p)\n> -\t\t\t\tv4l2Planes[p].m.fd = dmabufs[p].fd();\n> +\t\t\t\tv4l2Planes[p].m.fd = dmabufs[p]->fd();\n> \t\t} else {\n> -\t\t\tbuf.m.fd = dmabufs[0].fd();\n> +\t\t\tbuf.m.fd = dmabufs[0]->fd();\n> \t\t}\n> \t}\n> \n> @@ -1275,52 +1276,6 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n> \treturn 0;\n> }\n> \n> -/**\n> - * \\brief Queue all buffers into the video device\n> - *\n> - * When starting video capture users of the video device often need to queue\n> - * all allocated buffers to the device. This helper method simplifies the\n> - * implementation of the user by queuing all buffers and returning a vector of\n> - * Buffer instances for each queued buffer.\n> - *\n> - * This method is meant to be used with video capture devices internal to a\n> - * pipeline handler, such as ISP statistics capture devices, or raw CSI-2\n> - * receivers. For video capture devices facing applications, buffers shall\n> - * instead be queued when requests are received, and for video output devices,\n> - * buffers shall be queued when frames are ready to be output.\n> - *\n> - * The caller shall ensure that the returned buffers vector remains valid until\n> - * all the queued buffers are dequeued, either during capture, or by stopping\n> - * the video device.\n> - *\n> - * Calling this method on an output device or on a device that has buffers\n> - * already queued is an error and will return an empty vector.\n> - *\n> - * \\return A vector of queued buffers, which will be empty if an error occurs\n> - */\n> -std::vector> V4L2VideoDevice::queueAllBuffers()\n> -{\n> -\tint ret;\n> -\n> -\tif (!queuedBuffers_.empty())\n> -\t\treturn {};\n> -\n> -\tif (V4L2_TYPE_IS_OUTPUT(bufferType_))\n> -\t\treturn {};\n> -\n> -\tstd::vector> buffers;\n> -\n> -\tfor (unsigned int i = 0; i < bufferPool_->count(); ++i) {\n> -\t\tBuffer *buffer = new Buffer(i);\n> -\t\tbuffers.emplace_back(buffer);\n> -\t\tret = queueBuffer(buffer);\n> -\t\tif (ret)\n> -\t\t\treturn {};\n> -\t}\n> -\n> -\treturn buffers;\n> -}\n> -\n> /**\n> * \\brief Dequeue the next available buffer from the video device\n> *\n> @@ -1329,7 +1284,7 @@ std::vector> V4L2VideoDevice::queueAllBuffers()\n> *\n> * \\return A pointer to the dequeued buffer on success, or nullptr otherwise\n> */\n> -Buffer *V4L2VideoDevice::dequeueBuffer()\n> +FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> {\n> \tstruct v4l2_buffer buf = {};\n> \tstruct v4l2_plane planes[VIDEO_MAX_PLANES] = {};\n> @@ -1352,15 +1307,15 @@ Buffer *V4L2VideoDevice::dequeueBuffer()\n> \n> \tLOG(V4L2, Debug) << \"Buffer \" << buf.index << \" is available\";\n> \n> +\tcache_->put(buf.index);\n> +\n> \tauto it = queuedBuffers_.find(buf.index);\n> -\tBuffer *buffer = it->second;\n> +\tFrameBuffer *buffer = it->second;\n> \tqueuedBuffers_.erase(it);\n> \n> \tif (queuedBuffers_.empty())\n> \t\tfdEvent_->setEnabled(false);\n> \n> -\tbuffer->index_ = buf.index;\n> -\n> \tBufferInfo::Status status = buf.flags & V4L2_BUF_FLAG_ERROR\n> \t\t? BufferInfo::BufferError : BufferInfo::BufferSuccess;\n> \tuint64_t timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> @@ -1383,7 +1338,7 @@ Buffer *V4L2VideoDevice::dequeueBuffer()\n> */\n\nThe documentation of this function also mentions Buffer.\n\n> void V4L2VideoDevice::bufferAvailable(EventNotifier *notifier)\n> {\n> -\tBuffer *buffer = dequeueBuffer();\n> +\tFrameBuffer *buffer = dequeueBuffer();\n> \tif (!buffer)\n> \t\treturn;\n> \n> @@ -1418,7 +1373,7 @@ int V4L2VideoDevice::streamOn()\n> * \\brief Stop the video stream\n> *\n> * Buffers that are still queued when the video stream is stopped are\n> - * immediately dequeued with their status set to Buffer::BufferError,\n> + * immediately dequeued with their status set to BufferInfo::BufferCancelled,\n> * and the bufferReady signal is emitted for them. The order in which those\n> * buffers are dequeued is not specified.\n> *\n> @@ -1437,11 +1392,10 @@ int V4L2VideoDevice::streamOff()\n> \n> \t/* Send back all queued buffers. */\n> \tfor (auto it : queuedBuffers_) {\n> -\t\tunsigned int index = it.first;\n> -\t\tBuffer *buffer = it.second;\n> +\t\tFrameBuffer *buffer = it.second;\n> +\n> +\t\tbuffer->info_.update(BufferInfo::BufferCancelled, 0, 0, { {} });\n> \n> -\t\tbuffer->index_ = index;\n> -\t\tbuffer->cancel();\n> \t\tbufferReady.emit(buffer);\n> \t}\n> \n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 771020112f09b1ef..9650bd970f89aa41 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -22,7 +22,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> @@ -36,8 +36,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_ = new BufferAllocator(camera_);\n> \t\tret = startCapture();\n> +\t}\n> \n> \tif (ret < 0)\n> \t\tQTimer::singleShot(0, QCoreApplication::instance(),\n> @@ -51,6 +53,8 @@ MainWindow::~MainWindow()\n> \t\tcamera_->release();\n> \t\tcamera_.reset();\n> \t}\n> +\n> +\tdelete allocator_;\n> }\n> \n> void MainWindow::updateTitle()\n> @@ -175,8 +179,14 @@ int MainWindow::startCapture()\n> \t\treturn ret;\n> \t}\n> \n> +\tret = allocator_->allocate(stream, cfg);\n> +\tif (ret < 0) {\n> +\t\tstd::cerr << \"Failed to allocate internal buffers\" << std::endl;\n\nMaybe s/internal buffers/capture buffers/ ? From the point of view of a\nuser of que qcam application, \"internal buffers\" could be confusing.\n\n> +\t\treturn ret;\n> +\t}\n> +\n> \tstd::vector requests;\n> -\tfor (unsigned int i = 0; i < cfg.bufferCount; ++i) {\n> +\tfor (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> @@ -184,13 +194,7 @@ int MainWindow::startCapture()\n> \t\t\tgoto error;\n> \t\t}\n> \n> -\t\tstd::unique_ptr 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);\n> \t\tif (ret < 0) {\n> \t\t\tstd::cerr << \"Can't set buffer for request\" << std::endl;\n> \t\t\tgoto error;\n> @@ -253,11 +257,11 @@ void MainWindow::requestComplete(Request *request)\n> \tif (request->status() == Request::RequestCancelled)\n> \t\treturn;\n> \n> -\tconst std::map &buffers = request->buffers();\n> +\tconst std::map &buffers = request->buffers();\n> \n> \tframesCaptured_++;\n> \n> -\tBuffer *buffer = buffers.begin()->second;\n> +\tFrameBuffer *buffer = buffers.begin()->second;\n> \tconst BufferInfo &info = buffer->info();\n> \n> \tdouble fps = info.timestamp() - lastBufferTime_;\n> @@ -280,29 +284,20 @@ 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 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> -\tBufferMemory *mem = buffer->mem();\n> -\tif (mem->planes().size() != 1)\n> +\tif (buffer->planes().size() != 1)\n> \t\treturn -EINVAL;\n> \n> -\tDmabuf &dmabuf = mem->planes().front();\n> -\tunsigned char *raw = static_cast(dmabuf.mem());\n> +\tunsigned char *raw = static_cast(buffer->dmabufs()[0]->mem());\n> \tviewfinder_->display(raw, buffer->info().planes()[0].bytesused);\n> \n> \treturn 0;\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index 0786e915ec512255..b38aa2ac959f44aa 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -14,6 +14,8 @@\n> #include \n> #include \n> \n> +#include \n> +#include \n> #include \n> #include \n> #include \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_;\n> +\tBufferAllocator *allocator_;\n> +\n> \tbool isCapturing_;\n> \tstd::unique_ptr config_;\n> \n> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> index 5dbaed9255d3d60c..c98afeb9597e21c2 100644\n> --- a/test/camera/buffer_import.cpp\n> +++ b/test/camera/buffer_import.cpp\n> @@ -21,35 +21,48 @@\n> #include \"test.h\"\n> \n> using namespace libcamera;\n> +using namespace std;\n\nThis patch is big enough, let's not include this change. I'm actually\nthinking that tests should move away from using namespace std.\n\n> \n> -/* Keep SINK_BUFFER_COUNT > CAMERA_BUFFER_COUNT + 1 */\n> -static constexpr unsigned int SINK_BUFFER_COUNT = 8;\n> -static constexpr unsigned int CAMERA_BUFFER_COUNT = 4;\n> +namespace {\n\nWhy the anonymous namespace here ?\n\n> \n> -class FrameSink\n\nLet's add a small comment.\n\n/* A provider of external buffers, suitable for import by a Camera. */\n\n> +class BufferSource\n> {\n> public:\n> -\tFrameSink()\n> +\tBufferSource()\n> \t\t: video_(nullptr)\n> \t{\n> \t}\n> \n> -\tint init()\n> +\t~BufferSource()\n> +\t{\n> +\t\tif (video_) {\n> +\t\t\tvideo_->releaseBuffers();\n> +\t\t\tvideo_->close();\n> +\t\t}\n> +\n> +\t\tdelete video_;\n> +\t\tvideo_ = nullptr;\n> +\n> +\t\tif (media_)\n> +\t\t\tmedia_->release();\n> +\t}\n> +\n> +\tint allocate(const StreamConfiguration &config)\n> \t{\n> \t\tint ret;\n> \n> \t\t/* Locate and open the video device. */\n> -\t\tstd::string videoDeviceName = \"vivid-000-vid-out\";\n> +\t\tstring videoDeviceName = \"vivid-000-vid-out\";\n> \n> -\t\tstd::unique_ptr enumerator =\n> +\t\tunique_ptr enumerator =\n> \t\t\tDeviceEnumerator::create();\n> \t\tif (!enumerator) {\n> -\t\t\tstd::cout << \"Failed to create device enumerator\" << std::endl;\n> +\t\t\tcout << \"Failed to create device enumerator\" << std::endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> \t\tif (enumerator->enumerate()) {\n> -\t\t\tstd::cout << \"Failed to enumerate media devices\" << std::endl;\n> +\t\t\tcout << \"Failed to enumerate media devices\" << std::endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> @@ -58,13 +71,13 @@ public:\n> \n> \t\tmedia_ = enumerator->search(dm);\n> \t\tif (!media_) {\n> -\t\t\tstd::cout << \"No vivid output device available\" << std::endl;\n> +\t\t\tcout << \"No vivid output device available\" << std::endl;\n> \t\t\treturn TestSkip;\n> \t\t}\n> \n> \t\tvideo_ = V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName);\n> \t\tif (!video_) {\n> -\t\t\tstd::cout << \"Unable to open \" << videoDeviceName << std::endl;\n> +\t\t\tcout << \"Unable to open \" << videoDeviceName << std::endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> @@ -72,366 +85,178 @@ public:\n> \t\t\treturn TestFail;\n> \n> \t\t/* Configure the format. */\n> -\t\tret = video_->getFormat(&format_);\n> +\t\tV4L2DeviceFormat format;\n> +\t\tret = video_->getFormat(&format);\n> \t\tif (ret) {\n> -\t\t\tstd::cout << \"Failed to get format on output device\" << std::endl;\n> +\t\t\tcout << \"Failed to get format on output device\" << std::endl;\n> \t\t\treturn ret;\n> \t\t}\n> \n> -\t\tformat_.size.width = 1920;\n> -\t\tformat_.size.height = 1080;\n> -\t\tformat_.fourcc = V4L2_PIX_FMT_RGB24;\n> -\t\tformat_.planesCount = 1;\n> -\t\tformat_.planes[0].size = 1920 * 1080 * 3;\n> -\t\tformat_.planes[0].bpl = 1920 * 3;\n> -\n> -\t\tif (video_->setFormat(&format_)) {\n> -\t\t\tcleanup();\n> +\t\tformat.size = config.size;\n> +\t\tformat.fourcc = V4L2VideoDevice::toV4L2Fourcc(config.pixelFormat, false);\n\nIsn't it a problem not to set planesCount, planes.size and planes.bpl ?\nI would at least set planesCount to 1 and initialize the other two\nfields to 0.\n\n> +\t\tif (video_->setFormat(&format))\n> \t\t\treturn TestFail;\n> -\t\t}\n> \n> -\t\t/* Export the buffers to a pool. */\n> -\t\tpool_.createBuffers(SINK_BUFFER_COUNT);\n> -\t\tret = video_->exportBuffers(&pool_);\n> -\t\tif (ret) {\n> -\t\t\tstd::cout << \"Failed to export buffers\" << std::endl;\n> -\t\t\tcleanup();\n> -\t\t\treturn TestFail;\n> -\t\t}\n> -\n> -\t\t/* Only use the first CAMERA_BUFFER_COUNT buffers to start with. */\n> -\t\tavailableBuffers_.resize(CAMERA_BUFFER_COUNT);\n> -\t\tstd::iota(availableBuffers_.begin(), availableBuffers_.end(), 0);\n> -\n> -\t\t/* Connect the buffer ready signal. */\n> -\t\tvideo_->bufferReady.connect(this, &FrameSink::bufferComplete);\n> -\n> -\t\treturn TestPass;\n> +\t\treturn video_->allocateBuffers(config.bufferCount, &buffers_);\n> \t}\n> \n> -\tvoid cleanup()\n> -\t{\n> -\t\tif (video_) {\n> -\t\t\tvideo_->streamOff();\n> -\t\t\tvideo_->releaseBuffers();\n> -\t\t\tvideo_->close();\n> -\n> -\t\t\tdelete video_;\n> -\t\t\tvideo_ = nullptr;\n> -\t\t}\n> -\n> -\t\tif (media_)\n> -\t\t\tmedia_->release();\n> -\t}\n> -\n> -\tint start()\n> -\t{\n> -\t\trequestsCount_ = 0;\n> -\t\tdone_ = false;\n> -\n> -\t\tint ret = video_->streamOn();\n> -\t\tif (ret < 0)\n> -\t\t\treturn ret;\n> -\n> -\t\t/* Queue all the initial requests. */\n> -\t\tfor (unsigned int index = 0; index < CAMERA_BUFFER_COUNT; ++index)\n> -\t\t\tqueueRequest(index);\n> -\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tint stop()\n> -\t{\n> -\t\treturn video_->streamOff();\n> -\t}\n> -\n> -\tvoid requestComplete(uint64_t cookie, const Buffer *metadata)\n> -\t{\n> -\t\tunsigned int index = cookie;\n> -\n> -\t\tBuffer *buffer = new Buffer(index, metadata);\n> -\t\tint ret = video_->queueBuffer(buffer);\n> -\t\tif (ret < 0)\n> -\t\t\tstd::cout << \"Failed to queue buffer to sink\" << std::endl;\n> -\t}\n> -\n> -\tbool done() const { return done_; }\n> -\n> -\tPixelFormat format() const\n> -\t{\n> -\t\treturn video_->toPixelFormat(format_.fourcc);\n> -\t}\n> -\n> -\tconst Size &size() const\n> -\t{\n> -\t\treturn format_.size;\n> -\t}\n> -\n> -\tSignal requestReady;\n> +\tconst vector &buffers() { return buffers_; };\n> \n> private:\n> -\tvoid queueRequest(unsigned int index)\n> -\t{\n> -\t\tauto it = std::find(availableBuffers_.begin(),\n> -\t\t\t\t availableBuffers_.end(), index);\n> -\t\tavailableBuffers_.erase(it);\n> -\n> -\t\tuint64_t cookie = index;\n> -\t\tBufferMemory &mem = pool_.buffers()[index];\n> -\t\tint dmabuf = mem.planes()[0].fd();\n> -\n> -\t\trequestReady.emit(cookie, dmabuf);\n> -\n> -\t\trequestsCount_++;\n> -\t}\n> -\n> -\tvoid bufferComplete(Buffer *buffer)\n> -\t{\n> -\t\tavailableBuffers_.push_back(buffer->index());\n> -\n> -\t\t/*\n> -\t\t * Pick the buffer for the next request among the available\n> -\t\t * buffers.\n> -\t\t *\n> -\t\t * For the first 20 frames, select the buffer that has just\n> -\t\t * completed to keep the mapping of dmabuf fds to buffers\n> -\t\t * unchanged in the camera.\n> -\t\t *\n> -\t\t * For the next 20 frames, cycle randomly over the available\n> -\t\t * buffers. The mapping should still be kept unchanged, as the\n> -\t\t * camera should map using the cached fds.\n> -\t\t *\n> -\t\t * For the last 20 frames, cycles through all buffers, which\n> -\t\t * should trash the mappings.\n> -\t\t */\n> -\t\tunsigned int index = buffer->index();\n> -\t\tdelete buffer;\n> -\n> -\t\tstd::cout << \"Completed buffer, request=\" << requestsCount_\n> -\t\t\t << \", available buffers=\" << availableBuffers_.size()\n> -\t\t\t << std::endl;\n> -\n> -\t\tif (requestsCount_ >= 60) {\n> -\t\t\tif (availableBuffers_.size() == SINK_BUFFER_COUNT)\n> -\t\t\t\tdone_ = true;\n> -\t\t\treturn;\n> -\t\t}\n> -\n> -\t\tif (requestsCount_ == 40) {\n> -\t\t\t/* Add the remaining of the buffers. */\n> -\t\t\tfor (unsigned int i = CAMERA_BUFFER_COUNT;\n> -\t\t\t i < SINK_BUFFER_COUNT; ++i)\n> -\t\t\t\tavailableBuffers_.push_back(i);\n> -\t\t}\n> -\n> -\t\tif (requestsCount_ >= 20) {\n> -\t\t\t/*\n> -\t\t\t * Wait until we have enough buffers to make this\n> -\t\t\t * meaningful. Preferably half of the camera buffers,\n> -\t\t\t * but no less than 2 in any case.\n> -\t\t\t */\n> -\t\t\tconst unsigned int min_pool_size =\n> -\t\t\t\tstd::min(CAMERA_BUFFER_COUNT / 2, 2U);\n> -\t\t\tif (availableBuffers_.size() < min_pool_size)\n> -\t\t\t\treturn;\n> -\n> -\t\t\t/* Pick a buffer at random. */\n> -\t\t\tunsigned int pos = random_() % availableBuffers_.size();\n> -\t\t\tindex = availableBuffers_[pos];\n> -\t\t}\n> -\n> -\t\tqueueRequest(index);\n> -\t}\n\nIf I understand this properly, the BufferSource class is now only used\nas a source of buffers, and you don't cycle buffers back and forth\nbetween the camera and the exporter. We're thus losing the ability to\ntrash the V4L2 buffer cache, which I think we should test. Isn't there a\nway to keep this ?\n\n> -\n> -\tstd::shared_ptr media_;\n> +\tshared_ptr media_;\n> \tV4L2VideoDevice *video_;\n> -\tBufferPool pool_;\n> -\tV4L2DeviceFormat format_;\n> -\n> -\tunsigned int requestsCount_;\n> -\tstd::vector availableBuffers_;\n> -\tstd::random_device random_;\n> -\n> -\tbool done_;\n> +\tvector buffers_;\n> };\n> \n> -class BufferImportTest : public CameraTest, public Test\n> +class BufferImport : public CameraTest, public Test\n\nTest classes should be named with a Test suffix. The other camera tests\ndon't follow the rule, they should be fixed.\n\n> {\n> public:\n> -\tBufferImportTest()\n> +\tBufferImport()\n> \t\t: CameraTest(\"VIMC Sensor B\")\n> \t{\n> \t}\n> \n> -\tvoid queueRequest(uint64_t cookie, int dmabuf)\n> +protected:\n> +\tunsigned int completeBuffersCount_;\n> +\tunsigned int completeRequestsCount_;\n\nHow about moving private members (variables and methods) to the end in a\nprivate section ? Variables should also go after methods.\n\n> +\n> +\tvoid bufferComplete(Request *request, FrameBuffer *buffer)\n> \t{\n> -\t\tRequest *request = camera_->createRequest(cookie);\n> +\t\tif (buffer->info().status() != BufferInfo::BufferSuccess)\n> +\t\t\treturn;\n> \n> -\t\tstd::unique_ptr buffer = stream_->createBuffer({ dmabuf, -1, -1 });\n> -\t\trequest->addBuffer(stream_, move(buffer));\n> -\t\tcamera_->queueRequest(request);\n> +\t\tcompleteBuffersCount_++;\n> \t}\n> \n> -protected:\n> -\tvoid bufferComplete(Request *request, Buffer *buffer)\n> +\tvoid requestComplete(Request *request)\n> \t{\n> -\t\tif (buffer->info().status() != BufferInfo::BufferSuccess)\n> +\t\tif (request->status() != Request::RequestComplete)\n> \t\t\treturn;\n> \n> -\t\tunsigned int index = buffer->index();\n> -\t\tint dmabuf = buffer->dmabufs()[0];\n> +\t\tconst map &buffers = request->buffers();\n> \n> -\t\t/* Record dmabuf to index remappings. */\n> -\t\tbool remapped = false;\n> -\t\tif (bufferMappings_.find(index) != bufferMappings_.end()) {\n> -\t\t\tif (bufferMappings_[index] != dmabuf)\n> -\t\t\t\tremapped = true;\n> -\t\t}\n> +\t\tcompleteRequestsCount_++;\n> \n> -\t\tstd::cout << \"Completed request \" << framesCaptured_\n> -\t\t\t << \": dmabuf fd \" << dmabuf\n> -\t\t\t << \" -> index \" << index\n> -\t\t\t << \" (\" << (remapped ? 'R' : '-') << \")\"\n> -\t\t\t << std::endl;\n> +\t\t/* Create a new request. */\n> +\t\tStream *stream = buffers.begin()->first;\n> +\t\tFrameBuffer *buffer = buffers.begin()->second;\n> \n> -\t\tif (remapped)\n> -\t\t\tbufferRemappings_.push_back(framesCaptured_);\n> +\t\trequest = camera_->createRequest();\n> +\t\trequest->addBuffer(stream, buffer);\n> +\t\tcamera_->queueRequest(request);\n> +\t}\n> \n> -\t\tbufferMappings_[index] = dmabuf;\n> -\t\tframesCaptured_++;\n> +\tint init() override\n> +\t{\n> +\t\tif (status_ != TestPass)\n> +\t\t\treturn status_;\n> \n> -\t\tsink_.requestComplete(request->cookie(), buffer);\n> +\t\tconfig_ = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> +\t\tif (!config_ || config_->size() != 1) {\n> +\t\t\tcout << \"Failed to generate default configuration\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> \n> -\t\tif (framesCaptured_ == 60)\n> -\t\t\tsink_.stop();\n> +\t\treturn TestPass;\n> \t}\n> \n> -\tint initCamera()\n> +\tint run() override\n> \t{\n> -\t\tif (camera_->acquire()) {\n> -\t\t\tstd::cout << \"Failed to acquire the camera\" << std::endl;\n> -\t\t\treturn TestFail;\n> -\t\t}\n> +\t\tStreamConfiguration &cfg = config_->at(0);\n> \n> -\t\t/*\n> -\t\t * Configure the Stream to work with externally allocated\n> -\t\t * buffers by setting the memoryType to ExternalMemory.\n> -\t\t */\n\nThis is hard to review as it's a rewrite of the test, and removing all\ncomments without adding new ones don't help. Please add comments where\nappropriate, and explain how the buffer import test is modified in the\ncommit message. Ideally this should have been split to a separate\ncommit, but I understand it will break bisection (although I'm not sure\nany issue could be meaningfully bisected to the middle of this series\n:-)).\n\n> -\t\tstd::unique_ptr config;\n> -\t\tconfig = camera_->generateConfiguration({ StreamRole::VideoRecording });\n> -\t\tif (!config || config->size() != 1) {\n> -\t\t\tstd::cout << \"Failed to generate configuration\" << std::endl;\n> +\t\tif (camera_->acquire()) {\n> +\t\t\tcout << \"Failed to acquire the camera\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tStreamConfiguration &cfg = config->at(0);\n> -\t\tcfg.size = sink_.size();\n> -\t\tcfg.pixelFormat = sink_.format();\n> -\t\tcfg.bufferCount = CAMERA_BUFFER_COUNT;\n> -\t\tcfg.memoryType = ExternalMemory;\n> -\n> -\t\tif (camera_->configure(config.get())) {\n> -\t\t\tstd::cout << \"Failed to set configuration\" << std::endl;\n> +\t\tif (camera_->configure(config_.get())) {\n> +\t\t\tcout << \"Failed to set default configuration\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tstream_ = cfg.stream();\n> -\n> -\t\t/* Allocate buffers. */\n> \t\tif (camera_->allocateBuffers()) {\n> -\t\t\tstd::cout << \"Failed to allocate buffers\" << std::endl;\n> +\t\t\tcout << \"Failed to allocate buffers\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\t/* Connect the buffer completed signal. */\n> -\t\tcamera_->bufferCompleted.connect(this, &BufferImportTest::bufferComplete);\n> +\t\tStream *stream = cfg.stream();\n> \n> -\t\treturn TestPass;\n> -\t}\n> +\t\tBufferSource source;\n> +\t\tint ret = source.allocate(cfg);\n> +\t\tif (ret < 0)\n> +\t\t\treturn TestFail;\n> \n> -\tint init()\n> -\t{\n> -\t\tif (status_ != TestPass)\n> -\t\t\treturn status_;\n> +\t\tvector requests;\n> +\t\tfor (FrameBuffer *buffer : source.buffers()) {\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\tint ret = sink_.init();\n> -\t\tif (ret != TestPass) {\n> -\t\t\tcleanup();\n> -\t\t\treturn ret;\n> -\t\t}\n> +\t\t\tif (request->addBuffer(stream, buffer)) {\n> +\t\t\t\tcout << \"Failed to associating buffer with request\" << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> \n> -\t\tret = initCamera();\n> -\t\tif (ret != TestPass) {\n> -\t\t\tcleanup();\n> -\t\t\treturn ret;\n> +\t\t\trequests.push_back(request);\n> \t\t}\n> \n> -\t\tsink_.requestReady.connect(this, &BufferImportTest::queueRequest);\n> -\t\treturn TestPass;\n> -\t}\n> -\n> -\tint run()\n> -\t{\n> -\t\tint ret;\n> +\t\tcompleteRequestsCount_ = 0;\n> +\t\tcompleteBuffersCount_ = 0;\n> \n> -\t\tframesCaptured_ = 0;\n> +\t\tcamera_->bufferCompleted.connect(this, &BufferImport::bufferComplete);\n> +\t\tcamera_->requestCompleted.connect(this, &BufferImport::requestComplete);\n> \n> \t\tif (camera_->start()) {\n> -\t\t\tstd::cout << \"Failed to start camera\" << std::endl;\n> +\t\t\tcout << \"Failed to start camera\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tret = sink_.start();\n> -\t\tif (ret < 0) {\n> -\t\t\tstd::cout << \"Failed to start sink\" << std::endl;\n> -\t\t\treturn TestFail;\n> +\t\tfor (Request *request : requests) {\n> +\t\t\tif (camera_->queueRequest(request)) {\n> +\t\t\t\tcout << \"Failed to queue request\" << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> \t\t}\n> \n> \t\tEventDispatcher *dispatcher = cm_->eventDispatcher();\n> \n> \t\tTimer timer;\n> -\t\ttimer.start(5000);\n> -\t\twhile (timer.isRunning() && !sink_.done())\n> +\t\ttimer.start(1000);\n> +\t\twhile (timer.isRunning())\n> \t\t\tdispatcher->processEvents();\n> \n> -\t\tstd::cout << framesCaptured_ << \" frames captured, \"\n> -\t\t\t << bufferRemappings_.size() << \" buffers remapped\"\n> -\t\t\t << std::endl;\n> +\t\tunsigned int nbuffers = source.buffers().size();\n> \n> -\t\tif (framesCaptured_ < 60) {\n> -\t\t\tstd::cout << \"Too few frames captured\" << std::endl;\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 << nbuffers * 2 << \")\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tif (bufferRemappings_.empty()) {\n> -\t\t\tstd::cout << \"No buffer remappings\" << std::endl;\n> +\t\tif (completeRequestsCount_ != completeBuffersCount_) {\n> +\t\t\tcout << \"Number of completed buffers and requests differ\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tif (bufferRemappings_[0] < 40) {\n> -\t\t\tstd::cout << \"Early buffer remapping\" << std::endl;\n> +\t\tif (camera_->stop()) {\n> +\t\t\tcout << \"Failed to stop camera\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\treturn TestPass;\n> -\t}\n> -\n> -\tvoid cleanup()\n> -\t{\n> -\t\tsink_.cleanup();\n> +\t\tif (camera_->freeBuffers()) {\n> +\t\t\tcout << \"Failed to free buffers\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> \n> -\t\tcamera_->stop();\n> -\t\tcamera_->freeBuffers();\n> +\t\treturn TestPass;\n> \t}\n> \n> -private:\n> -\tStream *stream_;\n> -\n> -\tstd::map bufferMappings_;\n> -\tstd::vector bufferRemappings_;\n> -\tunsigned int framesCaptured_;\n> -\n> -\tFrameSink sink_;\n\nShouldn't this be private ?\n\n> +\tunique_ptr config_;\n> };\n> \n> -TEST_REGISTER(BufferImportTest);\n> +} /* namespace */\n> +\n> +TEST_REGISTER(BufferImport);\n> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> index 8307ea2629801679..ee6d0a1bc1fa839d 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->info().status() != BufferInfo::BufferSuccess)\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 &buffers = request->buffers();\n> +\t\tconst std::map &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 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> @@ -87,21 +86,21 @@ protected:\n> \t\t}\n> \n> \t\tStream *stream = cfg.stream();\n> +\n> +\t\tBufferAllocator allocator(camera_);\n> +\t\tint ret = allocator.allocate(stream, cfg);\n> +\t\tif (ret < 0)\n> +\t\t\treturn TestFail;\n> +\n> \t\tstd::vector requests;\n> -\t\tfor (unsigned int i = 0; i < cfg.bufferCount; ++i) {\n> +\t\tfor (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 = 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)) {\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 +133,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\nThis is an interesting change, and ideally it shouldn't be needed.\nShouldn't the allocator guarantee that it will allocate the number of\nbuffers specified in the stream configuration ? Otherwise we'll have two\nsources for the same piece of information, without a clear rule to tell\nwhich one to trust. This is a potential issue in the API and needs to be\naddressed (at least recorded in a \\todo item, with some idea on how to\nfix it, to make sure it won't require major changes in the API).\n\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> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> index f627b8f37422350e..e9468d806f977a63 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_ = new BufferAllocator(camera_);\n> +\t\tStream *stream = *camera_->streams().begin();\n> +\t\tif (allocator_->allocate(stream, defconf_->at(0)) < 0)\n> +\t\t\treturn TestFail;\n\nYou're leaking allocator_, and allocating it in testConfigured() and\ndeleting it in testRuning() is a hack. Let's allocate it in init() and\ndelete it in cleanup(). Please run this test in valgrind to ensure there\nis no memory leak or use after free.\n\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 = stream->createBuffer(0);\n> -\t\tif (request->addBuffer(stream, std::move(buffer)))\n> +\t\tif (request->addBuffer(stream, allocator_->buffers(stream)[0]))\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 defconf_;\n> +\tBufferAllocator *allocator_;\n> };\n> \n> } /* namespace */\n> diff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp\n> index 6b71caef111693d6..1b6ff756ac53a96a 100644\n> --- a/test/v4l2_videodevice/buffer_sharing.cpp\n> +++ b/test/v4l2_videodevice/buffer_sharing.cpp\n> @@ -73,16 +73,14 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tpool_.createBuffers(bufferCount);\n> -\n> -\t\tret = capture_->exportBuffers(&pool_);\n> -\t\tif (ret) {\n> -\t\t\tstd::cout << \"Failed to export buffers\" << std::endl;\n> +\t\tret = capture_->allocateBuffers(bufferCount, &buffers_);\n> +\t\tif (ret < 0) {\n> +\t\t\tstd::cout << \"Failed to allocate buffers\" << std::endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tret = output_->importBuffers(&pool_);\n> -\t\tif (ret) {\n> +\t\tret = output_->externalBuffers(bufferCount);\n> +\t\tif (ret < 0) {\n> \t\t\tstd::cout << \"Failed to import buffers\" << std::endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -90,7 +88,7 @@ protected:\n> \t\treturn 0;\n> \t}\n> \n> -\tvoid captureBufferReady(Buffer *buffer)\n> +\tvoid captureBufferReady(FrameBuffer *buffer)\n> \t{\n> \t\tconst BufferInfo &info = buffer->info();\n> \n> @@ -101,10 +99,11 @@ protected:\n> \t\t\treturn;\n> \n> \t\toutput_->queueBuffer(buffer);\n> +\n\nNot necessarily needed.\n\n> \t\tframesCaptured_++;\n> \t}\n> \n> -\tvoid outputBufferReady(Buffer *buffer)\n> +\tvoid outputBufferReady(FrameBuffer *buffer)\n> \t{\n> \t\tconst BufferInfo &info = buffer->info();\n> \n> @@ -127,10 +126,8 @@ protected:\n> \t\tcapture_->bufferReady.connect(this, &BufferSharingTest::captureBufferReady);\n> \t\toutput_->bufferReady.connect(this, &BufferSharingTest::outputBufferReady);\n> \n> -\t\tstd::vector> buffers;\n> -\t\tbuffers = capture_->queueAllBuffers();\n> -\t\tif (buffers.empty())\n> -\t\t\treturn TestFail;\n> +\t\tfor (FrameBuffer *buffer : buffers_)\n> +\t\t\tcapture_->queueBuffer(buffer);\n\nShouldn't you check the return value of queueBuffer() ? Same in the\nother tests below. You could add a queueAllBuffers() helper method to\nV4L2VideoDeviceTest() to wrap the loop and perform the error checking, I\nthink it would help.\n\n> \n> \t\tret = capture_->streamOn();\n> \t\tif (ret) {\n> diff --git a/test/v4l2_videodevice/capture_async.cpp b/test/v4l2_videodevice/capture_async.cpp\n> index 0bc0067c50909c9d..78ac34c4799e9e5a 100644\n> --- a/test/v4l2_videodevice/capture_async.cpp\n> +++ b/test/v4l2_videodevice/capture_async.cpp\n> @@ -20,7 +20,7 @@ public:\n> \tCaptureAsyncTest()\n> \t\t: V4L2VideoDeviceTest(\"vimc\", \"Raw Capture 0\"), frames(0) {}\n> \n> -\tvoid receiveBuffer(Buffer *buffer)\n> +\tvoid receiveBuffer(FrameBuffer *buffer)\n> \t{\n> \t\tstd::cout << \"Received buffer\" << std::endl;\n> \t\tframes++;\n> @@ -38,18 +38,14 @@ protected:\n> \t\tTimer timeout;\n> \t\tint ret;\n> \n> -\t\tpool_.createBuffers(bufferCount);\n> -\n> -\t\tret = capture_->exportBuffers(&pool_);\n> -\t\tif (ret)\n> +\t\tret = capture_->allocateBuffers(bufferCount, &buffers_);\n> +\t\tif (ret < 0)\n> \t\t\treturn TestFail;\n> \n> \t\tcapture_->bufferReady.connect(this, &CaptureAsyncTest::receiveBuffer);\n> \n> -\t\tstd::vector> buffers;\n> -\t\tbuffers = capture_->queueAllBuffers();\n> -\t\tif (buffers.empty())\n> -\t\t\treturn TestFail;\n> +\t\tfor (FrameBuffer *buffer : buffers_)\n> +\t\t\tcapture_->queueBuffer(buffer);\n> \n> \t\tret = capture_->streamOn();\n> \t\tif (ret)\n> diff --git a/test/v4l2_videodevice/request_buffers.cpp b/test/v4l2_videodevice/request_buffers.cpp\n> index c4aedf7b3cd61e80..2f8dfe1cafb111df 100644\n> --- a/test/v4l2_videodevice/request_buffers.cpp\n> +++ b/test/v4l2_videodevice/request_buffers.cpp\n> @@ -16,17 +16,10 @@ public:\n> protected:\n> \tint run()\n> \t{\n> -\t\t/*\n> -\t\t * TODO:\n> -\t\t * Test invalid requests\n> -\t\t * Test different buffer allocations\n> -\t\t */\n\nAre we doing this now ?\n\n> \t\tconst unsigned int bufferCount = 8;\n> \n> -\t\tpool_.createBuffers(bufferCount);\n> -\n> -\t\tint ret = capture_->exportBuffers(&pool_);\n> -\t\tif (ret)\n> +\t\tint ret = capture_->allocateBuffers(bufferCount, &buffers_);\n> +\t\tif (ret != bufferCount)\n> \t\t\treturn TestFail;\n> \n> \t\treturn TestPass;\n> diff --git a/test/v4l2_videodevice/stream_on_off.cpp b/test/v4l2_videodevice/stream_on_off.cpp\n> index 7664adc4c1f07046..ce48310aa2b7c3a8 100644\n> --- a/test/v4l2_videodevice/stream_on_off.cpp\n> +++ b/test/v4l2_videodevice/stream_on_off.cpp\n> @@ -17,10 +17,8 @@ protected:\n> \t{\n> \t\tconst unsigned int bufferCount = 8;\n> \n> -\t\tpool_.createBuffers(bufferCount);\n> -\n> -\t\tint ret = capture_->exportBuffers(&pool_);\n> -\t\tif (ret)\n> +\t\tint ret = capture_->allocateBuffers(bufferCount, &buffers_);\n> +\t\tif (ret < 0)\n> \t\t\treturn TestFail;\n> \n> \t\tret = capture_->streamOn();\n> diff --git a/test/v4l2_videodevice/v4l2_m2mdevice.cpp b/test/v4l2_videodevice/v4l2_m2mdevice.cpp\n> index 442bcac5dc49cc59..2dd4b9440b4d4d71 100644\n> --- a/test/v4l2_videodevice/v4l2_m2mdevice.cpp\n> +++ b/test/v4l2_videodevice/v4l2_m2mdevice.cpp\n> @@ -29,7 +29,7 @@ public:\n> \t{\n> \t}\n> \n> -\tvoid outputBufferComplete(Buffer *buffer)\n> +\tvoid outputBufferComplete(FrameBuffer *buffer)\n> \t{\n> \t\tcout << \"Received output buffer\" << endl;\n> \n> @@ -39,7 +39,7 @@ public:\n> \t\tvim2m_->output()->queueBuffer(buffer);\n> \t}\n> \n> -\tvoid receiveCaptureBuffer(Buffer *buffer)\n> +\tvoid receiveCaptureBuffer(FrameBuffer *buffer)\n> \t{\n> \t\tcout << \"Received capture buffer\" << endl;\n> \n> @@ -112,17 +112,14 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tcapturePool_.createBuffers(bufferCount);\n> -\t\toutputPool_.createBuffers(bufferCount);\n> -\n> -\t\tret = capture->exportBuffers(&capturePool_);\n> -\t\tif (ret) {\n> +\t\tret = capture->allocateBuffers(bufferCount, &captureBuffers_);\n> +\t\tif (ret < 0) {\n> \t\t\tcerr << \"Failed to export Capture Buffers\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> -\t\tret = output->exportBuffers(&outputPool_);\n> -\t\tif (ret) {\n> +\t\tret = output->allocateBuffers(bufferCount, &outputBuffers_);\n> +\t\tif (ret < 0) {\n> \t\t\tcerr << \"Failed to export Output Buffers\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -130,24 +127,11 @@ protected:\n> \t\tcapture->bufferReady.connect(this, &V4L2M2MDeviceTest::receiveCaptureBuffer);\n> \t\toutput->bufferReady.connect(this, &V4L2M2MDeviceTest::outputBufferComplete);\n> \n> -\t\tstd::vector> captureBuffers;\n> -\t\tcaptureBuffers = capture->queueAllBuffers();\n> -\t\tif (captureBuffers.empty()) {\n> -\t\t\tcerr << \"Failed to queue all Capture Buffers\" << endl;\n> -\t\t\treturn TestFail;\n> -\t\t}\n> +\t\tfor (FrameBuffer *buffer : captureBuffers_)\n> +\t\t\tcapture->queueBuffer(buffer);\n\nYou need to check for errors here too, as well as below.\n\n> \n> -\t\t/* We can't \"queueAllBuffers()\" on an output device, so we do it manually */\n> -\t\tstd::vector> outputBuffers;\n> -\t\tfor (unsigned int i = 0; i < outputPool_.count(); ++i) {\n> -\t\t\tBuffer *buffer = new Buffer(i);\n> -\t\t\toutputBuffers.emplace_back(buffer);\n> -\t\t\tret = output->queueBuffer(buffer);\n> -\t\t\tif (ret) {\n> -\t\t\t\tcerr << \"Failed to queue output buffer\" << i << endl;\n> -\t\t\t\treturn TestFail;\n> -\t\t\t}\n> -\t\t}\n> +\t\tfor (FrameBuffer *buffer : outputBuffers_)\n> +\t\t\toutput->queueBuffer(buffer);\n> \n> \t\tret = capture->streamOn();\n> \t\tif (ret) {\n> @@ -202,8 +186,8 @@ private:\n> \tstd::shared_ptr media_;\n> \tV4L2M2MDevice *vim2m_;\n> \n> -\tBufferPool capturePool_;\n> -\tBufferPool outputPool_;\n> +\tstd::vector captureBuffers_;\n> +\tstd::vector outputBuffers_;\n\nThis is leaked. The good news is that turning it into a vector of unique\npointers will help :-)\n\n> \n> \tunsigned int outputFrames_;\n> \tunsigned int captureFrames_;\n> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.h b/test/v4l2_videodevice/v4l2_videodevice_test.h\n> index 34dd231c6d9d108d..02800c2a16f7b0f7 100644\n> --- a/test/v4l2_videodevice/v4l2_videodevice_test.h\n> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.h\n> @@ -41,7 +41,7 @@ protected:\n> \tCameraSensor *sensor_;\n> \tV4L2Subdevice *debayer_;\n> \tV4L2VideoDevice *capture_;\n> -\tBufferPool pool_;\n> +\tstd::vector buffers_;\n\nSame here.\n\n> };\n> \n> #endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */","headers":{"Return-Path":"","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 4380E60100\n\tfor ;\n\tSun, 15 Dec 2019 16:20:56 +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 637D3DD;\n\tSun, 15 Dec 2019 16:20:55 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1576423255;\n\tbh=/ojlQZwm7rHQXEj1nOrhf7CDikPUL/wuY0iMUZUFYoQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WuB7Jk2bpmxC2tZWpIFuNu3JpfvukShZ+HYRbibR3llnTc5gSzWvFF1eKbQSbYzA2\n\tZ+V13nu4gX3Z/Z0L3uY5fDwdBjb2zCnO51shvO8YWp3qVEXQ1/4eRVfpul0BgQaHBa\n\t7My1rjJ9yBPlIjGzi+gD8pZIGqhGDeN+SpvwNJVo=","Date":"Sun, 15 Dec 2019 17:20:46 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191215152046.GE4889@pendragon.ideasonboard.com>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-28-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":"<20191126233620.1695316-28-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 27/30] libcamera: Switch to\n\tFrameBuffer interface","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 15 Dec 2019 15:20:56 -0000"}}]