Message ID | 20210730082832.152626-4-umang.jain@ideasonboard.com |
---|---|
State | Superseded |
Delegated to: | Umang Jain |
Headers | show |
Series |
|
Related | show |
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@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 >
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; + 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()) { + 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; }
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. 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. /* 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@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 --- src/libcamera/pipeline/ipu3/cio2.cpp | 62 +++++++++++++++++----------- 1 file changed, 38 insertions(+), 24 deletions(-)