[v3] treewide: Query list of cameras just once

19964 diff mbox series

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

Hi Barnabás

On Mon, Apr 29, 2024 at 02:24:09PM GMT, 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.
>
> 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(-)
>
> 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);

I guess this is ok even if the window between the two accesses to
cameras() is really tiny and the optimization is relatively minor

>
>  	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];

ack!

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>  	}
>
>  	GST_INFO_OBJECT(self, "Using camera '%s'", cam->id().c_str());
>
> base-commit: fb74bb7df66b96dbe28702155cddfc96a1b30f78
> --
> 2.44.0
>

Quoting Barnabás Pőcze (2024-04-29 15:24:09)
> 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(-)
> 
> 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.
>      */

We should probably do the same corresponding change to simple-cam which
'implements' this application-developer guide too when this is merged.

But as that's a separate repository - it's definitely a separate patch.

>  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_)

Is camera_ already initialised to false/0/null by default? Checking...

It's a shared_ptr, so I think we're good here.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>                 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());
> 
> base-commit: fb74bb7df66b96dbe28702155cddfc96a1b30f78
> --
> 2.44.0
>

Hi


2024. április 29., hétfő 16:24 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.
> 
> [...]


Are there any additional changes I should make?


Regards,
Barnabás Pőcze