<div dir="auto"><div>Hi Laurent,<br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 27 Jul 2022, 6:10 pm Laurent Pinchart, <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Naush,<br>
<br>
On Wed, Jul 27, 2022 at 01:27:21PM +0100, Naushir Patuck wrote:<br>
> On Wed, 27 Jul 2022 at 03:38, Laurent Pinchart wrote:<br>
> <br>
> > The Raspberry Pi IPA module depends on boost only to parse the JSON<br>
> > tuning data files. As libcamera depends on libyaml, use the YamlParser<br>
> > class to parse those files and drop the dependency on boost.<br>
> ><br>
> > Signed-off-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank" rel="noreferrer">laurent.pinchart@ideasonboard.com</a>><br>
> > ---<br>
> > Changes since v6:<br>
> ><br>
> > - Propagate tuning data read errors<br>
> > ---<br>
> > README.rst | 6 -<br>
> > src/ipa/raspberrypi/controller/algorithm.cpp | 2 +-<br>
> > src/ipa/raspberrypi/controller/algorithm.h | 6 +-<br>
> > src/ipa/raspberrypi/controller/controller.cpp | 27 ++--<br>
> > src/ipa/raspberrypi/controller/pwl.cpp | 12 +-<br>
> > src/ipa/raspberrypi/controller/pwl.h | 4 +-<br>
> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 136 ++++++++++--------<br>
> > src/ipa/raspberrypi/controller/rpi/agc.h | 10 +-<br>
> > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 105 +++++++-------<br>
> > src/ipa/raspberrypi/controller/rpi/alsc.h | 2 +-<br>
> > src/ipa/raspberrypi/controller/rpi/awb.cpp | 134 ++++++++++-------<br>
> > src/ipa/raspberrypi/controller/rpi/awb.h | 8 +-<br>
> > .../controller/rpi/black_level.cpp | 12 +-<br>
> > .../raspberrypi/controller/rpi/black_level.h | 2 +-<br>
> > src/ipa/raspberrypi/controller/rpi/ccm.cpp | 47 +++---<br>
> > src/ipa/raspberrypi/controller/rpi/ccm.h | 4 +-<br>
> > .../raspberrypi/controller/rpi/contrast.cpp | 28 ++--<br>
> > src/ipa/raspberrypi/controller/rpi/contrast.h | 2 +-<br>
> > src/ipa/raspberrypi/controller/rpi/dpc.cpp | 7 +-<br>
> > src/ipa/raspberrypi/controller/rpi/dpc.h | 2 +-<br>
> > src/ipa/raspberrypi/controller/rpi/geq.cpp | 10 +-<br>
> > src/ipa/raspberrypi/controller/rpi/geq.h | 2 +-<br>
> > src/ipa/raspberrypi/controller/rpi/lux.cpp | 30 +++-<br>
> > src/ipa/raspberrypi/controller/rpi/lux.h | 2 +-<br>
> > src/ipa/raspberrypi/controller/rpi/noise.cpp | 14 +-<br>
> > src/ipa/raspberrypi/controller/rpi/noise.h | 2 +-<br>
> > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 6 +-<br>
> > src/ipa/raspberrypi/controller/rpi/sdn.h | 2 +-<br>
> > .../raspberrypi/controller/rpi/sharpen.cpp | 8 +-<br>
> > src/ipa/raspberrypi/controller/rpi/sharpen.h | 2 +-<br>
> > src/ipa/raspberrypi/meson.build | 1 -<br>
> > src/ipa/raspberrypi/raspberrypi.cpp | 1 +<br>
> > 32 files changed, 362 insertions(+), 274 deletions(-)<br>
<br>
[snip]<br>
<br>
> > diff --git a/src/ipa/raspberrypi/controller/pwl.cpp b/src/ipa/raspberrypi/controller/pwl.cpp<br>
> > index fde0b298c6ce..8e6201920062 100644<br>
> > --- a/src/ipa/raspberrypi/controller/pwl.cpp<br>
> > +++ b/src/ipa/raspberrypi/controller/pwl.cpp<br>
> > @@ -12,13 +12,15 @@<br>
> ><br>
> > using namespace RPiController;<br>
> ><br>
> > -int Pwl::read(boost::property_tree::ptree const ¶ms)<br>
> > +int Pwl::read(const libcamera::YamlObject ¶ms)<br>
> > {<br>
> > - for (auto it = params.begin(); it != params.end(); it++) {<br>
> > - double x = it->second.get_value<double>();<br>
> > - assert(it == params.begin() || x > points_.back().x);<br>
> > + const auto &list = params.asList();<br>
> > +<br>
> > + for (auto it = list.begin(); it != list.end(); it++) {<br>
> > + double x = it->get<double>(0.0);<br>
> > + assert(it == list.begin() || x > points_.back().x);<br>
> > it++;<br>
> > - double y = it->second.get_value<double>();<br>
> > + double y = it->get<double>(0.0);<br>
> <br>
> This would be a candidate to not provide a default value and test<br>
> the std::optional validity. If we have an odd number of points, that<br>
> would be a malformed PWL.<br>
<br>
You're right, that was a leftover of the previous version. I'll add<br>
error checks.<br>
<br>
> > points_.push_back(Point(x, y));<br>
> > }<br>
> > assert(points_.size() >= 2);<br>
<br>
[snip]<br>
<br>
> > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp<br>
> > index 10c49c126341..d6d312993cdd 100644<br>
> > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp<br>
> > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp<br>
<br>
[snip]<br>
<br>
> > -static int readCtCurve(Pwl &ctR, Pwl &ctB,<br>
> > - boost::property_tree::ptree const ¶ms)<br>
> > +static int readCtCurve(Pwl &ctR, Pwl &ctB, const libcamera::YamlObject ¶ms)<br>
> > {<br>
> > - int num = 0;<br>
> > - for (auto it = params.begin(); it != params.end(); it++) {<br>
> > - double ct = it->second.get_value<double>();<br>
> > - assert(it == params.begin() || ct != ctR.domain().end);<br>
> > - if (++it == params.end()) {<br>
> > - LOG(RPiAwb, Error) << "AwbConfig: incomplete CT curve entry";<br>
> > - return -EINVAL;<br>
> > - }<br>
> > - ctR.append(ct, it->second.get_value<double>());<br>
> > - if (++it == params.end()) {<br>
> > - LOG(RPiAwb, Error) << "AwbConfig: incomplete CT curve entry";<br>
> > - return -EINVAL;<br>
> > - }<br>
> > - ctB.append(ct, it->second.get_value<double>());<br>
> > - num++;<br>
> > + if (params.size() % 3) {<br>
> > + LOG(RPiAwb, Error) << "AwbConfig: incomplete CT curve entry";<br>
> > + return -EINVAL;<br>
> > }<br>
> > - if (num < 2) {<br>
> > +<br>
> > + if (params.size() < 6) {<br>
> > LOG(RPiAwb, Error) << "AwbConfig: insufficient points in CT curve";<br>
> > return -EINVAL;<br>
> > }<br>
> > +<br>
> > + const auto &list = params.asList();<br>
> > +<br>
> > + for (auto it = list.begin(); it != list.end(); it++) {<br>
> > + auto value = it->get<double>();<br>
> > + if (!value)<br>
> > + return -EINVAL;<br>
> > + double ct = *value;<br>
> > +<br>
> > + assert(it == list.begin() || ct != ctR.domain().end);<br>
> > +<br>
> > + value = (++it)->get<double>();<br>
> > + if (!value)<br>
> > + return -EINVAL;<br>
> > + ctR.append(ct, *value);<br>
> > +<br>
> > + value = (++it)->get<double>();<br>
> > + if (!value)<br>
> > + return -EINVAL;<br>
> > + ctB.append(ct, *value);<br>
> > + }<br>
> <br>
> The if (!value) tests are probably unneeded in this loop as you test<br>
> if (params.size() % 3) earlier. Or are we also guarding against values<br>
> that may not be numeric, in which case, the test is needed?<br>
<br>
That was the idea, yes. I don't want to write<br>
<br>
ctR.append(ct, *(++it)->get<double>());<br>
<br>
as it will crash if the value isn't convertible to double. If you think<br>
the only problem we should protect against is an incorrect number of<br>
values, but incorrect values should be blamed on the user and silently<br>
ignored, then I would write<br>
<br>
ctR.append(ct, (++it)->get<double>(0.0));<br>
<br>
What would you prefer ? I'll apply the same to the Pwl::read() above,<br>
and will either only test that the number of elements is even, or test<br>
each element, and to Matrix::read() below.</blockquote></div></div><div dir="auto"><br></div><div dir="auto">Feel free to leave the changes as is, I just wanted to check for my understanding.</div><div dir="auto"><br></div><div dir="auto">Naush</div><div dir="auto"><br></div><div dir="auto"></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> > +<br>
> > return 0;<br>
> > }<br>
<br>
[snip]<br>
<br>
> > diff --git a/src/ipa/raspberrypi/controller/rpi/ccm.cpp b/src/ipa/raspberrypi/controller/rpi/ccm.cpp<br>
> > index 9588e94adeb1..2e2e66647e86 100644<br>
> > --- a/src/ipa/raspberrypi/controller/rpi/ccm.cpp<br>
> > +++ b/src/ipa/raspberrypi/controller/rpi/ccm.cpp<br>
> > @@ -39,21 +39,22 @@ Matrix::Matrix(double m0, double m1, double m2, double m3, double m4, double m5,<br>
> > m[0][0] = m0, m[0][1] = m1, m[0][2] = m2, m[1][0] = m3, m[1][1] = m4,<br>
> > m[1][2] = m5, m[2][0] = m6, m[2][1] = m7, m[2][2] = m8;<br>
> > }<br>
> > -int Matrix::read(boost::property_tree::ptree const ¶ms)<br>
> > +int Matrix::read(const libcamera::YamlObject ¶ms)<br>
> > {<br>
> > double *ptr = (double *)m;<br>
> > - int n = 0;<br>
> > - for (auto it = params.begin(); it != params.end(); it++) {<br>
> > - if (n++ == 9) {<br>
> > - LOG(RPiCcm, Error) << "Too many values in CCM";<br>
> > - return -EINVAL;<br>
> > - }<br>
> > - *ptr++ = it->second.get_value<double>();<br>
> > - }<br>
> > - if (n < 9) {<br>
> > - LOG(RPiCcm, Error) << "Too few values in CCM";<br>
> > +<br>
> > + if (params.size() != 9) {<br>
> > + LOG(RPiCcm, Error) << "Wrong number of values in CCM";<br>
> > return -EINVAL;<br>
> > }<br>
> > +<br>
> > + for (const auto ¶m : params.asList()) {<br>
> > + auto value = param.get<double>();<br>
> > + if (!value)<br>
> > + return -EINVAL;<br>
> > + *ptr++ = *value;<br>
> > + }<br>
> > +<br>
> <br>
> Again, if (!value) not needed here (with the same provision as earlier)?<br>
> <br>
> > return 0;<br>
> > }<br>
> ><br>
<br>
[snip]<br>
<br>
> > diff --git a/src/ipa/raspberrypi/controller/rpi/lux.cpp b/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
> > index ca1e827191fd..ea1623dda345 100644<br>
> > --- a/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
> > +++ b/src/ipa/raspberrypi/controller/rpi/lux.cpp<br>
> > @@ -38,14 +38,30 @@ char const *Lux::name() const<br>
> > return NAME;<br>
> > }<br>
> ><br>
> > -int Lux::read(boost::property_tree::ptree const ¶ms)<br>
> > +int Lux::read(const libcamera::YamlObject ¶ms)<br>
> > {<br>
> > - referenceShutterSpeed_ =<br>
> > - params.get<double>("reference_shutter_speed") * 1.0us;<br>
> > - referenceGain_ = params.get<double>("reference_gain");<br>
> > - referenceAperture_ = params.get<double>("reference_aperture", 1.0);<br>
> > - referenceY_ = params.get<double>("reference_Y");<br>
> > - referenceLux_ = params.get<double>("reference_lux");<br>
> > + auto value = params["reference_shutter_speed"].get<double>();<br>
> > + if (!value)<br>
> > + return -EINVAL;<br>
> > + referenceShutterSpeed_ = *value * 1.0us;<br>
> > +<br>
> > + value = params["reference_gain"].get<double>();<br>
> > + if (!value)<br>
> > + return -EINVAL;<br>
> > + referenceGain_ = *value;<br>
> > +<br>
> > + referenceAperture_ = params["reference_aperture"].get<double>(1.0);<br>
> > +<br>
> > + value = params["reference_Y"].get<double>(0.0);<br>
> > + if (!value)<br>
> > + return -EINVAL;<br>
> > + referenceY_ = *value;<br>
> > +<br>
> > + value = params["reference_lux"].get<double>(0.0);<br>
> > + if (!value)<br>
> > + return -EINVAL;<br>
> > + referenceLux_ = *value;<br>
> > +<br>
> <br>
> reference_Y and reference_lux should not take in default values above.<br>
<br>
I'll fix that too.<br>
<br>
> > currentAperture_ = referenceAperture_;<br>
> > return 0;<br>
> > }<br>
<br>
[snip]<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div></div>