[libcamera-devel] [PATCH] libcamera: ipu3: Always use sensor full frame size

[Kieran Bingham]

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 at ragnatech.se>
>>>> Signed-off-by: Jacopo Mondi <jacopo at 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 at 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 at ideasonboard.com>
>>
>>>>  	 */
>>>> -	cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize);
>>>> +	cio2Configuration_ = data_->cio2_.generateConfiguration({});
>>>>  	if (!cio2Configuration_.pixelFormat.isValid())
>>>>  		return Invalid;

-- 
Regards
--
Kieran