<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 25 Jul 2022 at 16:49, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi everyone<br>
<br>
On Mon, 25 Jul 2022 at 15:48, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> Hi Laurent,<br>
><br>
> On Mon, 25 Jul 2022 at 14:50, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br>
>><br>
>> Hello,<br>
>><br>
>> On Mon, Jul 25, 2022 at 11:50:06AM +0100, David Plowman wrote:<br>
>> > On Mon, 25 Jul 2022 at 11:26, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
>> > > On Fri, 22 Jul 2022 at 20:25, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br>
>> > >> On Mon, Jul 18, 2022 at 09:15:58AM +0100, Naushir Patuck wrote:<br>
>> > >> > From: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><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">laurent.pinchart@ideasonboard.com</a>><br>
>> > >><br>
>> > >> I assume you're fine with this patch as you've posted it as part of your<br>
>> > >> series, but could you give it a review ? :-)<br>
>> > ><br>
>> > > I did have one outstanding query on this about default values.<br>
>> > ><br>
>> > > If you recall, we discussed having YamlObject::get() not take in a default value<br>
>> > > and complain loudly if the key is not present, similar to what the Boost parser<br>
>> > > does now. This will help avoid possibly hard to debug issues if a critical<br>
>> > > parameter is missing from the tuning config. David, what are your thoughts?<br>
>> ><br>
>> > I agree it would be nice to get some kind of error or at least warning<br>
>> > if something that we really expected is not present. Possibly we could<br>
>> > review some of those and provide default values in more cases, but I'm<br>
>> > guessing that's not the kind of thing to "hide" inside a patch that<br>
>> > isn't meant to change the behaviour of any algorithms.<br>
>><br>
>> I agree, that's a good point.<br>
>><br>
>> The boost json parser will throw an exception in this case, which I<br>
>> can't replicate in YamlObject. I can update the Read() function to<br>
>> return an error, would it be enough to log a message at the top level,<br>
>> or would you like individual log statements in every location to tell<br>
>> which parameter was missing ?<br>
><br>
><br>
> Having read() return an error code - provided no default is set is a good idea.<br>
> Then the top level can decide how to handle this - a LOG(Fatal) message<br>
> in our case. David, what do you think?<br>
<br>
Works for me!<br></blockquote><div><br></div><div>Would it make sense to use std::optional as a return value from YamlObject::get() in</div><div>a similar way to what we now do for ControlList::get()?</div><div><br></div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
David<br>
<br>
><br>
> Regards,<br>
> Naush<br>
><br>
>><br>
>> > > Do you think this is something that we can add easily to this patch?<br>
>> > ><br>
>> > >> > ---<br>
>> > >> > README.rst | 6 --<br>
>> > >> > src/ipa/raspberrypi/controller/algorithm.cpp | 2 +-<br>
>> > >> > src/ipa/raspberrypi/controller/algorithm.hpp | 6 +-<br>
>> > >> > src/ipa/raspberrypi/controller/controller.cpp | 27 ++++--<br>
>> > >> > src/ipa/raspberrypi/controller/pwl.cpp | 12 ++-<br>
>> > >> > src/ipa/raspberrypi/controller/pwl.hpp | 5 +-<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 94 +++++++++----------<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 10 +-<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 94 +++++++++----------<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/alsc.hpp | 2 +-<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/awb.cpp | 89 +++++++++---------<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/awb.hpp | 8 +-<br>
>> > >> > .../controller/rpi/black_level.cpp | 12 +--<br>
>> > >> > .../controller/rpi/black_level.hpp | 2 +-<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/ccm.cpp | 28 +++---<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/ccm.hpp | 4 +-<br>
>> > >> > .../raspberrypi/controller/rpi/contrast.cpp | 18 ++--<br>
>> > >> > .../raspberrypi/controller/rpi/contrast.hpp | 2 +-<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/dpc.cpp | 4 +-<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/dpc.hpp | 2 +-<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/geq.cpp | 10 +-<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/geq.hpp | 2 +-<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/lux.cpp | 12 +--<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/lux.hpp | 2 +-<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/noise.cpp | 6 +-<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/noise.hpp | 2 +-<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 6 +-<br>
>> > >> > src/ipa/raspberrypi/controller/rpi/sdn.hpp | 2 +-<br>
>> > >> > .../raspberrypi/controller/rpi/sharpen.cpp | 8 +-<br>
>> > >> > .../raspberrypi/controller/rpi/sharpen.hpp | 2 +-<br>
>> > >> > src/ipa/raspberrypi/meson.build | 1 -<br>
>> > >> > src/ipa/raspberrypi/raspberrypi.cpp | 1 +<br>
>> > >> > 32 files changed, 241 insertions(+), 240 deletions(-)<br>
>><br>
>> --<br>
>> Regards,<br>
>><br>
>> Laurent Pinchart<br>
</blockquote></div></div>