[1/2] apps: cam: Do not override Request::controls()

25160 diff mbox series

The ControlList associated with a Request are created with
the libcamera::controls::controls id map.

The cam application, when parsing controls from a script
assigns to the Request's control list a newly created one, which is
initialized without a valid idmap or info map.

This causes IPA that run in isolated mode to fail, as the IPC
serialization/deserialization code requires all ControlList to have
a valid idmap or info map associated.

Instead of overwriting the Request::controls() list, merge it with
the one created by cam when parsing the capture script.

Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/apps/cam/camera_session.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoting Jacopo Mondi (2025-11-24 16:43:44)
> The ControlList associated with a Request are created with
> the libcamera::controls::controls id map.
> 
> The cam application, when parsing controls from a script
> assigns to the Request's control list a newly created one, which is
> initialized without a valid idmap or info map.
> 
> This causes IPA that run in isolated mode to fail, as the IPC
> serialization/deserialization code requires all ControlList to have
> a valid idmap or info map associated.
> 
> Instead of overwriting the Request::controls() list, merge it with
> the one created by cam when parsing the capture script.
> 
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/apps/cam/camera_session.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..0a24b7fafc5333f88053cb52a500cdafd7bb603f 100644
> --- a/src/apps/cam/camera_session.cpp
> +++ b/src/apps/cam/camera_session.cpp
> @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request)
>                 return 0;
>  
>         if (script_)
> -               request->controls() = script_->frameControls(queueCount_);
> +               request->controls().merge(script_->frameControls(queueCount_));

That looks accurate even after we find a way to fix the permissions to
prevent the underlying list itself being replaced.

Is that possible by making it const somehow? a const pointer, to
non-const data ?

Anyway,

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

>  
>         queueCount_++;
>  
> 
> -- 
> 2.51.1
>

Hi Kieran

On Mon, Nov 24, 2025 at 05:14:50PM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2025-11-24 16:43:44)
> > The ControlList associated with a Request are created with
> > the libcamera::controls::controls id map.
> >
> > The cam application, when parsing controls from a script
> > assigns to the Request's control list a newly created one, which is
> > initialized without a valid idmap or info map.
> >
> > This causes IPA that run in isolated mode to fail, as the IPC
> > serialization/deserialization code requires all ControlList to have
> > a valid idmap or info map associated.
> >
> > Instead of overwriting the Request::controls() list, merge it with
> > the one created by cam when parsing the capture script.
> >
> > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/295
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  src/apps/cam/camera_session.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> > index 1596a25a3abed9c2d93e6657b92e35fdfd3d1a26..0a24b7fafc5333f88053cb52a500cdafd7bb603f 100644
> > --- a/src/apps/cam/camera_session.cpp
> > +++ b/src/apps/cam/camera_session.cpp
> > @@ -447,7 +447,7 @@ int CameraSession::queueRequest(Request *request)
> >                 return 0;
> >
> >         if (script_)
> > -               request->controls() = script_->frameControls(queueCount_);
> > +               request->controls().merge(script_->frameControls(queueCount_));
>
> That looks accurate even after we find a way to fix the permissions to
> prevent the underlying list itself being replaced.
>
> Is that possible by making it const somehow? a const pointer, to
> non-const data ?

I'm looking into that

Simply making "ControlList &Request::controls()" a "const ControlList
&Request::controls() const" doesn't work

1) The Camera class modifies the control list of a request, and we can
use the usual PIMPL pattern to expose to the library and interface
which is different than the one exposed to applications
2) However a ControlList::merge()/set() can't be called on a const
instance of ControlList, so application won't be able to add controls
to a Request if we simply return "const ControlList" and we should add
methods to the Request class to set/merge controls like
"Request::addControls()" I'm not sure I like it though

>
> Anyway,
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> >
> >         queueCount_++;
> >
> >
> > --
> > 2.51.1
> >