| Message ID | 20210810075854.86191-5-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
On Tue, Aug 10, 2021 at 01:28:54PM +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. Just "lead to size selection mismatches" maybe ? The example below is explicative enough > > 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 s/that, that/that/ > 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> > Tested-by: Umang Jain <umang.jain@ideasonboard.com> > Tested-by: Jacopo Mondi <jacopo@jmondi.org> I recall I asked about if we want rounding or truncating is fine. I think you confirmed truncating is fine, which I agree. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > src/libcamera/pipeline/ipu3/cio2.cpp | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > index 3c9331e3..6ccef301 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -290,6 +290,11 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int> > continue; > > float ratio = static_cast<float>(sz.width) / sz.height; > + /* > + * Comparing ratios with a single precision digit > + * is enough. > + */ > + 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.1 >
On 25/08/2021 11:12, Jacopo Mondi wrote: > On Tue, Aug 10, 2021 at 01:28:54PM +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. > > Just "lead to size selection mismatches" maybe ? > The example below is explicative enough > >> >> 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 > > s/that, that/that/ > >> 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> >> Tested-by: Umang Jain <umang.jain@ideasonboard.com> >> Tested-by: Jacopo Mondi <jacopo@jmondi.org> > > I recall I asked about if we want rounding or truncating is fine. I > think you confirmed truncating is fine, which I agree. > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > >> --- >> src/libcamera/pipeline/ipu3/cio2.cpp | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp >> index 3c9331e3..6ccef301 100644 >> --- a/src/libcamera/pipeline/ipu3/cio2.cpp >> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp >> @@ -290,6 +290,11 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int> >> continue; >> >> float ratio = static_cast<float>(sz.width) / sz.height; >> + /* >> + * Comparing ratios with a single precision digit >> + * is enough. >> + */ It might be worth expanding this to say why it's enough, or rather what happens if greater precision is used. It's covered by the commit message, but that might not be seen by someone trying to work out 'why' single precision is used rather than the full float. But either way, I can see how this is a direct advantage. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> + 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.1 >>
diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index 3c9331e3..6ccef301 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -290,6 +290,11 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int> continue; float ratio = static_cast<float>(sz.width) / sz.height; + /* + * Comparing ratios with a single precision digit + * is enough. + */ + 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> Tested-by: Umang Jain <umang.jain@ideasonboard.com> Tested-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/cio2.cpp | 5 +++++ 1 file changed, 5 insertions(+)