| Message ID | 20250401093308.475810-1-stanislaw.gruszka@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Stanislaw, thank you for the patch. Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> writes: > ccmEnabled variable is not initialized by default, what result of > 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. > The bug manifest itself as seldom reproducible corrupted video stream. > 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@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. From my side, in either case: Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > { > }
Hi 2025. 04. 01. 12:17 keltezéssel, Milan Zamazal írta: > Hi Stanislaw, > > thank you for the patch. > > Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> writes: > >> ccmEnabled variable is not initialized by default, what result of >> 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. >> 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@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. Regards, Barnabás Pőcze > > From my side, in either case: > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > >> { >> } >
Hi Milan, On Tue, Apr 01, 2025 at 12:17:56PM +0200, Milan Zamazal wrote: > Hi Stanislaw, > > thank you for the patch. > > Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> writes: > > > ccmEnabled variable is not initialized by default, what result of > > 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. I think class members are not initialized if not explicitly done so. And can have random values unfortunately. I can not really confirm the fix because, it happened randomly and very seldom. But I initialized ccmEnabled to true and got the same effect as when the bug happened: first collorfull images with only coutures recognizable and then all black. > > 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. > > From my side, in either case: > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> Thanks! Regards Stanislaw
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@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@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@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@redhat.com> > > > >> { > >> }
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) { }
ccmEnabled variable is not initialized by default, what result of usage of CCM when the algorithm itself is not enabled and configured. The bug manifest itself as seldom reproducible corrupted video stream. 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@linux.intel.com> --- src/ipa/simple/ipa_context.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)