[v3] treewide: Query list of cameras just once
diff mbox series

Message ID 20240430192543.779374-1-pobrn@protonmail.com
State Superseded
Headers show
Series
  • [v3] treewide: Query list of cameras just once
Related show

Commit Message

Barnabás Pőcze April 30, 2024, 7:25 p.m. UTC
This is more efficient since only a single vector will be constructed,
and furthermore, it prevents the TOCTOU issue that might arise when
the list of cameras changes between the two queries.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---

changes in v3
  * drop std::move() from this change
  * extend note in documentation
  * limit scope of `cameras` in `CameraSession::CameraSession()`

changes in v2
  * fix code block in documentation
  * add comment noting that the camera may disappear

---
 Documentation/guides/application-developer.rst | 12 +++++++-----
 src/apps/cam/camera_session.cpp                | 11 ++++++++---
 src/gstreamer/gstlibcamerasrc.cpp              |  5 +++--
 3 files changed, 18 insertions(+), 10 deletions(-)


base-commit: fb74bb7df66b96dbe28702155cddfc96a1b30f78
--
2.44.0

Comments

Barnabás Pőcze April 30, 2024, 7:27 p.m. UTC | #1
2024. április 30., kedd 21:25 keltezéssel, Barnabás Pőcze <pobrn@protonmail.com> írta:

> This is more efficient since only a single vector will be constructed,
> and furthermore, it prevents the TOCTOU issue that might arise when
> the list of cameras changes between the two queries.
> 
> Signed-off-by: Barnabás Pőcze pobrn@protonmail.com
> 
> ---
> 
> changes in v3
> * drop std::move() from this change
> * extend note in documentation
> * limit scope of `cameras` in `CameraSession::CameraSession()`
> 
> changes in v2
> * fix code block in documentation
> * add comment noting that the camera may disappear
> 
> ---
> Documentation/guides/application-developer.rst | 12 +++++++-----
> src/apps/cam/camera_session.cpp | 11 ++++++++---
> src/gstreamer/gstlibcamerasrc.cpp | 5 +++--
> 3 files changed, 18 insertions(+), 10 deletions(-)
> 
> [...]


Sorry, please ignore, I resent it by accident.


Regards,
Barnabás Pőcze

Patch
diff mbox series

diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst
index 9a9905b1..92e2a373 100644
--- a/Documentation/guides/application-developer.rst
+++ b/Documentation/guides/application-developer.rst
@@ -116,19 +116,21 @@  available.

 .. code:: cpp

-   if (cm->cameras().empty()) {
+   auto cameras = cm->cameras();
+   if (cameras.empty()) {
        std::cout << "No cameras were identified on the system."
                  << std::endl;
        cm->stop();
        return EXIT_FAILURE;
    }

-   std::string cameraId = cm->cameras()[0]->id();
-   camera = cm->get(cameraId);
+   std::string cameraId = cameras[0]->id();

+   auto camera = cm->get(cameraId);
    /*
-    * Note that is equivalent to:
-    * camera = cm->cameras()[0];
+    * Note that `camera` may not compare equal to `cameras[0]`.
+    * In fact, it might simply be a `nullptr`, as the particular
+    * device might have disappeared (and reappeared) in the meantime.
     */

 Once a camera has been selected an application needs to acquire an exclusive
diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
index 8447f932..334d2ed8 100644
--- a/src/apps/cam/camera_session.cpp
+++ b/src/apps/cam/camera_session.cpp
@@ -39,9 +39,14 @@  CameraSession::CameraSession(CameraManager *cm,
 {
 	char *endptr;
 	unsigned long index = strtoul(cameraId.c_str(), &endptr, 10);
-	if (*endptr == '\0' && index > 0 && index <= cm->cameras().size())
-		camera_ = cm->cameras()[index - 1];
-	else
+
+	if (*endptr == '\0' && index > 0) {
+		auto cameras = cm->cameras();
+		if (index <= cameras.size())
+			camera_ = cameras[index - 1];
+	}
+
+	if (!camera_)
 		camera_ = cm->get(cameraId);

 	if (!camera_) {
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
index f015c6d2..4eddfa3e 100644
--- a/src/gstreamer/gstlibcamerasrc.cpp
+++ b/src/gstreamer/gstlibcamerasrc.cpp
@@ -385,13 +385,14 @@  gst_libcamera_src_open(GstLibcameraSrc *self)
 			return false;
 		}
 	} else {
-		if (cm->cameras().empty()) {
+		auto cameras = cm->cameras();
+		if (cameras.empty()) {
 			GST_ELEMENT_ERROR(self, RESOURCE, NOT_FOUND,
 					  ("Could not find any supported camera on this system."),
 					  ("libcamera::CameraMananger::cameras() is empty"));
 			return false;
 		}
-		cam = cm->cameras()[0];
+		cam = cameras[0];
 	}

 	GST_INFO_OBJECT(self, "Using camera '%s'", cam->id().c_str());