[libcamera-devel] [RFC PATCH 3/3] ipu3: cio2: Customize sensor format selection logic
Jacopo Mondi
jacopo at jmondi.org
Mon Aug 2 18:55:36 CEST 2021
Hi Umang,
On Fri, Jul 30, 2021 at 01:58:28PM +0530, Umang Jain wrote:
> Adapt the sensor format selection logic such that it address
> platform constraints on Soraka and Nautilus. This is best-effort
> basis hence, new platforms can bring new constraints in future
> and we will need to adapt accordingly.
>
> Currently we prioritise sensor format resolutions which closest to the
> FoV of desired output. However, Intel seems to prioritize the selection
> based on the closest FoV with respect to sensor's maximum resolution.
>
> For e.g. for a desired output of 1080p, Soraka will select 2112x1568
> since it's a better FoV match to sensor's maximum resolution
> (4224x3136). It will not match the provided resolution of 2112x1188,
> even if that matches exactly to the desired ratio of 1080p.
Sorry if it might seems picky, but this is not easy and when we'll read it
in 1 year time (or in 1 month ?) we'll probably be confused, so I'll
rather spend a bit more time discussing the commit message too.
So, the culprit here is the ImgU configuration procedure. I don't
think we have particular constraints in the hw itself, I'm sure the
ImgU could work with either resolutions, but the parameter calculation
procedure is what fail us, and we currently have no better
alternatives, nor documentation to try to fix it. Hence we have to
please the configuration procedure, and the way to do has been reverse
engineered looking at the sizes selected by the manufacturer and
recorded in an xml configuration file part of the Android HAL
implementation. That said, I would rework this paragraphs slightly
as:
"Rework the sensor frame size selection procedure to take into account
IPU3 platform specific requirements.
The correct operations of the ImgU configuration procedure depend on
the selected sensor frame size provided as input to the ISP. The
current implementation, based on a vendor provided configuration
script, fails to produce any result if the input frame does not match
the expected size. In order to guarantee correctness of the
configuration procedure, the vendor implementation of the Android HAL
is shipped with a sensor specific configuration file where each
supported output resolution combination (main and viewfinder output)
is associated with the 'correct' sensor frame size to use. Both the
configuration script and configuration file content are tightly
coupled and hand-tuned to guarantee correctness of the operations.
As the libcamera IPU3 pipeline handler does not limit the number of
output combinations to the ones supported by the vendor Android HAL,
it can't make use of any configuration file, but needs anyhow to
ensure that the selected sensor frame size matches what is expected by
the configuration procedure.
Based on the reverse-engineering of the configuration file content it
seems that the ImgU configuration procedure is more reliable when the
capture pipeline is configured to maximize scaling and cropping in the
ISP rather than in the sensor. Currently, the IPU3 pipeline is based
on the frame size selection procedure as implemented by
CameraSensor::getFormat() which prioritizes resolutions with the same
FOV of the requested output size. This however requires a certain
amount of cropping to happen on the sensor's pixel array to produce
frames in the same resolution as the desired output size.
Modify the frame size selection procedure to prioritize resolutions
with the same FOV as the sensor's native one, to avoid cropping in the
sensor pixel array.
In example, with the current implementation :
- on a Soraka device equipped with ov13858 as back sensor, with a
native resolution of 4224x3136 (4:3), when requested to select the
sensor output size to produce 1080p (16:9) a frame size of 2112x1188
(16:9) is selected causing the ImgU configuration procedure to fail.
If a resolution with the same FOV as the sensor's native size, such
as 2112x1568 (4:3), is selected the pipeline works correctly.
- Umang could you elaborate a similar example for Nautilus here ?
>
> On the other hand, on Nautilus, currently the sensor's maximum
> resolution(4208x3118) is the only selection for any 16:9 desired
> formats, whether it is 640x360 or 1080p or 3840x2160. That means
> the sensor will run at full bandwidth even for ridiculously smaller
> resolutions so, we probably don't want that either.
>
> This patch how the sensor format resolution is selected:
> - It prioritizes FoV with respect to sensor's maximum resolution size
> - It shall never return sensor's maximum resolution provided if there
> are other lower resolutions available.
I think that's not desirable and actually breaks capture with ov5670
on my Soraka, as it causes a smaller resolution, with a FOV !=
sensor's one to be selected.
As you have clarified me offline and
have reported in the "[RFC PATCH] camera_sensor: Do not always
prioritize aspect-ratios serie" cover letter, the issue with Nautilus
is that the imx258 sensor driver supports the following resolutions
- 4208x3118 = 1.349583066
- 2104x1560 = 1.348717949
- 1048x780 = 1.343589744
The FOV ratio diff with the 1080p one will always result smaller for max
size, by a very tiny factor, hence max size is always selected.
I think we should rather round the FOV ratios to the first decimal
digit and consider all of the above 4:3 resolutions. We can have a
rounding table that associates each FOV ratio to one the standard ones
https://en.wikipedia.org/wiki/VGA_resolution but that might be an
overkill.
>
> /* DNI: Preliminary testing of getSensorFormat():
> * cam -c1 -swidth=640,height=360,role=raw
> * cam -c1 -swidth=1280,height=720,role=raw
> * cam -c1 -swidth=1920,height=1080,role=raw
> * cam -c1 -swidth=3840,height=2160,role=raw
> */
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> Output of preliminary testing:
> ($) cam -c1 -swidth=640,height=360,role=raw
> INFO IPU3 cio2.cpp:326 Desired Size: 640x360, Found SGRBG10_IPU3 with Size: 2104x1560
>
> ($) cam -c1 -swidth=1280,height=720,role=raw
> INFO IPU3 cio2.cpp:326 Desired Size: 1280x720, Found SGRBG10_IPU3 with Size: 2104x1560
>
> ($) cam -c1 -swidth=1920,height=1080,role=raw
> INFO IPU3 cio2.cpp:326 Desired Size: 1920x1080, Found SGRBG10_IPU3 with Size: 2104x1560
>
> ($) cam -c1 -swidth=3840,height=2160,role=raw
> INFO IPU3 cio2.cpp:326 Desired Size: 3840x2160, Found SGRBG10_IPU3 with Size: 4208x3118
For a comparison it would help making sure the selected frame size has
the same FOV as the sensor's one.
(also, using FOV as I've done here in this mail is not really
correct, as the FOV is already a ratio of the selected output size and
the sensor's native field of view. Probably using 'resolution' would
be more correct, but there's a clear shortage of terms and we would
have a lot of confusing repetitions :)
> ---
> src/libcamera/pipeline/ipu3/cio2.cpp | 62 +++++++++++++++++-----------
> 1 file changed, 38 insertions(+), 24 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index aef88afd..4fd57ef7 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -245,20 +245,16 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const
> *
> * - The desired \a size shall fit in the sensor output size to avoid the need
> * to up-scale.
> - * - The sensor output size shall match the desired aspect ratio to avoid the
> - * need to crop the field of view.
> - * - The sensor output size shall be as small as possible to lower the required
> - * bandwidth.
> + * - The sensor output size will be set to maximum resolution only when there
> + * are no other available lower sensor resolutions from any of \a mbusCodes.
> + * - The sensor output size shall have the closest FoV with respect
> + * to the sensor's maximum resolution.
> * - The desired \a size shall be supported by one of the media bus code listed
> * in \a mbusCodes.
> *
> * When multiple media bus codes can produce the same size, the code at the
> * lowest position in \a mbusCodes is selected.
> *
> - * The use of this method is optional, as the above criteria may not match the
> - * needs of all pipeline handlers. Pipeline handlers may implement custom
> - * sensor format selection when needed.
> - *
> * The returned sensor output format is guaranteed to be acceptable by the
> * setFormat() method without any modification.
> *
> @@ -268,14 +264,15 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const
> V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int> &mbusCodes,
> const Size &size) const
> {
> - unsigned int desiredArea = size.width * size.height;
> - unsigned int bestArea = UINT_MAX;
> - float desiredRatio = static_cast<float>(size.width) / size.height;
> - float bestRatio = FLT_MAX;
> - const Size *bestSize = nullptr;
> + std::map<Size, float> selectedSizes;
> + Size bestSize;
> + float maxResRatio = FLT_MAX;
> + float bestRatioDiff = -1.0;
> uint32_t bestCode = 0;
>
> for (unsigned int code : mbusCodes) {
> + Size maxSensorResolution;
This is should be an absolute parameter simply computed using
sensor->resolution() instead of re-initializing it for each format.
> +
> const auto formats = formats_.find(code);
> if (formats == formats_.end())
> continue;
> @@ -286,33 +283,50 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int>
> if (sz.width < size.width || sz.height < size.height)
> continue;
>
> + if (sz > maxSensorResolution)
> + maxSensorResolution = sz;
> +
> float ratio = static_cast<float>(sz.width) / sz.height;
> - float ratioDiff = fabsf(ratio - desiredRatio);
> - unsigned int area = sz.width * sz.height;
> - unsigned int areaDiff = area - desiredArea;
> + selectedSizes.emplace(sz, ratio);
> + }
>
> - if (ratioDiff > bestRatio)
> - continue;
> + if (!selectedSizes.empty()) {
I'm still not sure I got why you need a double pass and you can't
select the best one in a single one, as it was done before. I
understand you want to remove the sensor's max size which I don't
think is desirable but you could have (if you take my suggestion of
adding CameraSensor::sizes(mbusCode) in
for (code : sensor->mbusCodes()) {
for (size: sensor->sizes(code)) {
/* sizes selection here */
}
}
Or have I missed a constraint on Nautilus ?
I actually managed to get rid of all CTS failures on Soraka by simply
--- a/src/libcamera/pipeline/ipu3/cio2.cpp
+++ b/src/libcamera/pipeline/ipu3/cio2.cpp
@@ -7,7 +7,8 @@
#include "cio2.h"
-#include <float.h>
+#include <limits.h>
+#include <math.h>
#include <linux/media-bus-format.h>
@@ -270,7 +271,9 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int>
{
unsigned int desiredArea = size.width * size.height;
unsigned int bestArea = UINT_MAX;
- float desiredRatio = static_cast<float>(size.width) / size.height;
+ const Size &resolution = sensor_->resolution();
+ float desiredRatio = static_cast<float>(resolution.width) /
+ resolution.height;
float bestRatio = FLT_MAX;
const Size *bestSize = nullptr;
uint32_t bestCode = 0;
Which basically just do what we have been discussing about: make the
desired FOV match the sensor's one instead of the output size one.
It's amazing how many words one can produce to describe 4 lines of
code :)
Anyway, if you're willing to try rounding the Nautilus sizes and test
the above small patch on top to see how it behaves on nautilus, be
aware you need to revert
208e70211a6 ("libcamera: ipu3: Always use sensor full frame size")
I can prepare a branch with also the update of the FrameDuration
properties for a complete CTS run.
Thanks
j
> + maxResRatio = selectedSizes[maxSensorResolution];
>
> - if (ratioDiff < bestRatio || areaDiff < bestArea) {
> - bestRatio = ratioDiff;
> - bestArea = areaDiff;
> - bestSize = &sz;
> + selectedSizes.erase(maxSensorResolution);
> + if (selectedSizes.empty()) {
> bestCode = code;
> + bestSize = maxSensorResolution;
> + } else { /* Find the best FoV to sensor's max resolution */
> + for (auto iter : selectedSizes) {
> + float ratioDiff = fabsf(maxResRatio - iter.second);
> +
> + if (bestRatioDiff < 0 || (ratioDiff < bestRatioDiff)) {
> + bestRatioDiff = ratioDiff;
> + bestCode = code;
> + bestSize = iter.first;
> + }
> + }
> }
> }
> + selectedSizes.clear();
> }
>
> - if (!bestSize) {
> + if (bestSize.isNull()) {
> LOG(IPU3, Debug) << "No supported format or size found";
> return {};
> }
>
> V4L2SubdeviceFormat format{
> .mbus_code = bestCode,
> - .size = *bestSize,
> + .size = bestSize,
> };
>
> + LOG(IPU3, Info)
> + << "Desired Size: " << size.toString()
> + << ", Found " << mbusCodesToPixelFormat.at(format.mbus_code).toString()
> + << " with Size: " << format.size.toString();
> +
> return format;
> }
>
> --
> 2.31.0
>
More information about the libcamera-devel
mailing list