[PATCH v2] treewide: Query list of cameras just once
Barnabás Pőcze
pobrn at protonmail.com
Fri Apr 26 15:58:44 CEST 2024
Hi
2024. április 26., péntek 15:11 keltezéssel, Jacopo Mondi <jacopo.mondi at 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
More information about the libcamera-devel
mailing list