[{"id":977,"web_url":"https://patchwork.libcamera.org/comment/977/","msgid":"<20190228232823.GP899@bigcity.dyn.berto.se>","date":"2019-02-28T23:28:23","subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: Handle request\n\tcompletion explicitly in pipeline handlers","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your hard work.\n\nOn 2019-02-28 18:29:12 +0200, Laurent Pinchart wrote:\n> Request complete by themselves when all the buffers they contain have\n> completed, connecting the buffer's completed signal to be notified of\n> buffer completion. While this works for now, it prevents pipelines from\n> delaying request completion until all metadata is available, and makes\n> it impossible to ensure that requests complete in the order they are\n> queued.\n> \n> To fix this, make request completion handling explicit in pipeline\n> handlers. The base PipelineHandler class is extended with\n> implementations of the queueRequest() and stop() methods and gets new\n> completeBuffer() and completeRequest() methods to help pipeline handlers\n> tracking requests and buffers.\n> \n> The three existing pipeline handlers connect the bufferReady signal of\n> their capture video node to a slot of their respective camera data\n> instance, where they use the PipelineHandler helpers to notify buffer\n> and request completion. Request completion is handled synchronously with\n> buffer completion as the pipeline handlers don't need to support more\n> advanced use cases, but this paves the road for future work.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nI was a bit afraid of this change at first, but provided I managed to \ngrasp all the fine details I like the end result, nice work.\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  include/libcamera/buffer.h               |  5 +-\n>  include/libcamera/camera.h               |  3 +\n>  include/libcamera/request.h              |  4 +-\n>  src/libcamera/buffer.cpp                 |  5 --\n>  src/libcamera/camera.cpp                 | 21 ++++++\n>  src/libcamera/include/pipeline_handler.h | 10 ++-\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 18 ++++-\n>  src/libcamera/pipeline/uvcvideo.cpp      | 19 ++++-\n>  src/libcamera/pipeline/vimc.cpp          | 19 ++++-\n>  src/libcamera/pipeline_handler.cpp       | 93 +++++++++++++++++++++++-\n>  src/libcamera/request.cpp                | 30 +++-----\n>  src/libcamera/v4l2_device.cpp            |  3 -\n>  12 files changed, 192 insertions(+), 38 deletions(-)\n> \n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index f740ade9bb4f..0c844d126a27 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -10,8 +10,6 @@\n>  #include <stdint.h>\n>  #include <vector>\n>  \n> -#include <libcamera/signal.h>\n> -\n>  namespace libcamera {\n>  \n>  class BufferPool;\n> @@ -55,10 +53,9 @@ public:\n>  \tStatus status() const { return status_; }\n>  \tstd::vector<Plane> &planes() { return planes_; }\n>  \n> -\tSignal<Buffer *> completed;\n> -\n>  private:\n>  \tfriend class BufferPool;\n> +\tfriend class PipelineHandler;\n>  \tfriend class V4L2Device;\n>  \n>  \tvoid cancel();\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index bf70255a6a5e..74eca3d86094 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -34,6 +34,7 @@ public:\n>  \n>  \tconst std::string &name() const;\n>  \n> +\tSignal<Request *, Buffer *> bufferCompleted;\n>  \tSignal<Request *, const std::map<Stream *, Buffer *> &> requestCompleted;\n>  \tSignal<Camera *> disconnected;\n>  \n> @@ -62,6 +63,8 @@ private:\n>  \tvoid disconnect();\n>  \tint exclusiveAccess();\n>  \n> +\tvoid requestComplete(Request *request);\n> +\n>  \tstd::shared_ptr<PipelineHandler> pipe_;\n>  \tstd::string name_;\n>  \tstd::vector<Stream *> streams_;\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 0b75f9d9bd19..0dbd425115e8 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -39,10 +39,12 @@ public:\n>  \n>  private:\n>  \tfriend class Camera;\n> +\tfriend class PipelineHandler;\n>  \n>  \tint prepare();\n>  \tvoid complete(Status status);\n> -\tvoid bufferCompleted(Buffer *buffer);\n> +\n> +\tbool completeBuffer(Buffer *buffer);\n>  \n>  \tCamera *camera_;\n>  \tstd::map<Stream *, Buffer *> bufferMap_;\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 524eb47d4364..e2d1cf04411e 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -212,11 +212,6 @@ Buffer::Buffer()\n>   * \\return A reference to a vector holding all Planes within the buffer\n>   */\n>  \n> -/**\n> - * \\var Buffer::completed\n> - * \\brief A Signal to provide notifications that the specific Buffer is ready\n> - */\n> -\n>  /**\n>   * \\fn Buffer::bytesused()\n>   * \\brief Retrieve the number of bytes occupied by the data in the buffer\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 3789732b95d1..3ef760943ff9 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -98,6 +98,12 @@ const std::string &Camera::name() const\n>  \treturn name_;\n>  }\n>  \n> +/**\n> + * \\var Camera::bufferCompleted\n> + * \\brief Signal emitted when a buffer for a request queued to the camera has\n> + * completed\n> + */\n> +\n>  /**\n>   * \\var Camera::requestCompleted\n>   * \\brief Signal emitted when a request queued to the camera has completed\n> @@ -421,4 +427,19 @@ int Camera::exclusiveAccess()\n>  \treturn 0;\n>  }\n>  \n> +/**\n> + * \\brief Handle request completion and notify application\n> + * \\param[in] request The request that has completed\n> + *\n> + * This function is called by the pipeline handler to notify the camera that\n> + * the request has completed. It emits the requestCompleted signal and deletes\n> + * the request.\n> + */\n> +void Camera::requestComplete(Request *request)\n> +{\n> +\tstd::map<Stream *, Buffer *> buffers(std::move(request->bufferMap_));\n> +\trequestCompleted.emit(request, buffers);\n> +\tdelete request;\n> +}\n> +\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index b2b9c94cebdc..cbc953696816 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -7,6 +7,7 @@\n>  #ifndef __LIBCAMERA_PIPELINE_HANDLER_H__\n>  #define __LIBCAMERA_PIPELINE_HANDLER_H__\n>  \n> +#include <list>\n>  #include <map>\n>  #include <memory>\n>  #include <string>\n> @@ -14,6 +15,7 @@\n>  \n>  namespace libcamera {\n>  \n> +class Buffer;\n>  class BufferPool;\n>  class Camera;\n>  class CameraManager;\n> @@ -35,6 +37,7 @@ public:\n>  \n>  \tCamera *camera_;\n>  \tPipelineHandler *pipe_;\n> +\tstd::list<Request *> queuedRequests_;\n>  \n>  private:\n>  \tCameraData(const CameraData &) = delete;\n> @@ -58,9 +61,12 @@ public:\n>  \tvirtual int freeBuffers(Camera *camera, Stream *stream) = 0;\n>  \n>  \tvirtual int start(Camera *camera) = 0;\n> -\tvirtual void stop(Camera *camera) = 0;\n> +\tvirtual void stop(Camera *camera);\n>  \n> -\tvirtual int queueRequest(Camera *camera, Request *request) = 0;\n> +\tvirtual int queueRequest(Camera *camera, Request *request);\n> +\n> +\tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n> +\tvoid completeRequest(Camera *camera, Request *request);\n>  \n>  protected:\n>  \tvoid registerCamera(std::shared_ptr<Camera> camera,\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 776b7f07f78d..4695ec99470e 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -63,6 +63,8 @@ private:\n>  \t\t\tdelete sensor_;\n>  \t\t}\n>  \n> +\t\tvoid bufferReady(Buffer *buffer);\n> +\n>  \t\tV4L2Device *cio2_;\n>  \t\tV4L2Subdevice *csi2_;\n>  \t\tV4L2Subdevice *sensor_;\n> @@ -236,6 +238,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n>  \n>  \tif (data->cio2_->streamOff())\n>  \t\tLOG(IPU3, Info) << \"Failed to stop camera \" << camera->name();\n> +\n> +\tPipelineHandler::stop(camera);\n>  }\n>  \n>  int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> @@ -250,7 +254,11 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n>  \t\treturn -ENOENT;\n>  \t}\n>  \n> -\tdata->cio2_->queueBuffer(buffer);\n> +\tint ret = data->cio2_->queueBuffer(buffer);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tPipelineHandler::queueRequest(camera, request);\n>  \n>  \treturn 0;\n>  }\n> @@ -417,6 +425,14 @@ void PipelineHandlerIPU3::registerCameras()\n>  \t}\n>  }\n>  \n> +void PipelineHandlerIPU3::IPU3CameraData::bufferReady(Buffer *buffer)\n> +{\n> +\tRequest *request = queuedRequests_.front();\n> +\n> +\tpipe_->completeBuffer(camera_, request, buffer);\n> +\tpipe_->completeRequest(camera_, request);\n> +}\n> +\n>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index f121d3c5633e..29c0c25ae7a8 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -56,6 +56,8 @@ private:\n>  \t\t\tdelete video_;\n>  \t\t}\n>  \n> +\t\tvoid bufferReady(Buffer *buffer);\n> +\n>  \t\tV4L2Device *video_;\n>  \t\tStream stream_;\n>  \t};\n> @@ -153,6 +155,7 @@ void PipelineHandlerUVC::stop(Camera *camera)\n>  {\n>  \tUVCCameraData *data = cameraData(camera);\n>  \tdata->video_->streamOff();\n> +\tPipelineHandler::stop(camera);\n>  }\n>  \n>  int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> @@ -166,7 +169,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n>  \t\treturn -ENOENT;\n>  \t}\n>  \n> -\tdata->video_->queueBuffer(buffer);\n> +\tint ret = data->video_->queueBuffer(buffer);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tPipelineHandler::queueRequest(camera, request);\n>  \n>  \treturn 0;\n>  }\n> @@ -198,6 +205,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>  \t\treturn false;\n>  \t}\n>  \n> +\tdata->video_->bufferReady.connect(data.get(), &UVCCameraData::bufferReady);\n> +\n>  \t/* Create and register the camera. */\n>  \tstd::vector<Stream *> streams{ &data->stream_ };\n>  \tstd::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams);\n> @@ -209,6 +218,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>  \treturn true;\n>  }\n>  \n> +void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer)\n> +{\n> +\tRequest *request = queuedRequests_.front();\n> +\n> +\tpipe_->completeBuffer(camera_, request, buffer);\n> +\tpipe_->completeRequest(camera_, request);\n> +}\n> +\n>  REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 6d022c37eb9f..8c7c2d05828f 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -56,6 +56,8 @@ private:\n>  \t\t\tdelete video_;\n>  \t\t}\n>  \n> +\t\tvoid bufferReady(Buffer *buffer);\n> +\n>  \t\tV4L2Device *video_;\n>  \t\tStream stream_;\n>  \t};\n> @@ -153,6 +155,7 @@ void PipelineHandlerVimc::stop(Camera *camera)\n>  {\n>  \tVimcCameraData *data = cameraData(camera);\n>  \tdata->video_->streamOff();\n> +\tPipelineHandler::stop(camera);\n>  }\n>  \n>  int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n> @@ -166,7 +169,11 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n>  \t\treturn -ENOENT;\n>  \t}\n>  \n> -\tdata->video_->queueBuffer(buffer);\n> +\tint ret = data->video_->queueBuffer(buffer);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tPipelineHandler::queueRequest(camera, request);\n>  \n>  \treturn 0;\n>  }\n> @@ -205,6 +212,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n>  \tif (data->video_->open())\n>  \t\treturn false;\n>  \n> +\tdata->video_->bufferReady.connect(data.get(), &VimcCameraData::bufferReady);\n> +\n>  \t/* Create and register the camera. */\n>  \tstd::vector<Stream *> streams{ &data->stream_ };\n>  \tstd::shared_ptr<Camera> camera = Camera::create(this, \"VIMC Sensor B\",\n> @@ -214,6 +223,14 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n>  \treturn true;\n>  }\n>  \n> +void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer)\n> +{\n> +\tRequest *request = queuedRequests_.front();\n> +\n> +\tpipe_->completeBuffer(camera_, request, buffer);\n> +\tpipe_->completeRequest(camera_, request);\n> +}\n> +\n>  REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 54f237942a48..1a858f2638ce 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -5,6 +5,7 @@\n>   * pipeline_handler.cpp - Pipeline handler infrastructure\n>   */\n>  \n> +#include <libcamera/buffer.h>\n>  #include <libcamera/camera.h>\n>  #include <libcamera/camera_manager.h>\n>  \n> @@ -74,6 +75,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n>   * and remains valid until the instance is destroyed.\n>   */\n>  \n> +/**\n> + * \\var CameraData::queuedRequests_\n> + * \\brief The list of queued and not yet completed request\n> + *\n> + * The list of queued request is used to track requests queued in order to\n> + * ensure completion of all requests when the pipeline handler is stopped.\n> + *\n> + * \\sa PipelineHandler::queueRequest(), PipelineHandler::stop(),\n> + * PipelineHandler::completeRequest()\n> + */\n> +\n>  /**\n>   * \\class PipelineHandler\n>   * \\brief Create and manage cameras based on a set of media devices\n> @@ -226,8 +238,30 @@ PipelineHandler::~PipelineHandler()\n>   * This method stops capturing and processing requests immediately. All pending\n>   * requests are cancelled and complete immediately in an error state.\n>   *\n> - * \\todo Complete the pending requests immediately\n> + * Pipeline handlers shall override this method to stop the pipeline, ensure\n> + * that all pending request completion signaled through completeRequest() have\n> + * returned, and call the base implementation of the stop() method as the last\n> + * step of their implementation. The base implementation cancels all requests\n> + * queued but not yet complete.\n>   */\n> +void PipelineHandler::stop(Camera *camera)\n> +{\n> +\tCameraData *data = cameraData(camera);\n> +\n> +\twhile (!data->queuedRequests_.empty()) {\n> +\t\tRequest *request = data->queuedRequests_.front();\n> +\t\tdata->queuedRequests_.pop_front();\n> +\n> +\t\twhile (!request->pending_.empty()) {\n> +\t\t\tBuffer *buffer = *request->pending_.begin();\n> +\t\t\tbuffer->cancel();\n> +\t\t\tcompleteBuffer(camera, request, buffer);\n> +\t\t}\n> +\n> +\t\trequest->complete(Request::RequestCancelled);\n> +\t\tcamera->requestComplete(request);\n> +\t}\n> +}\n>  \n>  /**\n>   * \\fn PipelineHandler::queueRequest()\n> @@ -241,8 +275,65 @@ PipelineHandler::~PipelineHandler()\n>   * parameters will be applied to the frames captured in the buffers provided in\n>   * the request.\n>   *\n> + * Pipeline handlers shall override this method. The base implementation in the\n> + * PipelineHandler class keeps track of queued requests in order to ensure\n> + * completion of all requests when the pipeline handler is stopped with stop().\n> + * Requests completion shall be signaled by the pipeline handler using the\n> + * completeRequest() method.\n> + *\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n> +int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> +{\n> +\tCameraData *data = cameraData(camera);\n> +\tdata->queuedRequests_.push_back(request);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Complete a buffer for a request\n> + * \\param[in] camera The camera the request belongs to\n> + * \\param[in] request The request the buffer belongs to\n> + * \\param[in] buffer The buffer that has completed\n> + *\n> + * This method shall be called by pipeline handlers to signal completion of the\n> + * \\a buffer part of the \\a request. It notifies applications of buffer\n> + * completion and updates the request's internal buffer tracking. The request\n> + * is not completed automatically when the last buffer completes, pipeline\n> + * handlers shall complete requests explicitly with completeRequest().\n> + *\n> + * \\return True if all buffers contained in the request have completed, false\n> + * otherwise\n> + */\n> +bool PipelineHandler::completeBuffer(Camera *camera, Request *request,\n> +\t\t\t\t     Buffer *buffer)\n> +{\n> +\tcamera->bufferCompleted.emit(request, buffer);\n> +\treturn request->completeBuffer(buffer);\n> +}\n> +\n> +/**\n> + * \\brief Signal request completion\n> + * \\param[in] camera The camera that the request belongs to\n> + * \\param[in] request The request that has completed\n> + *\n> + * The pipeline handler shall call this method to notify the \\a camera that the\n> + * request request has complete. The request is deleted and shall not be\n> + * accessed once this method returns.\n> + *\n> + * The pipeline handler shall ensure that requests complete in the same order\n> + * they are submitted.\n> + */\n> +void PipelineHandler::completeRequest(Camera *camera, Request *request)\n> +{\n> +\tCameraData *data = cameraData(camera);\n> +\tASSERT(request == data->queuedRequests_.front());\n> +\tdata->queuedRequests_.pop_front();\n> +\n> +\trequest->complete(Request::RequestComplete);\n> +\tcamera->requestComplete(request);\n> +}\n>  \n>  /**\n>   * \\brief Register a camera to the camera manager and pipeline handler\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index cb170930fbb6..e0e77e972411 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -113,7 +113,6 @@ int Request::prepare()\n>  {\n>  \tfor (auto const &pair : bufferMap_) {\n>  \t\tBuffer *buffer = pair.second;\n> -\t\tbuffer->completed.connect(this, &Request::bufferCompleted);\n>  \t\tpending_.insert(buffer);\n>  \t}\n>  \n> @@ -133,31 +132,24 @@ void Request::complete(Status status)\n>  }\n>  \n>  /**\n> - * \\brief Slot for the buffer completed signal\n> + * \\brief Complete a buffer for the request\n> + * \\param[in] buffer The buffer that has completed\n>   *\n> - * The bufferCompleted method serves as slot where to connect the\n> - * Buffer::completed signal that is emitted when a buffer has available\n> - * data.\n> + * A request tracks the status of all buffers it contains through a set of\n> + * pending buffers. This function removes the \\a buffer from the set to mark it\n> + * as complete. All buffers associate with the request shall be marked as\n> + * complete by calling this function once and once only before reporting the\n> + * request as complete with the complete() method.\n>   *\n> - * The request completes when all the buffers it contains are ready to be\n> - * presented to the application. It then emits the Camera::requestCompleted\n> - * signal and is automatically deleted.\n> + * \\return True if all buffers contained in the request have completed, false\n> + * otherwise\n>   */\n> -void Request::bufferCompleted(Buffer *buffer)\n> +bool Request::completeBuffer(Buffer *buffer)\n>  {\n> -\tbuffer->completed.disconnect(this, &Request::bufferCompleted);\n> -\n>  \tint ret = pending_.erase(buffer);\n>  \tASSERT(ret == 1);\n>  \n> -\tif (!pending_.empty())\n> -\t\treturn;\n> -\n> -\tcomplete(RequestComplete);\n> -\n> -\tstd::map<Stream *, Buffer *> buffers(std::move(bufferMap_));\n> -\tcamera_->requestCompleted.emit(this, buffers);\n> -\tdelete this;\n> +\treturn pending_.empty();\n>  }\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 054499e4b888..52167a3e047f 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -819,9 +819,6 @@ void V4L2Device::bufferAvailable(EventNotifier *notifier)\n>  \n>  \t/* Notify anyone listening to the device. */\n>  \tbufferReady.emit(buffer);\n> -\n> -\t/* Notify anyone listening to the buffer specifically. */\n> -\tbuffer->completed.emit(buffer);\n>  }\n>  \n>  /**\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 37597610BF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  1 Mar 2019 00:28:26 +0100 (CET)","by mail-lf1-x133.google.com with SMTP id g12so16569200lfb.13\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 28 Feb 2019 15:28:26 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tk23sm4205947ljk.69.2019.02.28.15.28.24\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 28 Feb 2019 15:28:24 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=m5ajqpZ4/4Fabx/9ifsVi6aS9hwMQY/FVWeDbMG78dg=;\n\tb=0ix2VTLAYTlOzLbvra32mMxhkYhVRHRU9hDWsMIzQF/z33sXs4ClvikhbNfQeclmIA\n\t5ZpDQiVic21HqBKaUYBAPhenIEHp3SeNRpEtuDwkdFk+LjkADHuT7qGZzRfU8LtTWGRi\n\ty50HTqEYQ0y9NjnKa1wY2DDF2KGXb3PESRWls/lGwaZ2ObEQi4J7GsUGpTojwfIP4zg4\n\tbFl/SqWiUQbyZpcEHhMSvTtgLp820IvIc+zU4WywKd0UkYMr5LHibz552tQ3fulL/rjl\n\ttyA9775gCNd5ZRI3dkvf7QXJ6D9De1QwkzKR6NdVvWav9rmVr8PnKM3fgYq4i+vW/7zn\n\tsxsQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=m5ajqpZ4/4Fabx/9ifsVi6aS9hwMQY/FVWeDbMG78dg=;\n\tb=LcBFA3l3pWmfJ/RAMbQWUOVvbrHeGT7PJ/KH4Dcr8bdBg4tijS94jiMzdfCojoY2Az\n\tyMRCgRzrHGYzVqmGS2Tfz6W1jupAXCcoQpSiu9Slo6OFOw/9VBr/K8iqSVD+O9g7ypKW\n\tUR4ZgoU6cJYzkmBX0HfSLQufoxj40P77Qz1doTU/f4FXiuIkBvfbnf4K2w/BVA5hpoAb\n\tAgHqUDWX6Q/iKfbQDsXsqjECaXF0qOaIoav+EJbsJp0ky4r1bkmZIimb+MjnWn/JM9qE\n\tWrzXpBJwpHqSpaeCOMsiNOPlHWHxR5HfuE9FH/dOU6UlI6ztP2I7Pr3i7TQr5LRFOA9r\n\twjCA==","X-Gm-Message-State":"APjAAAXPpC/T7z2iF7q6kiXiJUntADZ7fcULOFBFChG2aJt4tWsNSTj+\n\tgVGqzyJWAtGuUSJRhDfXDlb2ZOvga7s=","X-Google-Smtp-Source":"APXvYqw4CA3JzQNF6rvXh2pS8GjTaRCwsm51L517i5BOeCdoElTRAMfhH6YkDlJh1m7lqF/8fIC9vg==","X-Received":"by 2002:ac2:551a:: with SMTP id j26mr1173047lfk.59.1551396505300;\n\tThu, 28 Feb 2019 15:28:25 -0800 (PST)","Date":"Fri, 1 Mar 2019 00:28:23 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190228232823.GP899@bigcity.dyn.berto.se>","References":"<20190228162913.6508-1-laurent.pinchart@ideasonboard.com>\n\t<20190228162913.6508-10-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190228162913.6508-10-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: Handle request\n\tcompletion explicitly in pipeline handlers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 28 Feb 2019 23:28:26 -0000"}},{"id":1000,"web_url":"https://patchwork.libcamera.org/comment/1000/","msgid":"<20190301150947.eccf6iql5pkgfj2o@uno.localdomain>","date":"2019-03-01T15:09:47","subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: Handle request\n\tcompletion explicitly in pipeline handlers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Feb 28, 2019 at 06:29:12PM +0200, Laurent Pinchart wrote:\n> Request complete by themselves when all the buffers they contain have\n> completed, connecting the buffer's completed signal to be notified of\n> buffer completion. While this works for now, it prevents pipelines from\n> delaying request completion until all metadata is available, and makes\n> it impossible to ensure that requests complete in the order they are\n> queued.\n>\n> To fix this, make request completion handling explicit in pipeline\n> handlers. The base PipelineHandler class is extended with\n> implementations of the queueRequest() and stop() methods and gets new\n> completeBuffer() and completeRequest() methods to help pipeline handlers\n> tracking requests and buffers.\n>\n> The three existing pipeline handlers connect the bufferReady signal of\n> their capture video node to a slot of their respective camera data\n> instance, where they use the PipelineHandler helpers to notify buffer\n> and request completion. Request completion is handled synchronously with\n> buffer completion as the pipeline handlers don't need to support more\n> advanced use cases, but this paves the road for future work.\n>\n\nThe only punctual comment I have is that the bufferReady signal is not\nconnected in the ipu3.cpp file.\n\nYou've heard enough of my concerns and I've heard enough of your\nreasons, so the architecture is fine for the moment.\n\nA few more question below though.\n\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  include/libcamera/buffer.h               |  5 +-\n>  include/libcamera/camera.h               |  3 +\n>  include/libcamera/request.h              |  4 +-\n>  src/libcamera/buffer.cpp                 |  5 --\n>  src/libcamera/camera.cpp                 | 21 ++++++\n>  src/libcamera/include/pipeline_handler.h | 10 ++-\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     | 18 ++++-\n>  src/libcamera/pipeline/uvcvideo.cpp      | 19 ++++-\n>  src/libcamera/pipeline/vimc.cpp          | 19 ++++-\n>  src/libcamera/pipeline_handler.cpp       | 93 +++++++++++++++++++++++-\n>  src/libcamera/request.cpp                | 30 +++-----\n>  src/libcamera/v4l2_device.cpp            |  3 -\n>  12 files changed, 192 insertions(+), 38 deletions(-)\n>\n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index f740ade9bb4f..0c844d126a27 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -10,8 +10,6 @@\n>  #include <stdint.h>\n>  #include <vector>\n>\n> -#include <libcamera/signal.h>\n> -\n>  namespace libcamera {\n>\n>  class BufferPool;\n> @@ -55,10 +53,9 @@ public:\n>  \tStatus status() const { return status_; }\n>  \tstd::vector<Plane> &planes() { return planes_; }\n>\n> -\tSignal<Buffer *> completed;\n> -\n>  private:\n>  \tfriend class BufferPool;\n> +\tfriend class PipelineHandler;\n>  \tfriend class V4L2Device;\n>\n>  \tvoid cancel();\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index bf70255a6a5e..74eca3d86094 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -34,6 +34,7 @@ public:\n>\n>  \tconst std::string &name() const;\n>\n> +\tSignal<Request *, Buffer *> bufferCompleted;\n\nIs this reserved for applications as well? I don't see it used in the library\ncode, am I wrong?\n\n>  \tSignal<Request *, const std::map<Stream *, Buffer *> &> requestCompleted;\n>  \tSignal<Camera *> disconnected;\n>\n> @@ -62,6 +63,8 @@ private:\n>  \tvoid disconnect();\n>  \tint exclusiveAccess();\n>\n> +\tvoid requestComplete(Request *request);\n> +\n>  \tstd::shared_ptr<PipelineHandler> pipe_;\n>  \tstd::string name_;\n>  \tstd::vector<Stream *> streams_;\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 0b75f9d9bd19..0dbd425115e8 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -39,10 +39,12 @@ public:\n>\n>  private:\n>  \tfriend class Camera;\n> +\tfriend class PipelineHandler;\n>\n>  \tint prepare();\n>  \tvoid complete(Status status);\n> -\tvoid bufferCompleted(Buffer *buffer);\n> +\n> +\tbool completeBuffer(Buffer *buffer);\n>\n>  \tCamera *camera_;\n>  \tstd::map<Stream *, Buffer *> bufferMap_;\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 524eb47d4364..e2d1cf04411e 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -212,11 +212,6 @@ Buffer::Buffer()\n>   * \\return A reference to a vector holding all Planes within the buffer\n>   */\n>\n> -/**\n> - * \\var Buffer::completed\n> - * \\brief A Signal to provide notifications that the specific Buffer is ready\n> - */\n> -\n>  /**\n>   * \\fn Buffer::bytesused()\n>   * \\brief Retrieve the number of bytes occupied by the data in the buffer\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 3789732b95d1..3ef760943ff9 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -98,6 +98,12 @@ const std::string &Camera::name() const\n>  \treturn name_;\n>  }\n>\n> +/**\n> + * \\var Camera::bufferCompleted\n> + * \\brief Signal emitted when a buffer for a request queued to the camera has\n> + * completed\n> + */\n> +\n>  /**\n>   * \\var Camera::requestCompleted\n>   * \\brief Signal emitted when a request queued to the camera has completed\n> @@ -421,4 +427,19 @@ int Camera::exclusiveAccess()\n>  \treturn 0;\n>  }\n>\n> +/**\n> + * \\brief Handle request completion and notify application\n> + * \\param[in] request The request that has completed\n> + *\n> + * This function is called by the pipeline handler to notify the camera that\n> + * the request has completed. It emits the requestCompleted signal and deletes\n> + * the request.\n> + */\n> +void Camera::requestComplete(Request *request)\n> +{\n> +\tstd::map<Stream *, Buffer *> buffers(std::move(request->bufferMap_));\n> +\trequestCompleted.emit(request, buffers);\n> +\tdelete request;\n> +}\n> +\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index b2b9c94cebdc..cbc953696816 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -7,6 +7,7 @@\n>  #ifndef __LIBCAMERA_PIPELINE_HANDLER_H__\n>  #define __LIBCAMERA_PIPELINE_HANDLER_H__\n>\n> +#include <list>\n>  #include <map>\n>  #include <memory>\n>  #include <string>\n> @@ -14,6 +15,7 @@\n>\n>  namespace libcamera {\n>\n> +class Buffer;\n>  class BufferPool;\n>  class Camera;\n>  class CameraManager;\n> @@ -35,6 +37,7 @@ public:\n>\n>  \tCamera *camera_;\n>  \tPipelineHandler *pipe_;\n> +\tstd::list<Request *> queuedRequests_;\n>\n>  private:\n>  \tCameraData(const CameraData &) = delete;\n> @@ -58,9 +61,12 @@ public:\n>  \tvirtual int freeBuffers(Camera *camera, Stream *stream) = 0;\n>\n>  \tvirtual int start(Camera *camera) = 0;\n> -\tvirtual void stop(Camera *camera) = 0;\n> +\tvirtual void stop(Camera *camera);\n>\n> -\tvirtual int queueRequest(Camera *camera, Request *request) = 0;\n> +\tvirtual int queueRequest(Camera *camera, Request *request);\n> +\n> +\tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n> +\tvoid completeRequest(Camera *camera, Request *request);\n>\n>  protected:\n>  \tvoid registerCamera(std::shared_ptr<Camera> camera,\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 776b7f07f78d..4695ec99470e 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -63,6 +63,8 @@ private:\n>  \t\t\tdelete sensor_;\n>  \t\t}\n>\n> +\t\tvoid bufferReady(Buffer *buffer);\n> +\n>  \t\tV4L2Device *cio2_;\n>  \t\tV4L2Subdevice *csi2_;\n>  \t\tV4L2Subdevice *sensor_;\n> @@ -236,6 +238,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n>\n>  \tif (data->cio2_->streamOff())\n>  \t\tLOG(IPU3, Info) << \"Failed to stop camera \" << camera->name();\n> +\n> +\tPipelineHandler::stop(camera);\n>  }\n>\n>  int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> @@ -250,7 +254,11 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n>  \t\treturn -ENOENT;\n>  \t}\n>\n> -\tdata->cio2_->queueBuffer(buffer);\n> +\tint ret = data->cio2_->queueBuffer(buffer);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tPipelineHandler::queueRequest(camera, request);\n>\n>  \treturn 0;\n>  }\n> @@ -417,6 +425,14 @@ void PipelineHandlerIPU3::registerCameras()\n>  \t}\n>  }\n>\n> +void PipelineHandlerIPU3::IPU3CameraData::bufferReady(Buffer *buffer)\n> +{\n> +\tRequest *request = queuedRequests_.front();\n> +\n> +\tpipe_->completeBuffer(camera_, request, buffer);\n> +\tpipe_->completeRequest(camera_, request);\n> +}\n> +\n>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n>\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index f121d3c5633e..29c0c25ae7a8 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -56,6 +56,8 @@ private:\n>  \t\t\tdelete video_;\n>  \t\t}\n>\n> +\t\tvoid bufferReady(Buffer *buffer);\n> +\n>  \t\tV4L2Device *video_;\n>  \t\tStream stream_;\n>  \t};\n> @@ -153,6 +155,7 @@ void PipelineHandlerUVC::stop(Camera *camera)\n>  {\n>  \tUVCCameraData *data = cameraData(camera);\n>  \tdata->video_->streamOff();\n> +\tPipelineHandler::stop(camera);\n>  }\n>\n>  int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> @@ -166,7 +169,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n>  \t\treturn -ENOENT;\n>  \t}\n>\n> -\tdata->video_->queueBuffer(buffer);\n> +\tint ret = data->video_->queueBuffer(buffer);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tPipelineHandler::queueRequest(camera, request);\n>\n>  \treturn 0;\n>  }\n> @@ -198,6 +205,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>  \t\treturn false;\n>  \t}\n>\n> +\tdata->video_->bufferReady.connect(data.get(), &UVCCameraData::bufferReady);\n> +\n>  \t/* Create and register the camera. */\n>  \tstd::vector<Stream *> streams{ &data->stream_ };\n>  \tstd::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams);\n> @@ -209,6 +218,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>  \treturn true;\n>  }\n>\n> +void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer)\n> +{\n> +\tRequest *request = queuedRequests_.front();\n> +\n> +\tpipe_->completeBuffer(camera_, request, buffer);\n> +\tpipe_->completeRequest(camera_, request);\n> +}\n> +\n>  REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);\n>\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 6d022c37eb9f..8c7c2d05828f 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -56,6 +56,8 @@ private:\n>  \t\t\tdelete video_;\n>  \t\t}\n>\n> +\t\tvoid bufferReady(Buffer *buffer);\n> +\n>  \t\tV4L2Device *video_;\n>  \t\tStream stream_;\n>  \t};\n> @@ -153,6 +155,7 @@ void PipelineHandlerVimc::stop(Camera *camera)\n>  {\n>  \tVimcCameraData *data = cameraData(camera);\n>  \tdata->video_->streamOff();\n> +\tPipelineHandler::stop(camera);\n>  }\n>\n>  int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n> @@ -166,7 +169,11 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n>  \t\treturn -ENOENT;\n>  \t}\n>\n> -\tdata->video_->queueBuffer(buffer);\n> +\tint ret = data->video_->queueBuffer(buffer);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tPipelineHandler::queueRequest(camera, request);\n>\n>  \treturn 0;\n>  }\n> @@ -205,6 +212,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n>  \tif (data->video_->open())\n>  \t\treturn false;\n>\n> +\tdata->video_->bufferReady.connect(data.get(), &VimcCameraData::bufferReady);\n> +\n>  \t/* Create and register the camera. */\n>  \tstd::vector<Stream *> streams{ &data->stream_ };\n>  \tstd::shared_ptr<Camera> camera = Camera::create(this, \"VIMC Sensor B\",\n> @@ -214,6 +223,14 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n>  \treturn true;\n>  }\n>\n> +void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer)\n> +{\n> +\tRequest *request = queuedRequests_.front();\n> +\n> +\tpipe_->completeBuffer(camera_, request, buffer);\n> +\tpipe_->completeRequest(camera_, request);\n> +}\n> +\n>  REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);\n>\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 54f237942a48..1a858f2638ce 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -5,6 +5,7 @@\n>   * pipeline_handler.cpp - Pipeline handler infrastructure\n>   */\n>\n> +#include <libcamera/buffer.h>\n>  #include <libcamera/camera.h>\n>  #include <libcamera/camera_manager.h>\n>\n> @@ -74,6 +75,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n>   * and remains valid until the instance is destroyed.\n>   */\n>\n> +/**\n> + * \\var CameraData::queuedRequests_\n> + * \\brief The list of queued and not yet completed request\n> + *\n> + * The list of queued request is used to track requests queued in order to\n> + * ensure completion of all requests when the pipeline handler is stopped.\n> + *\n> + * \\sa PipelineHandler::queueRequest(), PipelineHandler::stop(),\n> + * PipelineHandler::completeRequest()\n> + */\n> +\n>  /**\n>   * \\class PipelineHandler\n>   * \\brief Create and manage cameras based on a set of media devices\n> @@ -226,8 +238,30 @@ PipelineHandler::~PipelineHandler()\n>   * This method stops capturing and processing requests immediately. All pending\n>   * requests are cancelled and complete immediately in an error state.\n>   *\n> - * \\todo Complete the pending requests immediately\n> + * Pipeline handlers shall override this method to stop the pipeline, ensure\n> + * that all pending request completion signaled through completeRequest() have\n> + * returned, and call the base implementation of the stop() method as the last\n> + * step of their implementation. The base implementation cancels all requests\n> + * queued but not yet complete.\n>   */\n> +void PipelineHandler::stop(Camera *camera)\n> +{\n> +\tCameraData *data = cameraData(camera);\n> +\n> +\twhile (!data->queuedRequests_.empty()) {\n> +\t\tRequest *request = data->queuedRequests_.front();\n> +\t\tdata->queuedRequests_.pop_front();\n> +\n> +\t\twhile (!request->pending_.empty()) {\n> +\t\t\tBuffer *buffer = *request->pending_.begin();\n> +\t\t\tbuffer->cancel();\n> +\t\t\tcompleteBuffer(camera, request, buffer);\n> +\t\t}\n> +\n> +\t\trequest->complete(Request::RequestCancelled);\n> +\t\tcamera->requestComplete(request);\n> +\t}\n> +}\n>\n>  /**\n>   * \\fn PipelineHandler::queueRequest()\n> @@ -241,8 +275,65 @@ PipelineHandler::~PipelineHandler()\n>   * parameters will be applied to the frames captured in the buffers provided in\n>   * the request.\n>   *\n> + * Pipeline handlers shall override this method. The base implementation in the\n> + * PipelineHandler class keeps track of queued requests in order to ensure\n> + * completion of all requests when the pipeline handler is stopped with stop().\n> + * Requests completion shall be signaled by the pipeline handler using the\n> + * completeRequest() method.\n> + *\n>   * \\return 0 on success or a negative error code otherwise\n>   */\n> +int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> +{\n> +\tCameraData *data = cameraData(camera);\n> +\tdata->queuedRequests_.push_back(request);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Complete a buffer for a request\n> + * \\param[in] camera The camera the request belongs to\n> + * \\param[in] request The request the buffer belongs to\n> + * \\param[in] buffer The buffer that has completed\n> + *\n> + * This method shall be called by pipeline handlers to signal completion of the\n> + * \\a buffer part of the \\a request. It notifies applications of buffer\n> + * completion and updates the request's internal buffer tracking. The request\n> + * is not completed automatically when the last buffer completes, pipeline\n> + * handlers shall complete requests explicitly with completeRequest().\n> + *\n> + * \\return True if all buffers contained in the request have completed, false\n> + * otherwise\n> + */\n> +bool PipelineHandler::completeBuffer(Camera *camera, Request *request,\n> +\t\t\t\t     Buffer *buffer)\n> +{\n> +\tcamera->bufferCompleted.emit(request, buffer);\n> +\treturn request->completeBuffer(buffer);\n> +}\n> +\n> +/**\n> + * \\brief Signal request completion\n> + * \\param[in] camera The camera that the request belongs to\n> + * \\param[in] request The request that has completed\n> + *\n> + * The pipeline handler shall call this method to notify the \\a camera that the\n> + * request request has complete. The request is deleted and shall not be\n> + * accessed once this method returns.\n> + *\n> + * The pipeline handler shall ensure that requests complete in the same order\n> + * they are submitted.\n> + */\n> +void PipelineHandler::completeRequest(Camera *camera, Request *request)\n> +{\n> +\tCameraData *data = cameraData(camera);\n> +\tASSERT(request == data->queuedRequests_.front());\n> +\tdata->queuedRequests_.pop_front();\n> +\n> +\trequest->complete(Request::RequestComplete);\n> +\tcamera->requestComplete(request);\n> +}\n>\n>  /**\n>   * \\brief Register a camera to the camera manager and pipeline handler\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index cb170930fbb6..e0e77e972411 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -113,7 +113,6 @@ int Request::prepare()\n>  {\n>  \tfor (auto const &pair : bufferMap_) {\n>  \t\tBuffer *buffer = pair.second;\n> -\t\tbuffer->completed.connect(this, &Request::bufferCompleted);\n>  \t\tpending_.insert(buffer);\n>  \t}\n>\n> @@ -133,31 +132,24 @@ void Request::complete(Status status)\n>  }\n>\n>  /**\n> - * \\brief Slot for the buffer completed signal\n> + * \\brief Complete a buffer for the request\n> + * \\param[in] buffer The buffer that has completed\n>   *\n> - * The bufferCompleted method serves as slot where to connect the\n> - * Buffer::completed signal that is emitted when a buffer has available\n> - * data.\n> + * A request tracks the status of all buffers it contains through a set of\n> + * pending buffers. This function removes the \\a buffer from the set to mark it\n> + * as complete. All buffers associate with the request shall be marked as\n> + * complete by calling this function once and once only before reporting the\n> + * request as complete with the complete() method.\n>   *\n> - * The request completes when all the buffers it contains are ready to be\n> - * presented to the application. It then emits the Camera::requestCompleted\n> - * signal and is automatically deleted.\n> + * \\return True if all buffers contained in the request have completed, false\n> + * otherwise\n>   */\n> -void Request::bufferCompleted(Buffer *buffer)\n> +bool Request::completeBuffer(Buffer *buffer)\n>  {\n> -\tbuffer->completed.disconnect(this, &Request::bufferCompleted);\n> -\n>  \tint ret = pending_.erase(buffer);\n>  \tASSERT(ret == 1);\n\nThis was probably in a previous patch, but this\nhttps://en.cppreference.com/w/cpp/container/unordered_set/erase\ndoesn't mention anything on the return value.\n\n>\n> -\tif (!pending_.empty())\n> -\t\treturn;\n> -\n> -\tcomplete(RequestComplete);\n> -\n> -\tstd::map<Stream *, Buffer *> buffers(std::move(bufferMap_));\n> -\tcamera_->requestCompleted.emit(this, buffers);\n> -\tdelete this;\n> +\treturn pending_.empty();\n>  }\n>\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 054499e4b888..52167a3e047f 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -819,9 +819,6 @@ void V4L2Device::bufferAvailable(EventNotifier *notifier)\n>\n>  \t/* Notify anyone listening to the device. */\n>  \tbufferReady.emit(buffer);\n> -\n> -\t/* Notify anyone listening to the buffer specifically. */\n> -\tbuffer->completed.emit(buffer);\n>  }\n>\n>  /**\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 383B8610BF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  1 Mar 2019 16:09:19 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 8FC9A100010;\n\tFri,  1 Mar 2019 15:09:17 +0000 (UTC)"],"Date":"Fri, 1 Mar 2019 16:09:47 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190301150947.eccf6iql5pkgfj2o@uno.localdomain>","References":"<20190228162913.6508-1-laurent.pinchart@ideasonboard.com>\n\t<20190228162913.6508-10-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"w3lbikfdzhhj74iz\"","Content-Disposition":"inline","In-Reply-To":"<20190228162913.6508-10-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: Handle request\n\tcompletion explicitly in pipeline handlers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 01 Mar 2019 15:09:19 -0000"}},{"id":1002,"web_url":"https://patchwork.libcamera.org/comment/1002/","msgid":"<20190301184607.GG32244@pendragon.ideasonboard.com>","date":"2019-03-01T18:46:07","subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: Handle request\n\tcompletion explicitly in pipeline handlers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Mar 01, 2019 at 04:09:47PM +0100, Jacopo Mondi wrote:\n> On Thu, Feb 28, 2019 at 06:29:12PM +0200, Laurent Pinchart wrote:\n> > Request complete by themselves when all the buffers they contain have\n> > completed, connecting the buffer's completed signal to be notified of\n> > buffer completion. While this works for now, it prevents pipelines from\n> > delaying request completion until all metadata is available, and makes\n> > it impossible to ensure that requests complete in the order they are\n> > queued.\n> >\n> > To fix this, make request completion handling explicit in pipeline\n> > handlers. The base PipelineHandler class is extended with\n> > implementations of the queueRequest() and stop() methods and gets new\n> > completeBuffer() and completeRequest() methods to help pipeline handlers\n> > tracking requests and buffers.\n> >\n> > The three existing pipeline handlers connect the bufferReady signal of\n> > their capture video node to a slot of their respective camera data\n> > instance, where they use the PipelineHandler helpers to notify buffer\n> > and request completion. Request completion is handled synchronously with\n> > buffer completion as the pipeline handlers don't need to support more\n> > advanced use cases, but this paves the road for future work.\n> \n> The only punctual comment I have is that the bufferReady signal is not\n> connected in the ipu3.cpp file.\n\nThank you for catching this. I'll fix it.\n\n> You've heard enough of my concerns and I've heard enough of your\n> reasons, so the architecture is fine for the moment.\n> \n> A few more question below though.\n> \n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  include/libcamera/buffer.h               |  5 +-\n> >  include/libcamera/camera.h               |  3 +\n> >  include/libcamera/request.h              |  4 +-\n> >  src/libcamera/buffer.cpp                 |  5 --\n> >  src/libcamera/camera.cpp                 | 21 ++++++\n> >  src/libcamera/include/pipeline_handler.h | 10 ++-\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp     | 18 ++++-\n> >  src/libcamera/pipeline/uvcvideo.cpp      | 19 ++++-\n> >  src/libcamera/pipeline/vimc.cpp          | 19 ++++-\n> >  src/libcamera/pipeline_handler.cpp       | 93 +++++++++++++++++++++++-\n> >  src/libcamera/request.cpp                | 30 +++-----\n> >  src/libcamera/v4l2_device.cpp            |  3 -\n> >  12 files changed, 192 insertions(+), 38 deletions(-)\n> >\n> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > index f740ade9bb4f..0c844d126a27 100644\n> > --- a/include/libcamera/buffer.h\n> > +++ b/include/libcamera/buffer.h\n> > @@ -10,8 +10,6 @@\n> >  #include <stdint.h>\n> >  #include <vector>\n> >\n> > -#include <libcamera/signal.h>\n> > -\n> >  namespace libcamera {\n> >\n> >  class BufferPool;\n> > @@ -55,10 +53,9 @@ public:\n> >  \tStatus status() const { return status_; }\n> >  \tstd::vector<Plane> &planes() { return planes_; }\n> >\n> > -\tSignal<Buffer *> completed;\n> > -\n> >  private:\n> >  \tfriend class BufferPool;\n> > +\tfriend class PipelineHandler;\n> >  \tfriend class V4L2Device;\n> >\n> >  \tvoid cancel();\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index bf70255a6a5e..74eca3d86094 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -34,6 +34,7 @@ public:\n> >\n> >  \tconst std::string &name() const;\n> >\n> > +\tSignal<Request *, Buffer *> bufferCompleted;\n> \n> Is this reserved for applications as well? I don't see it used in the library\n> code, am I wrong?\n\nCorrect. The Buffer class provided the ability for applications to get\nnotified of buffer completion events, so I wanted to retain that. As\nit's unused I'm also fine dropping it, I assume the API will be reworked\nanyway.\n\n> >  \tSignal<Request *, const std::map<Stream *, Buffer *> &> requestCompleted;\n> >  \tSignal<Camera *> disconnected;\n> >\n> > @@ -62,6 +63,8 @@ private:\n> >  \tvoid disconnect();\n> >  \tint exclusiveAccess();\n> >\n> > +\tvoid requestComplete(Request *request);\n> > +\n> >  \tstd::shared_ptr<PipelineHandler> pipe_;\n> >  \tstd::string name_;\n> >  \tstd::vector<Stream *> streams_;\n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index 0b75f9d9bd19..0dbd425115e8 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -39,10 +39,12 @@ public:\n> >\n> >  private:\n> >  \tfriend class Camera;\n> > +\tfriend class PipelineHandler;\n> >\n> >  \tint prepare();\n> >  \tvoid complete(Status status);\n> > -\tvoid bufferCompleted(Buffer *buffer);\n> > +\n> > +\tbool completeBuffer(Buffer *buffer);\n> >\n> >  \tCamera *camera_;\n> >  \tstd::map<Stream *, Buffer *> bufferMap_;\n> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > index 524eb47d4364..e2d1cf04411e 100644\n> > --- a/src/libcamera/buffer.cpp\n> > +++ b/src/libcamera/buffer.cpp\n> > @@ -212,11 +212,6 @@ Buffer::Buffer()\n> >   * \\return A reference to a vector holding all Planes within the buffer\n> >   */\n> >\n> > -/**\n> > - * \\var Buffer::completed\n> > - * \\brief A Signal to provide notifications that the specific Buffer is ready\n> > - */\n> > -\n> >  /**\n> >   * \\fn Buffer::bytesused()\n> >   * \\brief Retrieve the number of bytes occupied by the data in the buffer\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 3789732b95d1..3ef760943ff9 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -98,6 +98,12 @@ const std::string &Camera::name() const\n> >  \treturn name_;\n> >  }\n> >\n> > +/**\n> > + * \\var Camera::bufferCompleted\n> > + * \\brief Signal emitted when a buffer for a request queued to the camera has\n> > + * completed\n> > + */\n> > +\n> >  /**\n> >   * \\var Camera::requestCompleted\n> >   * \\brief Signal emitted when a request queued to the camera has completed\n> > @@ -421,4 +427,19 @@ int Camera::exclusiveAccess()\n> >  \treturn 0;\n> >  }\n> >\n> > +/**\n> > + * \\brief Handle request completion and notify application\n> > + * \\param[in] request The request that has completed\n> > + *\n> > + * This function is called by the pipeline handler to notify the camera that\n> > + * the request has completed. It emits the requestCompleted signal and deletes\n> > + * the request.\n> > + */\n> > +void Camera::requestComplete(Request *request)\n> > +{\n> > +\tstd::map<Stream *, Buffer *> buffers(std::move(request->bufferMap_));\n> > +\trequestCompleted.emit(request, buffers);\n> > +\tdelete request;\n> > +}\n> > +\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > index b2b9c94cebdc..cbc953696816 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -7,6 +7,7 @@\n> >  #ifndef __LIBCAMERA_PIPELINE_HANDLER_H__\n> >  #define __LIBCAMERA_PIPELINE_HANDLER_H__\n> >\n> > +#include <list>\n> >  #include <map>\n> >  #include <memory>\n> >  #include <string>\n> > @@ -14,6 +15,7 @@\n> >\n> >  namespace libcamera {\n> >\n> > +class Buffer;\n> >  class BufferPool;\n> >  class Camera;\n> >  class CameraManager;\n> > @@ -35,6 +37,7 @@ public:\n> >\n> >  \tCamera *camera_;\n> >  \tPipelineHandler *pipe_;\n> > +\tstd::list<Request *> queuedRequests_;\n> >\n> >  private:\n> >  \tCameraData(const CameraData &) = delete;\n> > @@ -58,9 +61,12 @@ public:\n> >  \tvirtual int freeBuffers(Camera *camera, Stream *stream) = 0;\n> >\n> >  \tvirtual int start(Camera *camera) = 0;\n> > -\tvirtual void stop(Camera *camera) = 0;\n> > +\tvirtual void stop(Camera *camera);\n> >\n> > -\tvirtual int queueRequest(Camera *camera, Request *request) = 0;\n> > +\tvirtual int queueRequest(Camera *camera, Request *request);\n> > +\n> > +\tbool completeBuffer(Camera *camera, Request *request, Buffer *buffer);\n> > +\tvoid completeRequest(Camera *camera, Request *request);\n> >\n> >  protected:\n> >  \tvoid registerCamera(std::shared_ptr<Camera> camera,\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 776b7f07f78d..4695ec99470e 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -63,6 +63,8 @@ private:\n> >  \t\t\tdelete sensor_;\n> >  \t\t}\n> >\n> > +\t\tvoid bufferReady(Buffer *buffer);\n> > +\n> >  \t\tV4L2Device *cio2_;\n> >  \t\tV4L2Subdevice *csi2_;\n> >  \t\tV4L2Subdevice *sensor_;\n> > @@ -236,6 +238,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> >\n> >  \tif (data->cio2_->streamOff())\n> >  \t\tLOG(IPU3, Info) << \"Failed to stop camera \" << camera->name();\n> > +\n> > +\tPipelineHandler::stop(camera);\n> >  }\n> >\n> >  int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> > @@ -250,7 +254,11 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\n> >  \t\treturn -ENOENT;\n> >  \t}\n> >\n> > -\tdata->cio2_->queueBuffer(buffer);\n> > +\tint ret = data->cio2_->queueBuffer(buffer);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tPipelineHandler::queueRequest(camera, request);\n> >\n> >  \treturn 0;\n> >  }\n> > @@ -417,6 +425,14 @@ void PipelineHandlerIPU3::registerCameras()\n> >  \t}\n> >  }\n> >\n> > +void PipelineHandlerIPU3::IPU3CameraData::bufferReady(Buffer *buffer)\n> > +{\n> > +\tRequest *request = queuedRequests_.front();\n> > +\n> > +\tpipe_->completeBuffer(camera_, request, buffer);\n> > +\tpipe_->completeRequest(camera_, request);\n> > +}\n> > +\n> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index f121d3c5633e..29c0c25ae7a8 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -56,6 +56,8 @@ private:\n> >  \t\t\tdelete video_;\n> >  \t\t}\n> >\n> > +\t\tvoid bufferReady(Buffer *buffer);\n> > +\n> >  \t\tV4L2Device *video_;\n> >  \t\tStream stream_;\n> >  \t};\n> > @@ -153,6 +155,7 @@ void PipelineHandlerUVC::stop(Camera *camera)\n> >  {\n> >  \tUVCCameraData *data = cameraData(camera);\n> >  \tdata->video_->streamOff();\n> > +\tPipelineHandler::stop(camera);\n> >  }\n> >\n> >  int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> > @@ -166,7 +169,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)\n> >  \t\treturn -ENOENT;\n> >  \t}\n> >\n> > -\tdata->video_->queueBuffer(buffer);\n> > +\tint ret = data->video_->queueBuffer(buffer);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tPipelineHandler::queueRequest(camera, request);\n> >\n> >  \treturn 0;\n> >  }\n> > @@ -198,6 +205,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> >  \t\treturn false;\n> >  \t}\n> >\n> > +\tdata->video_->bufferReady.connect(data.get(), &UVCCameraData::bufferReady);\n> > +\n> >  \t/* Create and register the camera. */\n> >  \tstd::vector<Stream *> streams{ &data->stream_ };\n> >  \tstd::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams);\n> > @@ -209,6 +218,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> >  \treturn true;\n> >  }\n> >\n> > +void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer)\n> > +{\n> > +\tRequest *request = queuedRequests_.front();\n> > +\n> > +\tpipe_->completeBuffer(camera_, request, buffer);\n> > +\tpipe_->completeRequest(camera_, request);\n> > +}\n> > +\n> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index 6d022c37eb9f..8c7c2d05828f 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -56,6 +56,8 @@ private:\n> >  \t\t\tdelete video_;\n> >  \t\t}\n> >\n> > +\t\tvoid bufferReady(Buffer *buffer);\n> > +\n> >  \t\tV4L2Device *video_;\n> >  \t\tStream stream_;\n> >  \t};\n> > @@ -153,6 +155,7 @@ void PipelineHandlerVimc::stop(Camera *camera)\n> >  {\n> >  \tVimcCameraData *data = cameraData(camera);\n> >  \tdata->video_->streamOff();\n> > +\tPipelineHandler::stop(camera);\n> >  }\n> >\n> >  int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n> > @@ -166,7 +169,11 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)\n> >  \t\treturn -ENOENT;\n> >  \t}\n> >\n> > -\tdata->video_->queueBuffer(buffer);\n> > +\tint ret = data->video_->queueBuffer(buffer);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tPipelineHandler::queueRequest(camera, request);\n> >\n> >  \treturn 0;\n> >  }\n> > @@ -205,6 +212,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> >  \tif (data->video_->open())\n> >  \t\treturn false;\n> >\n> > +\tdata->video_->bufferReady.connect(data.get(), &VimcCameraData::bufferReady);\n> > +\n> >  \t/* Create and register the camera. */\n> >  \tstd::vector<Stream *> streams{ &data->stream_ };\n> >  \tstd::shared_ptr<Camera> camera = Camera::create(this, \"VIMC Sensor B\",\n> > @@ -214,6 +223,14 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> >  \treturn true;\n> >  }\n> >\n> > +void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer)\n> > +{\n> > +\tRequest *request = queuedRequests_.front();\n> > +\n> > +\tpipe_->completeBuffer(camera_, request, buffer);\n> > +\tpipe_->completeRequest(camera_, request);\n> > +}\n> > +\n> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index 54f237942a48..1a858f2638ce 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -5,6 +5,7 @@\n> >   * pipeline_handler.cpp - Pipeline handler infrastructure\n> >   */\n> >\n> > +#include <libcamera/buffer.h>\n> >  #include <libcamera/camera.h>\n> >  #include <libcamera/camera_manager.h>\n> >\n> > @@ -74,6 +75,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> >   * and remains valid until the instance is destroyed.\n> >   */\n> >\n> > +/**\n> > + * \\var CameraData::queuedRequests_\n> > + * \\brief The list of queued and not yet completed request\n> > + *\n> > + * The list of queued request is used to track requests queued in order to\n> > + * ensure completion of all requests when the pipeline handler is stopped.\n> > + *\n> > + * \\sa PipelineHandler::queueRequest(), PipelineHandler::stop(),\n> > + * PipelineHandler::completeRequest()\n> > + */\n> > +\n> >  /**\n> >   * \\class PipelineHandler\n> >   * \\brief Create and manage cameras based on a set of media devices\n> > @@ -226,8 +238,30 @@ PipelineHandler::~PipelineHandler()\n> >   * This method stops capturing and processing requests immediately. All pending\n> >   * requests are cancelled and complete immediately in an error state.\n> >   *\n> > - * \\todo Complete the pending requests immediately\n> > + * Pipeline handlers shall override this method to stop the pipeline, ensure\n> > + * that all pending request completion signaled through completeRequest() have\n> > + * returned, and call the base implementation of the stop() method as the last\n> > + * step of their implementation. The base implementation cancels all requests\n> > + * queued but not yet complete.\n> >   */\n> > +void PipelineHandler::stop(Camera *camera)\n> > +{\n> > +\tCameraData *data = cameraData(camera);\n> > +\n> > +\twhile (!data->queuedRequests_.empty()) {\n> > +\t\tRequest *request = data->queuedRequests_.front();\n> > +\t\tdata->queuedRequests_.pop_front();\n> > +\n> > +\t\twhile (!request->pending_.empty()) {\n> > +\t\t\tBuffer *buffer = *request->pending_.begin();\n> > +\t\t\tbuffer->cancel();\n> > +\t\t\tcompleteBuffer(camera, request, buffer);\n> > +\t\t}\n> > +\n> > +\t\trequest->complete(Request::RequestCancelled);\n> > +\t\tcamera->requestComplete(request);\n> > +\t}\n> > +}\n> >\n> >  /**\n> >   * \\fn PipelineHandler::queueRequest()\n> > @@ -241,8 +275,65 @@ PipelineHandler::~PipelineHandler()\n> >   * parameters will be applied to the frames captured in the buffers provided in\n> >   * the request.\n> >   *\n> > + * Pipeline handlers shall override this method. The base implementation in the\n> > + * PipelineHandler class keeps track of queued requests in order to ensure\n> > + * completion of all requests when the pipeline handler is stopped with stop().\n> > + * Requests completion shall be signaled by the pipeline handler using the\n> > + * completeRequest() method.\n> > + *\n> >   * \\return 0 on success or a negative error code otherwise\n> >   */\n> > +int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> > +{\n> > +\tCameraData *data = cameraData(camera);\n> > +\tdata->queuedRequests_.push_back(request);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Complete a buffer for a request\n> > + * \\param[in] camera The camera the request belongs to\n> > + * \\param[in] request The request the buffer belongs to\n> > + * \\param[in] buffer The buffer that has completed\n> > + *\n> > + * This method shall be called by pipeline handlers to signal completion of the\n> > + * \\a buffer part of the \\a request. It notifies applications of buffer\n> > + * completion and updates the request's internal buffer tracking. The request\n> > + * is not completed automatically when the last buffer completes, pipeline\n> > + * handlers shall complete requests explicitly with completeRequest().\n> > + *\n> > + * \\return True if all buffers contained in the request have completed, false\n> > + * otherwise\n> > + */\n> > +bool PipelineHandler::completeBuffer(Camera *camera, Request *request,\n> > +\t\t\t\t     Buffer *buffer)\n> > +{\n> > +\tcamera->bufferCompleted.emit(request, buffer);\n> > +\treturn request->completeBuffer(buffer);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Signal request completion\n> > + * \\param[in] camera The camera that the request belongs to\n> > + * \\param[in] request The request that has completed\n> > + *\n> > + * The pipeline handler shall call this method to notify the \\a camera that the\n> > + * request request has complete. The request is deleted and shall not be\n> > + * accessed once this method returns.\n> > + *\n> > + * The pipeline handler shall ensure that requests complete in the same order\n> > + * they are submitted.\n> > + */\n> > +void PipelineHandler::completeRequest(Camera *camera, Request *request)\n> > +{\n> > +\tCameraData *data = cameraData(camera);\n> > +\tASSERT(request == data->queuedRequests_.front());\n> > +\tdata->queuedRequests_.pop_front();\n> > +\n> > +\trequest->complete(Request::RequestComplete);\n> > +\tcamera->requestComplete(request);\n> > +}\n> >\n> >  /**\n> >   * \\brief Register a camera to the camera manager and pipeline handler\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index cb170930fbb6..e0e77e972411 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -113,7 +113,6 @@ int Request::prepare()\n> >  {\n> >  \tfor (auto const &pair : bufferMap_) {\n> >  \t\tBuffer *buffer = pair.second;\n> > -\t\tbuffer->completed.connect(this, &Request::bufferCompleted);\n> >  \t\tpending_.insert(buffer);\n> >  \t}\n> >\n> > @@ -133,31 +132,24 @@ void Request::complete(Status status)\n> >  }\n> >\n> >  /**\n> > - * \\brief Slot for the buffer completed signal\n> > + * \\brief Complete a buffer for the request\n> > + * \\param[in] buffer The buffer that has completed\n> >   *\n> > - * The bufferCompleted method serves as slot where to connect the\n> > - * Buffer::completed signal that is emitted when a buffer has available\n> > - * data.\n> > + * A request tracks the status of all buffers it contains through a set of\n> > + * pending buffers. This function removes the \\a buffer from the set to mark it\n> > + * as complete. All buffers associate with the request shall be marked as\n> > + * complete by calling this function once and once only before reporting the\n> > + * request as complete with the complete() method.\n> >   *\n> > - * The request completes when all the buffers it contains are ready to be\n> > - * presented to the application. It then emits the Camera::requestCompleted\n> > - * signal and is automatically deleted.\n> > + * \\return True if all buffers contained in the request have completed, false\n> > + * otherwise\n> >   */\n> > -void Request::bufferCompleted(Buffer *buffer)\n> > +bool Request::completeBuffer(Buffer *buffer)\n> >  {\n> > -\tbuffer->completed.disconnect(this, &Request::bufferCompleted);\n> > -\n> >  \tint ret = pending_.erase(buffer);\n> >  \tASSERT(ret == 1);\n> \n> This was probably in a previous patch, but this\n> https://en.cppreference.com/w/cpp/container/unordered_set/erase\n> doesn't mention anything on the return value.\n\nDoesn't it ?\n\nReturn value\n[...]\n3) Number of elements removed.\n\n> > -\tif (!pending_.empty())\n> > -\t\treturn;\n> > -\n> > -\tcomplete(RequestComplete);\n> > -\n> > -\tstd::map<Stream *, Buffer *> buffers(std::move(bufferMap_));\n> > -\tcamera_->requestCompleted.emit(this, buffers);\n> > -\tdelete this;\n> > +\treturn pending_.empty();\n> >  }\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 054499e4b888..52167a3e047f 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -819,9 +819,6 @@ void V4L2Device::bufferAvailable(EventNotifier *notifier)\n> >\n> >  \t/* Notify anyone listening to the device. */\n> >  \tbufferReady.emit(buffer);\n> > -\n> > -\t/* Notify anyone listening to the buffer specifically. */\n> > -\tbuffer->completed.emit(buffer);\n> >  }\n> >\n> >  /**","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C437610BF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  1 Mar 2019 19:46:14 +0100 (CET)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 006DB49;\n\tFri,  1 Mar 2019 19:46:12 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551465973;\n\tbh=iVVgmP/R5+I8XmvkpntxmSJlh1yndjfg9ToQjwJk7zs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=g2xul8X89bvqJa/Pwv/QtddxYZ3xxdwe2/8hgtG3vnwJSfU6T81zgV69gQa9yNjUm\n\tbxQmIAss0tdpwKWvRyV5sbpcRgcUdnCdkq82+OGUOHJ6dLdJcFDmsBFnApa8iLnXB6\n\tgkKs46ecDZUohDTyLTHt9sTqtva6FjIMrdR9A/60=","Date":"Fri, 1 Mar 2019 20:46:07 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190301184607.GG32244@pendragon.ideasonboard.com>","References":"<20190228162913.6508-1-laurent.pinchart@ideasonboard.com>\n\t<20190228162913.6508-10-laurent.pinchart@ideasonboard.com>\n\t<20190301150947.eccf6iql5pkgfj2o@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190301150947.eccf6iql5pkgfj2o@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 09/10] libcamera: Handle request\n\tcompletion explicitly in pipeline handlers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 01 Mar 2019 18:46:14 -0000"}}]