<div dir="ltr"><div dir="ltr">Hi Laurent,<br><br>Thank you for your work. It would indeed be very nice to get rid of the boost<br>json parser dependency if we can. I've had a quick go at trying this series out<br>and unfortunately hit a fundamental problem.<br><br>In the existing boost parser, Controller::Read() would populate the algorithms<br>vector in the order they appeared in the json file. With the new yaml parser,<br>they are populated in alphabetical order. This breaks certain behavior in our<br>controllers where, for example, we want the Lux algorithm to always run first<br>and populate the metadata with the estimated lux value for other algorithms to<br>use. I've not fully checked, but I think Lux, Noise and AWB needs to run (in<br>that order) before any other algorithm. The other algorithms can run in any</div><div dir="ltr">given order.<br><br>Perhaps a way forward would be to have an "order" key in our json files that<br>specify algorithm ordering, so we decouple it from the parser used? I worry<br>this could lead to unexpected problems where a user might add a block but it may<br>not run as it's not in the order list. Maybe this order list does not need to<br>list all algorithms, just the one it wants ordered?<br><br>Regards,<br>Naush<br><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 24 May 2022 at 23:58, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.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">Hello,<br>
<br>
As mentioned in the subject, this patch series replaces the boost JSON<br>
parser with the YamlParser implementation based on libyaml for the<br>
Raspberry Pi IPA module.<br>
<br>
The series starts with a few cleanups and extensions in YamlParser, with<br>
patch 01/12 turning the enum Type into an enum class, patch 02/12<br>
extending the size() function to support dictionaries in addition to<br>
lists, and patch 03/12 moving the parser from using FILE to File for a<br>
better RAII-style management of files. Patch 04/12 continues with a<br>
small cleanup in the YamlParser unit tests.<br>
<br>
Patch 05/12 introduces the first big change in the series: a new<br>
iterator API for YamlObject. This allows iterating over the elements of<br>
a list of dictionaly object. Unlike the boost property_tree<br>
implementation that iterates over a pair of std::string key and ptree<br>
value, the YamlObject iterators differ for lists and dictionaries. This<br>
provides a more familiar API for lists, at the expense of requiring<br>
adapter objects (see the asDict() and asList() functions). I'm<br>
interested in feedback on this approach, hence the RFC. Patch 06/12<br>
then extends the unit tests to cover the iterator API, and patch 07/12<br>
uses the API in the Android camera HAL. Patch 08/12 completes that part<br>
of the series by removing the then unused memberNames() function.<br>
<br>
The series continue with two extensions to the YamlParser. In patch<br>
09/12 the get() function receives a fix to properly check the value<br>
range for 32-bit integers, and in patch 10/12 new specializations of<br>
the function are added for 16-bit integers, needed by the Raspberry Pi<br>
IPA.<br>
<br>
The last part of the series converts the Raspberry Pi IPA from boost to<br>
YamlParser. Small changes are needed in patch 11/12 to convert tabs to<br>
spaces in the tuning JSON files, as they confuse the YAML parser. Patch<br>
12/12 then converts the IPA module, and drops the dependency on boost.<br>
<br>
When compiled in release mode with clang-13, the Raspberry Pi IPA module<br>
.text section shrank from 1317086 to 963614 bytes.<br>
<br>
I haven't been able to test the series on a Raspberry Pi yet (second<br>
reason for the RFC) as I don't have access to my board at the moment.<br>
<br>
Laurent Pinchart (12):<br>
libcamera: yaml_object: Turn Type into an enum class<br>
libcamera: yaml_parser: Extend YamlObject::size() to dictionaries<br>
libcamera: yaml_parser: Switch from FILE to File<br>
test: yaml-parser: Use write() instead of fwrite()<br>
libcamera: yaml_parser: Add iterator API<br>
test: yaml_parser: Extend tests to cover the iterator API<br>
android: Use the YamlObject iterator API<br>
libcamera: yaml_parser: Remove memberNames() function<br>
libcamera: yaml_parser: Fix range checks for 32-bit integers<br>
libcamera: yaml_parser: Add get() specializations for 16-bit integers<br>
ipa: raspberrypi: Replace tabs with spaces in tuning data files<br>
ipa: raspberrypi: Use YamlParser to replace dependency on boost<br>
<br>
README.rst | 6 -<br>
include/libcamera/internal/yaml_parser.h | 133 +++++++++-<br>
src/android/camera_hal_config.cpp | 22 +-<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/data/imx219.json | 8 +-<br>
src/ipa/raspberrypi/data/imx219_noir.json | 10 +-<br>
src/ipa/raspberrypi/data/imx290.json | 18 +-<br>
src/ipa/raspberrypi/data/imx477.json | 8 +-<br>
src/ipa/raspberrypi/data/imx477_noir.json | 10 +-<br>
src/ipa/raspberrypi/data/ov5647.json | 10 +-<br>
src/ipa/raspberrypi/data/ov5647_noir.json | 12 +-<br>
src/ipa/raspberrypi/data/se327m12.json | 6 +-<br>
src/ipa/raspberrypi/meson.build | 1 -<br>
src/ipa/raspberrypi/raspberrypi.cpp | 1 +<br>
src/libcamera/yaml_parser.cpp | 232 +++++++++++++-----<br>
test/yaml-parser.cpp | 109 +++++---<br>
44 files changed, 659 insertions(+), 400 deletions(-)<br>
<br>
<br>
base-commit: 153b468930a9df22debb28889312f8a5c511ee04<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
<br>
</blockquote></div></div>