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

[Naushir Patuck]

On Mon, 25 Jul 2022 at 16:49, David Plowman <david.plowman at raspberrypi.com>
wrote:

> Hi everyone
>
> On Mon, 25 Jul 2022 at 15:48, Naushir Patuck <naush at raspberrypi.com>
> wrote:
> >
> > Hi Laurent,
> >
> > On Mon, 25 Jul 2022 at 14:50, Laurent Pinchart <
> laurent.pinchart at ideasonboard.com> wrote:
> >>
> >> Hello,
> >>
> >> On Mon, Jul 25, 2022 at 11:50:06AM +0100, David Plowman wrote:
> >> > On Mon, 25 Jul 2022 at 11:26, Naushir Patuck <naush at raspberrypi.com>
> wrote:
> >> > > On Fri, 22 Jul 2022 at 20:25, Laurent Pinchart <
> laurent.pinchart at ideasonboard.com> wrote:
> >> > >> 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.
> >>
> >> I agree, that's a good point.
> >>
> >> The boost json parser will throw an exception in this case, which I
> >> can't replicate in YamlObject. I can update the Read() function to
> >> return an error, would it be enough to log a message at the top level,
> >> or would you like individual log statements in every location to tell
> >> which parameter was missing ?
> >
> >
> > Having read() return an error code - provided no default is set is a
> good idea.
> > Then the top level can decide how to handle this - a LOG(Fatal) message
> > in our case.  David, what do you think?
>
> Works for me!
>

Would it make sense to use std::optional as a return value from
YamlObject::get() in
a similar way to what we now do for ControlList::get()?

Naush


>
> David
>
> >
> > Regards,
> > Naush
> >
> >>
> >> > > Do you think this is something that we can add easily to this patch?
> >> > >
> >> > >> > ---
> >> > >> >  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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220726/d9a452d2/attachment.htm>