[libcamera-devel] pipeline: rpi: Reset the frame lengths queue during configure

19054 diff mbox series

The IPA stores a list of the last 10 frame lengths applied to the
sensor for determining the timeout to use. This list gets reset on
start(), but there is a path through the code that accesses this list
in configure() which happens earlier, causing a logical error.

Fix this by constructing the list with 10 initial values of 0s.

Bug: https://github.com/raspberrypi/libcamera/issues/64
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Quoting Naushir Patuck (2023-09-18 10:10:45)
> The IPA stores a list of the last 10 frame lengths applied to the
> sensor for determining the timeout to use. This list gets reset on
> start(), but there is a path through the code that accesses this list
> in configure() which happens earlier, causing a logical error.
> 
> Fix this by constructing the list with 10 initial values of 0s.
> 
> Bug: https://github.com/raspberrypi/libcamera/issues/64
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Are there any other members of IpaBase which also need to be initialised
? (Not specifically for this patch, just a question of ... if we missed
this - do we need to check for others here?, but that's a separate patch
if there are)

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

> ---
>  src/ipa/rpi/common/ipa_base.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index f7e7ad5ee499..f0ce6c75dc03 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(IPARPI)
>  namespace ipa::RPi {
>  
>  IpaBase::IpaBase()
> -       : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0),
> -         firstStart_(true), flickerState_({ 0, 0s })
> +       : controller_(), frameLengths_(FrameLengthsQueueSize, 0s), frameCount_(0),
> +         mistrustCount_(0), lastRunTimestamp_(0), firstStart_(true), flickerState_({ 0, 0s })
>  {
>  }
>  
> -- 
> 2.34.1
>

Quoting Kieran Bingham (2023-09-18 10:32:01)
> Quoting Naushir Patuck (2023-09-18 10:10:45)
> > The IPA stores a list of the last 10 frame lengths applied to the
> > sensor for determining the timeout to use. This list gets reset on
> > start(), but there is a path through the code that accesses this list
> > in configure() which happens earlier, causing a logical error.
> > 
> > Fix this by constructing the list with 10 initial values of 0s.
> > 
> > Bug: https://github.com/raspberrypi/libcamera/issues/64
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> 
> Are there any other members of IpaBase which also need to be initialised
> ? (Not specifically for this patch, just a question of ... if we missed
> this - do we need to check for others here?, but that's a separate patch
> if there are)
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Hopefully this is an easy one unless anyone spots anything different.
Could I get one more tag so I can collect this patch please?

--
Kieran


> 
> > ---
> >  src/ipa/rpi/common/ipa_base.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > index f7e7ad5ee499..f0ce6c75dc03 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(IPARPI)
> >  namespace ipa::RPi {
> >  
> >  IpaBase::IpaBase()
> > -       : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0),
> > -         firstStart_(true), flickerState_({ 0, 0s })
> > +       : controller_(), frameLengths_(FrameLengthsQueueSize, 0s), frameCount_(0),
> > +         mistrustCount_(0), lastRunTimestamp_(0), firstStart_(true), flickerState_({ 0, 0s })
> >  {
> >  }
> >  
> > -- 
> > 2.34.1
> >

Hi Naush

Thanks for this patch.

On Mon, 18 Sept 2023 at 10:10, Naushir Patuck via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> The IPA stores a list of the last 10 frame lengths applied to the
> sensor for determining the timeout to use. This list gets reset on
> start(), but there is a path through the code that accesses this list
> in configure() which happens earlier, causing a logical error.
>
> Fix this by constructing the list with 10 initial values of 0s.
>
> Bug: https://github.com/raspberrypi/libcamera/issues/64
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

> ---
>  src/ipa/rpi/common/ipa_base.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index f7e7ad5ee499..f0ce6c75dc03 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(IPARPI)
>  namespace ipa::RPi {
>
>  IpaBase::IpaBase()
> -       : controller_(), frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0),
> -         firstStart_(true), flickerState_({ 0, 0s })
> +       : controller_(), frameLengths_(FrameLengthsQueueSize, 0s), frameCount_(0),
> +         mistrustCount_(0), lastRunTimestamp_(0), firstStart_(true), flickerState_({ 0, 0s })
>  {
>  }
>
> --
> 2.34.1
>