[PATCH] libcamera: rkisp1: Clamp stream configuration to ISP limit on raw path
Umang Jain
umang.jain at ideasonboard.com
Tue Oct 8 20:40:37 CEST 2024
Hi Jacopo,
On 08/10/24 1:18 pm, Jacopo Mondi wrote:
> Umang,
> seems like I'm failing to make my point across, sorry about this
>
> On Fri, Oct 04, 2024 at 04:45:01PM GMT, Umang Jain wrote:
>> Hi Jacopo,
>>
>> On 04/10/24 3:58 pm, Jacopo Mondi wrote:
>>> Hi Umang
>>>
>>> On Fri, Oct 04, 2024 at 03:17:34PM GMT, Umang Jain wrote:
>>>> Hi Jacopo
>>>>
>>>> On 04/10/24 3:07 pm, Jacopo Mondi wrote:
>>>>> Hi Umang
>>>>>
>>>>> On Fri, Oct 04, 2024 at 10:23:38AM GMT, Umang Jain wrote:
>>>>>> Commit 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not
>>>>>> supported by the pipeline") introduced a mechanism to determine maximum
>>>>>> supported sensor resolution and filter out resolutions that cannot be
>>>>>> supported by the ISP.
>>>>>>
>>>>>> However, it missed to update the raw stream configuration path, where
>>>>>> it should have clamped the raw stream configuration size to the maximum
>>>>>> sensor supported resolution.
>>>>>>
>>>>>> This patch fixes the above issue and can be confirmed with IMX283
>>>>>> on i.MX8MP:
>>>>>>
>>>>>> From:
>>>>>> ($) cam -c1 -srole=raw,width=5472,height=3072
>>>>>> INFO Camera camera.cpp:1197 configuring streams: (0) 5472x3648-SRGGB12
>>>>>> ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12
>>>>>> Failed to configure camera
>>>>>> Failed to start camera session
>>>>>>
>>>>>> To:
>>>>>> ($) cam -c1 -srole=raw,width=5472,height=3072
>>>>>> INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12
>>>>>> cam0: Capture until user interrupts by SIGINT
>>>>>> 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824
>>>>>> 536.182378 (10.00 fps) cam0-stream0 seq: 000001 bytesused: 25165824
>>>>>> 536.282375 (10.00 fps) cam0-stream0 seq: 000002 bytesused: 25165824
>>>>>> ...
>>>>>>
>>>>>> Fixes: 761545407c76 ("pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline")
>>>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>>>>> ---
>>>>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 4 +++-
>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>>>>>> index da8d25c3..feb6d89f 100644
>>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
>>>>>> @@ -316,9 +316,11 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
>>>>>> if (isRaw) {
>>>>>> /*
>>>>>> * Use the sensor output size closest to the requested stream
>>>>>> - * size.
>>>>>> + * size while ensuring the output size doesn't exceed ISP limits.
>>>>>> */
>>>>>> uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
>>>>>> + cfg->size.boundTo(resolution);
>>>>>> +
>>>>>> V4L2SubdeviceFormat sensorFormat =
>>>>>> sensor->getFormat({ mbusCode }, cfg->size);
>>>>> CameraSensor::getFormat() returns a sensor resolution large enough to
>>>>> accomodate the requested size, at least that's how I read the
>>>>> following part of the documentation
>>>>>
>>>>> * \a size indicates the desired size at the output of the sensor. This function
>>>>> * selects the best media bus code and size supported by the sensor according
>>>>> * to the following criteria.
>>>>> *
>>>>> * - The desired \a size shall fit in the sensor output size to avoid the need
>>>>> * to up-scale.
>>>>>
>>>>> And this part of the code
>>>>>
>>>>> if (sz.width < size.width || sz.height < size.height)
>>>>> continue;
>>>>>
>>>>> So, at least in my understanding "sensorFormat" could represent a size
>>>>> larger than cfg->size. Is this your understanding as well ?
>>>> "sensorFormat" could represent a size larger than cfg->size
>>>>
>>>> and
>>>>
>>>> could also represent a size large than max supported resolution (i.e.
>>>> 'resolution' variable in this case)
>>>>
>>>> For e.g. in IMX283 case it would, 5472x3648
>>>>
>>>>> In the lines below this function we have
>>>>>
>>>>> minResolution = sensorFormat.size;
>>>>> maxResolution = sensorFormat.size;
>>>> From the above e.g. of IMX283 minResolution = maxResolution = 5472x3648
>>>>> cfg->size.boundTo(maxResolution);
>>>>> cfg->size.expandTo(minResolution);
>>>>>
>>>>> So if ((maxResolution = minResolution) > cfg->size) will the calls to
>>>>> boundTo() followed by expandTo() enlarge cfg->size ?
>>>> so cfg->size is 5472x3648 here, which can't be supported.
>>> Sorry, I'm confused. Aren't you confirming me that the resulting
>>> cfg->size cannot be supported (with this patch I mean) ?
>> So user passes a relatively high resolution, supported by the sensor (but
>> not with ISP)
>>
>> cam -c1 -srole=raw,width=5472,height=3648
>>
>> so cfg-size here is 5472x3648.
>>
>> So without this patch, you would right now get:
>>
>> ERROR RkISP1 rkisp1_path.cpp:425 Unable to configure capture in 5472x3648-SRGGB12
>> Failed to configure camera
>> Failed to start camera session
>>
>> With this patch patched, you would get:
>>
>> INFO Camera camera.cpp:1197 configuring streams: (0) 4096x3072-SRGGB12
>> cam0: Capture until user interrupts by SIGINT
>> 536.082380 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 25165824
>> ....
>>
>> So What has happened:
>>
>> User has requested a raw stream with 5472x3648. It was found that this can't
>> be supported hence,
>> the camera configuration is adjusted to a (lower) but maximum supported
>> resolution and delivers raw
>> frames with 4096x3072.
> I understand this fixes your use case but my concern, as explained in
> he previous email is that
>
> cfg->size.boundTo(resolution);
>
> V4L2SubdeviceFormat sensorFormat =
> sensor->getFormat({ mbusCode }, cfg->size);
>
> minResolution = sensorFormat.size;
> maxResolution = sensorFormat.size;
>
>
> cfg->size.boundTo(maxResolution);
> cfg->size.expandTo(minResolution);
>
> as CameraSensor::getFormat() can return a resolution -higher- than the
> requested one (cfg->size) there is a risk that cfg->size is later
> expanded to a larger value.
>
> Now, if I get this right "resolution" is returned by the newly introduced
> RkISP1Path::filterSensorResolution() and it's guaranteed to be the
> "higher sensor's resolution supported by the ISP".
yes, resolution is is the 'highest sensor's resolution supported by the
ISP' hence, we never allow cfg->size to go beyond that.
>
> if that's the case, then CameraSensor::getFormat() will never return
> a size larger than "cfg->size.boundedTo(resolution)" as "resolution"
> is known to be a size supported by the sensor itself.
Indeed, that's what's the intent of the patch.
>
> If my understanding is correct, then please add my tag
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
You're understanding is correct.
Do you think this understand needs to be documented in form of comments ?
>
>> One can argue that we should never adjust camera configuration to a lower
>> size than cfg->size (i.e. what the user asked).
>>
> I don't think that's a problem
>
>> But then,
>>
>> * \var CameraConfiguration::Adjusted
>> * The configuration has been adjusted to a valid configuration
>>
>> and
>>
>> * \retval CameraConfigutation::Adjusted The configuration has been adjusted
>> * and is now valid. Parameters may have changed for any stream, and stream
>> * configurations may have been removed. The caller shall check the
>> * configuration carefully.
>>
>>
>>>> Have you noticed the failure case I mentioned in the commit message?
>>> While the commmit message suggests this patch makes it valid ?
>>>
>>> What am I missing ? :)
>>>
>>>>> Wouldn't it be better to use 'resolution' in
>>>>>
>>>>> V4L2SubdeviceFormat sensorFormat =
>>>>> sensor->getFormat({ mbusCode }, cfg->size);
>>>>>
>>>>> instead of cfg->size ?
>>>> No, otherwise it will always pick the highest sensor output size.
>>>>
>>>> For e.g. I request -srole=raw,width=1920,height=1080
>>>>
>>>> "cfg->size" will pick 2736x1824 for IMX283
>>>>
>>>> "resolution" will always pick 4096x3072.
>>>>> Thanks
>>>>> j
>>>>>
>>>>>> --
>>>>>> 2.45.2
>>>>>>
More information about the libcamera-devel
mailing list