There is nothing inherently non-constexpr in the `Quantized` type. Whether
it can work in `constexpr` contexts depends on the traits type. There is
no reason to explicitly disallow `constexpr` operation. So mark all eligible
methods `constexpr`.
In addition, add some `static_assert()`s to the "quantized" test to check
constexpr operation.
For example, `FixedPointQTraits<...>::toFloat()` is `constexpr`, so this
enables the construction of `{U,}Q<...>` from the underlying quantized
value in `constexpr` contexts, which can be useful for example for
storing default values in e.g. `static constexpr` variables.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
`CameraSensorHelper::gain(uint32_t)` maps a gain code to the actual floating
point gain value. Calling it with `1.0` as the argument will simply get
the real gain for gain code 1. This is most likely not what was intended.
For example, in the case of the `ov2740` sensor, `againMin` is 1, but the
calculated `again10` (1 / 128 ~ 0.078) ends up being < 1, meaning that the
agc algorithm will never lower the exposure.
Fix that by using the maximum of the minimum gain and 1 as `again10`.
Fixes: 950ca85e8a ("ipa: software_isp: AGC: Do not lower gain below 1.0")
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
The controller min frame duration is used to rate limit how often we
run IPAs. Historically this has been set to 33333us, meaning that the
algorithms effectively skip frames when the camera is running faster
than 30fps.
This patch adds a small amount of plumbing that allows this value to
be set in the Raspberry Pi configuration file. Some applications or
platforms (such as Pi 5) are easily capable of running these more
often, should there be a need to do so.
Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
'const auto' and 'auto const' are interchangeable in C++. There are 446
occurrences of the former in the code base, and 67 occurrences of the
latter. Standardize on the winner.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
The standard C library header for the poll() API is poll.h, not
sys/poll.h. The musl C library warns about this:
In file included from src/android/camera_stream.cpp:13:
host/i586-buildroot-linux-musl/sysroot/usr/include/sys/poll.h:1:2: error: #warning redirecting incorrect #include <sys/poll.h> to <poll.h> [-Werror=cpp]
1 | #warning redirecting incorrect #include <sys/poll.h> to <poll.h>
| ^~~~~~~
Fix it by including the correct header.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8
format, a range of 0.0 to (very nearly) 32.0.
Convert usage to the new UQ<5, 8> FixedPoint Quantised type which will
support the conversion, clamping and quantisation so that the metadata
and debug prints can now report the effective gain applied instead of
the potentially inaccurate float.
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
The RKISP1 supports a configurable Hue as part of the colour processing
unit (cproc).
Implement the new control converting to the hardware scale accordingly
and report the applied control in the completed request metadata.
This is implemented as a phase shift of the chrominance values between
-90 and +87.188 degrees according to the datasheet however the type
itself would imply that this is a range between -90 and 89.2969.
Moreover, the hardware applies the inverse phase shift to the operation
expected and documented by libcamera, so we apply a negative scale when
converting the libcamera control to the Q<1,7> type, resulting in a
range of -89.2969 to +90.0 degrees control.
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Presently the colour processing component exposes controls for
brightness, saturation, and contrast to the applications which are
handled and processed accordingly on the ISP.
The implementation lacks reporting the values that are set back to the
application.
Utilise the new Quantised types to provide the values that were applied
to the hardware and report them in the completed request metadata.
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Extend the new Quantized type infrastructure by providing a
FixedPointQTraits template.
This allows construction of fixed point types with a Quantized storage
that allows easy reading of both the underlying quantized type value and
a floating point representation of that same value.
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
The fixedToFloatingPoint does not support unsigned Q types, and
incorrectly sign-extends all values which have the top most bit set in
the quantized values.
Fix this by ensuring that only signed types perform sign extension, and
simplify the calculation for unsigned types.
Convert the storage of the test cases to signed types to correctly
represent their intended purpose, to prevent test failures.
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Frequently when handling data in IPA components we must convert and
store user interface values which may be floating point values, and
perform a specific operation or conversion to quantize this to a
hardware value.
This value may be to a fixed point type, or more custom code mappings,
but in either case it is important to contain both the required hardware
value, with its effective quantized value.
Provide a new storage type 'Quantized' which can be defined based on a
set of type specific Traits to perform the conversions between floats
and the underlying hardware type.
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
With the GPU accelerated softISP 2 separate benchmark results are printed,
1 for the generation of the output images on the GPU and a separate one
for generating the statistics on the CPU.
Add a new name argument to the Benchmark class descriptor and print this
out when printing the benchmark result.
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
The lens dewarp implementation done in commit 1784e08be3 ("libcamera:
dw100_vertexmap: Implement parametric dewarping") handles the dewarp
parameter p2 incorrectly because it uses the already dewarped x value as
input for the calculation of the y value. Fix that by using separate
variables for the output value. Do so for x and y to keep the code
symmetric even if it is only strictly required for y.
Fixes: 1784e08be3 ("libcamera: dw100_vertexmap: Implement parametric dewarping")
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
When calling `Debayer::process()` from `SoftwareIsp::process()`, the
`DebayerParams` object is copied multiple times:
(1) call of `BoundMethodMember<...>::activate()`
inside `Object::invokeMethod()`
(2) constructor of `BoundMethodArgs<...>`
inside `BoundMethodMember<...>::activate()`
(3) call of `BoundMethodMember<...>::invoke()`
inside `BoundMethodArgs::invokePack()`
(4) call of the actual pointer to member function
inside `BoundMethodMember::invoke()`
While compilers might avoid one or two of the above copies, this is still
not ideal. By making `Debayer::process()` take the parameter object by
const lvalue reference, only the copy in the `BoundMethodArgs` constructor
remains. So do that.
Before:
[0:12:51.133836595] [12424] DEBUG SoftwareIsp software_isp.cpp:399 params=0x7d0a691f57d0
copy from 0x7d0a691f57d0 into 0x7baa65f2bf30
copy from 0x7baa65f2bf30 into 0x7c6a69209758
copy from 0x7c6a69209758 into 0x7baa63223930
copy from 0x7baa63223930 into 0x7baa63223a70
[0:12:51.134559602] [12426] DEBUG eGL debayer_egl.cpp:538 params=0x7baa63223a70
771.099877 (30.06 fps) cam0-stream0 seq: 000031 bytesused: 8666112
After:
[0:13:42.861691943] [12543] DEBUG SoftwareIsp software_isp.cpp:399 params=0x7cfaad5f57d0
copy from 0x7cfaad5f57d0 into 0x7c5aad609758
[0:13:42.862453917] [12545] DEBUG eGL debayer_egl.cpp:538 params=0x7c5aad609758
822.827388 (30.02 fps) cam0-stream0 seq: 000031 bytesused: 8666112
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
The matrix multiplication in Awb is swapped: the gains should be applied
after combinedMatrix, i.e. on the left side. The mistake happened when
`ccm' was replaced with combinedMatrix and gainMatrix multiplication was
moved to Awb. While CCM must be applied after gains, the gains must be
applied after the combined matrix, which no longer corresponds to CCM in
Awb.
Since there is currently no algorithm modifying combinedMatrix before
Awb, combinedMatrix is an identity matrix there and the wrong order
doesn't influence the output at the moment.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
In order for ADL to find the function, it must be in the namespace of any of
its arguments. Previously, however, that was not the case, and it has only
really worked by accident and could be easily made to fail by introducing
other `operator<<` overloads.
For example, a user of this function in `libcamera::ipa` would no longer
compile after introducing an `operator<<` into the `libcamera::ipa`
namespace as that would essentially hide this overload, and without ADL
it would not be found.
So move the function into the `utils` namespace.
Fixes: 5055ca747c ("libcamera: utils: Add helper class for std::chrono::duration")
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
`utils::Duration` derives from `std::chrono::duration<...>`, but multiplying
it yields an `std::chrono::duration<...>`, not `Duration`. chrono duration
types only have `operator<<` in C++20 or later, so this usage should not
compile. It only did so because the `operator<<` for `Duration` was in
the `libcamera` namespace and `Duration` has an implicit constructor from
any chrono duration type.
This will cease to work when that operator is moved into the `utils` namespace
for ADL purposes. So fix it by making the cast to `Duration` explicit.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
There is no real need for a function template. It is not defined in a
header file, so it has limited availability, and there exists only a
single instantion.
So convert it to use `std::ostream` directly, like most `operator<<`
in the code base.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
gcc 14.3.0, cross-compiling from amd64 to arm64, warns about a possibly
uninitialized variable in YamlObject::Getter<>::get():
In file included from ../../include/libcamera/internal/yaml_parser.h:12,
from ../../src/libcamera/yaml_parser.cpp:8:
In constructor ‘constexpr std::_Optional_payload_base<_Tp>::_Storage<_Up, <anonymous> >::_Storage(std::in_place_t, _Args&& ...) [with _Args = {unsigned char}; _Up = unsigned char; bool <anonymous> = true; _Tp = unsigned char]’,
inlined from ‘constexpr std::_Optional_payload_base<_Tp>::_Optional_payload_base(std::in_place_t, _Args&& ...) [with _Args = {unsigned char}; _Tp = unsigned char]’ at include/c++/14.3.0/optional:122:4,
inlined from ‘constexpr std::_Optional_payload<unsigned char, true, true, true>::_Optional_payload(std::in_place_t, _Args&& ...) [with _Args = {unsigned char}][inherited from std::_Optional_payload_base<unsigned char>]’ at include/c++/14.3.0/optional:337:42,
inlined from ‘constexpr std::_Optional_base<_Tp, true, true>::_Optional_base(std::in_place_t, _Args&& ...) [with _Args = {unsigned char}; typename std::enable_if<is_constructible_v<_Tp, _Args ...>, bool>::type <anonymous> = false; _Tp = unsigned char]’ at include/c++/14.3.0/optional:648:4,
inlined from ‘constexpr std::optional<_Tp>::optional(_Up&&) [with _Up = unsigned char; typename std::enable_if<__and_v<std::__not_<std::is_same<std::optional<_Tp>, typename std::remove_cv<typename std::remove_reference<_Up>::type>::type> >, std::__not_<std::is_same<std::in_place_t, typename std::remove_cv<typename std::remove_reference<_Up>::type>::type> >, std::is_constructible<_T1, _U1>, std::is_convertible<_Iter, _Iterator> >, bool>::type <anonymous> = true; _Tp = unsigned char]’ at include/c++/14.3.0/optional:747:47,
inlined from ‘std::optional<_Tp> libcamera::YamlObject::Getter<T, typename std::enable_if<(((((is_same_v<signed char, T> || is_same_v<unsigned char, T>) || is_same_v<short int, T>) || is_same_v<short unsigned int, T>) || is_same_v<int, T>) || is_same_v<unsigned int, T>), void>::type>::get(const libcamera::YamlObject&) const [with T = unsigned char]’ at ../../src/libcamera/yaml_parser.cpp:172:10:
include/c++/14.3.0/optional:210:15: error: ‘value’ may be used uninitialized [-Werror=maybe-uninitialized]
210 | : _M_value(std::forward<_Args>(__args)...)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/libcamera/yaml_parser.cpp: In member function ‘std::optional<_Tp> libcamera::YamlObject::Getter<T, typename std::enable_if<(((((is_same_v<signed char, T> || is_same_v<unsigned char, T>) || is_same_v<short int, T>) || is_same_v<short unsigned int, T>) || is_same_v<int, T>) || is_same_v<unsigned int, T>), void>::type>::get(const libcamera::YamlObject&) const [with T = unsigned char]’:
../../src/libcamera/yaml_parser.cpp:165:19: note: ‘value’ was declared here
165 | T value;
| ^~~~~
This appears to be a false positive, as the std::from_chars() function
should set value when it returns without an error. Commit bb1d216113
("libcamera: base: log: Fix uninitialized variable warning") fixed a
similar warning with gcc 13.3.0.
The warning is easy to work around by initializing the variable, and
doing so shouldn't be too costly as the type T is restricted to being an
integer. Fix the build by doing so.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Introduce a LensShadingCorrectionEnable control to enable and disable
LSC. This is useful to assess the working and quality of the lens
shading correction at runtime.
While at it drop the reference to the tuning file in the description of
the LensDewarpEnable control, as that information doesn't belong to the
controls.
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
The lens shading correction is always applied based on the sensor crop
bounds. This leads to incorrect lens shading correction for analog crops
that do not cover the whole sensor.
To fix that, we need to adapt the lens shading table for the selected
analog crop at configure time. Introduce an abstract ShadingDescriptor
class that holds the lens shading information that can then be sampled
at configure time for a specific crop rectangle.
Resampling for a specific crop is only implemented for polynomial lsc
data. For tabular data, a warning is logged and the unmodified table is
returned. This matches the current functionality for tabular data and is
a huge improvement for polynomial data.
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
There is no need to recalculate the sampling positions over and over.
Pass them as parameter into the sampling function. The vectors are still
kept inside the loop as this is also a preparatory change for the
upcoming refactoring.
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
The quantization of the interpolation key was only used by the LSC
algorithm. There it lead to difficult to read code was removed. As there
is no remaining user of it, drop it from the Interpolator class.
While at it, cleanup the includes.
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
The quantization functionality in the Interpolator type hinders in
writing nice code. Don't use it and implement the functionality directly
in the algorithm.
While at it, reduce the threshold to half of the quantization step size,
otherwise it might happen that we skip a full quantization step. Rename
the kColourTemperatureChangeThreshhold to kColourTemperatureQuantization
to better express the usecase.
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Move the function definitions out of the related classes. This was noted
in review after the series was already merged. After trying it out I
must admit that it really improves readability and reduces the
indentation level by one.
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Rui Wang <rui.wang@ideasonboard.com>
Barnabás Pőcze
Naushir PatuckandKieran Bingham
David PlowmanandKieran Bingham
Laurent Pinchart
van Veen, StephanandKieran Bingham
Milan ZamazalandKieran Bingham
Hans de GoedeandKieran Bingham
Stefan Klug