The 'found' flag was mistakenly understood that a compatible sensor
format has been found when a sensor configuration is passed in. However,
'found' related to the stream configuration's pixelformat, whether it is
supported by the RkISP1Path video node or not. It does not relate to the
sensor format, hence the check:
if (sensorConfig && !found)
doesn't make sense.
Rectify the above check with:
if (sensorConfig && !rawFormat.isValid())
to ensure a sensor format compatible with sensor configuration has been
set to rawFormat.
Fixes: 047d647452 ("libcamera: rkisp1: Integrate SensorConfiguration support")
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Commit 761545407c ("pipeline: rkisp1: Filter out sensor sizes not
supported by the pipeline") introduced a mechanism to determine maximum
supported sensor resolution and filter out resolutions that cannot be
supported by the ISP.
However, it missed to update the raw stream configuration path, where
it should have clamped the raw stream configuration size to the maximum
sensor supported resolution.
This patch fixes the above issue and can be confirmed with IMX283
on i.MX8MP:
From:
($) cam -c1 -srole=raw,width=5472,height=3072
INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12
ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12
Failed to configure camera
Failed to start camera session
To:
($) cam -c1 -srole=raw,width=5472,height=3072
INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12
cam0: Capture until user interrupts by SIGINT
536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824
536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824
536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824
...
Fixes: 761545407c ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline")
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
Integrate the RkISP1 pipeline handler to support sensor configuration
provided by applications through CameraConfiguration::sensorConfig.
The SensorConfiguration must be validated on both RkISP1Path (mainPath
and selfPath), so the parameters of RkISP1Path::validate() have been
updated to include sensorConfig.
The camera configuration will be marked as invalid when the sensor
configuration is supplied, if:
- Invalid sensor configuration (SensorConfiguration::isValid())
- Bit depth not supported by RkISP1 pipeline
- Sensor configuration output size is larger than maximum supported
sensor's size on RkISP1 pipeline
- No matching sensor configuration output size supplied by the sensor
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
Tested-by: Stefan Klug <stefan.klug@ideasonboard.com>
In many cases a static string literal is used as key. Thus
having the argument type be `const std::string&` is suboptimal
since an `std::string` object needs to be constructed before
the call.
C++17 introduced `std::string_view`, using which the call
can be done with less overhead, as the `std::string_view`
is non-owning and may be passed in registers entirely.
So make `YamlObject::{contains,operator[]}` take the string keys
in `std::string_view`s.
Unfortunately, that is not sufficient yet, because `std::map::find()`
takes an reference to `const key_type`, which would be `const std::string&`
in the case of `YamlParser`. However, with a transparent comparator
such as `std::less<>` `std::map::find()` is able to accept any
object as the argument, and it forwards it to the comparator.
So make `YamlParser::dictionary_` use `std::less<>` as the comparator
to enable the use of `std::map::find()` with any type of argument.
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
The libcamera::Rectangle class allows defining rectangles regardless of
the orientation of the reference system where a rectangle is used in.
This implies that, depending on the reference system in use, the
rectangle's top-left corner, as defined by libcamera, doesn't correspond
to the visual top-left position.
^
|
| -------------------
| ^ | h
| | |
y| o---->-------------
| w
------------------------------->
(0,0) x
(0,0) x
------------------------------>
| w
y| o---->-------------
| | | h
| v |
| -------------------
|
V
Clarify that a Rectangle's top-left corner corresponds to the point
with the smaller x and y coordinates and that the horizontal and
vertical dimensions are obtained by positive increments along the
corresponding axes.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Harvey Yang <chenghaoyang@chromium.org>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
As described in the coding style document, libcamera favours <cmath>
over <math.h>. Replace the last few occurrences of the latter with the
former and adapt the code accordingly.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
As explained in the coding style document, usage of std::lround() is
preferred over lroundf() as it picks the correct function based on
the argument type. Replace calls to lroundf() with std::lround() through
libcamera.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
When constructing a ControlValue from an enum value, an explicit cast to
int32_t is needed as we use int32_t as the underlying type for all
enumerated controls. This makes users of ControlValue more complex. To
simplify them, specialize the control_type template for enum types, to
support construction of ControlValue directly without a cast.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
It is possible that the maximum sensor size (returned by
CameraSensor::resolution()) is not supported by the pipeline. In such
cases, a filter function is required to filter out resolutions of the
camera sensor, which cannot be supported by the pipeline.
Introduce the filter function filterSensorResolution() in RkISP1Path
class and use it for validate() and generateConfiguration(). The
maximum sensor resolution is picked from that filtered set of
resolutions instead of CameraSensor::resolution().
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
The minResolution_ and maxResolution_ limits are dynamically queried
in populateFormats() from the RkISP1Path video node. Therefore,
initializing these limits with the resizer limits in the constructor is
unnecessary.
This change allows us to remove the hard-coded max/min resolution limits
of the resizer from RkISP1Path, simplifying its constructor further.
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Which is not only what many other pipeline handlers use, but also a good
lower limit when dealing with DRM and similar APIs. Even Mesas EGL and
Vulkan WSI implementations use for the reason outlined in mesa commit
992a2dbba80aba35efe83202e1013bd6143f0dba:
> When the compositor is directly scanning out from the application's buffer it
> may end up holding on to three buffers. These are the one that is is currently
> scanning out from, one that has been given to DRM as the next buffer to flip
> to, and one that has been attached and will be given to DRM as soon as the
> previous flip completes. When we attach a fourth buffer to the compositor it
> should replace that third buffer so we should get a release event immediately
> after that. This patch therefore also changes the number of buffer slots to 4
> so that we can accomodate that situation.
Given the popularity of this buffer number the bump should be unlikely
to cause problems. At the same time it may help with performance or
even work around glitches.
The previous number was introduced in commit
a8964c28c8 without mentioning a specific
reason against the change at hand.
Signed-off-by: Robert Mader <robert.mader@collabora.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
This is the last step to fully convert software ISP to Algorithm-based
processing.
The newly introduced frameContext.sensor parameters are set, and the
updated code moved, before calling Algorithm::process() to have the
values up-to-date in stats processing.
Resolves software ISP TODO #10.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Use the standard libcamera mechanism to report the "current" controls
rather than delaying updates by counting from the last update.
A problem is that with software ISP we cannot be sure about the sensor
delay. The original implementation simply skips exposure updates for 2
frames, which should be enough in most cases. After this change, we
assume the delay being exactly 2 frames, which may or may not be correct
and may work with outdated values if the delay is shorter.
According to Kieran, the wrong parts are also wrong on the
IPU3/RKISP1/Mali pipelines and only RPi have this correct. We need to
fix this, by correctly specifying the gains in the libipa camera sensor
helpers. The sooner the better because this change could introduce a
risk of increasing oscillations.
This patch also prepares moving exposure+gain to an algorithm module
where the original delay mechanism would be a (possibly unnecessary)
complication.
Resolves software ISP TODO #11 + #12.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
This patch adds Algorithm::queueRequest call for the defined algorithms.
As there are currently no control knobs in software ISP nor the
corresponding queueRequest call chain, the patch also introduces the
queueRequest methods and calls from the pipeline to the IPA.
This is preparation only since there are currently no Algorithm based
algorithms defined and no current software ISP algorithms support
control knobs.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
This patch adds Algorithm::configure call for the defined algorithms.
This is preparation only since there are currently no Algorithm based
algorithms defined.
A part of this change is passing IPAConfigInfo instead of ControlInfoMap
to configure() calls as this is what Algorithm::configure expects.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
A previous preparation patch implemented passing frame ids to stats
processing but without actual meaningful frame id value passed there.
This patch extends that by actually providing the frame id and passing
it through to the stats processor.
The frame id is taken from the request sequence number, the same as in
hardware pipelines.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
This patch adds frame and bufferId arguments to stats related calls.
Although the parameters are currently unused, because frame ids are not
tracked and used and the stats buffer is passed around directly rather
than being referred by its id, they bring the internal APIs closer to
their counterparts in hardware pipelines.
It serves as a preparation for followup patches that will introduce:
- Frame number tracking in order to switch to DelayedControls
(software ISP TODO #11 + #12).
- A ring buffer for stats in order to improve passing the stats
(software ISP TODO #2).
Frame and buffer ids are unrelated for the given purposes but since they
are passed together at the same places, the change is implemented as a
single patch rather than two, basically the same, patches.
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Add to ControlId information about the names and values of enum, in the
event that the ControlId is an enum type. This allows applications to
query the ControlId for the names of the enum values, so that they can
be displayed on a UI, for example. Without this, it was necessary to use
macros of NameOfControlNameValueMap, which is difficult to use and is
very inflexible.
There already exists a map from name -> value in generated code. Reuse
this and pass it to the ControlId constructor, which in turn generates
the reverse map. The reverse map is then exposed to applications.
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
When accessing a nonexistent key on a dict the YamlObject returns an
empty element. This element can happily be cast to a string which is
unexpected. For example the following statement:
yamlDict["nonexistent"].get<string>("default")
is expected to return "default" but actually returns "". Fix this by
introducing an empty type to distinguish between an empty YamlObject and
a YamlObject of type value containing an empty string. For completeness
add an isEmpty() function and an explicit cast to bool to be able to
test for that type.
Extend the tests accordingly.
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Using `DMA_BUF_IOCTL_SYNC` is required for DMABUFs in order to ensure
correct output. Not doing so currently results in occasional tearing
and/or backlashes in GL/VK clients that use the buffers directly for
rendering.
An alternative approach to have the sync code in `MappedFrameBuffer` was
considered but rejected for now, in order to allow clients more
flexibility.
While the new helper is added to an annoymous namespace, add
timeDiff to the same namespace and remove the static definition as a
drive by.
Signed-off-by: Robert Mader <robert.mader@collabora.com>
Tested-by: Milan Zamazal <mzamazal@redhat.com> # Debix
Tested-by: Hans de Goede <hdegoede@redhat.com> # IPU6 + ov2740
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo X13s + OV5675
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Replace the two open-coded implementations of a link representation
with the operator<< overload string representation to simplify
the code and unify appearance of reporting MediaLinks.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Various parts of libcamera print the representation of a MediaLink by
inline joining the parts to make a string representation.
This repeated use case can be supported with a common helper to print
the MediaLink in a common manner using the existing toString() and
operator<< overload style to make it easier to report on MediaLink
types.
This implementation will report in the following style:
'imx283 1-001a'[0] -> 'video-mux'[0]
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
The handling for the sequence number validation within
V4L2VideoDevice::dequeueBuffer makes use of a std::optional, which can
be used as a boolean in conditional statements. This has the impact in
this use case that it can be mis-read to be interpretting the value for
firstFrame_ which is assigned as the buf.sequence.
Remove this potential for confusion by making it clear that the first
frame handling is only performed when firstFrame_ does not have a value
assigned.
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[Kieran: Rework commit message]
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
The ipa_interface.h file includes a number of headers that are not
directly used. Remove them, and add them to the source files that
include ipa_interface.h as required.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
The LSP autoformatter doesn't like some of the current formatting, let's
make it happier. Note that not all of its suggestions were accepted
because readability is preferred and adjusting .clang-format may not be
easy or possible.
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>
