[libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset ControlSerializer on worker

[paul.elder at ideasonboard.com]

Hi Kieran,

On Thu, Sep 02, 2021 at 03:05:23PM +0100, Kieran Bingham wrote:
> On 01/09/2021 15:37, Jacopo Mondi wrote:
> > When running the IPA in isolated mode, each side of the IPC boundary
> > has an instance of the ControlSerializer class which is used to
> > serializer/deserialize controls before transmitting them on the wire.
> > 
> > The IPAProxyWorker, which creates and manages the process the IPA runs into,
> > does not reset its ControlSerializer upon an IPA::configure() call, while
> > the IPAProxy does so, effectively creating a misalignment between the
> > two sides of the fence.
> > 
> > This obviously creates issues as one side of the IPC runs with a
> > populated and possibly stale cache of ControlInfoMap references, while the
> > other side gets reset every time a new configuration is applied to the
> > Camera.
> > 
> > Fix that by resetting the IPAProxyWorker ControlSerializer on an
> > IPA configure() call.
> > 
> > This change fixes an issue which is easily triggered  by running two
> > consecutive capture sessions with the IPA running in isolated mode:
> > ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap
> > 
> > Fixes: 7832e19a599e ("utils: ipc: add templates for code generation for IPC mechanism")
> 
> Is it also technically fixing the patch that added the reset on the
> other side? as that's when the misalignment begins?
> 
> 
> 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl      | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > index 8a88bd467da7..22cbc15c5eff 100644
> > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > @@ -79,6 +79,10 @@ public:
> >  
> >  {% for method in interface_main.methods %}
> >  		case {{cmd_enum_name}}::{{method.mojom_name|cap}}: {
> > +
> > +{%- if method.mojom_name == "configure" %}
> > +		controlSerializer_.reset();
> 
> one more level indentation >> ?

Yeah, that one line should be indented one more in to align in the case
statement.


Paul

> 
> It looks like we're "inside" this case statement, so it should be one
> level deeper.
> 
> 
> > +{%- endif %}
> 
> 
> Wow, these templates are hard to parse :-)
> 
> But ... it looks like this is right ;-)
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> >  		{{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}}
> >  {% for param in method|method_param_outputs %}
> >  			{{param|name}} {{param.mojom_name}};
> >