[libcamera-devel,v4,0/9] Add DPF tuning support for RkISP1

17127 mbox series

Hello,

It turned out to be one of those days where you're about to push a
series of patches, and the last minute compilation breakage with an
obscure combination of compiler version and optimization flags turn 3
patches into 9.

So, compared to v3, this series starts with 6 patches that add support
for 8-bit integer parsing to the YamlObject class. That could have been
done in a single patch (5/9), if it wasn't for the fact that the
corresponding unit test uncovered an issue with bounds checking when
parsing 16-bit integers, as shown by a new unit test (3/9) and then
fixed (4/9). I wasn't happy with the resulting code duplication, which I
fixed for the unit tests (1/9 and 2/9) and the YamlObject class (6/9).

After that, it's "just" a new version of the DPF patches. Patch 9/9 now
uses the uint8_t version of the YamlObject::getList() function for the
domain filter coefficients, which fixes the aforementioned compilation
warning with gcc 12.1.0 in -O3 mode (OK, it's not *that* obscure).

If anyone is curious about the gcc warning, which I believe is a false
positive and a compiler bug, here's a minimal test case:

--------
#include <algorithm>
#include <vector>

extern unsigned char dst[6];

int foo(const std::vector<unsigned short> &src)
{
	if (src.size() != 5 && src.size() != 6)
		return 1;

	std::copy_n(src.begin(), src.size(), std::begin(dst));

	return 0;
}
--------

$ g++-12.1.0 -Wall -Werror -std=c++17 -O3 -o stringop-overflow.o -c stringop-overflow.cpp 
In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/algorithm:60,
                 from stringop-overflow.cpp:1:
In static member function ‘static _OI std::__copy_move<false, false, std::random_access_iterator_tag>::__copy_m(_II, _II, _OI) [with _II = const short unsigned int*; _OI = unsigned char*]’,
    inlined from ‘_OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = false; _II = const short unsigned int*; _OI = unsigned char*]’ at /usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algobase.h:495:30,
    inlined from ‘_OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove = false; _II = const short unsigned int*; _OI = unsigned char*]’ at /usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algobase.h:522:42,
    inlined from ‘_OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = false; _II = __gnu_cxx::__normal_iterator<const short unsigned int*, vector<short unsigned int> >; _OI = unsigned char*]’ at /usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algobase.h:529:31,
    inlined from ‘_OI std::copy(_II, _II, _OI) [with _II = __gnu_cxx::__normal_iterator<const short unsigned int*, vector<short unsigned int> >; _OI = unsigned char*]’ at /usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algobase.h:620:7,
    inlined from ‘_OutputIterator std::__copy_n(_RandomAccessIterator, _Size, _OutputIterator, random_access_iterator_tag) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<const short unsigned int*, vector<short unsigned int> >; _Size = long unsigned int; _OutputIterator = unsigned char*]’ at /usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algo.h:728:23,
    inlined from ‘_OIter std::copy_n(_IIter, _Size, _OIter) [with _IIter = __gnu_cxx::__normal_iterator<const short unsigned int*, vector<short unsigned int> >; _Size = long unsigned int; _OIter = unsigned char*]’ at /usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algo.h:760:27,
    inlined from ‘int foo(const std::vector<short unsigned int>&)’ at stringop-overflow.cpp:11:13:
/usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algobase.h:385:25: error: writing 8 bytes into a region of size 6 [-Werror=stringop-overflow=]
  385 |               *__result = *__first;
      |               ~~~~~~~~~~^~~~~~~~~~
stringop-overflow.cpp: In function ‘int foo(const std::vector<short unsigned int>&)’:
stringop-overflow.cpp:4:22: note: destination object ‘dst’ of size 6
    4 | extern unsigned char dst[6];
      |                      ^~~
