[libcamera-devel] [PATCH 09/10] libcamera: Handle request completion explicitly in pipeline handlers
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Mar 1 19:46:07 CET 2019
Hi Jacopo,
On Fri, Mar 01, 2019 at 04:09:47PM +0100, Jacopo Mondi wrote:
> On Thu, Feb 28, 2019 at 06:29:12PM +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.
>
> The only punctual comment I have is that the bufferReady signal is not
> connected in the ipu3.cpp file.
Thank you for catching this. I'll fix it.
> You've heard enough of my concerns and I've heard enough of your
> reasons, so the architecture is fine for the moment.
>
> A few more question below though.
>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > 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;
>
> Is this reserved for applications as well? I don't see it used in the library
> code, am I wrong?
Correct. The Buffer class provided the ability for applications to get
notified of buffer completion events, so I wanted to retain that. As
it's unused I'm also fine dropping it, I assume the API will be reworked
anyway.
> > 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);
>
> This was probably in a previous patch, but this
> https://en.cppreference.com/w/cpp/container/unordered_set/erase
> doesn't mention anything on the return value.
Doesn't it ?
Return value
[...]
3) Number of elements removed.
> > - 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
More information about the libcamera-devel
mailing list