The YamlObject class is now a generic data container to model trees of
values. Rename it to ValueNode and expand the class documentation.
While at it, drop the unneeded libcamera:: namespace prefix when using
the ValueNode class.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
The YamlObject class was designed to represent data parsed from YAML
files. It is outgrowing the initial needs. Move it to a separate file to
prepare for a rename.
Most files that include yaml_parser.h only need access to a YamlObject.
Switch them to yaml_object.h. pipeline_base.cpp was including
yaml_parser.h indirectly through pipeline_base.h, include it directly
now.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
The YamlObject::Accessor structure is designed to extend the YamlObject
class with new getter and setter types without modifying
yaml_parser.cpp. This feature is used for various libcamera classes,
while standard C++ types are handled in yaml_parser.cpp.
The Size class is an outlier: it is a libcamera class, but is handled in
yaml_parser.cpp. Move it to geometry.cpp. Drop the std::vector<Size>
specialization as it is currently unused.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
YamlParserContext is a friend of the YamlObject class to access private
member variables. Now that YamlObject exposes functions to set a value
and add children, this is not needed anymore. Decouple the two classes.
The YamlParserContext::readValue() function now takes a const reference
to the EventPtr as the YamlParserContext::parseNextYamlObject() function
needs to access it in the error handler.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Add YamlObject::add() functions to add children to a list or dictionary
object. This will be used by the YamlParserContext to replace direct
access to YamlObject member variables to decouple the two classes.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
The YamlObject class has two member function templates to get values:
the get() function gets a scalar value, while the getList() function
gets a vector of scalar values.
As get() is a function template, we can provide specializations for
vector types. This makes the code more systematic, and therefore more
readable. Replace all getList() occurrences, and drop the getList()
function.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
The YamlObject class defines two private types, Container and
ListContainer. The format is an alias to std::vector<Value>, and is used
to store child elements. The latter hasn't been used since commit
38987e165c ("libcamera: yaml_parser: Preserve order of items in
dictionary").
To prepare for upcoming reworks that will use the name 'Container' as a
template parameter, rename Container to ValueContainer for clarity, and
drop the unused ListContainer type.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
std::visit() allows quite elegant type-matching implementation of the
visitor pattern.
The 'overloaded' type helpers allow to define a hierarchy of overloaded
operator() implementations which can be used by std::visit().
Currently only the Virtual pipeline handler uses this type-matching
implementation of std::visit(). To prepare to add another user in the
Mali C55 pipeline handler move the 'overloaded' helper type to
libcamera::utils for easier re-use.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.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>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
The ipa_interface unit test uses a FIFO to communicate with the vimc IPA
module. FIFOs are named pipes, created in the file system. The path to
the FIFO is hardcoded so that both the unit test and IPA module know
where to locate the file.
If the ipa_interface crashes for any reason, the FIFO will not be
removed, and subsequent usage of the vimc IPA module will hang when
trying to write to the FIFO in the IPA module.
Fix this by replacing the FIFO with a pipe. Pipes are unidirectional
data channels that are represented by a pair of file descriptors,
without any presence in the file system. The write end of the pipe is
passed to the vimc IPA module init() function, and then used the same
way as the FIFO.
While at it, use a std::unique_ptr to manage the notifier in the unit
test instead of manually allocating and deleting the object.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
IPA proxies are created with a call to IPAManager::createIPA(), which is
a static member function. This requires a complicated dance in the
createIPA() function to retrieve the IPAManager instance through the
camera manager, itself retrieved from the pipeline handler. Simplify the
code by turning IPAManager::createIPA() into a non-static member
function, and providing a wrapper in the PipelineHandler class to keep
instantiation of IPA proxies easy in pipeline handlers.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
libcamera uses std::unique_ptr<> to simplify life time management of
objects and avoid leaks. For historical reasons there are a fair number
of plain pointers with manual memory management. Replace them with
std::unique_ptr<> when the conversion is simple.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
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>
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>
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>
Debayer parameters and processing are currently run asynchronously.
This can lead to assertion errors in case the processing tries to use
not yet computed debayer parameters. To prevent this situation, specify
some default values for DebayerParams members. This doesn't make
correct parameters but prevents crashes or other crazy behaviours at
least.
Note this patch is just a workaround. The mutually asynchronous
parameters computation and processing can cause more problems, like
using parameters computed for a different frame. But it is non-trivial
to fix that; in the meantime, setting the default values solves the
worst problem.
Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/311
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.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>
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>
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>
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>
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>
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>
Introduce an eGL base helper class which provides an eGL context based on a
passed width and height.
The initGLContext function could be overloaded to provide an interface to a
real display.
A set of helper functions is provided to compile and link GLSL shaders.
linkShaderProgram currently compiles vertex/fragment pairs but could be
overloaded or passed a parameter to link a compute shader instead.
Breaking the eGL interface away from debayering - allows to use the eGL
context inside of a dma-buf heap cleanly, reuse that context inside of a
debayer layer and conceivably reuse the context in a multi-stage shader
pass.
Small note the image_attrs[] array doesn't pass checkstyle.py however the
elements of the array are in pairs.
Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
[bod: Takes fix from Hans for constructor stride bpp]
[bod: Drops eglClientWaitSync in favour of glFinish Robert/Milan]
Co-developed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Co-developed-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Robert Mader <robert.mader@collabora.com>
Tested-by: Robert Mader <robert.mader@collabora.com>
Tested-by: Hans de Goede <johannes.goede@oss.qualcomm.com> # ThinkPad T14s gen 6 (arm64) ov02c10 + X1c gen 12 ov08x40
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo X13s
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Pass contrastExp as calculated in lut to debayer params not the raw
contrast. This way we calculate contrastExp once per frame in lut and pass
the calculated value into the shaders, instead of passing contrast and
calculating contrastExp once per pixel in the shaders.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Tested-by: Robert Mader <robert.mader@collabora.com>
Tested-by: Hans de Goede <johannes.goede@oss.qualcomm.com> # ThinkPad T14s gen 6 (arm64) ov02c10 + X1c gen 12 ov08x40
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo X13s
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Address a long standing \todo item that suggested to implement a
read-only interface for the Request::metadata() accessor and deflect to
the internal implementation for the read-write accessor used by pipeline
handlers.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Acked-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
It is possible that same device is processed multiple times, leading to
multiple `MediaDevice`s being instantiated, mostly likely leading to
a fatal error:
Trying to register a camera with a duplicated ID xyzw...
There is a time window after the `udev_monitor` has been created in `init()`
and the first (and only) enumeration done in `enumerate()`. If e.g. a UVC
camera is connected in this time frame, then it is possible that it will be
processed both by the `udevNotify()` and the initial `enumerate()`, leading
to the fatal error. This can be reproduced as follows:
1. $ gdb --args cam -m
2. (gdb) break libcamera::DeviceEnumeratorUdev::enumerate
3. (gdb) run
4. when the breakpoint is hit, connect a usb camera
5. (gdb) continue
6. observe fatal error
To address this, keep track of the devnums of all devices reported by
udev, and reject devices with already known devnums. This ensures that
the same device won't be reported multiple times (assuming that udev
reports "add" / "remove" events in the correct order).
Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/293
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: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
