[libcamera-devel] [PATCH 09/10] libcamera: Handle request completion explicitly in pipeline handlers
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri Mar 1 00:28:23 CET 2019
Hi Laurent,
Thanks for your hard work.
On 2019-02-28 18:29:12 +0200, Laurent Pinchart wrote:
> Request complete by themselves when all the buffers they contain have
> completed, connecting the buffer's completed signal to be notified of
> buffer completion. While this works for now, it prevents pipelines from
> delaying request completion until all metadata is available, and makes
> it impossible to ensure that requests complete in the order they are
> queued.
>
> To fix this, make request completion handling explicit in pipeline
> handlers. The base PipelineHandler class is extended with
> implementations of the queueRequest() and stop() methods and gets new
> completeBuffer() and completeRequest() methods to help pipeline handlers
> tracking requests and buffers.
>
> The three existing pipeline handlers connect the bufferReady signal of
> their capture video node to a slot of their respective camera data
> instance, where they use the PipelineHandler helpers to notify buffer
> and request completion. Request completion is handled synchronously with
> buffer completion as the pipeline handlers don't need to support more
> advanced use cases, but this paves the road for future work.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
I was a bit afraid of this change at first, but provided I managed to
grasp all the fine details I like the end result, nice work.
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> include/libcamera/buffer.h | 5 +-
> include/libcamera/camera.h | 3 +
> include/libcamera/request.h | 4 +-
> src/libcamera/buffer.cpp | 5 --
> src/libcamera/camera.cpp | 21 ++++++
> src/libcamera/include/pipeline_handler.h | 10 ++-
> src/libcamera/pipeline/ipu3/ipu3.cpp | 18 ++++-
> src/libcamera/pipeline/uvcvideo.cpp | 19 ++++-
> src/libcamera/pipeline/vimc.cpp | 19 ++++-
> src/libcamera/pipeline_handler.cpp | 93 +++++++++++++++++++++++-
> src/libcamera/request.cpp | 30 +++-----
> src/libcamera/v4l2_device.cpp | 3 -
> 12 files changed, 192 insertions(+), 38 deletions(-)
>
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index f740ade9bb4f..0c844d126a27 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -10,8 +10,6 @@
> #include <stdint.h>
> #include <vector>
>
> -#include <libcamera/signal.h>
> -
> namespace libcamera {
>
> class BufferPool;
> @@ -55,10 +53,9 @@ public:
> Status status() const { return status_; }
> std::vector<Plane> &planes() { return planes_; }
>
> - Signal<Buffer *> completed;
> -
> private:
> friend class BufferPool;
> + friend class PipelineHandler;
> friend class V4L2Device;
>
> void cancel();
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index bf70255a6a5e..74eca3d86094 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -34,6 +34,7 @@ public:
>
> const std::string &name() const;
>
> + Signal<Request *, Buffer *> bufferCompleted;
> Signal<Request *, const std::map<Stream *, Buffer *> &> requestCompleted;
> Signal<Camera *> disconnected;
>
> @@ -62,6 +63,8 @@ private:
> void disconnect();
> int exclusiveAccess();
>
> + void requestComplete(Request *request);
> +
> std::shared_ptr<PipelineHandler> pipe_;
> std::string name_;
> std::vector<Stream *> streams_;
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 0b75f9d9bd19..0dbd425115e8 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -39,10 +39,12 @@ public:
>
> private:
> friend class Camera;
> + friend class PipelineHandler;
>
> int prepare();
> void complete(Status status);
> - void bufferCompleted(Buffer *buffer);
> +
> + bool completeBuffer(Buffer *buffer);
>
> Camera *camera_;
> std::map<Stream *, Buffer *> bufferMap_;
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index 524eb47d4364..e2d1cf04411e 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -212,11 +212,6 @@ Buffer::Buffer()
> * \return A reference to a vector holding all Planes within the buffer
> */
>
> -/**
> - * \var Buffer::completed
> - * \brief A Signal to provide notifications that the specific Buffer is ready
> - */
> -
> /**
> * \fn Buffer::bytesused()
> * \brief Retrieve the number of bytes occupied by the data in the buffer
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 3789732b95d1..3ef760943ff9 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -98,6 +98,12 @@ const std::string &Camera::name() const
> return name_;
> }
>
> +/**
> + * \var Camera::bufferCompleted
> + * \brief Signal emitted when a buffer for a request queued to the camera has
> + * completed
> + */
> +
> /**
> * \var Camera::requestCompleted
> * \brief Signal emitted when a request queued to the camera has completed
> @@ -421,4 +427,19 @@ int Camera::exclusiveAccess()
> return 0;
> }
>
> +/**
> + * \brief Handle request completion and notify application
> + * \param[in] request The request that has completed
> + *
> + * This function is called by the pipeline handler to notify the camera that
> + * the request has completed. It emits the requestCompleted signal and deletes
> + * the request.
> + */
> +void Camera::requestComplete(Request *request)
> +{
> + std::map<Stream *, Buffer *> buffers(std::move(request->bufferMap_));
> + requestCompleted.emit(request, buffers);
> + delete request;
> +}
> +
> } /* namespace libcamera */
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index b2b9c94cebdc..cbc953696816 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -7,6 +7,7 @@
> #ifndef __LIBCAMERA_PIPELINE_HANDLER_H__
> #define __LIBCAMERA_PIPELINE_HANDLER_H__
>
> +#include <list>
> #include <map>
> #include <memory>
> #include <string>
> @@ -14,6 +15,7 @@
>
> namespace libcamera {
>
> +class Buffer;
> class BufferPool;
> class Camera;
> class CameraManager;
> @@ -35,6 +37,7 @@ public:
>
> Camera *camera_;
> PipelineHandler *pipe_;
> + std::list<Request *> queuedRequests_;
>
> private:
> CameraData(const CameraData &) = delete;
> @@ -58,9 +61,12 @@ public:
> virtual int freeBuffers(Camera *camera, Stream *stream) = 0;
>
> virtual int start(Camera *camera) = 0;
> - virtual void stop(Camera *camera) = 0;
> + virtual void stop(Camera *camera);
>
> - virtual int queueRequest(Camera *camera, Request *request) = 0;
> + virtual int queueRequest(Camera *camera, Request *request);
> +
> + bool completeBuffer(Camera *camera, Request *request, Buffer *buffer);
> + void completeRequest(Camera *camera, Request *request);
>
> protected:
> void registerCamera(std::shared_ptr<Camera> camera,
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 776b7f07f78d..4695ec99470e 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -63,6 +63,8 @@ private:
> delete sensor_;
> }
>
> + void bufferReady(Buffer *buffer);
> +
> V4L2Device *cio2_;
> V4L2Subdevice *csi2_;
> V4L2Subdevice *sensor_;
> @@ -236,6 +238,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>
> if (data->cio2_->streamOff())
> LOG(IPU3, Info) << "Failed to stop camera " << camera->name();
> +
> + PipelineHandler::stop(camera);
> }
>
> int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> @@ -250,7 +254,11 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> return -ENOENT;
> }
>
> - data->cio2_->queueBuffer(buffer);
> + int ret = data->cio2_->queueBuffer(buffer);
> + if (ret < 0)
> + return ret;
> +
> + PipelineHandler::queueRequest(camera, request);
>
> return 0;
> }
> @@ -417,6 +425,14 @@ void PipelineHandlerIPU3::registerCameras()
> }
> }
>
> +void PipelineHandlerIPU3::IPU3CameraData::bufferReady(Buffer *buffer)
> +{
> + Request *request = queuedRequests_.front();
> +
> + pipe_->completeBuffer(camera_, request, buffer);
> + pipe_->completeRequest(camera_, request);
> +}
> +
> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index f121d3c5633e..29c0c25ae7a8 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -56,6 +56,8 @@ private:
> delete video_;
> }
>
> + void bufferReady(Buffer *buffer);
> +
> V4L2Device *video_;
> Stream stream_;
> };
> @@ -153,6 +155,7 @@ void PipelineHandlerUVC::stop(Camera *camera)
> {
> UVCCameraData *data = cameraData(camera);
> data->video_->streamOff();
> + PipelineHandler::stop(camera);
> }
>
> int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
> @@ -166,7 +169,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
> return -ENOENT;
> }
>
> - data->video_->queueBuffer(buffer);
> + int ret = data->video_->queueBuffer(buffer);
> + if (ret < 0)
> + return ret;
> +
> + PipelineHandler::queueRequest(camera, request);
>
> return 0;
> }
> @@ -198,6 +205,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> return false;
> }
>
> + data->video_->bufferReady.connect(data.get(), &UVCCameraData::bufferReady);
> +
> /* Create and register the camera. */
> std::vector<Stream *> streams{ &data->stream_ };
> std::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams);
> @@ -209,6 +218,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> return true;
> }
>
> +void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer)
> +{
> + Request *request = queuedRequests_.front();
> +
> + pipe_->completeBuffer(camera_, request, buffer);
> + pipe_->completeRequest(camera_, request);
> +}
> +
> REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 6d022c37eb9f..8c7c2d05828f 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -56,6 +56,8 @@ private:
> delete video_;
> }
>
> + void bufferReady(Buffer *buffer);
> +
> V4L2Device *video_;
> Stream stream_;
> };
> @@ -153,6 +155,7 @@ void PipelineHandlerVimc::stop(Camera *camera)
> {
> VimcCameraData *data = cameraData(camera);
> data->video_->streamOff();
> + PipelineHandler::stop(camera);
> }
>
> int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)
> @@ -166,7 +169,11 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)
> return -ENOENT;
> }
>
> - data->video_->queueBuffer(buffer);
> + int ret = data->video_->queueBuffer(buffer);
> + if (ret < 0)
> + return ret;
> +
> + PipelineHandler::queueRequest(camera, request);
>
> return 0;
> }
> @@ -205,6 +212,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> if (data->video_->open())
> return false;
>
> + data->video_->bufferReady.connect(data.get(), &VimcCameraData::bufferReady);
> +
> /* Create and register the camera. */
> std::vector<Stream *> streams{ &data->stream_ };
> std::shared_ptr<Camera> camera = Camera::create(this, "VIMC Sensor B",
> @@ -214,6 +223,14 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> return true;
> }
>
> +void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer)
> +{
> + Request *request = queuedRequests_.front();
> +
> + pipe_->completeBuffer(camera_, request, buffer);
> + pipe_->completeRequest(camera_, request);
> +}
> +
> REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 54f237942a48..1a858f2638ce 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -5,6 +5,7 @@
> * pipeline_handler.cpp - Pipeline handler infrastructure
> */
>
> +#include <libcamera/buffer.h>
> #include <libcamera/camera.h>
> #include <libcamera/camera_manager.h>
>
> @@ -74,6 +75,17 @@ LOG_DEFINE_CATEGORY(Pipeline)
> * and remains valid until the instance is destroyed.
> */
>
> +/**
> + * \var CameraData::queuedRequests_
> + * \brief The list of queued and not yet completed request
> + *
> + * The list of queued request is used to track requests queued in order to
> + * ensure completion of all requests when the pipeline handler is stopped.
> + *
> + * \sa PipelineHandler::queueRequest(), PipelineHandler::stop(),
> + * PipelineHandler::completeRequest()
> + */
> +
> /**
> * \class PipelineHandler
> * \brief Create and manage cameras based on a set of media devices
> @@ -226,8 +238,30 @@ PipelineHandler::~PipelineHandler()
> * This method stops capturing and processing requests immediately. All pending
> * requests are cancelled and complete immediately in an error state.
> *
> - * \todo Complete the pending requests immediately
> + * Pipeline handlers shall override this method to stop the pipeline, ensure
> + * that all pending request completion signaled through completeRequest() have
> + * returned, and call the base implementation of the stop() method as the last
> + * step of their implementation. The base implementation cancels all requests
> + * queued but not yet complete.
> */
> +void PipelineHandler::stop(Camera *camera)
> +{
> + CameraData *data = cameraData(camera);
> +
> + while (!data->queuedRequests_.empty()) {
> + Request *request = data->queuedRequests_.front();
> + data->queuedRequests_.pop_front();
> +
> + while (!request->pending_.empty()) {
> + Buffer *buffer = *request->pending_.begin();
> + buffer->cancel();
> + completeBuffer(camera, request, buffer);
> + }
> +
> + request->complete(Request::RequestCancelled);
> + camera->requestComplete(request);
> + }
> +}
>
> /**
> * \fn PipelineHandler::queueRequest()
> @@ -241,8 +275,65 @@ PipelineHandler::~PipelineHandler()
> * parameters will be applied to the frames captured in the buffers provided in
> * the request.
> *
> + * Pipeline handlers shall override this method. The base implementation in the
> + * PipelineHandler class keeps track of queued requests in order to ensure
> + * completion of all requests when the pipeline handler is stopped with stop().
> + * Requests completion shall be signaled by the pipeline handler using the
> + * completeRequest() method.
> + *
> * \return 0 on success or a negative error code otherwise
> */
> +int PipelineHandler::queueRequest(Camera *camera, Request *request)
> +{
> + CameraData *data = cameraData(camera);
> + data->queuedRequests_.push_back(request);
> +
> + return 0;
> +}
> +
> +/**
> + * \brief Complete a buffer for a request
> + * \param[in] camera The camera the request belongs to
> + * \param[in] request The request the buffer belongs to
> + * \param[in] buffer The buffer that has completed
> + *
> + * This method shall be called by pipeline handlers to signal completion of the
> + * \a buffer part of the \a request. It notifies applications of buffer
> + * completion and updates the request's internal buffer tracking. The request
> + * is not completed automatically when the last buffer completes, pipeline
> + * handlers shall complete requests explicitly with completeRequest().
> + *
> + * \return True if all buffers contained in the request have completed, false
> + * otherwise
> + */
> +bool PipelineHandler::completeBuffer(Camera *camera, Request *request,
> + Buffer *buffer)
> +{
> + camera->bufferCompleted.emit(request, buffer);
> + return request->completeBuffer(buffer);
> +}
> +
> +/**
> + * \brief Signal request completion
> + * \param[in] camera The camera that the request belongs to
> + * \param[in] request The request that has completed
> + *
> + * The pipeline handler shall call this method to notify the \a camera that the
> + * request request has complete. The request is deleted and shall not be
> + * accessed once this method returns.
> + *
> + * The pipeline handler shall ensure that requests complete in the same order
> + * they are submitted.
> + */
> +void PipelineHandler::completeRequest(Camera *camera, Request *request)
> +{
> + CameraData *data = cameraData(camera);
> + ASSERT(request == data->queuedRequests_.front());
> + data->queuedRequests_.pop_front();
> +
> + request->complete(Request::RequestComplete);
> + camera->requestComplete(request);
> +}
>
> /**
> * \brief Register a camera to the camera manager and pipeline handler
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index cb170930fbb6..e0e77e972411 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -113,7 +113,6 @@ int Request::prepare()
> {
> for (auto const &pair : bufferMap_) {
> Buffer *buffer = pair.second;
> - buffer->completed.connect(this, &Request::bufferCompleted);
> pending_.insert(buffer);
> }
>
> @@ -133,31 +132,24 @@ void Request::complete(Status status)
> }
>
> /**
> - * \brief Slot for the buffer completed signal
> + * \brief Complete a buffer for the request
> + * \param[in] buffer The buffer that has completed
> *
> - * The bufferCompleted method serves as slot where to connect the
> - * Buffer::completed signal that is emitted when a buffer has available
> - * data.
> + * A request tracks the status of all buffers it contains through a set of
> + * pending buffers. This function removes the \a buffer from the set to mark it
> + * as complete. All buffers associate with the request shall be marked as
> + * complete by calling this function once and once only before reporting the
> + * request as complete with the complete() method.
> *
> - * The request completes when all the buffers it contains are ready to be
> - * presented to the application. It then emits the Camera::requestCompleted
> - * signal and is automatically deleted.
> + * \return True if all buffers contained in the request have completed, false
> + * otherwise
> */
> -void Request::bufferCompleted(Buffer *buffer)
> +bool Request::completeBuffer(Buffer *buffer)
> {
> - buffer->completed.disconnect(this, &Request::bufferCompleted);
> -
> int ret = pending_.erase(buffer);
> ASSERT(ret == 1);
>
> - if (!pending_.empty())
> - return;
> -
> - complete(RequestComplete);
> -
> - std::map<Stream *, Buffer *> buffers(std::move(bufferMap_));
> - camera_->requestCompleted.emit(this, buffers);
> - delete this;
> + return pending_.empty();
> }
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 054499e4b888..52167a3e047f 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -819,9 +819,6 @@ void V4L2Device::bufferAvailable(EventNotifier *notifier)
>
> /* Notify anyone listening to the device. */
> bufferReady.emit(buffer);
> -
> - /* Notify anyone listening to the buffer specifically. */
> - buffer->completed.emit(buffer);
> }
>
> /**
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list