[libcamera-devel] [PATCH] libcamera: Fix crash caused by reading uninitialised delayed controls

[Jacopo Mondi]

Hi David,

On Wed, Sep 29, 2021 at 11:51:24AM +0100, David Plowman wrote:
> Hi Jacopo
>
> Thanks for clarifying!
>
> On Wed, 29 Sept 2021 at 11:30, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi David
> >
> > On Wed, Sep 29, 2021 at 11:17:23AM +0100, David Plowman wrote:
> > > Hi Jacopo
> > >
> > > Thanks for the question!
> > >
> > > On Wed, 29 Sept 2021 at 10:51, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > >
> > > > Hello
> > > >
> > > > On Wed, Sep 29, 2021 at 12:58:27AM +0300, Laurent Pinchart wrote:
> > > > > Hi David,
> > > > >
> > > > > Thank you for the patch.
> > > > >
> > > > > On Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote:
> > > > > > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74.
> > > > >
> > > > > This should be moved before the Signed-off-by line, with
> > > > >
> > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=74.
> > > > >
> > > > > > The cause is that we read out delayed values using a frame's sequence
> > > > > > number (DelayedControls::get). But we fill the values up
> > > > > > (DelayedControls::applyControls) incrementing writeCount by only one
> > > > > > even if the sequence number has jumped by several since last
> > > > > > time. This is exactly what happens when frames are being dropped.
> > > > > >
> > > > > > So the fix is to increment writeCount by "as much as the sequence
> > > > > > number has jumped since last time", which means that we just follow
> > > > > > the sequence number directly.
> > > > > >
> > > > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > > > ---
> > > > > >  src/libcamera/delayed_controls.cpp | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> > > > > > index 90ce7e0b..9667187e 100644
> > > > > > --- a/src/libcamera/delayed_controls.cpp
> > > > > > +++ b/src/libcamera/delayed_controls.cpp
> > > > > > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence)
> > > > > >             }
> > > > > >     }
> > > > > >
> > > > > > -   writeCount_++;
> > > > > > +   writeCount_ = sequence - firstSequence_ + 1;
> > > > >
> > > > > I'm always confused when I read this file, I think one day I'll dive
> > > > > deep and do something about it :-) Until then, this looks good to me.
> > > > >
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > >
> > > > > A second pair of eyes would be nice (and testing on IPU3 too).
> > > >
> > > > I've run a few CTS runs and it doesn't seem to introduce regressions.
> > > >
> > > > However I'm wondering how this chaneg plays with queueCount
> > > >
> > > > >
> > > > > >
> > > > > >     while (writeCount_ > queueCount_) {
> > > >                ^
> > > >                this
> > > >
> > > > What I wonder about is: if a frame is dropped, should we advance the
> > > > count and 'burn' the controls that were meant for it ? Because in this
> > > > case it seems to me that if the number of dropped frames exceeds the
> > > > number of controls available in the queue, we end up pushing {} but we
> > > > advance queueCount by one only.
> > >
> > > That loop will run, incrementing queueCount (in "push") by 1 each
> > > time, until it has caught up with writeCount - which I believe to be
> > > the correct behaviour. Each call will copy the previous control
> > > entries to the next one, so anyone who asks for values for a frame we
> > > dropped will simply get the most recent values that we did see.
> > >
> > > Even if the number of dropped frames exceeds the length of the queue
> > > the behaviour seems fine - the entire queue will fill up with the last
> > > seen values.
> > >
> > > >
> > > > If it's intended to burn all the controls meant for dropped frames we
> > > > should probably increment queueCount by the same amount writeCount_
> > > > has been incremented with.
> > >
> > > Not sure what you mean by "burn" the controls? The client code can ask
> > > for values from sequence numbers that were never actually seen (if it
> > > wants), and will get the last known values back. Does that make sense?
> >
> > Let's assume that we have 3 controls in the values_ queue waiting to
> > be applied to the 'next' frames
> >
> > writeCount = x
> > queueCount = x + 3
> > values_ = [ C(x+1), C(x+2), C(x+3) ]
> >
> > We drop 4 frames hence
> >
> > writeCount = x + 4
> >
> > and we have to advance queueCount by pushing {} and copying the last
> > values
> >
> > values_ = [ C(x+1), C(x+2), C(x+3), C(x+3) ]
>
> Yes, this is what we get.
>
> >
> > What I meant by burnt is that if I'm not mistaken C(x+1) and C(x+2)
> > are lost and I wonder if we shouldn't instead
>
> I wouldn't describe them as "lost". The client code can still access
> them with get(x+1), get(x+3) etc.
>
> >
> > values_ = [ C(x+1), C(x+2), C(x+3), C(x+1), C(x+2), C(x+3) ]
>
> I think the trouble with this is that DelayedControls are more of an
> array than a queue. Don't think of it in terms of accessing "the most
> recent values" or "the values from n frames ago". We access them with
> an array index, which happens to be the actual frame sequence number
> (more or less). (The fact that only the 16 most recent items are
> stored is kind of an "implementation detail".)
>
> So with those values, the code would, for frame x+4, call get(x+4) and
> come back with C(x+1) - not the right values.
>

My understanding was that ideally if we drop a frame, the setting that
were applied to it should be re-applied to the next one, and the whole
'queue' is shifted by one place. That's not the case as I understand
it. Thanks for the explanation then!

Tested-by: Jacopo Mondi <jacopo at jmondi.org>
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j


> >
> > when frames are dropped.
> >
> > Or have I got it all wrong ?
>
> Have I explained it any better?  :)
>
> David
>
> >
> >
> > >
> > > Best regards
> > > David
> > >
> > > >
> > > >
> > > > > >             LOG(DelayedControls, Debug)
> > > >
> > > > >
> > > > > --
> > > > > Regards,
> > > > >
> > > > > Laurent Pinchart