| Message ID | 20210830082150.42514-5-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Me, On 8/30/21 1:51 PM, 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 This cam command is invalid as of today. It was fine when I wrote the series. The invalidation comes from 4fec25a4b ("ipu3: Disallow raw only camera configuration"). I am tempted to remove this cam command (since it's not valid today), but I don't think I would have a better explanation than this. All I can do is to replace it with a block of text (essentially making the commit message still longer) Any ideas anyone? > > 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: Jacopo Mondi <jacopo@jmondi.org> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > index ceaf665e..9cedcb5b 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -295,6 +295,14 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int> > continue; > > float ratio = static_cast<float>(sz.width) / sz.height; > + /* > + * Ratios can differ by small mantissa difference which > + * can affect the selection of the sensor output size > + * wildly. We are interested in selection of the closest > + * size with respect to the desired output size, hence > + * comparing it 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;
On Mon, Aug 30, 2021 at 02:02:27PM +0530, Umang Jain wrote: > Hi Me, > > On 8/30/21 1:51 PM, 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 > > > This cam command is invalid as of today. It was fine when I wrote the > series. > > The invalidation comes from 4fec25a4b ("ipu3: Disallow raw only camera > configuration"). I am tempted to remove this cam command (since it's not > valid today), but I don't think I would have a better explanation than this. > All I can do is to replace it with a block of text (essentially making the > commit message still longer) > > Any ideas anyone? > What about adding a YUV stream of the same size to the command ? > > > > 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: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > index ceaf665e..9cedcb5b 100644 > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > @@ -295,6 +295,14 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int> > > continue; > > float ratio = static_cast<float>(sz.width) / sz.height; > > + /* > > + * Ratios can differ by small mantissa difference which > > + * can affect the selection of the sensor output size > > + * wildly. We are interested in selection of the closest > > + * size with respect to the desired output size, hence > > + * comparing it 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;
Hi Jacopo On 8/30/21 2:22 PM, Jacopo Mondi wrote: > On Mon, Aug 30, 2021 at 02:02:27PM +0530, Umang Jain wrote: >> Hi Me, >> >> On 8/30/21 1:51 PM, 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 >> >> This cam command is invalid as of today. It was fine when I wrote the >> series. >> >> The invalidation comes from 4fec25a4b ("ipu3: Disallow raw only camera >> configuration"). I am tempted to remove this cam command (since it's not >> valid today), but I don't think I would have a better explanation than this. >> All I can do is to replace it with a block of text (essentially making the >> commit message still longer) >> >> Any ideas anyone? >> > What about adding a YUV stream of the same size to the command ? As of master, 7208e70211a6 ("libcamera: ipu3: Always use sensor full frame size") is in effect right? So it's always selecting the highest sensor format resolution. I cherry-picked your "DNI: libcamera: ipu3: Use the optimal sensor side" which generates the same results as the commit message, to double-check. So I don't adding YUV stream helps. Probably I'll just need to drop these commands and explain everything in a text. > >>> 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: Jacopo Mondi <jacopo@jmondi.org> >>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp >>> index ceaf665e..9cedcb5b 100644 >>> --- a/src/libcamera/pipeline/ipu3/cio2.cpp >>> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp >>> @@ -295,6 +295,14 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int> >>> continue; >>> float ratio = static_cast<float>(sz.width) / sz.height; >>> + /* >>> + * Ratios can differ by small mantissa difference which >>> + * can affect the selection of the sensor output size >>> + * wildly. We are interested in selection of the closest >>> + * size with respect to the desired output size, hence >>> + * comparing it 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;
Hello Umang, On Mon, Aug 30, 2021 at 03:38:44PM +0530, Umang Jain wrote: > Hi Jacopo > > On 8/30/21 2:22 PM, Jacopo Mondi wrote: > > On Mon, Aug 30, 2021 at 02:02:27PM +0530, Umang Jain wrote: > > > Hi Me, > > > > > > On 8/30/21 1:51 PM, 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 > > > > > > This cam command is invalid as of today. It was fine when I wrote the > > > series. > > > > > > The invalidation comes from 4fec25a4b ("ipu3: Disallow raw only camera > > > configuration"). I am tempted to remove this cam command (since it's not > > > valid today), but I don't think I would have a better explanation than this. > > > All I can do is to replace it with a block of text (essentially making the > > > commit message still longer) > > > > > > Any ideas anyone? > > > > > What about adding a YUV stream of the same size to the command ? > > > As of master, 7208e70211a6 ("libcamera: ipu3: Always use sensor full frame > size") is in effect right? So it's always selecting the highest sensor > format resolution. Ah ups, I'm running with my branch :) > > I cherry-picked your "DNI: libcamera: ipu3: Use the optimal sensor side" > which generates the same results as the commit message, to double-check. > > So I don't adding YUV stream helps. Probably I'll just need to drop these > commands and explain everything in a text. You're right! Or maybe, for the sake of the example, just revert the commit that blocks the single RAW stream ? Anyway, up to you :) > > > > > > > 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: Jacopo Mondi <jacopo@jmondi.org> > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > --- > > > > src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > index ceaf665e..9cedcb5b 100644 > > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > @@ -295,6 +295,14 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int> > > > > continue; > > > > float ratio = static_cast<float>(sz.width) / sz.height; > > > > + /* > > > > + * Ratios can differ by small mantissa difference which > > > > + * can affect the selection of the sensor output size > > > > + * wildly. We are interested in selection of the closest > > > > + * size with respect to the desired output size, hence > > > > + * comparing it 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;
Hi Jacopo On 8/30/21 3:53 PM, Jacopo Mondi wrote: > Hello Umang, > > On Mon, Aug 30, 2021 at 03:38:44PM +0530, Umang Jain wrote: >> Hi Jacopo >> >> On 8/30/21 2:22 PM, Jacopo Mondi wrote: >>> On Mon, Aug 30, 2021 at 02:02:27PM +0530, Umang Jain wrote: >>>> Hi Me, >>>> >>>> On 8/30/21 1:51 PM, 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 >>>> This cam command is invalid as of today. It was fine when I wrote the >>>> series. >>>> >>>> The invalidation comes from 4fec25a4b ("ipu3: Disallow raw only camera ops, wrong commit message here, it should have been 0536a9aa7189f75c898 >>>> configuration"). I am tempted to remove this cam command (since it's not >>>> valid today), but I don't think I would have a better explanation than this. >>>> All I can do is to replace it with a block of text (essentially making the >>>> commit message still longer) >>>> >>>> Any ideas anyone? >>>> >>> What about adding a YUV stream of the same size to the command ? >> >> As of master, 7208e70211a6 ("libcamera: ipu3: Always use sensor full frame >> size") is in effect right? So it's always selecting the highest sensor >> format resolution. > Ah ups, I'm running with my branch :) > >> I cherry-picked your "DNI: libcamera: ipu3: Use the optimal sensor side" >> which generates the same results as the commit message, to double-check. >> >> So I don't adding YUV stream helps. Probably I'll just need to drop these >> commands and explain everything in a text. > You're right! > Or maybe, for the sake of the example, just revert the commit that > blocks the single RAW stream ? Yes, I can probably mention that with a '*' on the command. * Please revert 0536a9aa718(" ipu3: Disallow raw only camera configuration ") if the configuration is reported invalid It seems we will have raw-only configs in the future (it's a acceptable case to have, but just not sure when/priority), so at the time those command will be fine. > > Anyway, up to you :) > >>>>> 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: Jacopo Mondi <jacopo@jmondi.org> >>>>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> >>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>> --- >>>>> src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++ >>>>> 1 file changed, 8 insertions(+) >>>>> >>>>> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp >>>>> index ceaf665e..9cedcb5b 100644 >>>>> --- a/src/libcamera/pipeline/ipu3/cio2.cpp >>>>> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp >>>>> @@ -295,6 +295,14 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int> >>>>> continue; >>>>> float ratio = static_cast<float>(sz.width) / sz.height; >>>>> + /* >>>>> + * Ratios can differ by small mantissa difference which >>>>> + * can affect the selection of the sensor output size >>>>> + * wildly. We are interested in selection of the closest >>>>> + * size with respect to the desired output size, hence >>>>> + * comparing it 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;
diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index ceaf665e..9cedcb5b 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -295,6 +295,14 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int> continue; float ratio = static_cast<float>(sz.width) / sz.height; + /* + * Ratios can differ by small mantissa difference which + * can affect the selection of the sensor output size + * wildly. We are interested in selection of the closest + * size with respect to the desired output size, hence + * comparing it 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: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++ 1 file changed, 8 insertions(+)