| Message ID | 20220616150535.21742-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Would you be able to review this ? And could I volunteer you to handle the items listed below to make this a non-RFC series ? :-) That would be - Adding support for a new tuning data format that doesn't rely on YAML mapping ordering (it's really just about turning the mapping into a list of single-entry mappings, and while at it, adding a top-level version number element). - Update ctt to generate the new format. - Add a tool (Python seems to be the best option) to convert tuning files from the existing format to the new format. On Thu, Jun 16, 2022 at 06:05:31PM +0300, Laurent Pinchart via libcamera-devel 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. > > Compared to v2, the base work that introduces the iterator API has been > sent separately in "[PATCH 0/7] libcamera: yaml_parser: Add iterator > API". This series then addresses the specific needs of the Raspberry Pi > IPA module. > > The Raspberry Pi IPA tuning data contains a list of algorithms stored in > a mapping. Both the JSON and the YAML specifications explicitly state > that mappings are not ordered, but the IPA relies on the JSON data order > being preserved by the parser. This is an implementation-specific > behaviour that happens to be match the boost JSON parser implementation, > but not the YamlParser class. > > While in a perfect world we would update the tuning data format to avoid > this problem, we don't want to break every tuning file out there, and > thus need to ensure backward compatibility. This is why patches 1/4 and > 2/4 update the parser and iterator implementation to preserve the data > order, and patches 3/4 and 4/4 then move the Raspberry Pi IPA from boost > to YamlParser. > > If this approach is deemed reasonable, then the next non-RFC version of > the series should also include a new format for the Raspberry Pi IPA > tuning data that would be compliant with the JSON specification, an > update to the camera tuning tool to generate tuning files in that > format, and possibly a Python script to convert existing files to the > new format. > > Laurent Pinchart (4): > test: yaml-parser: Test dictionary items ordering > libcamera: yaml_parser: Preserve order of items in dictionary > 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 | 37 +++++--- > 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 | 35 ++++--- > test/yaml-parser.cpp | 17 ++-- > 43 files changed, 334 insertions(+), 318 deletions(-) > > > base-commit: c5ab0f3b64280733a10b2da39e522fe87d0d51f0 > -- > Regards, > > Laurent Pinchart >
Hi Laurent, On Mon, 11 Jul 2022 at 21:18, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Would you be able to review this ? And could I volunteer you to handle > the items listed below to make this a non-RFC series ? :-) That would be > > - Adding support for a new tuning data format that doesn't rely on YAML > mapping ordering (it's really just about turning the mapping into a > list of single-entry mappings, and while at it, adding a top-level > version number element). > > - Update ctt to generate the new format. > > - Add a tool (Python seems to be the best option) to convert tuning > files from the existing format to the new format. > Sure, I'll get to that as soon as I can. Regards, Naush > > On Thu, Jun 16, 2022 at 06:05:31PM +0300, Laurent Pinchart via > libcamera-devel 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. > > > > Compared to v2, the base work that introduces the iterator API has been > > sent separately in "[PATCH 0/7] libcamera: yaml_parser: Add iterator > > API". This series then addresses the specific needs of the Raspberry Pi > > IPA module. > > > > The Raspberry Pi IPA tuning data contains a list of algorithms stored in > > a mapping. Both the JSON and the YAML specifications explicitly state > > that mappings are not ordered, but the IPA relies on the JSON data order > > being preserved by the parser. This is an implementation-specific > > behaviour that happens to be match the boost JSON parser implementation, > > but not the YamlParser class. > > > > While in a perfect world we would update the tuning data format to avoid > > this problem, we don't want to break every tuning file out there, and > > thus need to ensure backward compatibility. This is why patches 1/4 and > > 2/4 update the parser and iterator implementation to preserve the data > > order, and patches 3/4 and 4/4 then move the Raspberry Pi IPA from boost > > to YamlParser. > > > > If this approach is deemed reasonable, then the next non-RFC version of > > the series should also include a new format for the Raspberry Pi IPA > > tuning data that would be compliant with the JSON specification, an > > update to the camera tuning tool to generate tuning files in that > > format, and possibly a Python script to convert existing files to the > > new format. > > > > Laurent Pinchart (4): > > test: yaml-parser: Test dictionary items ordering > > libcamera: yaml_parser: Preserve order of items in dictionary > > 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 | 37 +++++--- > > 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 | 35 ++++--- > > test/yaml-parser.cpp | 17 ++-- > > 43 files changed, 334 insertions(+), 318 deletions(-) > > > > > > base-commit: c5ab0f3b64280733a10b2da39e522fe87d0d51f0 > > -- > > Regards, > > > > Laurent Pinchart > > > > -- > Regards, > > Laurent Pinchart >