Add the missing `override` specifiers in the derived classes of `Debayer`,
so that the compiler can ensure that the functions in the derived classes
really override the functions in the base class.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Bryan O'Donoghue <bod.linux@nxsw.ie>
This member stores an egl image handle, but currently there is no need for it
since the image handle is only really used in `createDMABufTexture2D()`.
(`destroyDMABufTexture()` is an unused function.)
So remove the member (and the unused function), and instead destroy the image
immediately after calling `glEGLImageTargetTexture2DOES()`. The texture will
keep a reference to the image, so this is safe to do. In fact, this solves
an issue, specifically, the egl images were never destroyed, and continuously
leaked during streaming.
Fixes: f520b29fe9 ("libcamera: software_isp: debayer_egl: Add an eGL Debayer class")
Closes: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/322
Signed-off-by: Gianfranco Mariotti <gianfranco.mariotti94@gmail.com>
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
All boolean options in libcamera other than the soft ISP options are
printed as YES/NO (with colours when supported by the terminal). Extend
this to the soft ISP options for consistency.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Multiple files in the Raspberry Pi pipeline handler and IPA module are
missing SPDX headers. Add them with the following licenses:
- For code and documentation, use the BSD-2-Clause license, as for the
rest of the Raspberry Pi code.
- For the example pipeline handler configuration files, use the CC0-1.0
license to facilitate their usage.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Multiple files in libcamera are missing SPDX headers. Add them with the
following licenses:
- CC-BY-SA-4.0 for documentation
- CC0-1.0 for build system, development tool configuration files and
TODO lists, as we consider them non-copyrightable, and for example
configuration files to facilitate their use
- BSD-2-Clause for a file copied from the Raspberry Pi tuning tool (and
add the corresponding copyright information)
While at it, add a missing blank line to
src/libcamera/pipeline/virtual/README.md.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Add the Pipeline handler name to the Camera Properties when a camera is
constructed.
This helps support and identify how the camera is being managed
internally and what configuration has taken effect, especially as the
pipeline handler chosen can be impacted by both environment variables
and configuration files.
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
The intent of the outputCfgs argument to the configure() function of
converter classes and the softISP is to allow the passed in stream-configs
to not be changed.
But only the vector is const, the reference inside the vector are not
const, which allows modifying the stream-configs as can be seen inside
DebayerEGL::configure() which was using a non const reference outputCfg
helper variable.
Fix this by making the references inside the vector const.
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
As shown by commit 94d32fdc55 ("pipeline: simple: Consider output sizes
when choosing pipe config"), the extra pixel columns CPU debayering
requires on the input side makes resolution selection non trivial.
Add logging of the selected input config on a successful configure() so
that the logs clearly show which sensor mode has been selected.
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Add CPU soft ISP multi-threading support.
Benchmark results for the Arduino Uno-Q with a weak CPU which is good for
performance testing, all numbers with an IMX219 running at
3280x2464 -> 3272x2464:
1 thread : 147ms / frame, ~6.5 fps
2 threads: 80ms / frame, ~12.5 fps
3 threads: 65ms / frame, ~15 fps
Adding a 4th thread does not improve performance.
Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> # ThinkPad X1 Yoga Gen 7 + ov2740
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Add a DebayerCpuThreadclass and use this in the inner render loop.
This contains data which needs to be separate per thread.
This is a preparation patch for making DebayerCpu support multi-threading.
Benchmarking on the Arduino Uno-Q with a weak CPU which is good for
performance testing, shows 146-147ms per 3272x2464 frame both before and
after this change, with things maybe being 0.5 ms slower after this change.
Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> # ThinkPad X1 Yoga Gen 7 + ov2740
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Make the storage used to accumulate the RGB sums and the Y histogram
value a vector of SwIspStats objects instead of a single object so
that when using multi-threading every thread can use its own storage to
collect intermediate stats to avoid cache-line bouncing.
Benchmarking with the GPU-ISP which does separate swstats benchmarking,
on the Arduino Uno-Q which has a weak CPU which is good for performance
testing, shows 20ms to generate stats for a 3272x2464 frame both before
and after this change.
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
`std::atomic_{load,store}()` with `std::shared_ptr` has been deprecated
in C++20 in favour of `std::atomic<std::shared_ptr<>>`. However, it is
not available on all supported platforms. So ignore the deprecation warnings.
The specialization is available since gcc (libstdc++) 12 and llvm (libc++) 15.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.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>
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>
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>
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 default CCM in uncalibrated.yaml is just an identity transformation
and has been enabled by default only to always provide a correction
matrix to GPU ISP. It slows down CPU ISP when CCM is not used.
Now, when a default correction matrix is always provided to GPU ISP, we
can disable the Ccm algorithm in uncalibrated.yaml again. The check for
ccmEnabled in GPU ISP is no longer needed and it must be removed in
order not to fail when Ccm algorithm is not enabled. ccmEnabled flag is
still needed in CPU ISP where the processing differs based on whether
CCM is present or not.
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Robert Mader <robert.mader@collabora.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
The Lut algorithm is not really an algorithm. Moreover, algorithms may
be enabled or disabled but with Lut disabled, nothing will work.
Let's move the construction of lookup tables to CPU debayering, where it
is used. The implied and related changes are:
- DebayerParams is changed to contain the real params rather than lookup
tables.
- contrastExp parameter introduced by GPU ISP is used for CPU ISP too.
- The params must be initialised so that debayering gets meaningful
parameter values even when some algorithms are disabled.
- combinedMatrix must be put to params everywhere where it is modified.
- Matrix changes needn't be tracked in the algorithms any more.
- CPU debayering must watch for changes of the corresponding parameters
to update the lookup tables when and only when needed.
- Swapping red and blue is integrated into lookup table constructions.
- gpuIspEnabled flags are removed as they are not needed any more.
Reviewed-by: Robert Mader <robert.mader@collabora.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
The black level offset subtracted in AWB is wrong. It assumes that the
stats contain sums of the individual colour pixels. But they actually
contain sums of the colour channels of larger "superpixels" consisting
of the individual colour pixels. Each of the RGB colour values and the
computed luminosity (a histogram entry) are added once to the stats per
such a superpixel. This means the offset computed from the black level
and the number of pixels should be used as it is, not divided.
The patch fixes the subtracted offset. Since the evaluation is the same
for all the three colours now, the individual class variables are
replaced with a single RGB variable.
Fixes: 4e13c6f55b ("Honor black level in AWB")
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Robert Mader <robert.mader@collabora.com>
Tested-by: Robert Mader <robert.mader@collabora.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Pointer to the contiguous data of a container is to be retrieved
using the `data()` member function. Using `begin()` in contexts
that require pointers is not correct as the iterator may be
something entirely different. So use `data()` where applicable.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
Mesa surfaceless platform appears to be a better fit for the use-case at hand:
1. Like GBM it is Mesa specific, so no change in supported setups is
expected. If ever required, a fallback to the generic device platform
could be added on top.
2. It leaves the complexity of selecting a renderer device to the
driver, reducing code and dependencies.
3. It allows to use llvmpipe / software drivers without dri device,
which can be useful on CI or debugging (with LIBGL_ALWAYS_SOFTWARE=1).
Signed-off-by: Robert Mader <robert.mader@collabora.com>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sm8250/rb5, x1e/Dell Insprion14p
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Tested-by: Milan Zamazal <mzamazal@redhat.com> # TI AM69
Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> # ThinkPad X1 Yoga Gen 7 + ov2740
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
