[v1] Documentation: guides: application-developer: Fix variable shadowing
diff mbox series

Message ID 20250130074712.959046-1-pobrn@protonmail.com
State Accepted
Headers show
Series
  • [v1] Documentation: guides: application-developer: Fix variable shadowing
Related show

Commit Message

Barnabás Pőcze Jan. 30, 2025, 7:47 a.m. UTC
The mentioned commit mistakenly introduced a new variable for storing
the camera instead of just assigining to the global variable defined
earlier in the tutorial. Fix that by making it an assignment.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=252
Fixes: e77a2751100e38 ("treewide: Query list of cameras just once")
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 Documentation/guides/application-developer.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kieran Bingham Jan. 30, 2025, 9:04 a.m. UTC | #1
Quoting Barnabás Pőcze (2025-01-30 07:47:15)
> The mentioned commit mistakenly introduced a new variable for storing
> the camera instead of just assigining to the global variable defined
> earlier in the tutorial. Fix that by making it an assignment.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=252
> Fixes: e77a2751100e38 ("treewide: Query list of cameras just once")
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>  Documentation/guides/application-developer.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst
> index 67f5bd7e7..297586a1c 100644
> --- a/Documentation/guides/application-developer.rst
> +++ b/Documentation/guides/application-developer.rst
> @@ -128,7 +128,7 @@ available.
>  
>     std::string cameraId = cameras[0]->id();
>  
> -   auto camera = cm->get(cameraId);
> +   camera = cm->get(cameraId);

I always thought we should be providing a request->camera() helper to
return the associated camera with a request to make this easier for
applciations but it didn't seem to get any traction when I posted this: 

 https://patchwork.libcamera.org/patch/9328/

So for this patch indeed this stops the aliased variable.


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


>     /*
>      * Note that `camera` may not compare equal to `cameras[0]`.
>      * In fact, it might simply be a `nullptr`, as the particular
> -- 
> 2.48.1
> 
>
Barnabás Pőcze Jan. 30, 2025, 10:37 a.m. UTC | #2
2025. január 30., csütörtök 10:04 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:

> Quoting Barnabás Pőcze (2025-01-30 07:47:15)
> > The mentioned commit mistakenly introduced a new variable for storing
> > the camera instead of just assigining to the global variable defined
> > earlier in the tutorial. Fix that by making it an assignment.
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=252
> > Fixes: e77a2751100e38 ("treewide: Query list of cameras just once")
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > ---
> >  Documentation/guides/application-developer.rst | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst
> > index 67f5bd7e7..297586a1c 100644
> > --- a/Documentation/guides/application-developer.rst
> > +++ b/Documentation/guides/application-developer.rst
> > @@ -128,7 +128,7 @@ available.
> >
> >     std::string cameraId = cameras[0]->id();
> >
> > -   auto camera = cm->get(cameraId);
> > +   camera = cm->get(cameraId);
> 
> I always thought we should be providing a request->camera() helper to
> return the associated camera with a request to make this easier for
> applciations but it didn't seem to get any traction when I posted this:
> 
>  https://patchwork.libcamera.org/patch/9328/

I see the camera pointer has been moved to `Request::Private`, but I think
this still seems like a good idea. Although I have no idea what the mentioned
plans with `Request` were.


Regards,
Barnabás Pőcze

> 
> So for this patch indeed this stops the aliased variable.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
> >     /*
> >      * Note that `camera` may not compare equal to `cameras[0]`.
> >      * In fact, it might simply be a `nullptr`, as the particular
> > --
> > 2.48.1
> >
> >
>
Kieran Bingham Jan. 30, 2025, 1:05 p.m. UTC | #3
Quoting Barnabás Pőcze (2025-01-30 10:37:22)
> 2025. január 30., csütörtök 10:04 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> 
> > Quoting Barnabás Pőcze (2025-01-30 07:47:15)
> > > The mentioned commit mistakenly introduced a new variable for storing
> > > the camera instead of just assigining to the global variable defined
> > > earlier in the tutorial. Fix that by making it an assignment.
> > >
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=252
> > > Fixes: e77a2751100e38 ("treewide: Query list of cameras just once")
> > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > > ---
> > >  Documentation/guides/application-developer.rst | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst
> > > index 67f5bd7e7..297586a1c 100644
> > > --- a/Documentation/guides/application-developer.rst
> > > +++ b/Documentation/guides/application-developer.rst
> > > @@ -128,7 +128,7 @@ available.
> > >
> > >     std::string cameraId = cameras[0]->id();
> > >
> > > -   auto camera = cm->get(cameraId);
> > > +   camera = cm->get(cameraId);
> > 
> > I always thought we should be providing a request->camera() helper to
> > return the associated camera with a request to make this easier for
> > applciations but it didn't seem to get any traction when I posted this:
> > 
> >  https://patchwork.libcamera.org/patch/9328/
> 
> I see the camera pointer has been moved to `Request::Private`, but I think
> this still seems like a good idea. Although I have no idea what the mentioned
> plans with `Request` were.

I don't know what the resistance context was either, but I still think
it seems like a useful way to access the camera in the async delivery
especially as the camera is already stored /in/ the request and is
guaranteed to be correctly associated.

Maybe it's worth refreshing the patch to see.

--
Kieran

> 
> Regards,
> Barnabás Pőcze
> 
> > 
> > So for this patch indeed this stops the aliased variable.
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > 
> > >     /*
> > >      * Note that `camera` may not compare equal to `cameras[0]`.
> > >      * In fact, it might simply be a `nullptr`, as the particular
> > > --
> > > 2.48.1
> > >
> > >
> >
Laurent Pinchart Jan. 30, 2025, 2:55 p.m. UTC | #4
Hi Barnabás,

Thank you for the patch.

On Thu, Jan 30, 2025 at 07:47:15AM +0000, Barnabás Pőcze wrote:
> The mentioned commit mistakenly introduced a new variable for storing
> the camera instead of just assigining to the global variable defined
> earlier in the tutorial. Fix that by making it an assignment.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=252
> Fixes: e77a2751100e38 ("treewide: Query list of cameras just once")
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  Documentation/guides/application-developer.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst
> index 67f5bd7e7..297586a1c 100644
> --- a/Documentation/guides/application-developer.rst
> +++ b/Documentation/guides/application-developer.rst
> @@ -128,7 +128,7 @@ available.
>  
>     std::string cameraId = cameras[0]->id();
>  
> -   auto camera = cm->get(cameraId);
> +   camera = cm->get(cameraId);
>     /*
>      * Note that `camera` may not compare equal to `cameras[0]`.
>      * In fact, it might simply be a `nullptr`, as the particular

Patch
diff mbox series

diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst
index 67f5bd7e7..297586a1c 100644
--- a/Documentation/guides/application-developer.rst
+++ b/Documentation/guides/application-developer.rst
@@ -128,7 +128,7 @@  available.
 
    std::string cameraId = cameras[0]->id();
 
-   auto camera = cm->get(cameraId);
+   camera = cm->get(cameraId);
    /*
     * Note that `camera` may not compare equal to `cameras[0]`.
     * In fact, it might simply be a `nullptr`, as the particular