| Message ID | 20190713172351.25452-8-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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_++; }
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@ideasonboard.com> --- 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(-)