summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorUmang Jain <email@uajain.com>2020-06-16 19:45:33 +0000
committerLaurent Pinchart <laurent.pinchart@ideasonboard.com>2020-06-17 00:22:01 +0300
commite34d3a4f278ec8491ce3b710e989fb2083af92f3 (patch)
tree645857e65ebb6fa1f65458a49e062ffd0577fb52
parentbf650cf3fc43ea8d198d14612af6db4b45ef4b68 (diff)
libcamera: CameraManager: Drop the vector of created PipelineHandlers
The pipes_ vector was initially used to store pipeline handlers instances with the CameraManager when it cannot be referenced from anywhere else. It was used to retrieve cameras and deleting pipeline handlers when stopping the camera manager. In f3695e9b09ce ("libcamera: camera_manager: Register cameras with the camera manager"), cameras started to get registered directly with camera manager and in 5b02e03199b7 ("libcamera: camera: Associate cameras with their pipeline handler") pipeline handlers started to get stored in a std::shared_ptr<> with each camera starting to hold a strong reference to its associated pipeline-handler. At this point, both the camera manager and the camera held a strong reference to the pipeline handler. Since the additional reference held by the camera manager gets released only on cleanup(), this lurking reference held on pipeline handler did not allow it to get destroyed even when cameras instances have been destroyed. This situation of having a pipeline handler instance around without having a camera may lead to problems (one of them explained below) especially when the camera manager is still running. It was noticed that, there was a dangling driver directory issue (tested for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on 'unbind' → 'bind' operation while the CameraManager is running. The directories were still kept around even after 'unbind' because of the lurking reference of pipeline handler holding onto them. That reference would clear if and only if the CameraManager is stopped and then only directories were getting removed in the above stated path. Rather than writing a fix to release the pipeline handlers' reference from camera manager on camera disconnection, it is decided to eliminate the pipes_ vector from CameraManager moving forwards. There is no point in holding a reference to it from camera manager's point-of-view at this stage. It also helps us to fix the issue as explained above. Now that the pipeline handler instances are referenced via cameras only, it can happen that the destruction of last the camera instance may result in destruction of the pipeline handler itself. Such a possibility exists in PipelineHandler::disconnect(), where the pipeline handler itself can get destroyed while removing the camera. This is acceptable as long as we make sure that there is no access of pipeline handler's members later on in the code path. Address this situation and also add a detailed comment about it. Signed-off-by: Umang Jain <email@uajain.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
-rw-r--r--src/libcamera/camera_manager.cpp8
-rw-r--r--src/libcamera/pipeline_handler.cpp18
2 files changed, 17 insertions, 9 deletions
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index 576856ab..dbdc78e7 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -63,7 +63,6 @@ private:
bool initialized_;
int status_;
- std::vector<std::shared_ptr<PipelineHandler>> pipes_;
std::unique_ptr<DeviceEnumerator> enumerator_;
IPAManager ipaManager_;
@@ -144,7 +143,6 @@ int CameraManager::Private::init()
LOG(Camera, Debug)
<< "Pipeline handler \"" << factory->name()
<< "\" matched";
- pipes_.push_back(std::move(pipe));
}
}
@@ -158,11 +156,9 @@ void CameraManager::Private::cleanup()
/* TODO: unregister hot-plug callback here */
/*
- * Release all references to cameras and pipeline handlers to ensure
- * they all get destroyed before the device enumerator deletes the
- * media devices.
+ * Release all references to cameras to ensure they all get destroyed
+ * before the device enumerator deletes the media devices.
*/
- pipes_.clear();
cameras_.clear();
enumerator_.reset(nullptr);
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index a0f6b0f1..38c85779 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -559,7 +559,21 @@ void PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)
*/
void PipelineHandler::disconnect()
{
- for (std::weak_ptr<Camera> ptr : cameras_) {
+ /*
+ * Each camera holds a reference to its associated pipeline handler
+ * instance. Hence, when the last camera is dropped, the pipeline
+ * handler will get destroyed by the last manager_->removeCamera(camera)
+ * call in the loop below.
+ *
+ * This is acceptable as long as we make sure that the code path does not
+ * access any member of the (already destroyed) pipeline handler instance
+ * afterwards. Therefore, we move the cameras_ vector to a local temporary
+ * container to avoid accessing freed memory later i.e. to explicitly run
+ * cameras_.clear().
+ */
+ std::vector<std::weak_ptr<Camera>> cameras{ std::move(cameras_) };
+
+ for (std::weak_ptr<Camera> ptr : cameras) {
std::shared_ptr<Camera> camera = ptr.lock();
if (!camera)
continue;
@@ -567,8 +581,6 @@ void PipelineHandler::disconnect()
camera->disconnect();
manager_->removeCamera(camera.get());
}
-
- cameras_.clear();
}
/**