| Message ID | 20210803133205.6599-5-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On Tue, Aug 03, 2021 at 07:02:05PM +0530, Umang Jain wrote: > Do not compare higher precision of the ratios, as it might lead to > absurd selection of sensor size for a relatively low requested > resolution size. > > For example: > The imx258 driver supports the following sensor resolutions: > > - 4208x3118 = 1.349583066 > - 2104x1560 = 1.348717949 > - 1048x780 = 1.343589744 > > It can be inferred that, that the aspect ratio only differs by a small > factor with each other. It does not makes sense to select a 4208x3118 > for a requested size of say 640x480 or 1280x720, which is what is > happening currently. > ($) cam -c1 -swidth=640,height=480,role=raw > - CIO2 configuration: 4208x3118-SGRBG10_IPU3 > > In order to address this constraint, only compare the ratio with single > precision to make a better decision on the sensor resolution policy > selection. > > ($) cam -c1 -srole=raw,width=640,height=480 > - CIO2 configuration: 1048x780-SGRBG10_IPU3 > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/cio2.cpp | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > index 9980b8bd..887b850d 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -281,6 +281,13 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int> > continue; > > float ratio = static_cast<float>(sz.width) / sz.height; > + /* > + * Comparing ratios with a single precision is enough. > + * This avoids the selection of a frame size which is > + * 2x or 3x than the requested size, having a slightly > + * better ratio w.r.t native resolution ratio. Well, the fact the 2x or 3x sizes selection happens is Nautilus specific. I would stay simple and just /* * Comparing rations with a single precision * digit is enough. */ Be aware that this actually truncates instead of rounding.. is this the correct choice ? Also do we need to round the desired ratio too ? Thanks j > + */ > + ratio = static_cast<unsigned int>(ratio * 10) / 10.0; > float ratioDiff = fabsf(ratio - desiredRatio); > unsigned int area = sz.width * sz.height; > unsigned int areaDiff = area - desiredArea; > -- > 2.31.0 >
Hi Jacopo On 8/9/21 9:40 PM, Jacopo Mondi wrote: > Hi Umang, > > On Tue, Aug 03, 2021 at 07:02:05PM +0530, Umang Jain wrote: >> Do not compare higher precision of the ratios, as it might lead to >> absurd selection of sensor size for a relatively low requested >> resolution size. >> >> For example: >> The imx258 driver supports the following sensor resolutions: >> >> - 4208x3118 = 1.349583066 >> - 2104x1560 = 1.348717949 >> - 1048x780 = 1.343589744 >> >> It can be inferred that, that the aspect ratio only differs by a small >> factor with each other. It does not makes sense to select a 4208x3118 >> for a requested size of say 640x480 or 1280x720, which is what is >> happening currently. >> ($) cam -c1 -swidth=640,height=480,role=raw >> - CIO2 configuration: 4208x3118-SGRBG10_IPU3 >> >> In order to address this constraint, only compare the ratio with single >> precision to make a better decision on the sensor resolution policy >> selection. >> >> ($) cam -c1 -srole=raw,width=640,height=480 >> - CIO2 configuration: 1048x780-SGRBG10_IPU3 >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/libcamera/pipeline/ipu3/cio2.cpp | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp >> index 9980b8bd..887b850d 100644 >> --- a/src/libcamera/pipeline/ipu3/cio2.cpp >> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp >> @@ -281,6 +281,13 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int> >> continue; >> >> float ratio = static_cast<float>(sz.width) / sz.height; >> + /* >> + * Comparing ratios with a single precision is enough. >> + * This avoids the selection of a frame size which is >> + * 2x or 3x than the requested size, having a slightly >> + * better ratio w.r.t native resolution ratio. > Well, the fact the 2x or 3x sizes selection happens is Nautilus > specific. I would stay simple and just > > /* > * Comparing rations with a single precision > * digit is enough. > */ > > Be aware that this actually truncates instead of rounding.. is this > the correct choice ? Also do we need to round the desired ratio too ? I think it's the correct choice of truncating for now. No, desired ratio can stay as is, as it's constant. We need to nuke the subtle differences between "competing" sizes and either respective ratios . > > Thanks > j > >> + */ >> + ratio = static_cast<unsigned int>(ratio * 10) / 10.0; >> float ratioDiff = fabsf(ratio - desiredRatio); >> unsigned int area = sz.width * sz.height; >> unsigned int areaDiff = area - desiredArea; >> -- >> 2.31.0 >>
diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index 9980b8bd..887b850d 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -281,6 +281,13 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int> continue; float ratio = static_cast<float>(sz.width) / sz.height; + /* + * Comparing ratios with a single precision is enough. + * This avoids the selection of a frame size which is + * 2x or 3x than the requested size, having a slightly + * better ratio w.r.t native resolution ratio. + */ + ratio = static_cast<unsigned int>(ratio * 10) / 10.0; float ratioDiff = fabsf(ratio - desiredRatio); unsigned int area = sz.width * sz.height; unsigned int areaDiff = area - desiredArea;
Do not compare higher precision of the ratios, as it might lead to absurd selection of sensor size for a relatively low requested resolution size. For example: The imx258 driver supports the following sensor resolutions: - 4208x3118 = 1.349583066 - 2104x1560 = 1.348717949 - 1048x780 = 1.343589744 It can be inferred that, that the aspect ratio only differs by a small factor with each other. It does not makes sense to select a 4208x3118 for a requested size of say 640x480 or 1280x720, which is what is happening currently. ($) cam -c1 -swidth=640,height=480,role=raw - CIO2 configuration: 4208x3118-SGRBG10_IPU3 In order to address this constraint, only compare the ratio with single precision to make a better decision on the sensor resolution policy selection. ($) cam -c1 -srole=raw,width=640,height=480 - CIO2 configuration: 1048x780-SGRBG10_IPU3 Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/libcamera/pipeline/ipu3/cio2.cpp | 7 +++++++ 1 file changed, 7 insertions(+)