[libcamera-devel] [PATCH v6 4/8] ipa: raspberrypi: Use YamlParser to replace dependency on boost

[David Plowman]

Hi Naush, Laurent

On Mon, 25 Jul 2022 at 11:26, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Hi Laurent,
>
>
> On Fri, 22 Jul 2022 at 20:25, Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:
>>
>> Hi Naush,
>>
>> On Mon, Jul 18, 2022 at 09:15:58AM +0100, Naushir Patuck wrote:
>> > From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> >
>> > The Raspberry Pi IPA module depends on boost only to parse the JSON
>> > tuning data files. As libcamera depends on libyaml, use the YamlParser
>> > class to parse those files and drop the dependency on boost.
>> >
>> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>
>> I assume you're fine with this patch as you've posted it as part of your
>> series, but could you give it a review ? :-)
>
>
> I did have one outstanding query on this about default values.
>
> If you recall, we discussed having YamlObject::get() not take in a default value
> and complain loudly if the key is not present, similar to what the Boost parser
> does now.  This will help avoid possibly hard to debug issues if a critical
> parameter is missing from the tuning config.  David, what are your thoughts?

I agree it would be nice to get some kind of error or at least warning
if something that we really expected is not present. Possibly we could
review some of those and provide default values in more cases, but I'm
guessing that's not the kind of thing to "hide" inside a patch that
isn't meant to change the behaviour of any algorithms.

Thanks!
David

>
> Do you think this is something that we can add easily to this patch?
>
> Regards,
> Naush
>
>>
>> > ---
>> >  README.rst                                    |  6 --
>> >  src/ipa/raspberrypi/controller/algorithm.cpp  |  2 +-
>> >  src/ipa/raspberrypi/controller/algorithm.hpp  |  6 +-
>> >  src/ipa/raspberrypi/controller/controller.cpp | 27 ++++--
>> >  src/ipa/raspberrypi/controller/pwl.cpp        | 12 ++-
>> >  src/ipa/raspberrypi/controller/pwl.hpp        |  5 +-
>> >  src/ipa/raspberrypi/controller/rpi/agc.cpp    | 94 +++++++++----------
>> >  src/ipa/raspberrypi/controller/rpi/agc.hpp    | 10 +-
>> >  src/ipa/raspberrypi/controller/rpi/alsc.cpp   | 94 +++++++++----------
>> >  src/ipa/raspberrypi/controller/rpi/alsc.hpp   |  2 +-
>> >  src/ipa/raspberrypi/controller/rpi/awb.cpp    | 89 +++++++++---------
>> >  src/ipa/raspberrypi/controller/rpi/awb.hpp    |  8 +-
>> >  .../controller/rpi/black_level.cpp            | 12 +--
>> >  .../controller/rpi/black_level.hpp            |  2 +-
>> >  src/ipa/raspberrypi/controller/rpi/ccm.cpp    | 28 +++---
>> >  src/ipa/raspberrypi/controller/rpi/ccm.hpp    |  4 +-
>> >  .../raspberrypi/controller/rpi/contrast.cpp   | 18 ++--
>> >  .../raspberrypi/controller/rpi/contrast.hpp   |  2 +-
>> >  src/ipa/raspberrypi/controller/rpi/dpc.cpp    |  4 +-
>> >  src/ipa/raspberrypi/controller/rpi/dpc.hpp    |  2 +-
>> >  src/ipa/raspberrypi/controller/rpi/geq.cpp    | 10 +-
>> >  src/ipa/raspberrypi/controller/rpi/geq.hpp    |  2 +-
>> >  src/ipa/raspberrypi/controller/rpi/lux.cpp    | 12 +--
>> >  src/ipa/raspberrypi/controller/rpi/lux.hpp    |  2 +-
>> >  src/ipa/raspberrypi/controller/rpi/noise.cpp  |  6 +-
>> >  src/ipa/raspberrypi/controller/rpi/noise.hpp  |  2 +-
>> >  src/ipa/raspberrypi/controller/rpi/sdn.cpp    |  6 +-
>> >  src/ipa/raspberrypi/controller/rpi/sdn.hpp    |  2 +-
>> >  .../raspberrypi/controller/rpi/sharpen.cpp    |  8 +-
>> >  .../raspberrypi/controller/rpi/sharpen.hpp    |  2 +-
>> >  src/ipa/raspberrypi/meson.build               |  1 -
>> >  src/ipa/raspberrypi/raspberrypi.cpp           |  1 +
>> >  32 files changed, 241 insertions(+), 240 deletions(-)
>>
>> --
>> Regards,
>>
>> Laurent Pinchart