| Message ID | 20200917112839.5099-1-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Thu, Sep 17, 2020 at 01:28:39PM +0200, Jacopo Mondi wrote: > When calculating the pipeline configuration for the IPU3 platform, > libcamera tries to be smart and select the smallest sensor frame > resolution larger enough to accommodate the stream sizes s/larger/large/ > requested by the application. > > While this seems to make a lot of sense, in practice optimizing the s/seems to make/makes/ > selected sensor resolution makes the pipeline configuration calculation > process fail in multiple occasions, or results in stalls during capture. > > As a trivial example, capturing with cam with the following command > line results in a stall: > $ cam -swidth=1280,height=720 -swidth=640,height=480 -c1 -C > > Likewise, the Android HAL supported format enumeration fails in > reporting smaller resolutions as supported when used with the OV5670 > sensor. > > 320x240: > DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 648x486-SGRBG10_IPU3 > ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration > ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions. > > 640x480: > DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 320x240-SGRBG10_IPU3 > ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration > ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions. > > Furthermore the reference xml files used for the IPU3 camera > configuration on the ChromeOS platform restricts the number of sensor > resolution to be used for the OV5670 sensor to 2 from the 6 supported by > the driver [1]. > > The selection criteria of the correct CIO2 mode are not specified, and > for the time being, always use the sensor maximum resolution at the > expense of frame rate and bus bandwidth to allow the pipeline to successfully > support smaller modes for the OV5670 sensor and solves pipeline stalls s/solves/solve/ > when capturing with both sensors. I would make it clearer this is a temporary workaround. Maybe "For the time being, as a workaround, ..." ? > > [1] See the <sensor_modes> enumeration in: > https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov5670.xml > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > RFC->v1: > - Add Niklas' tag > - Expand the \todo comment and remove the existing comment block > > Just have a look at the \todo block wording, if no big questions I'll push this > one soon. > > Thanks > j > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 31 ++++++++++++---------------- > 1 file changed, 13 insertions(+), 18 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 2d881fe28f98..2ba00a2ca211 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -144,25 +144,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > status = Adjusted; > } > > - /* > - * Validate the requested stream configuration and select the sensor > - * format by collecting the maximum RAW stream width and height and > - * picking the closest larger match, as the IPU3 can downscale only. If > - * no resolution is requested for the RAW stream, use the one from the > - * largest YUV stream, plus margins pixels for the IF and BDS to scale. > - * If no resolution is requested for any stream, pick the largest one. > - */ > + /* Validate the requested stream configuration */ > unsigned int rawCount = 0; > unsigned int yuvCount = 0; > Size maxYuvSize; > - Size maxRawSize; > > for (const StreamConfiguration &cfg : config_) { > const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat); > > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { > rawCount++; > - maxRawSize.expandTo(cfg.size); > } else { > yuvCount++; > maxYuvSize.expandTo(cfg.size); > @@ -174,18 +165,22 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > return Invalid; > } > > - if (maxRawSize.isNull()) > - maxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, > - IMGU_OUTPUT_HEIGHT_MARGIN) > - .boundedTo(data_->cio2_.sensor()->resolution()); > - > /* > * Generate raw configuration from CIO2. > * > - * The output YUV streams will be limited in size to the maximum > - * frame size requested for the RAW stream. > + * \todo The image sensor frame size should be calculated as the > + * smallest possible resolution larger enough to accommodate the > + * requested stream sizes. However such a selection makes the pipeline Smallest possible size may not always be the best, we haven't really thought about this. I would write "The image sensor output size should be selected to optimize operation based on the sizes of the requested streams.". > + * configuration procedure fail for small resolution (in example: s/in example/for example/ > + * 640x480 with OV5670) and causes the capture operations to stall for > + * some streams size combinations (see the commit message of the patch s/streams/stream/ > + * that introduced this comment for more failure examples). > + * > + * Until the sensor frame size calculation criteria are not clarified, s/are not clarified/are clarified/ Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * always use the largest possible one which guarantees better results > + * at the expense of the frame rate and CSI-2 bus bandwidth. > */ > - cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize); > + cio2Configuration_ = data_->cio2_.generateConfiguration({}); > if (!cio2Configuration_.pixelFormat.isValid()) > return Invalid; >
Hi Jacopo, On 18/09/2020 03:57, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Sep 17, 2020 at 01:28:39PM +0200, Jacopo Mondi wrote: >> When calculating the pipeline configuration for the IPU3 platform, >> libcamera tries to be smart and select the smallest sensor frame >> resolution larger enough to accommodate the stream sizes > > s/larger/large/ > >> requested by the application. >> >> While this seems to make a lot of sense, in practice optimizing the > > s/seems to make/makes/ > >> selected sensor resolution makes the pipeline configuration calculation >> process fail in multiple occasions, or results in stalls during capture. >> >> As a trivial example, capturing with cam with the following command >> line results in a stall: >> $ cam -swidth=1280,height=720 -swidth=640,height=480 -c1 -C >> >> Likewise, the Android HAL supported format enumeration fails in >> reporting smaller resolutions as supported when used with the OV5670 >> sensor. >> >> 320x240: >> DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 648x486-SGRBG10_IPU3 >> ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration >> ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions. >> >> 640x480: >> DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 320x240-SGRBG10_IPU3 >> ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration >> ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions. >> >> Furthermore the reference xml files used for the IPU3 camera >> configuration on the ChromeOS platform restricts the number of sensor >> resolution to be used for the OV5670 sensor to 2 from the 6 supported by >> the driver [1]. >> >> The selection criteria of the correct CIO2 mode are not specified, and >> for the time being, always use the sensor maximum resolution at the >> expense of frame rate and bus bandwidth to allow the pipeline to successfully >> support smaller modes for the OV5670 sensor and solves pipeline stalls > > s/solves/solve/ > >> when capturing with both sensors. > > I would make it clearer this is a temporary workaround. Maybe "For the > time being, as a workaround, ..." ? > >> >> [1] See the <sensor_modes> enumeration in: >> https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov5670.xml >> >> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >> --- >> RFC->v1: >> - Add Niklas' tag >> - Expand the \todo comment and remove the existing comment block >> >> Just have a look at the \todo block wording, if no big questions I'll push this >> one soon. >> >> Thanks >> j >> --- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 31 ++++++++++++---------------- >> 1 file changed, 13 insertions(+), 18 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index 2d881fe28f98..2ba00a2ca211 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -144,25 +144,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() >> status = Adjusted; >> } >> >> - /* >> - * Validate the requested stream configuration and select the sensor >> - * format by collecting the maximum RAW stream width and height and >> - * picking the closest larger match, as the IPU3 can downscale only. If >> - * no resolution is requested for the RAW stream, use the one from the >> - * largest YUV stream, plus margins pixels for the IF and BDS to scale. >> - * If no resolution is requested for any stream, pick the largest one. >> - */ >> + /* Validate the requested stream configuration */ >> unsigned int rawCount = 0; >> unsigned int yuvCount = 0; >> Size maxYuvSize; >> - Size maxRawSize; >> >> for (const StreamConfiguration &cfg : config_) { >> const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat); >> >> if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { >> rawCount++; >> - maxRawSize.expandTo(cfg.size); >> } else { >> yuvCount++; >> maxYuvSize.expandTo(cfg.size); >> @@ -174,18 +165,22 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() >> return Invalid; >> } >> >> - if (maxRawSize.isNull()) >> - maxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, >> - IMGU_OUTPUT_HEIGHT_MARGIN) >> - .boundedTo(data_->cio2_.sensor()->resolution()); >> - >> /* >> * Generate raw configuration from CIO2. >> * >> - * The output YUV streams will be limited in size to the maximum >> - * frame size requested for the RAW stream. >> + * \todo The image sensor frame size should be calculated as the >> + * smallest possible resolution larger enough to accommodate the >> + * requested stream sizes. However such a selection makes the pipeline > > Smallest possible size may not always be the best, we haven't really > thought about this. I would write "The image sensor output size should > be selected to optimize operation based on the sizes of the requested > streams.". Indeed, we really need to think about how we present this as an option to the users/applications as well. This is /use case/ dependant. A mobile platform wants to reduce power for instance, and might want the lowest reasonable sensor size to capture the required results, but other use-cases (perhaps a digital microscope) would want to use as high a resolution as possible from the sensor to capture as much light information as is possible, and deal with any size constraints at the rescaler. And of course it can also depend on how the resolution selection affects the field of view ... >> + * configuration procedure fail for small resolution (in example: > s/resolution/resolutions/ > s/in example/for example/ > >> + * 640x480 with OV5670) and causes the capture operations to stall for >> + * some streams size combinations (see the commit message of the patch > > s/streams/stream/ > >> + * that introduced this comment for more failure examples). >> + * >> + * Until the sensor frame size calculation criteria are not clarified, > > s/are not clarified/are clarified/ > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> + * always use the largest possible one which guarantees better results >> + * at the expense of the frame rate and CSI-2 bus bandwidth. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> */ >> - cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize); >> + cio2Configuration_ = data_->cio2_.generateConfiguration({}); >> if (!cio2Configuration_.pixelFormat.isValid()) >> return Invalid; >> >
Hi Kieran, On Fri, Sep 18, 2020 at 09:24:51AM +0100, Kieran Bingham wrote: > Hi Jacopo, > [snip] > >> - .boundedTo(data_->cio2_.sensor()->resolution()); > >> - > >> /* > >> * Generate raw configuration from CIO2. > >> * > >> - * The output YUV streams will be limited in size to the maximum > >> - * frame size requested for the RAW stream. > >> + * \todo The image sensor frame size should be calculated as the > >> + * smallest possible resolution larger enough to accommodate the > >> + * requested stream sizes. However such a selection makes the pipeline > > > > Smallest possible size may not always be the best, we haven't really > > thought about this. I would write "The image sensor output size should > > be selected to optimize operation based on the sizes of the requested > > streams.". > > Indeed, we really need to think about how we present this as an option > to the users/applications as well. > > This is /use case/ dependant. > Is it something applications should be in control of ? I'm not sure we even can always provide to application a way to select the sensor size to use in a generic way. We should indeed report it, mostly for digital zoom implementation purposes, but the selection of the sensor size in some platform (ie RPi) is not even available to userspace. For IPU3 you see how fragile is that, I guess all platforms have specificities there. Furthermore, and here I might be very wrong, I don't see a real reason to do so. See below, mostly for sake of discussion on the application space our API should aim to cover. > A mobile platform wants to reduce power for instance, and might want the > lowest reasonable sensor size to capture the required results, but other > use-cases (perhaps a digital microscope) would want to use as high a > resolution as possible from the sensor to capture as much light > information as is possible, and deal with any size constraints at the > rescaler. I'm not sure I see a correlation between having longer exposure times being prevented by using a smaller frame size. The frame exposure time is bounded by the frame rate (the higher the frame rate the shorter the max exposure time) and the frame rate is bounded by the frame size (the larger the frame size the lower the frame rate) so it seems to me the smaller the frame is, the more it can be exposed without impacting the frame rate. I might be wrong on this, but to me this is not even the point. The the manual control of exposure time and frame duration are already enough correlated by how those things are controlled by the (usually mode-based and feature limited) sensor driver and having any guarantee they work in a generic enough way is hard enuogh. The frame size which is input to the ISP processing pipeline is yet another parameter that makes even harder to gaurantee what works on one platform makes sense on another. I'm not saying we should not allow so, but basically that's what an IPA does, and only very specific applications might benefit from that and probably a 'generic camera API' is not what they want to use ? That said, we are also considering the need to have a way to control with platform specific configuration files at which point of the processing pipeline any sub-sampling or scaling happens, if on the sensor or on the ISP, and throwing applications controllable parameters in the mix makes things quite hard to handle. I'm not saying I'm totally against providing that control to applications, I would love if we could get to that level of detail, but to me that seems to be quite low on the list.. > > And of course it can also depend on how the resolution selection affects > the field of view ... > > >> + * configuration procedure fail for small resolution (in example: > > > > s/resolution/resolutions/ > > > s/in example/for example/ > > > >> + * 640x480 with OV5670) and causes the capture operations to stall for > >> + * some streams size combinations (see the commit message of the patch > > > > s/streams/stream/ > > > >> + * that introduced this comment for more failure examples). > >> + * > >> + * Until the sensor frame size calculation criteria are not clarified, > > > > s/are not clarified/are clarified/ > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> + * always use the largest possible one which guarantees better results > >> + * at the expense of the frame rate and CSI-2 bus bandwidth. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> */ > >> - cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize); > >> + cio2Configuration_ = data_->cio2_.generateConfiguration({}); > >> if (!cio2Configuration_.pixelFormat.isValid()) > >> return Invalid; > >> > > > > -- > Regards > -- > Kieran
Hi Kieran, On Fri, Sep 18, 2020 at 09:24:51AM +0100, Kieran Bingham wrote: > On 18/09/2020 03:57, Laurent Pinchart wrote: > > On Thu, Sep 17, 2020 at 01:28:39PM +0200, Jacopo Mondi wrote: > >> When calculating the pipeline configuration for the IPU3 platform, > >> libcamera tries to be smart and select the smallest sensor frame > >> resolution larger enough to accommodate the stream sizes > > > > s/larger/large/ > > > >> requested by the application. > >> > >> While this seems to make a lot of sense, in practice optimizing the > > > > s/seems to make/makes/ > > > >> selected sensor resolution makes the pipeline configuration calculation > >> process fail in multiple occasions, or results in stalls during capture. > >> > >> As a trivial example, capturing with cam with the following command > >> line results in a stall: > >> $ cam -swidth=1280,height=720 -swidth=640,height=480 -c1 -C > >> > >> Likewise, the Android HAL supported format enumeration fails in > >> reporting smaller resolutions as supported when used with the OV5670 > >> sensor. > >> > >> 320x240: > >> DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 648x486-SGRBG10_IPU3 > >> ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration > >> ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions. > >> > >> 640x480: > >> DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 320x240-SGRBG10_IPU3 > >> ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration > >> ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions. > >> > >> Furthermore the reference xml files used for the IPU3 camera > >> configuration on the ChromeOS platform restricts the number of sensor > >> resolution to be used for the OV5670 sensor to 2 from the 6 supported by > >> the driver [1]. > >> > >> The selection criteria of the correct CIO2 mode are not specified, and > >> for the time being, always use the sensor maximum resolution at the > >> expense of frame rate and bus bandwidth to allow the pipeline to successfully > >> support smaller modes for the OV5670 sensor and solves pipeline stalls > > > > s/solves/solve/ > > > >> when capturing with both sensors. > > > > I would make it clearer this is a temporary workaround. Maybe "For the > > time being, as a workaround, ..." ? > > > >> > >> [1] See the <sensor_modes> enumeration in: > >> https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov5670.xml > >> > >> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> RFC->v1: > >> - Add Niklas' tag > >> - Expand the \todo comment and remove the existing comment block > >> > >> Just have a look at the \todo block wording, if no big questions I'll push this > >> one soon. > >> > >> Thanks > >> j > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 31 ++++++++++++---------------- > >> 1 file changed, 13 insertions(+), 18 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 2d881fe28f98..2ba00a2ca211 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -144,25 +144,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > >> status = Adjusted; > >> } > >> > >> - /* > >> - * Validate the requested stream configuration and select the sensor > >> - * format by collecting the maximum RAW stream width and height and > >> - * picking the closest larger match, as the IPU3 can downscale only. If > >> - * no resolution is requested for the RAW stream, use the one from the > >> - * largest YUV stream, plus margins pixels for the IF and BDS to scale. > >> - * If no resolution is requested for any stream, pick the largest one. > >> - */ > >> + /* Validate the requested stream configuration */ > >> unsigned int rawCount = 0; > >> unsigned int yuvCount = 0; > >> Size maxYuvSize; > >> - Size maxRawSize; > >> > >> for (const StreamConfiguration &cfg : config_) { > >> const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat); > >> > >> if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { > >> rawCount++; > >> - maxRawSize.expandTo(cfg.size); > >> } else { > >> yuvCount++; > >> maxYuvSize.expandTo(cfg.size); > >> @@ -174,18 +165,22 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > >> return Invalid; > >> } > >> > >> - if (maxRawSize.isNull()) > >> - maxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, > >> - IMGU_OUTPUT_HEIGHT_MARGIN) > >> - .boundedTo(data_->cio2_.sensor()->resolution()); > >> - > >> /* > >> * Generate raw configuration from CIO2. > >> * > >> - * The output YUV streams will be limited in size to the maximum > >> - * frame size requested for the RAW stream. > >> + * \todo The image sensor frame size should be calculated as the > >> + * smallest possible resolution larger enough to accommodate the > >> + * requested stream sizes. However such a selection makes the pipeline > > > > Smallest possible size may not always be the best, we haven't really > > thought about this. I would write "The image sensor output size should > > be selected to optimize operation based on the sizes of the requested > > streams.". > > Indeed, we really need to think about how we present this as an option > to the users/applications as well. > > This is /use case/ dependant. > > A mobile platform wants to reduce power for instance, and might want the > lowest reasonable sensor size to capture the required results, but other > use-cases (perhaps a digital microscope) would want to use as high a > resolution as possible from the sensor to capture as much light > information as is possible, and deal with any size constraints at the > rescaler. It's actually the other way around, you'll get a better SNR if you scale on the sensor with binning. > And of course it can also depend on how the resolution selection affects > the field of view ... That's a separate issue, scaling doesn't affect the field of view, and cropping shouldn't be applied behind the scenes to lower the resolution. > >> + * configuration procedure fail for small resolution (in example: > > s/resolution/resolutions/ > > > s/in example/for example/ > > > >> + * 640x480 with OV5670) and causes the capture operations to stall for > >> + * some streams size combinations (see the commit message of the patch > > > > s/streams/stream/ > > > >> + * that introduced this comment for more failure examples). > >> + * > >> + * Until the sensor frame size calculation criteria are not clarified, > > > > s/are not clarified/are clarified/ > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> + * always use the largest possible one which guarantees better results > >> + * at the expense of the frame rate and CSI-2 bus bandwidth. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> */ > >> - cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize); > >> + cio2Configuration_ = data_->cio2_.generateConfiguration({}); > >> if (!cio2Configuration_.pixelFormat.isValid()) > >> return Invalid; > >>
Hi Jacopo, Laurent, On 18/09/2020 14:33, Laurent Pinchart wrote: > Hi Kieran, > > On Fri, Sep 18, 2020 at 09:24:51AM +0100, Kieran Bingham wrote: >> On 18/09/2020 03:57, Laurent Pinchart wrote: >>> On Thu, Sep 17, 2020 at 01:28:39PM +0200, Jacopo Mondi wrote: >>>> When calculating the pipeline configuration for the IPU3 platform, >>>> libcamera tries to be smart and select the smallest sensor frame >>>> resolution larger enough to accommodate the stream sizes >>> >>> s/larger/large/ >>> >>>> requested by the application. >>>> >>>> While this seems to make a lot of sense, in practice optimizing the >>> >>> s/seems to make/makes/ >>> >>>> selected sensor resolution makes the pipeline configuration calculation >>>> process fail in multiple occasions, or results in stalls during capture. >>>> >>>> As a trivial example, capturing with cam with the following command >>>> line results in a stall: >>>> $ cam -swidth=1280,height=720 -swidth=640,height=480 -c1 -C >>>> >>>> Likewise, the Android HAL supported format enumeration fails in >>>> reporting smaller resolutions as supported when used with the OV5670 >>>> sensor. >>>> >>>> 320x240: >>>> DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 648x486-SGRBG10_IPU3 >>>> ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration >>>> ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions. >>>> >>>> 640x480: >>>> DEBUG IPU3 ipu3.cpp:192 CIO2 configuration: 320x240-SGRBG10_IPU3 >>>> ERROR IPU3 imgu.cpp:408 Failed to calculate pipe configuration >>>> ERROR IPU3 ipu3.cpp:299 Failed to calculate pipe configuration: unsupported resolutions. >>>> >>>> Furthermore the reference xml files used for the IPU3 camera >>>> configuration on the ChromeOS platform restricts the number of sensor >>>> resolution to be used for the OV5670 sensor to 2 from the 6 supported by >>>> the driver [1]. >>>> >>>> The selection criteria of the correct CIO2 mode are not specified, and >>>> for the time being, always use the sensor maximum resolution at the >>>> expense of frame rate and bus bandwidth to allow the pipeline to successfully >>>> support smaller modes for the OV5670 sensor and solves pipeline stalls >>> >>> s/solves/solve/ >>> >>>> when capturing with both sensors. >>> >>> I would make it clearer this is a temporary workaround. Maybe "For the >>> time being, as a workaround, ..." ? >>> >>>> >>>> [1] See the <sensor_modes> enumeration in: >>>> https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov5670.xml >>>> >>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>>> --- >>>> RFC->v1: >>>> - Add Niklas' tag >>>> - Expand the \todo comment and remove the existing comment block >>>> >>>> Just have a look at the \todo block wording, if no big questions I'll push this >>>> one soon. >>>> >>>> Thanks >>>> j >>>> --- >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 31 ++++++++++++---------------- >>>> 1 file changed, 13 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> index 2d881fe28f98..2ba00a2ca211 100644 >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> @@ -144,25 +144,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() >>>> status = Adjusted; >>>> } >>>> >>>> - /* >>>> - * Validate the requested stream configuration and select the sensor >>>> - * format by collecting the maximum RAW stream width and height and >>>> - * picking the closest larger match, as the IPU3 can downscale only. If >>>> - * no resolution is requested for the RAW stream, use the one from the >>>> - * largest YUV stream, plus margins pixels for the IF and BDS to scale. >>>> - * If no resolution is requested for any stream, pick the largest one. >>>> - */ >>>> + /* Validate the requested stream configuration */ >>>> unsigned int rawCount = 0; >>>> unsigned int yuvCount = 0; >>>> Size maxYuvSize; >>>> - Size maxRawSize; >>>> >>>> for (const StreamConfiguration &cfg : config_) { >>>> const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat); >>>> >>>> if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { >>>> rawCount++; >>>> - maxRawSize.expandTo(cfg.size); >>>> } else { >>>> yuvCount++; >>>> maxYuvSize.expandTo(cfg.size); >>>> @@ -174,18 +165,22 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() >>>> return Invalid; >>>> } >>>> >>>> - if (maxRawSize.isNull()) >>>> - maxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, >>>> - IMGU_OUTPUT_HEIGHT_MARGIN) >>>> - .boundedTo(data_->cio2_.sensor()->resolution()); >>>> - >>>> /* >>>> * Generate raw configuration from CIO2. >>>> * >>>> - * The output YUV streams will be limited in size to the maximum >>>> - * frame size requested for the RAW stream. >>>> + * \todo The image sensor frame size should be calculated as the >>>> + * smallest possible resolution larger enough to accommodate the >>>> + * requested stream sizes. However such a selection makes the pipeline >>> >>> Smallest possible size may not always be the best, we haven't really >>> thought about this. I would write "The image sensor output size should >>> be selected to optimize operation based on the sizes of the requested >>> streams.". >> >> Indeed, we really need to think about how we present this as an option >> to the users/applications as well. >> >> This is /use case/ dependant. >> >> A mobile platform wants to reduce power for instance, and might want the >> lowest reasonable sensor size to capture the required results, but other >> use-cases (perhaps a digital microscope) would want to use as high a >> resolution as possible from the sensor to capture as much light >> information as is possible, and deal with any size constraints at the >> rescaler. > > It's actually the other way around, you'll get a better SNR if you scale > on the sensor with binning. I expressed myself badly. (And also erroneously referenced using the scaler). Reducing SNR is essentially what I was trying to state. (or rather what I was thinking of) >> And of course it can also depend on how the resolution selection affects >> the field of view ... > > That's a separate issue, scaling doesn't affect the field of view, and > cropping shouldn't be applied behind the scenes to lower the resolution. Yes, scaling doesn't - but changing the sensor resolution does - doesn't it? Are all sensor modes implemented through binning? or are they also represented as crops? -- Kieran >>>> + * configuration procedure fail for small resolution (in example: >> >> s/resolution/resolutions/ >> >>> s/in example/for example/ >>> >>>> + * 640x480 with OV5670) and causes the capture operations to stall for >>>> + * some streams size combinations (see the commit message of the patch >>> >>> s/streams/stream/ >>> >>>> + * that introduced this comment for more failure examples). >>>> + * >>>> + * Until the sensor frame size calculation criteria are not clarified, >>> >>> s/are not clarified/are clarified/ >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>>> + * always use the largest possible one which guarantees better results >>>> + * at the expense of the frame rate and CSI-2 bus bandwidth. >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>>> */ >>>> - cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize); >>>> + cio2Configuration_ = data_->cio2_.generateConfiguration({}); >>>> if (!cio2Configuration_.pixelFormat.isValid()) >>>> return Invalid;
Hi Jacopo, On Fri, Sep 18, 2020 at 12:48:56PM +0200, Jacopo Mondi wrote: > On Fri, Sep 18, 2020 at 09:24:51AM +0100, Kieran Bingham wrote: > > Hi Jacopo, > > > [snip] > > > >> - .boundedTo(data_->cio2_.sensor()->resolution()); > > >> - > > >> /* > > >> * Generate raw configuration from CIO2. > > >> * > > >> - * The output YUV streams will be limited in size to the maximum > > >> - * frame size requested for the RAW stream. > > >> + * \todo The image sensor frame size should be calculated as the > > >> + * smallest possible resolution larger enough to accommodate the > > >> + * requested stream sizes. However such a selection makes the pipeline > > > > > > Smallest possible size may not always be the best, we haven't really > > > thought about this. I would write "The image sensor output size should > > > be selected to optimize operation based on the sizes of the requested > > > streams.". > > > > Indeed, we really need to think about how we present this as an option > > to the users/applications as well. > > > > This is /use case/ dependant. > > Is it something applications should be in control of ? Possibly, but not at this point. We'll have to gather use cases first. Note that applications are already indirectly in control, but only partly. A pipeline handler will likely bin on the sensor side for high speed video, and get the full frame out of the sensor for still capture. So to some extent applications can already influence the decision, but we don't offer any guarantee. > I'm not sure we even can always provide to application a way to > select the sensor size to use in a generic way. We should indeed > report it, mostly for digital zoom implementation purposes, but the > selection of the sensor size in some platform (ie RPi) is not even > available to userspace. For IPU3 you see how fragile is that, I guess > all platforms have specificities there. Furthermore, and here I might > be very wrong, I don't see a real reason to do so. See below, mostly > for sake of discussion on the application space our API should aim to > cover. > > > A mobile platform wants to reduce power for instance, and might want the > > lowest reasonable sensor size to capture the required results, but other > > use-cases (perhaps a digital microscope) would want to use as high a > > resolution as possible from the sensor to capture as much light > > information as is possible, and deal with any size constraints at the > > rescaler. > > I'm not sure I see a correlation between having longer exposure times being > prevented by using a smaller frame size. > > The frame exposure time is bounded by the frame rate (the higher the > frame rate the shorter the max exposure time) and the frame rate is > bounded by the frame size (the larger the frame size the lower the > frame rate) so it seems to me the smaller the frame is, the more it > can be exposed without impacting the frame rate. I might be wrong on > this, but to me this is not even the point. > > The the manual control of exposure time and frame duration are already > enough correlated by how those things are controlled by the (usually > mode-based and feature limited) sensor driver and having any > guarantee they work in a generic enough way is hard enuogh. The frame size > which is input to the ISP processing pipeline is yet another parameter > that makes even harder to gaurantee what works on one platform makes > sense on another. I'm not saying we should not allow so, but basically > that's what an IPA does, and only very specific applications might > benefit from that and probably a 'generic camera API' is not what they > want to use ? > > That said, we are also considering the need to have a way to control > with platform specific configuration files at which point of the > processing pipeline any sub-sampling or scaling happens, if on the > sensor or on the ISP, and throwing applications controllable > parameters in the mix makes things quite hard to handle. > > I'm not saying I'm totally against providing that control to > applications, I would love if we could get to that level of detail, > but to me that seems to be quite low on the list.. > > > And of course it can also depend on how the resolution selection affects > > the field of view ... > > > > >> + * configuration procedure fail for small resolution (in example: > > > > s/resolution/resolutions/ > > > > > s/in example/for example/ > > > > > >> + * 640x480 with OV5670) and causes the capture operations to stall for > > >> + * some streams size combinations (see the commit message of the patch > > > > > > s/streams/stream/ > > > > > >> + * that introduced this comment for more failure examples). > > >> + * > > >> + * Until the sensor frame size calculation criteria are not clarified, > > > > > > s/are not clarified/are clarified/ > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > >> + * always use the largest possible one which guarantees better results > > >> + * at the expense of the frame rate and CSI-2 bus bandwidth. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > >> */ > > >> - cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize); > > >> + cio2Configuration_ = data_->cio2_.generateConfiguration({}); > > >> if (!cio2Configuration_.pixelFormat.isValid()) > > >> return Invalid; > > >>
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 2d881fe28f98..2ba00a2ca211 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -144,25 +144,16 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() status = Adjusted; } - /* - * Validate the requested stream configuration and select the sensor - * format by collecting the maximum RAW stream width and height and - * picking the closest larger match, as the IPU3 can downscale only. If - * no resolution is requested for the RAW stream, use the one from the - * largest YUV stream, plus margins pixels for the IF and BDS to scale. - * If no resolution is requested for any stream, pick the largest one. - */ + /* Validate the requested stream configuration */ unsigned int rawCount = 0; unsigned int yuvCount = 0; Size maxYuvSize; - Size maxRawSize; for (const StreamConfiguration &cfg : config_) { const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat); if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { rawCount++; - maxRawSize.expandTo(cfg.size); } else { yuvCount++; maxYuvSize.expandTo(cfg.size); @@ -174,18 +165,22 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() return Invalid; } - if (maxRawSize.isNull()) - maxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN, - IMGU_OUTPUT_HEIGHT_MARGIN) - .boundedTo(data_->cio2_.sensor()->resolution()); - /* * Generate raw configuration from CIO2. * - * The output YUV streams will be limited in size to the maximum - * frame size requested for the RAW stream. + * \todo The image sensor frame size should be calculated as the + * smallest possible resolution larger enough to accommodate the + * requested stream sizes. However such a selection makes the pipeline + * configuration procedure fail for small resolution (in example: + * 640x480 with OV5670) and causes the capture operations to stall for + * some streams size combinations (see the commit message of the patch + * that introduced this comment for more failure examples). + * + * Until the sensor frame size calculation criteria are not clarified, + * always use the largest possible one which guarantees better results + * at the expense of the frame rate and CSI-2 bus bandwidth. */ - cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize); + cio2Configuration_ = data_->cio2_.generateConfiguration({}); if (!cio2Configuration_.pixelFormat.isValid()) return Invalid;