In static member function ‘static _OI std::__copy_move<false, false, std::random_access_iterator_tag>::__copy_m(_II, _II, _OI) [with _II = const short unsigned int*; _OI = unsigned char*]’,
    inlined from ‘_OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = false; _II = const short unsigned int*; _OI = unsigned char*]’ at /usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algobase.h:495:30,
    inlined from ‘_OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove = false; _II = const short unsigned int*; _OI = unsigned char*]’ at /usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algobase.h:522:42,
    inlined from ‘_OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = false; _II = __gnu_cxx::__normal_iterator<const short unsigned int*, vector<short unsigned int> >; _OI = unsigned char*]’ at /usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algobase.h:529:31,
    inlined from ‘_OI std::copy(_II, _II, _OI) [with _II = __gnu_cxx::__normal_iterator<const short unsigned int*, vector<short unsigned int> >; _OI = unsigned char*]’ at /usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algobase.h:620:7,
    inlined from ‘_OutputIterator std::__copy_n(_RandomAccessIterator, _Size, _OutputIterator, random_access_iterator_tag) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<const short unsigned int*, vector<short unsigned int> >; _Size = long unsigned int; _OutputIterator = unsigned char*]’ at /usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algo.h:728:23,
    inlined from ‘_OIter std::copy_n(_IIter, _Size, _OIter) [with _IIter = __gnu_cxx::__normal_iterator<const short unsigned int*, vector<short unsigned int> >; _Size = long unsigned int; _OIter = unsigned char*]’ at /usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algo.h:760:27,
    inlined from ‘int foo(const std::vector<short unsigned int>&)’ at stringop-overflow.cpp:11:13:
/usr/lib/gcc/x86_64-pc-linux-gnu/12.1.0/include/g++-v12/bits/stl_algobase.h:385:25: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
  385 |               *__result = *__first;
      |               ~~~~~~~~~~^~~~~~~~~~
stringop-overflow.cpp: In function ‘int foo(const std::vector<short unsigned int>&)’:
stringop-overflow.cpp:4:22: note: at offset [22, 4611686018427387894] into destination object ‘dst’ of size 6
    4 | extern unsigned char dst[6];
      |                      ^~~
stringop-overflow.cpp:4:22: note: at offset 6 into destination object ‘dst’ of size 6
stringop-overflow.cpp:4:22: note: at offset [22, 4611686018427387894] into destination object ‘dst’ of size 6
stringop-overflow.cpp:4:22: note: at offset 6 into destination object ‘dst’ of size 6
cc1plus: all warnings being treated as errors


Florian Sylvestre (3):
  ipa: rkisp1: Add enable field for AWB algorithm in IPA context
  ipa: rkisp1: Add enable field for LSC algorithm in IPA context
  ipa: rkisp1: Add support of Denoise Pre-Filter control

Laurent Pinchart (6):
  test: yaml-parser: Simplify code by centralizing parse error checks
  test: yaml-parser: Centralize integer parse checks
  test: yaml-parser: Test out-of-range checks on integer parsing
  libcamera: yaml_parser: Fix bounds checking for 16-bit
    YamlObject::get()
  libcamera: yaml_parser: Enable YamlObject::get() for int8_t and
    uint8_t
  libcamera: yaml_parser: De-duplicate common code in YamlObject::get()

 include/libcamera/internal/yaml_parser.h |   4 +
 src/ipa/rkisp1/algorithms/awb.cpp        |   2 +
 src/ipa/rkisp1/algorithms/dpf.cpp        | 258 ++++++++++++
 src/ipa/rkisp1/algorithms/dpf.h          |  36 ++
 src/ipa/rkisp1/algorithms/lsc.cpp        |  10 +
 src/ipa/rkisp1/algorithms/lsc.h          |   1 +
 src/ipa/rkisp1/algorithms/meson.build    |   1 +
 src/ipa/rkisp1/data/ov5640.yaml          |  15 +
 src/ipa/rkisp1/ipa_context.cpp           |  22 +
 src/ipa/rkisp1/ipa_context.h             |  10 +
 src/libcamera/yaml_parser.cpp            | 157 ++++---
 test/yaml-parser.cpp                     | 504 ++++++++++++-----------
 12 files changed, 712 insertions(+), 308 deletions(-)
 create mode 100644 src/ipa/rkisp1/algorithms/dpf.cpp
 create mode 100644 src/ipa/rkisp1/algorithms/dpf.h


base-commit: dfc6d711c9f7f0a9868afa5158aa2089163bded3