[libcamera-devel] [PATCH v6 4/8] ipa: raspberrypi: Use YamlParser to replace dependency on boost
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jul 26 10:15:08 CEST 2022
Hi Naush,
On Tue, Jul 26, 2022 at 08:04:57AM +0100, Naushir Patuck wrote:
> On Mon, 25 Jul 2022 at 16:49, David Plowman wrote:
> > On Mon, 25 Jul 2022 at 15:48, Naushir Patuck wrote:
> > > On Mon, 25 Jul 2022 at 14:50, Laurent Pinchart wrote:
> > >> On Mon, Jul 25, 2022 at 11:50:06AM +0100, David Plowman wrote:
> > >> > On Mon, 25 Jul 2022 at 11:26, Naushir Patuck wrote:
> > >> > > On Fri, 22 Jul 2022 at 20:25, Laurent Pinchart 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()?
Funny you mention that, it took part of last night to implement it :-)
I'll post patches soon.
> > >> > > 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
More information about the libcamera-devel
mailing list