[v1] ipa: rpi: awb: Fix incorrect parameter retrieval
diff mbox series

Message ID 20240520035021.205171-1-pobrn@protonmail.com
State Deferred
Headers show
Series
  • [v1] ipa: rpi: awb: Fix incorrect parameter retrieval
Related show

Commit Message

Barnabás Pőcze May 20, 2024, 3:50 a.m. UTC 
During the refactoring in the mentioned commit the quotation
marks around the word *fast* were missed. Unfortunately there
was a variable named *fast* and an suitable overload for `operator[]`
for things to compile fine.

Fixes: c1597f98965461 ("ipa: raspberrypi: Use YamlParser to replace dependency on boost")
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 src/ipa/rpi/controller/rpi/awb.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kieran Bingham May 20, 2024, 9:58 a.m. UTC | #1 
Quoting Barnabás Pőcze (2024-05-20 04:50:22)
> During the refactoring in the mentioned commit the quotation
> marks around the word *fast* were missed. Unfortunately there
> was a variable named *fast* and an suitable overload for `operator[]`
> for things to compile fine.

Ouch!

> Fixes: c1597f98965461 ("ipa: raspberrypi: Use YamlParser to replace dependency on boost")

And this has been in for quite some time, Great find!


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


> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>  src/ipa/rpi/controller/rpi/awb.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp
> index abe5906e..94ecbfc2 100644
> --- a/src/ipa/rpi/controller/rpi/awb.cpp
> +++ b/src/ipa/rpi/controller/rpi/awb.cpp
> @@ -161,7 +161,7 @@ int AwbConfig::read(const libcamera::YamlObject &params)
>                         bayes = false;
>                 }
>         }
> -       fast = params[fast].get<int>(bayes); /* default to fast for Bayesian, otherwise slow */
> +       fast = params["fast"].get<int>(bayes); /* default to fast for Bayesian, otherwise slow */
>         whitepointR = params["whitepoint_r"].get<double>(0.0);
>         whitepointB = params["whitepoint_b"].get<double>(0.0);
>         if (bayes == false)
> -- 
> 2.45.1
> 
>
Dan Scally May 20, 2024, 10:57 a.m. UTC | #2 
On 20/05/2024 10:58, Kieran Bingham wrote:
> Quoting Barnabás Pőcze (2024-05-20 04:50:22)
>> During the refactoring in the mentioned commit the quotation
>> marks around the word *fast* were missed. Unfortunately there
>> was a variable named *fast* and an suitable overload for `operator[]`
>> for things to compile fine.
> Ouch!
>
>> Fixes: c1597f98965461 ("ipa: raspberrypi: Use YamlParser to replace dependency on boost")
> And this has been in for quite some time, Great find!
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Great spot indeed.


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>
>
>> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
>> ---
>>   src/ipa/rpi/controller/rpi/awb.cpp | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp
>> index abe5906e..94ecbfc2 100644
>> --- a/src/ipa/rpi/controller/rpi/awb.cpp
>> +++ b/src/ipa/rpi/controller/rpi/awb.cpp
>> @@ -161,7 +161,7 @@ int AwbConfig::read(const libcamera::YamlObject &params)
>>                          bayes = false;
>>                  }
>>          }
>> -       fast = params[fast].get<int>(bayes); /* default to fast for Bayesian, otherwise slow */
>> +       fast = params["fast"].get<int>(bayes); /* default to fast for Bayesian, otherwise slow */
>>          whitepointR = params["whitepoint_r"].get<double>(0.0);
>>          whitepointB = params["whitepoint_b"].get<double>(0.0);
>>          if (bayes == false)
>> -- 
>> 2.45.1
>>
>>
Kieran Bingham May 20, 2024, 12:27 p.m. UTC | #3 
Quoting Dan Scally (2024-05-20 11:57:51)
> 
> On 20/05/2024 10:58, Kieran Bingham wrote:
> > Quoting Barnabás Pőcze (2024-05-20 04:50:22)
> >> During the refactoring in the mentioned commit the quotation
> >> marks around the word *fast* were missed. Unfortunately there
> >> was a variable named *fast* and an suitable overload for `operator[]`
> >> for things to compile fine.
> > Ouch!
> >
> >> Fixes: c1597f98965461 ("ipa: raspberrypi: Use YamlParser to replace dependency on boost")
> > And this has been in for quite some time, Great find!
> >
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Great spot indeed.
> 
> 
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1181634 : success : patchwork/4317

CI passes here, but I'll hold off to give RPi time to check here before
merging, but I believe this is good to go already.

--
Kieran




> 
> >
> >
> >> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> >> ---
> >>   src/ipa/rpi/controller/rpi/awb.cpp | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp
> >> index abe5906e..94ecbfc2 100644
> >> --- a/src/ipa/rpi/controller/rpi/awb.cpp
> >> +++ b/src/ipa/rpi/controller/rpi/awb.cpp
> >> @@ -161,7 +161,7 @@ int AwbConfig::read(const libcamera::YamlObject &params)
> >>                          bayes = false;
> >>                  }
> >>          }
> >> -       fast = params[fast].get<int>(bayes); /* default to fast for Bayesian, otherwise slow */
> >> +       fast = params["fast"].get<int>(bayes); /* default to fast for Bayesian, otherwise slow */
> >>          whitepointR = params["whitepoint_r"].get<double>(0.0);
> >>          whitepointB = params["whitepoint_b"].get<double>(0.0);
> >>          if (bayes == false)
> >> -- 
> >> 2.45.1
> >>
> >>
Naushir Patuck May 20, 2024, 12:57 p.m. UTC | #4 
Hi all,

