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>
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>
control_ids.h defines the contrast type as float, let's use the same in
simple IPA, instead of double. Saturation and gamma already use float,
except for the knobs initializers, let's use float for the knobs too.
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>
Saturation adjustments are implemented using matrix operations. They
are currently applied to the colour correction matrix. Let's move them
to a newly introduced separate "Adjust" algorithm.
This separation has the following advantages:
- It allows disabling general colour adjustments algorithms without
disabling the CCM algorithm.
- It keeps the CCM separated from other corrections.
- It's generally cleaner.
Reviewed-by: Kieran Bingham <kieran.bingham@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 combined matrix must be reset to the initial value before each frame
is prepared. This must be done outside algorithms because any of the
algorithms may be disabled while the matrix must be always initialised.
Let's initialise the combined matrix to the identity matrix (which keeps
the pixel values unchanged) in software ISP just before calling
`prepare' on the algorithms.
Matrix updates can no longer be skipped in ccm.cpp, otherwise the CCM
won't be applied if there is no temperature or saturation change. We
explicitly track whether the CCM has been set up completely rather than
relying on the frame number, to avoid missing the initialisation in case
the first frame is skipped due to some error.
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>
Let's introduce IPAActiveState::combinedMatrix that is separate from
IPAActiveState::ccm and represents the overall correction matrix, not
only the sensor colour correction matrix.
IPAActiveState::ccm still includes everything; this is changed in the
followup patch.
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
IPAActiveState::ccm stores the colour correction matrix (CCM) and
whether it has been changed. The change flag is later used when
recomputing or not the lookup tables.
But the CCM may include other corrections than just the sensor colour
correction, for example white balance and saturation adjustments. These
things should be separated and IPAActiveState::ccm should represent just
the CCM itself.
As the first step towards that cleanup, let's separate the change flag
from the CCM. And wrap the only remaining member of
IPAActiveState::ccm.
Also, let's reset the separated change flag in the lookup tables; it'll
be no longer tied to just CCM handling.
This patch doesn't change actual behaviour and it still reports the
combined matrix as CCM in metadata. This is addressed in the followup
patches.
Reviewed-by: Bryan O'Donoghue <bod.linux@nxsw.ie>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
GCC-16 has updated the warnings enabled and now includes
-Wsfinae-incomplete triggering breakage in the included headers from
Qt6:
/usr/include/qt6/QtCore/qregularexpression.h:31:21: error: defining ‘QRegularExpression’, which previously failed to be complete in a SFINAE context [-Werror=sfinae-incomplete=]
This is a Qt header issue outside of libcamera’s control. Disable the
warning for qcam to restore buildability with newer GCC versions.
Link: https://qt-project.atlassian.net/browse/QTBUG-143470
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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>
Update the tuning files to include the new AWB algorithm. It is
enabled by setting "enabled" to true for the AWB algorithm that you
want, and the same field to false for the one you don't want. Note
that you may enable only one of the two algorithms!
The AWB models themselves are not included with libcamera. They will
be supplied from the Raspberry Pi software repositories.
Signed-off-by: Peter Bailey <peter.bailey@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Algorithms may now contain an "enabled" field which can be set to
false to disable it and prevent it from being loaded. If not present,
algorithms are treated as enabled by default for backwards
compatability.
We additionally prevent duplicate versions of the same algorithm type
(such as AWB) from loading, and flag an error. This will prevent
undefined behaviour if (for example) two distinct AWB algorithms are
trying to run simultaneously.
Signed-off-by: Peter Bailey <peter.bailey@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Similar to what commit acfd602767 ("ipa: rkisp1: Fix algorithm controls
vanish after configure") did for the RkISP1 IPA, replace the usage of
unordered_map::merge() at updateControls() time with
unordered_map::insert().
As unordered_map::merge moves items from the source map, it deletes
controls registered at algorithms initialization time in the
ipaContext.ctrlMap. As at configure() time updateControls() is called
again and the list of Camera controls is refreshed, the controls
registered at algorithms initialization time are lost.
Fixes: 87353f2bba ("ipa: ipu3: Derive ipu3::algorithms::Agc from AgcMeanLuminance")
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Similar to what commit acfd602767 ("ipa: rkisp1: Fix algorithm controls
vanish after configure") did for the RkISP1 IPA, replace the usage of
unordered_map::merge() at updateControls() time with
unordered_map::insert().
As unordered_map::merge moves items from the source map, it deletes
controls registered at algorithms initialization time in the
ipaContext.ctrlMap. As at configure() time updateControls() is called
again and the list of Camera controls is refreshed, the controls
registered at algorithms initialization time are lost.
Fixes: fe989ee514 ("ipa: mali-c55: Add Mali-C55 ISP IPA module")
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-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>
The default values for controls::FrameDurationLimits is now an array but
the v4l2 proxy is fetching it as a scalar value, causing a runtime
error. Fix this by templating the getter with the correct
Span<const int64_t, 2> type.
This fix also requires the RPi initial default value for FrameDurationLimits
to be specified as a Span<const int64_t, 2>.
As a drive-by, remove the hard-coded 33ms min and 120ms max frame
duration values in the initial defaults, and use the defaultMinFrameDuration
and defaultMaxFrameDuration const values. This change is inconsequential
to runtime operation as these always get overridden on the first camera
configure call.
Fixes: 4e9be7d11b ("ipa: ipu3, mali-c55, rkisp1, rpi: Fix reporting non-scalar controls")
Closes: https://github.com/raspberrypi/libcamera/issues/321
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
For example, `std::span` does not have a `const_iterator` typedef before
C++23, so compilation fails. Simply use `auto`. The `const` qualifier on
`items` should already ensure, that such an iterator will be be used that
the container deems appropriate for "const" access.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@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>
A `LogMessage` instance cannot be moved or copied, so a severity of
`LogInvalid` is only possible if the message was constructed with that
log level explicitly. However, being a completely internal type, this
does not occur. So remove the check. And even if it does, it's probably
still better to print the message than to drop it silently.
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>
At the moment every `LOG()` macro invocation results in a `LogMessage` being
created, the message serialized into an `std::stringstream`. Only in the
destructor is it actually checked whether the given `LogCategory` enables
the given log level.
This is not too efficient, it would be better to skip the log message
construction and all the `operator<<()` invocations if the message will
just be discarded.
This could be easily done if the `LOG()` macro accepted its arguments like a
traditional function as in that case an appropriate `if` statement can be
injected in a do-while loop. However, that is not the case, the `LOG()` macro
should effectively "return" a stream.
It is not possible inject an `if` statement directly as that would
lead to issues:
if (...)
LOG(...)
else
...
The `else` would bind the to the `if` in the `LOG()` macro. This is
diagnosed by `-Wdangling-else`.
An alternative approach would be to use a `for` loop and force a single
iteration using a boolean flag or similar. This is entirely doable but
I think the implemented approach is easier to understand.
This change implements the early log level checking using a `switch` statement
as this avoids the dangling else related issues. One small issue arises
because having a boolean controlling expression is diagnosed by clang
(`-Wswitch-bool`); the result is cast to `int` to avoid the warning.
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>
