[libcamera-devel] [PATCH v2 07/16] libcamera: v4l2_videodevice: Signal buffer completion at streamoff time
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Jul 14 12:44:57 CEST 2019
Hi Laurent,
Thanks for your patch.
On 2019-07-13 20:23:42 +0300, Laurent Pinchart wrote:
> When stopping the stream buffers have been queued, in which case their
> completion is never be notified to the user. This can lead to memory
> leaks. Fix it by notifying completion of all queued buffers with the
> status set to error.
>
> As a result the base PipelineHandler implementation can be simplified,
> as all requests complete as the result of stopping the stream. The
> stop() method that manually completes all queued requests isn't needed
> anymore.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> include/libcamera/request.h | 3 ++-
> src/libcamera/include/pipeline_handler.h | 2 +-
> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 --
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 --
> src/libcamera/pipeline/uvcvideo.cpp | 1 -
> src/libcamera/pipeline/vimc.cpp | 1 -
> src/libcamera/pipeline_handler.cpp | 29 +++---------------------
> src/libcamera/request.cpp | 14 ++++++++----
> src/libcamera/v4l2_videodevice.cpp | 14 +++++++++++-
> test/v4l2_videodevice/buffer_sharing.cpp | 6 +++++
> 10 files changed, 34 insertions(+), 40 deletions(-)
>
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 7a60be617645..00c68345b5b4 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -51,7 +51,7 @@ private:
> friend class PipelineHandler;
>
> int prepare();
> - void complete(Status status);
> + void complete();
>
> bool completeBuffer(Buffer *buffer);
>
> @@ -62,6 +62,7 @@ private:
>
> uint64_t cookie_;
> Status status_;
> + bool cancelled_;
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index f836d5d1a600..1fdef9cea40f 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -74,7 +74,7 @@ public:
> const std::set<Stream *> &streams) = 0;
>
> virtual int start(Camera *camera) = 0;
> - virtual void stop(Camera *camera);
> + virtual void stop(Camera *camera) = 0;
>
> virtual int queueRequest(Camera *camera, Request *request);
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 50457891e288..ffc066a15ae9 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -711,8 +711,6 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> if (ret)
> LOG(IPU3, Warning) << "Failed to stop camera "
> << camera->name();
> -
> - PipelineHandler::stop(camera);
> }
>
> int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 383236435a7f..cc33a2cb2572 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -354,8 +354,6 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
> LOG(RkISP1, Warning)
> << "Failed to stop camera " << camera->name();
>
> - PipelineHandler::stop(camera);
> -
> activeCamera_ = nullptr;
> }
>
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 387d71ef567c..b299c5da8471 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -220,7 +220,6 @@ void PipelineHandlerUVC::stop(Camera *camera)
> {
> UVCCameraData *data = cameraData(camera);
> data->video_->streamOff();
> - PipelineHandler::stop(camera);
> }
>
> int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index ff2529c974fd..7d96c3645c06 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -222,7 +222,6 @@ void PipelineHandlerVimc::stop(Camera *camera)
> {
> VimcCameraData *data = cameraData(camera);
> data->video_->streamOff();
> - PipelineHandler::stop(camera);
> }
>
> int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index c609200252f1..83938a71f615 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -328,31 +328,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
> *
> * This method stops capturing and processing requests immediately. All pending
> * requests are cancelled and complete immediately in an error state.
> - *
> - * 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()
> @@ -420,15 +396,16 @@ bool PipelineHandler::completeBuffer(Camera *camera, Request *request,
> */
> void PipelineHandler::completeRequest(Camera *camera, Request *request)
> {
> - request->complete(Request::RequestComplete);
> + request->complete();
>
> CameraData *data = cameraData(camera);
>
> while (!data->queuedRequests_.empty()) {
> request = data->queuedRequests_.front();
> - if (request->hasPendingBuffers())
> + if (request->status() == Request::RequestPending)
> break;
>
> + ASSERT(!request->hasPendingBuffers());
> data->queuedRequests_.pop_front();
> camera->requestComplete(request);
> }
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 45e7133febb0..19131472710b 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -56,7 +56,7 @@ LOG_DEFINE_CATEGORY(Request)
> */
> Request::Request(Camera *camera, uint64_t cookie)
> : camera_(camera), controls_(camera), cookie_(cookie),
> - status_(RequestPending)
> + status_(RequestPending), cancelled_(false)
> {
> }
>
> @@ -199,14 +199,15 @@ int Request::prepare()
>
> /**
> * \brief Complete a queued request
> - * \param[in] status The request completion status
> *
> - * Mark the request as complete by updating its status to \a status.
> + * Mark the request as complete by updating its status to RequestComplete,
> + * unless buffers have been cancelled in which case the status is set to
> + * RequestCancelled.
> */
> -void Request::complete(Status status)
> +void Request::complete()
> {
> ASSERT(!hasPendingBuffers());
> - status_ = status;
> + status_ = cancelled_ ? RequestCancelled : RequestComplete;
> }
>
> /**
> @@ -229,6 +230,9 @@ bool Request::completeBuffer(Buffer *buffer)
>
> buffer->setRequest(nullptr);
>
> + if (buffer->status() == Buffer::BufferCancelled)
> + cancelled_ = true;
> +
> return !hasPendingBuffers();
> }
>
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 65b4098abc05..aa3136378997 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1090,7 +1090,9 @@ int V4L2VideoDevice::streamOn()
> * \brief Stop the video stream
> *
> * Buffers that are still queued when the video stream is stopped are
> - * implicitly dequeued, but no bufferReady signal is emitted for them.
> + * immediately dequeued with their status set to Buffer::BufferError,
> + * and the bufferReady signal is emitted for them. The order in which those
> + * buffers are dequeued is not specified.
> *
> * \return 0 on success or a negative error code otherwise
> */
> @@ -1105,6 +1107,16 @@ int V4L2VideoDevice::streamOff()
> return ret;
> }
>
> + /* Send back all queued buffers. */
> + for (auto it : queuedBuffers_) {
> + unsigned int index = it.first;
> + Buffer *buffer = it.second;
> +
> + buffer->index_ = index;
> + buffer->cancel();
> + bufferReady.emit(buffer);
> + }
> +
> queuedBuffers_.clear();
> fdEvent_->setEnabled(false);
>
> diff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp
> index 459bd238fe97..4da6ba466f40 100644
> --- a/test/v4l2_videodevice/buffer_sharing.cpp
> +++ b/test/v4l2_videodevice/buffer_sharing.cpp
> @@ -95,6 +95,9 @@ protected:
> std::cout << "Received capture buffer: " << buffer->index()
> << " sequence " << buffer->sequence() << std::endl;
>
> + if (buffer->status() != Buffer::BufferSuccess)
> + return;
> +
> output_->queueBuffer(buffer);
> framesCaptured_++;
> }
> @@ -104,6 +107,9 @@ protected:
> std::cout << "Received output buffer: " << buffer->index()
> << " sequence " << buffer->sequence() << std::endl;
>
> + if (buffer->status() != Buffer::BufferSuccess)
> + return;
> +
> capture_->queueBuffer(buffer);
> framesOutput_++;
> }
> --
> 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