[v2] treewide: Query list of cameras just once

19954 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.
---
 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(-)


base-commit: fb74bb7df66b96dbe28702155cddfc96a1b30f78

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

Hi


2024. április 26., péntek 15:11 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:

> 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

Oops indeed.

> 
> > ---
> 
> When sending a new version please append a changelog

Ack.


> 
> >  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 ?

I don't know, I don't really have an opinion about this.


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

Certainly, but that is not an issue as far as I can tell. I can change it to

  if (auto cameras = cm->cameras(); *endptr == '\0' && index > 0 && index <= cameras.size())
    ...

if that is preferred.

Or even

  if (*endptr == '\0' && index > 0) {
    auto cameras = cm->cameras();
    if (index <= cameras.size())
      camera_ = cameras[index - 1];
  }
  if (!camera_)
    camera_ = cm->get(cameraId);

which is very close to the original version in its behaviour I believe.


> 
> >  	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() ?

I have been planning on sending a separate commit that does some of these
long hanging shared pointer related optimizations, so I will remove the move()
in the next version.


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


Regards,
Barnabás Pőcze

Hi Barnabás

On Fri, Apr 26, 2024 at 01:58:44PM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> 2024. április 26., péntek 15:11 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:
>
> > 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
>
> Oops indeed.
>
> >
> > > ---
> >
> > When sending a new version please append a changelog
>
> Ack.
>
>
> >
> > >  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 ?
>
> I don't know, I don't really have an opinion about this.
>
>
> >
> > >
> > >  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())
>
> Certainly, but that is not an issue as far as I can tell. I can change it to

Sorry, that clearly was a typo

I meant "

 The move() doesn't hurt, but `cameras` will get out of scope at the
 end of the function, so this is functionally equivalent (if not
 that the entry in `cameras` is not valid anymore after a move())


What I meant was: move() doesn't really hurt (as long as we don't
access camera[index - 1] later) but as cameras will get out of scope,
at the end of the function the usage count camera_ is the same with
or without move()

>
>   if (auto cameras = cm->cameras(); *endptr == '\0' && index > 0 && index <= cameras.size())
>     ...
>
> if that is preferred.
>
> Or even
>
>   if (*endptr == '\0' && index > 0) {
>     auto cameras = cm->cameras();
>     if (index <= cameras.size())
>       camera_ = cameras[index - 1];
>   }
>   if (!camera_)
>     camera_ = cm->get(cameraId);
>
> which is very close to the original version in its behaviour I believe.
>
>
> >
> > >  	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() ?
>
> I have been planning on sending a separate commit that does some of these
> long hanging shared pointer related optimizations, so I will remove the move()
> in the next version.

That's fine with me! Sorry for the mis-understanding
>
>
> >
> >
> > >  	}
> > >
> > >  	GST_INFO_OBJECT(self, "Using camera '%s'", cam->id().c_str());
> > >
> > > base-commit: fb74bb7df66b96dbe28702155cddfc96a1b30f78
> > > --
> > > 2.44.0
> > >
> > >
> >
>
>
> Regards,
> Barnabás Pőcze