[PATCH] ipa: simple: Initialize ccmEnabled to false

[Laurent Pinchart]

On Tue, Apr 01, 2025 at 12:34:14PM +0200, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2025. 04. 01. 12:17 keltezéssel, Milan Zamazal írta:
> > Hi Stanislaw,
> > 
> > thank you for the patch.
> > 
> > Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com> writes:
> > 
> >> ccmEnabled variable is not initialized by default, what result of

s/what result of/which results in/

> >> usage of CCM when the algorithm itself is not enabled and configured.
> > 
> > Interesting, I thought the default boolean value in C++ is false.
> > If true or not, it doesn't harm to set it explicitly.
> 
> Sadly a default-initialized `bool` like `bool x;` has indeterminate value:
> https://en.cppreference.com/w/cpp/language/default_initialization
> 
> """
> The effects of default-initialization are:
>    * if T is a (possibly cv-qualified) class type, [...]
>    * if T is an array type, [...]
>    * otherwise, no initialization is performed [...]
> 
> [...]
> 
> If no initialization is performed for an object, that object retains an
> indeterminate value until that value is replaced.
> """
> 
> One needs something like `bool x{}, y = {}, z = bool(), w = false;` at least.
> 
> >> The bug manifest itself as seldom reproducible corrupted video stream.

s/manifest/manifests/

> >> Fix by initialize the variable in IPAContext class initialization list.
> >>
> >> Fixes: ac3068655643 ("libcamera: software_isp: Track whether CCM is enabled")
> >> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com>
> >> ---
> >>   src/ipa/simple/ipa_context.h | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> >> index 7dc2cd7ae828..afccf557121e 100644
> >> --- a/src/ipa/simple/ipa_context.h
> >> +++ b/src/ipa/simple/ipa_context.h
> >> @@ -82,7 +82,7 @@ struct IPAFrameContext : public FrameContext {
> >>   
> >>   struct IPAContext {
> >>   	IPAContext(unsigned int frameContextSize)
> >> -		: frameContexts(frameContextSize)
> >> +		: frameContexts(frameContextSize), ccmEnabled(false)
> > 
> > Or setting it
> > 
> >    bool ccmEnabled = false;
> > 
> > in IPAContext?  Up to the maintainers, what they prefer.
> 
> I would generally prefer initializing right where the member is declared
> as it is clearer than member init lists in my opinion, and is not affected
> negatively when additional constructors are introduced.

We've initially used initializer lists in constructors, probably because
I didn't know member variable initialization in the class definition was
possible :-) I agree with Barnabás that it would be better. With that,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Stanislaw, could you test a v2 with this change ? We can then merge it
in time for v0.5.

> >  From my side, in either case:
> > 
> > Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
> > 
> >>   	{
> >>   	}

-- 
Regards,

Laurent Pinchart