[PATCH v2] treewide: Query list of cameras just once
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Fri Apr 26 15:11:48 CEST 2024
Hi Barnabás
On Thu, Apr 25, 2024 at 04:03:19PM +0000, Barnabás Pőcze wrote:
> 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.
Missing signed-off-by
> ---
When sending a new version please append a changelog
> Documentation/guides/application-developer.rst | 11 ++++++-----
> src/apps/cam/camera_session.cpp | 6 ++++--
> src/gstreamer/gstlibcamerasrc.cpp | 5 +++--
> 3 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst
> index 9a9905b1..e09ddfb1 100644
> --- a/Documentation/guides/application-developer.rst
> +++ b/Documentation/guides/application-developer.rst
> @@ -116,20 +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();
>
> /*
> - * Note that is equivalent to:
> - * camera = cm->cameras()[0];
> + * Note that `camera` may be nullptr as the camera
> + * might have disappeared in the meantime.
> */
> + auto camera = cm->get(cameraId);
Should we then check or it is an overkill for a guide ?
>
> Once a camera has been selected an application needs to acquire an exclusive
> lock to it so no other application can use it.
> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> index 8447f932..5afd087f 100644
> --- a/src/apps/cam/camera_session.cpp
> +++ b/src/apps/cam/camera_session.cpp
> @@ -39,8 +39,10 @@ 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];
> + auto cameras = cm->cameras();
> +
> + if (*endptr == '\0' && index > 0 && index <= cameras.size())
> + camera_ = std::move(cameras[index - 1]);
The move() doesn't hurt, but `cameras` will get out of scope at the
end of the function, so this is not functionally equivalent (if not
that the entry in `cameras` is not valid anymore after a move())
> else
> camera_ = cm->get(cameraId);
>
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index f015c6d2..8613d66d 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 = std::move(cameras[0]);
Same here.
Would you prefer to keep the move() ?
> }
>
> GST_INFO_OBJECT(self, "Using camera '%s'", cam->id().c_str());
>
> base-commit: fb74bb7df66b96dbe28702155cddfc96a1b30f78
> --
> 2.44.0
>
>
More information about the libcamera-devel
mailing list