The `signature()` getter can just return a reference to the private vector
member variable, and let the caller make a copy if needed. Since the
return type is const qualified, this was likely the original intention.
Signed-off-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>
This reverts commit e85c7ddd38.
Linux kernel predating 6.4 (specifically commit 7cfb35d3a800 ("media:
rkisp1: Implement ENUM_FRAMESIZES") do not have the ioctl in rkisp1
driver required to dynamically query the resizer limits.
Because of that, maxResolution and minResolution are both {0, 0}
(default value for Size objects) which means filterSensorResolution()
will create an entry for the sensor in sensorSizesMap_ but because the
sensor resolution cannot fit inside the min and max resolution of the
rkisp1, no size is put into this entry in sensorSizesMap_.
On the next call to filterSensorResolution(),
sensorSizesMap_.find(sensor) will return the entry but when attempting
to call back() on iter->second, it'll trigger an assert because the size
array is empty.
Linux kernel 6.1 is supported until December 2027, so it seems premature
to get rid of those hard-coded resizer limits before this happens.
Let's restore the hard-coded resizer limits as fallbacks, actual limits
are still queried from the driver on recent enough kernels.
Fixes: 761545407c ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline")
Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
The DelayedControls instance for the camera sensor is created in
SimplePipelineHandler::configure(). Constant deletion and reconstruction
of a new object is unnecessary, as the control delays are an intrinsic
property of the sensor and are known at initialization time. Move the
DelayedControls creation to the SimpleCameraData class constructor.
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> # v6
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
The simple pipeline handler uses frame start events to apply sensor
controls through the DelayedControls class. The setSensorControls()
function applies the controls directly, which would result in controls
being applied twice, if it wasn't for the fact that the pipeline handler
forgot to enable the frame start events in the first place. Those two
issues cancel each other, but cause controls to always be applied
directly.
Fix the issue by only applying controls directly in setSensorControls()
if no frame start event emitter is available, and by enabling the frame
start events in startDevice() otherwise. Disable them in stopDevice()
for symmetry.
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> # v6
Co-developed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
If `!target`, then `*target` is undefined behaviour, so check if the optional
is empty when printing the error message. Simplify the check as well.
Fixes: 6c71ee1f15 ("pipeline: raspberrypi: Introduce PipelineHandlerBase class")
Fixes: 841ef2b4bb ("pipeline: rpi: Add support for Raspberry Pi 5")
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 mapping in `UVCCameraData::processControl()` is not entirely correct
because the control value is retrieved as a `bool` instead of `int32_t`.
Additionally, the available modes are not taken into account.
Retrieve the control value with the right type, `int32_t`, check if the
requested mode is available, and if so, set the appropriate V4L2 control
value selected by `addControl()` earlier.
Fixes: bad8d591f8 ("libcamera: uvcvideo: Register ExposureTimeMode control")
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
`ControlInfo(Span<const int32_t>{...})` calls the incorrect constructor
of `ControlInfo`. The intended constructor to be called is
`ControlInfo(Span<const ControlValue>, ...)` however that is not called
because a span of `const int32_t` is passed. Instead, the constructor
`ControlInfo(const ControlValue &min, const ControlValue &max, ...)`
will be called.
Furthermore, since `values.back()` is used, only the last element of
the array is actually set.
To fix this, convert the array to contain `ControlValue` objects and use
a separate variable to keep track of which element to set next.
For each of `ExposureTimeMode{Auto,Manual}` save the V4L2 control value
that is to be used when the libcamera control is set.
Fixes: bad8d591f8 ("libcamera: uvcvideo: Register ExposureTimeMode control")
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Report exposure and gain in metadata.
This is more complicated than it could be expected because the exposure
value should be in microseconds but it's handled using V4L2_CID_EXPOSURE
control, which doesn't specify the unit, see
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/control.html.
So the unit conversion is done in the way rkisp1 IPA uses.
This requires getting and passing IPACameraSensorInfo around. To avoid
naming confusion and to improve consistency with rkisp1 IPA,
sensorCtrlInfoMap parameter is renamed to sensorControls.
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Extend the Simple IPA IPC to support returning a metadata ControlList
when the process call has completed.
A new signal from the IPA is introduced to report the metadata,
similarly to what the hardware pipelines do.
Merge the metadata reported by the ISP into any completing request to
provide to the application. Completion of a request is delayed until
this is done; this doesn't apply to canceled requests.
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Hardware pipelines track requests and other information related to
particular frames. This hasn't been needed in software ISP so far. But
in order to be able to track metadata corresponding to a given frame,
frame-request tracking mechanism starts being useful. This patch
introduces the basic tracking structure, actual metadata handling is
added in the following patch.
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo X13s
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
gcc 13.3.0 from buildroot 2024.11.1 complains about an uninitialized
variable. This is a false positive as the cfe_ array can't be empty.
Nonetheless, it breaks builds, so initialize the variable to work around
the issue.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
This patch applies color correction matrix (CCM) in debayering if the
CCM is specified. Not using CCM must still be supported for performance
reasons.
The CCM is applied as follows:
[r1 g1 b1] [r]
[r2 g2 b2] * [g]
[r3 g3 b3] [b]
The CCM matrix (the left side of the multiplication) is constant during
single frame processing, while the input pixel (the right side) changes.
Because each of the color channels is only 8-bit in software ISP, we can
make 9 lookup tables with 256 input values for multiplications of each
of the r_i, g_i, b_i values. This way we don't have to multiply each
pixel, we can use table lookups and additions instead. Gamma (which is
non-linear and thus cannot be a part of the 9 lookup tables values) is
applied on the final values rounded to integers using another lookup
table.
Because the changing part is the pixel value with three color elements,
only three dynamic table lookups are needed. We use three lookup tables
to represent the multiplied matrix values, each of the tables
corresponding to the given matrix column and pixel color.
We use int16_t to store the precomputed multiplications. This seems to
be noticeably (>10%) faster than `float' for the price of slightly less
accuracy and it covers the range of values that sane CCMs produce. The
selection and structure of data is performance critical, for example
using bytes would add significant (>10%) speedup but would be too short
to cover the value range.
The color lookup tables can be represented either as unions,
accommodating tables for both the CCM and non-CCM cases, or as separate
tables for each of the cases, leaving the tables for the other case
unused. The latter is selected as a matter of preference.
The tables are copied (as before), which is not elegant but also not a
big problem. There are patches posted that use shared buffers for
parameters passing in software ISP (see software ISP TODO #5) and they
can be adjusted for the new parameter format.
Color gains from white balance are supposed not to be a part of the
specified CCM. They are applied on it using matrix multiplication,
which is simple and in correspondence with future additions in the form
of matrix multiplication, like saturation adjustment.
With this patch, the reported per-frame slowdown when applying CCM is
about 45% on Debix Model A and about 75% on TI AM69 SK.
Using std::clamp in debayering adds some performance penalty (a few
percent). The clamping is necessary to eliminate out of range values
possibly produced by the CCM. If it could be avoided by adjusting the
precomputed tables some way then performance could be improved a bit.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Applying color correction matrix (CCM) in software ISP is optional due
to performance reasons. CCM is applied if and only if `Ccm' algorithm
is present in the tuning file.
Software ISP debayering is a performance critical piece of code and we
do not want to use dynamic conditionals there. Therefore we pass
information about CCM application to debayering configuration and let it
select the right versions of debayering functions using templates. This
is a trick similar to the previously used one for adding or not adding
an alpha channel to the output.
Debayering gets this information but it ignores it in this patch.
Actual processing with CCM is added in the followup patch.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Assignments of the debayering methods to be used is a repetitive pattern
that can be (arguably) better expressed by using a macro. This removes
some duplication and also makes easier to introduce more complex
assignment patterns. This will be useful once color correction matrix
support is added.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Both `CameraManager::Private::{add,remove}Camera()` emit the
`camera{Added,Removed}` signals, respectively, while holding the
lock protecting the list of cameras.
This is problematic because if a callback tries to call `cameras()`,
then the same (non-recursive) lock would be locked again.
Furthermore, there is no real need to hold the lock while user code
is running, so release the lock as soon as possible.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Harvey Yang <chenghaoyang@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
`Camera::Private::properties_` is a default constructed `ControlList`,
therefore it does not have an associated `ControlIdMap`. `controlInfo_`
is in a similar situation.
Extend the `Camera::Private` constructor to initialize the control id map
of both properly.
Multiple pipeline handlers copy the sensor's property list and
set that as camera properties, and since the `CameraSensor{Legacy,Raw}`
classes set the proper id map, the camera properties will have it too.
However, some pipelines, e.g. `uvcvideo` or `virtual`, do not do so,
and thus there will be no id map set. To fix this, extend the
`Camera::Private` constructor to set `properties::properties`.
As for `controlInfo_`, all pipeline handlers overwrite it during
camera initialization (and thus it will have the correct id map),
but still initialize the id map so that it is set at all times.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
The `V4L2BufferCache` type is not thread-safe. Its `lastUsedCounter_`
member is not used in contexts where its atomicity would matter.
So it does not need to be have an atomic type.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
When `_FORTIFY_SOURCE` is enabled, the `lockf()` function might be marked
with the `warn_unused_result` attribute, leading to compilation failure.
Fix that by explicitly ignoring the return value.
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Add the following new stream flags:
Needs16bitEndianSwap - Indicates that a 16-bit endian swap needs to be
performed on the framebuffer in software.
Needs14bitUnpack - Indicates that a CSI-2 14-bit unpacking (to 16-bits)
needs to be performed on the framebuffer in software.
These are to workaround hardware restrictions in the CFE hardware that
will be supported in a future commit.
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
gcc 13.3.0, cross-compiling from amd64 to arm64, warns about a possibly
uninitialized variable in Logger::parseLogLevel():
src/libcamera/base/log.cpp: In static member function ‘static libcamera::LogSeverity libcamera::Logger::parseLogLevel(std::string_view)’:
../../src/libcamera/base/log.cpp:694:55: error: ‘severity’ may be used uninitialized [-Werror=maybe-uninitialized]
694 | if (ec != std::errc() || *end != '\0' || severity > LogFatal)
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
src/libcamera/base/log.cpp:690:22: note: ‘severity’ was declared here
690 | unsigned int severity;
| ^~~~~~~~
This appears to be a false positive, as the std::from_chars() function
should set severity value when it returns without an error. Still, the
warning is easy to solve, so fix it by initializing the severity
variable.
Fixes: 8fa119e0b5 ("libcamera: base: log: Use `std::from_chars()`")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
There may be pending messages in SoftwareIsp message queue when
SoftwareIsp stops. The call to IPAProxySoft::stop() will dispatch them
before SoftwareIsp::stop() finishes. But this is dependent on
IPAProxySoft::stop() implementation, let's break this dependency and
dispatch messages to SoftwareIsp explicitly in SoftwareIsp::stop().
This also allows dropping `running_' flag. Since the SoftwareIsp
messages get processed and invoke IPA calls before the IPA proxy is set
to ProxyStopping state and the SoftwareIsp worker thread is no longer
running, it's guaranteed that no new messages come to SoftwareIsp and
attempt to call the stopped IPA proxy.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
The Thread::dispatchMessage() function supports filtering messages based
on their type. It can be useful to also dispatch only messages posted
for a specific receiver. Add an optional receiver argument to the
dispatchMessage() function to do so. When set to null (the default
value), the behaviour of the function is not changed.
This facility is actually used in followup patches.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
When SoftwareIsp stops, input and output buffers queued to it may not
yet be fully processed. They will be eventually returned but stop means
stop, there should be no processing related actions invoked afterwards.
Let's stop forwarding processed input buffers from SoftwareIsp slots
when SoftwareIsp is stopped. Let's track the queued input buffers and
return them back for capture in SoftwareIsp::stop().
The returned input buffers are marked as cancelled. This is not
necessary at the moment but it gives the pipeline handlers chance to
deal with this if they need to.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
When SoftwareIsp stops, input and output buffers queued to it may not
yet be fully processed. They will be eventually returned but stop means
stop, there should be no processing related actions invoked afterwards.
Let's stop forwarding processed output buffers from the SoftwareIsp
slots once SoftwareIsp is stopped. Let's track the queued output
buffers and mark those still pending as cancelled in SoftwareIsp::stop
and return them to the pipeline handler.
Dealing with input buffers is addressed in a separate patch.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Software ISP runs debayering in a separate thread and debayering may
emit statsReady when software ISP (including the IPA) is being stopped.
The signal waits in a queue and gets invoked later, resulting in an
assertion error when attempting to invoke a method on the stopped IPA:
FATAL default soft_ipa_proxy.cpp:456 assertion
"state_ == ProxyRunning" failed in processStatsThread()
Let's prevent this problem by forwarding the ISP stats signal from
software ISP only when the IPA is running. To track this,
SoftwareISP::running_ variable is introduced.
Making processing of the other signals in SoftwareISP more robust is
addressed in the followup patches.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reported-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Log categories may be added from any thread, so it is important to
synchronize access to the `Logger::categories_` list between its two
users: category creation (by LogCategory::create(), which calls
Logger::findCategory() and Logger::registerCategory()); and log level
setting (by Logger::logSetLevel()).
The LogCategory::create() function uses a mutex to serialize category
creation, but Logger::logSetLevel() can access `Logger::categories_`
concurrently without any protection. To fix the issue, move the mutex to
the Logger class, and use it to protect all accesses to the categories
list. This requires moving all the logic of LogCategory::create() to a
new Logger::findOrCreateCategory() function that combines both
Logger::findCategory() and Logger::registerCategory() in order to make
the two operations exacute atomically.
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Use `std::string_view` to avoid some largely unnecessary copies, and
to make string comparisong potentially faster by eliminating repeated
`strlen()` calls.
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
The severity of a log category may be changed from a different thread,
so it is important to ensure that the reads and writes happen atomically.
Using `std::memory_order_relaxed` should not introduce any synchronization
overhead, it should only guarantee that the operation itself is atomic.
Secondly, inline `LogCategory::setSeverity()`, as it is merely an
assignment, so going through a DSO call is a big pessimization.
`LogCategory` is not part of the public API, so this change has
no external effects.
Thirdly, assert that the atomic variable is lock free so as to ensure
it won't silently fall back to libatomic (or similar) on any platform.
If this assertion fails, this needs to be revisited.
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Use the `std::from_chars()` function from `<charconv>` to
parse the integral log level instead of `strtoul` as it
provides an easier to use interface and better type safety.
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
C++17 guarantees move and copy elision in certain cases,
such as when returning a prvalue of the same type as the
return type of the function.
This is what the `_log()` functions do, thus there is no need
for the move constructor, so remove it. Furthermore, do not
just remove the implementation, but instead delete it as well.
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
