[v1] libcamera: camera: Do not call `generateConfiguration()` synchronously

23892 diff mbox series

Most pipeline handler methods are dispatched in the internal `CameraManager`
thread. `PipelineHandler::generateConfiguration()` was called directly,
however, which likely goes against the expectations, so call it on the
internal thread.

This could not be done before 14618cdd0c80a4 ("libcamera: base: bound_method: Move return value")
because the function returns `std::unique_ptr`, and that type cannot
be copied.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/libcamera/camera.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Quoting Barnabás Pőcze (2025-07-21 13:01:26)
> Most pipeline handler methods are dispatched in the internal `CameraManager`
> thread. `PipelineHandler::generateConfiguration()` was called directly,
> however, which likely goes against the expectations, so call it on the
> internal thread.
> 
> This could not be done before 14618cdd0c80a4 ("libcamera: base: bound_method: Move return value")
> because the function returns `std::unique_ptr`, and that type cannot
> be copied.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/libcamera/camera.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 5efeadd53..423c5f3aa 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -1206,7 +1206,8 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(Span<const St
>                 return nullptr;
>  
>         std::unique_ptr<CameraConfiguration> config =
> -               d->pipe_->generateConfiguration(this, roles);
> +               d->pipe_->invokeMethod(&PipelineHandler::generateConfiguration,
> +                                      ConnectionTypeBlocking, this, roles);

Interesting. I hope this doesn't have any (negative) impact on the
currect testing of
https://github.com/raspberrypi/rpicam-apps/issues/799.

I think this is correct to make sure we're consistent calling things on
the pipeline handler in the internal threads though so:


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

>         if (!config) {
>                 LOG(Camera, Debug)
>                         << "Pipeline handler failed to generate configuration";
> -- 
> 2.50.1
>

Hello,

On Mon, Jul 21, 2025 at 01:58:11PM +0100, Kieran Bingham wrote:
> Quoting Barnabás Pőcze (2025-07-21 13:01:26)
> > Most pipeline handler methods are dispatched in the internal `CameraManager`
> > thread. `PipelineHandler::generateConfiguration()` was called directly,

s/was/is/

> > however, which likely goes against the expectations, so call it on the
> > internal thread.
> > 
> > This could not be done before 14618cdd0c80a4 ("libcamera: base: bound_method: Move return value")

You can wrap the commit message and split this line.

> > because the function returns `std::unique_ptr`, and that type cannot
> > be copied.
> > 
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---
> >  src/libcamera/camera.cpp | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 5efeadd53..423c5f3aa 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -1206,7 +1206,8 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(Span<const St
> >                 return nullptr;
> >  
> >         std::unique_ptr<CameraConfiguration> config =
> > -               d->pipe_->generateConfiguration(this, roles);
> > +               d->pipe_->invokeMethod(&PipelineHandler::generateConfiguration,
> > +                                      ConnectionTypeBlocking, this, roles);
> 
> Interesting. I hope this doesn't have any (negative) impact on the
> currect testing of
> https://github.com/raspberrypi/rpicam-apps/issues/799.
> 
> I think this is correct to make sure we're consistent calling things on
> the pipeline handler in the internal threads though so:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

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

> >         if (!config) {
> >                 LOG(Camera, Debug)
> >                         << "Pipeline handler failed to generate configuration";

Hi

2025. 07. 24. 22:47 keltezéssel, Laurent Pinchart írta:
> Hello,
> 
> On Mon, Jul 21, 2025 at 01:58:11PM +0100, Kieran Bingham wrote:
>> Quoting Barnabás Pőcze (2025-07-21 13:01:26)
>>> Most pipeline handler methods are dispatched in the internal `CameraManager`
>>> thread. `PipelineHandler::generateConfiguration()` was called directly,
> 
> s/was/is/
> 
>>> however, which likely goes against the expectations, so call it on the
>>> internal thread.
>>>
>>> This could not be done before 14618cdd0c80a4 ("libcamera: base: bound_method: Move return value")
> 
> You can wrap the commit message and split this line.

I have rewritten it as follows:

     Before 14618cdd0c80a4 ("libcamera: base: bound_method: Move return value")
     this could not be done because the function returns `std::unique_ptr`, and
     that type cannot be copied.

I believe it is important to keep the commit title in one line.


Regards,
Barnabás Pőcze

> 
>>> because the function returns `std::unique_ptr`, and that type cannot
>>> be copied.
>>>
>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>> ---
>>>   src/libcamera/camera.cpp | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>>> index 5efeadd53..423c5f3aa 100644
>>> --- a/src/libcamera/camera.cpp
>>> +++ b/src/libcamera/camera.cpp
>>> @@ -1206,7 +1206,8 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(Span<const St
>>>                  return nullptr;
>>>   
>>>          std::unique_ptr<CameraConfiguration> config =
>>> -               d->pipe_->generateConfiguration(this, roles);
>>> +               d->pipe_->invokeMethod(&PipelineHandler::generateConfiguration,
>>> +                                      ConnectionTypeBlocking, this, roles);
>>
>> Interesting. I hope this doesn't have any (negative) impact on the
>> currect testing of
>> https://github.com/raspberrypi/rpicam-apps/issues/799.
>>
>> I think this is correct to make sure we're consistent calling things on
>> the pipeline handler in the internal threads though so:
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>>>          if (!config) {
>>>                  LOG(Camera, Debug)
>>>                          << "Pipeline handler failed to generate configuration";
>