[PATCH] libcamera: delayed_controls: Inherit from Object class

[Kieran Bingham]

Quoting Laurent Pinchart (2025-02-24 21:24:54)
> Hi Stanislaw,
> 
> On Mon, Feb 24, 2025 at 11:16:45AM +0100, Stanislaw Gruszka wrote:
> > On Mon, Feb 24, 2025 at 03:19:34AM +0200, Laurent Pinchart wrote:
> > > A second use-after-free bug related to signals staying connected after
> > > the receiver DelayedControls instance gets deleted has been found, this
> > > time in the simple pipeline handler. Fix the issue once and for all by
> > > making the DelayedControls class inherit from Object. This will
> > > disconnect signals automatically upon deletion of the receiver.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > > Stan, would you be able to test this with the simple pipeline handler ?
> > > It should work both with and without your series that deals with the
> > > frame start signal, and should fix the crash that Kieran has reported.
> > 
> > It does not make assertion
> > "state_ == ProxyRunning" failed in processStatsThread()
> > gone for me. 
> 
> Right, that one is addressed by Milan's "PATCH v2 0/5] Fix occasional
> software ISP assertion error on stop" series.
> 
> This patch should fix a different crash, but now that I wrote this,
> Kieran may not have reported it with the simple pipeline handler. He has
> reported it for the rkisp1 though.
> 

Yes, the delayed controls crash I hit was on rkisp1 with a video
multiplexor on the pipeline that lets me connect multiple cameras to a
single ISP
(https://www.arducam.com/product/multi-camera-v2-1-adapter-raspberry-pi/)

When I get chance, I'll also test this patch in the rkisp1 context, but
that may take some time.

--
Kieran

> Would you be able to test this patch on top of Milan's series, without
> your changes ? I'd like to make sure it introduces no regression. We can
> then apply your series on top.
> 
> > I tested this patch alone as well together with
> > 
> > [PATCH v4 0/2] libcamera: start frame events changes
> > [PATCH] pipeline: simple: Create DelayedControls instance once only
> > 
> > So far, only revert of 9bc8b6a573a63b8c9f5588cdea6da5ae71abd138
> > make the assertion gone.
> > 
> > One interesting thing is the issue is not reproducible when
> > killing qcam by sending TERM signal, like for example in
> > below script:
> > 
> > #!/bin/bash -x
> > for i in `seq 1 20` ; do
> >         ls core &> /dev/null && break;
> > 
> >         ./git/libcamera/build/src/apps/qcam/qcam &
> >         pid=$!
> > 
> >         sleep `expr $RANDOM % 10`
> >         kill $pid
> >         sleep `expr $RANDOM % 2`
> > done
> > 
> > I have to manually press X on qcam window to trigger the assertion.
> > Usually 2 or 3 times is enough.
> > 
> > >  include/libcamera/internal/delayed_controls.h | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
> > > index e8d3014d92cb..b64d8bba7cf7 100644
> > > --- a/include/libcamera/internal/delayed_controls.h
> > > +++ b/include/libcamera/internal/delayed_controls.h
> > > @@ -10,13 +10,15 @@
> > >  #include <stdint.h>
> > >  #include <unordered_map>
> > > 
> > > +#include <libcamera/base/object.h>
> > > +
> > >  #include <libcamera/controls.h>
> > > 
> > >  namespace libcamera {
> > > 
> > >  class V4L2Device;
> > > 
> > > -class DelayedControls
> > > +class DelayedControls : public Object
> > >  {
> > >  public:
> > >     struct ControlParams {
> > > 
> > > base-commit: d476f8358be1536d4edd680c6024f784ff022f5d
> 
> -- 
> Regards,
> 
> Laurent Pinchart