On Mon, 20 May 2024 at 13:27, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Dan Scally (2024-05-20 11:57:51)
> >
> > On 20/05/2024 10:58, Kieran Bingham wrote:
> > > Quoting Barnabás Pőcze (2024-05-20 04:50:22)
> > >> During the refactoring in the mentioned commit the quotation
> > >> marks around the word *fast* were missed. Unfortunately there
> > >> was a variable named *fast* and an suitable overload for `operator[]`
> > >> for things to compile fine.
> > > Ouch!
> > >
> > >> Fixes: c1597f98965461 ("ipa: raspberrypi: Use YamlParser to replace dependency on boost")
> > > And this has been in for quite some time, Great find!
> > >
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > Great spot indeed.

Indeed, good find.  In fact, it took a few minutes to understand what
that statement was actually doing.  It turns out, the fast config
param is never ever used in our algorithm.  So while this fix is
correct, we probably will actually remove the statement entirely.

Regards,
Naush


> >
> >
> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>
> https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1181634 : success : patchwork/4317
>
> CI passes here, but I'll hold off to give RPi time to check here before
> merging, but I believe this is good to go already.
>
> --
> Kieran
>
>
>
>
> >
> > >
> > >
> > >> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > >> ---
> > >>   src/ipa/rpi/controller/rpi/awb.cpp | 2 +-
> > >>   1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp
> > >> index abe5906e..94ecbfc2 100644
> > >> --- a/src/ipa/rpi/controller/rpi/awb.cpp
> > >> +++ b/src/ipa/rpi/controller/rpi/awb.cpp
> > >> @@ -161,7 +161,7 @@ int AwbConfig::read(const libcamera::YamlObject &params)
> > >>                          bayes = false;
> > >>                  }
> > >>          }
> > >> -       fast = params[fast].get<int>(bayes); /* default to fast for Bayesian, otherwise slow */
> > >> +       fast = params["fast"].get<int>(bayes); /* default to fast for Bayesian, otherwise slow */
> > >>          whitepointR = params["whitepoint_r"].get<double>(0.0);
> > >>          whitepointB = params["whitepoint_b"].get<double>(0.0);
> > >>          if (bayes == false)
> > >> --
> > >> 2.45.1
> > >>
> > >>
Barnabás Pőcze May 20, 2024, 1:20 p.m. UTC | #5 
2024. május 20., hétfő 14:57 keltezéssel, Naushir Patuck <naush@raspberrypi.com> írta:

> Hi all,
> 
> On Mon, 20 May 2024 at 13:27, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting Dan Scally (2024-05-20 11:57:51)
> > >
> > > On 20/05/2024 10:58, Kieran Bingham wrote:
> > > > Quoting Barnabás Pőcze (2024-05-20 04:50:22)
> > > >> During the refactoring in the mentioned commit the quotation
> > > >> marks around the word *fast* were missed. Unfortunately there
> > > >> was a variable named *fast* and an suitable overload for `operator[]`
> > > >> for things to compile fine.
> > > > Ouch!
> > > >
> > > >> Fixes: c1597f98965461 ("ipa: raspberrypi: Use YamlParser to replace dependency on boost")
> > > > And this has been in for quite some time, Great find!
> > > >
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > > Great spot indeed.
> 
> Indeed, good find.  In fact, it took a few minutes to understand what
> that statement was actually doing.  It turns out, the fast config
> param is never ever used in our algorithm.  So while this fix is
> correct, we probably will actually remove the statement entirely.

Ah, I didn't even check that but you're right; it's probably better
to just remove it at once then instead of this change.


Regards,
Barnabás Pőcze

> 
> Regards,
> Naush
> 
> 
> > >
> > >
> > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> >
> > https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1181634 : success : patchwork/4317
> >
> > CI passes here, but I'll hold off to give RPi time to check here before
> > merging, but I believe this is good to go already.
> >
> > --
> > Kieran
> >
> >
> >
> >
> > >
> > > >
> > > >
> > > >> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > > >> ---
> > > >>   src/ipa/rpi/controller/rpi/awb.cpp | 2 +-
> > > >>   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp
> > > >> index abe5906e..94ecbfc2 100644
> > > >> --- a/src/ipa/rpi/controller/rpi/awb.cpp
> > > >> +++ b/src/ipa/rpi/controller/rpi/awb.cpp
> > > >> @@ -161,7 +161,7 @@ int AwbConfig::read(const libcamera::YamlObject &params)
> > > >>                          bayes = false;
> > > >>                  }
> > > >>          }
> > > >> -       fast = params[fast].get<int>(bayes); /* default to fast for Bayesian, otherwise slow */
> > > >> +       fast = params["fast"].get<int>(bayes); /* default to fast for Bayesian, otherwise slow */
> > > >>          whitepointR = params["whitepoint_r"].get<double>(0.0);
> > > >>          whitepointB = params["whitepoint_b"].get<double>(0.0);
> > > >>          if (bayes == false)
> > > >> --
> > > >> 2.45.1
> > > >>
> > > >>
>

Patch
diff mbox series 

diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp
index abe5906e..94ecbfc2 100644
--- a/src/ipa/rpi/controller/rpi/awb.cpp
+++ b/src/ipa/rpi/controller/rpi/awb.cpp
@@ -161,7 +161,7 @@  int AwbConfig::read(const libcamera::YamlObject &params)
 			bayes = false;
 		}
 	}
-	fast = params[fast].get<int>(bayes); /* default to fast for Bayesian, otherwise slow */
+	fast = params["fast"].get<int>(bayes); /* default to fast for Bayesian, otherwise slow */
 	whitepointR = params["whitepoint_r"].get<double>(0.0);
 	whitepointB = params["whitepoint_b"].get<double>(0.0);
 	if (bayes == false)