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