[libcamera-devel] [PATCH v4] pipeline-rkisp1-Fix-sensor-ISP-format-mismatch
Dafna Hirschfeld
dafna.hirschfeld at collabora.com
Tue Feb 16 17:27:16 CET 2021
On 16.02.21 06:00, Sebastian Fricke wrote:
> Hey Dafna,
>
> Thank you for your review.
>
> On 15.02.2021 21:11, Dafna Hirschfeld wrote:
>> Hi
>>
>> On 15.02.21 14:00, Helen Koike wrote:
>>> Hi Sebastian,
>>>
>>>
>>> On 2/7/21 9:58 AM, Sebastian Fricke wrote:
>>>> This patch fixes a mismatch of image formats during the pipeline
>>>> creation of the RkISP1. The mismatch happens because the current code
>>>> does not check if the configured format exceeds the maximum viable
>>>> resolution of the ISP.
>>>>
>>>> Make sure to use a sensor format resolution that is smaller or equal to
>>>> the maximum allowed resolution for the RkISP1. The maximum resolution
>>>> is defined within the `rkisp1-common.h` file as:
>>>> define RKISP1_ISP_MAX_WIDTH 4032
>>>> define RKISP1_ISP_MAX_HEIGHT 3024
>>>>
>>>> Enumerate the frame-sizes of the ISP entity and compare the maximum with
>>>> the configured resolution. Additionally, make sure that the media bus code
>>>> of the format is valid on the ISP.
>>>>
>>>> This means that some camera sensors can never operate with their maximum
>>>> resolution, for example on my OV13850 camera sensor, there are two
>>>> possible resolutions: 4224x3136 & 2112x1568, the first of those two will
>>>> never be picked as it surpasses the maximum of the ISP.
>>>>
>>>> Signed-off-by: Sebastian Fricke <sebastian.fricke at posteo.net>
>>>
>>> This patch doesn't compile, I believe things changed since you submitted.
>>> Could you submit a fixed version please?
>>>
>>> Thanks
>>> Helen
>>>
>>>> ---
>>>>
>>>> v1: https://lists.libcamera.org/pipermail/libcamera-devel/2020-November/015113.html
>>>> v2: https://lists.libcamera.org/pipermail/libcamera-devel/2020-November/015136.html
>>>> v3: https://patchwork.libcamera.org/patch/10660/
>>>>
>>>> Changelog:
>>>>
>>>> Changes since v3:
>>>>
>>>> * Introduce a helper method for finding the correct sensor format:
>>>> `findBestFitFormat`
>>>> * Besides checking for a valid format resolution also check for a
>>>> matching media bus code
>>>> * Return -EINVAL on an error instead of -1
>>>> * Add documentation for the `findBestFitFormat` method
>>>>
>>>> Changes since v2:
>>>>
>>>> * Replace the act of attempting to set the ISP format before negotiating
>>>> the actual format for both the ISP input pad and the sensor in order
>>>> to get the maximum frame size. With a logic that involves enumerating
>>>> the maximum size directly from the subdevice and using that size for
>>>> the negotiation process.
>>>> * Improve the log messages
>>>>
>>>> Changes since v1:
>>>>
>>>> * Change snake_case variable names to camelCase
>>>> * Use the request comment style
>>>> * Correct the scope of the newly implemented variables
>>>> * Correct the subject of the debug log for the ISP format configuration
>>>> * Update the comment above the ISP format configuration
>>>> * Check if the original format is not equal to the configured ISP format
>>>> instead of checking if it is greater, this denies a false positive
>>>> where the height exceeds the maximum while the width is smaller.
>>>> If the configured format does not exceed the maximum resolution of the
>>>> ISP, it will stay untouched so the inequality always means that we
>>>> have to reconfigure the format.
>>>> * Adjust the comparison of the ISP format size with the available sensor
>>>> formats, to detect a false-positive were the width is smaller while
>>>> the height exceeds the maximum
>>>> * Use the standard function `max`
>>>>
>>>> -----
>>>>
>>>> The following tests were all able to create a working camera pipeline:
>>>> 1. Without stream configuration
>>>> 2. With a normal stream configuration
>>>> 3. With a stream configuration that exceeds the maximum of the ISP
>>>> 4. With a very small resolution stream configuration
>>>> 5. With a configuration that is closer to the upper than to the lower
>>>> resolution
>>>>
>>>> -----
>>>>
>>>> 1. LIBCAMERA_LOG_LEVELS=0 cam -c 1 --capture=3
>>>>
>>>> [6:41:00.383092648] [28903] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
>>>> [6:41:00.383284855] [28903] INFO Camera camera.cpp:890 configuring streams: (0) 1920x1920-NV12
>>>> ...
>>>> [6:41:00.384053394] [28904] INFO RkISP1 rkisp1.cpp:573 Sensor format 4224x3136 is not supported by the ISP (maximum: 4032x3024), trying to re-configure to a smaller sensor format
>>>> [6:41:00.384124851] [28904] DEBUG RkISP1 rkisp1.cpp:637 Sensor configured with 2112x1568-SBGGR10_1X10
>>>> [6:41:00.384184351] [28904] DEBUG RkISP1 rkisp1.cpp:648 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
>>>> [6:41:00.384214684] [28904] DEBUG RkISP1 rkisp1.cpp:654 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:41:00.384252309] [28904] DEBUG RkISP1 rkisp1.cpp:666 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:41:00.384295184] [28904] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:41:00.384324059] [28904] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 1920x1920-YUYV8_2X8
>>>> [6:41:00.384351475] [28904] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 1920x1920-YUYV8_1_5X8
>>>>
>>>> 2. LIBCAMERA_LOG_LEVELS=0 cam -c 1 --capture=3 -s width=900,height=600,pixelformat=NV12,role=video
>>>>
>>>> [6:51:11.037976397] [28922] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
>>>> [6:51:11.038439562] [28922] INFO Camera camera.cpp:890 configuring streams: (0) 900x600-NV12
>>>> ...
>>>> [6:51:11.040661843] [28923] DEBUG RkISP1 rkisp1.cpp:609 Sensor configured with 2112x1568-SBGGR10_1X10
>>>> [6:51:11.040884092] [28923] DEBUG RkISP1 rkisp1.cpp:620 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
>>>> [6:51:11.041015633] [28923] DEBUG RkISP1 rkisp1.cpp:626 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:51:11.041182757] [28923] DEBUG RkISP1 rkisp1.cpp:638 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:51:11.041374089] [28923] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:51:11.041498339] [28923] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 900x600-YUYV8_2X8
>>>> [6:51:11.041619380] [28923] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 900x600-YUYV8_1_5X8
>>>>
>>>> 3. LIBCAMERA_LOG_LEVELS=0 cam -c 1 --capture=3 -s width=4500,height=3500,pixelformat=NV12,role=video
>>>>
>>>> [6:52:41.156286265] [28926] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
>>>> [6:52:41.156612639] [28926] DEBUG RkISP1 rkisp1_path.cpp:94 Adjusting format from 4500x3500-NV12 to 4416x3312-NV12
>>>> [6:52:41.156745055] [28926] DEBUG RkISP1 rkisp1_path.cpp:94 Adjusting format from 4500x3500-NV12 to 1920x1920-NV12
>>>> [6:52:41.156849471] [28926] DEBUG RkISP1 rkisp1_path.cpp:94 Adjusting format from 4500x3500-NV12 to 4416x3312-NV12
>>>> [6:52:41.156959137] [28926] DEBUG CameraSensor camera_sensor.cpp:560 'ov13850 1-0010': No supported format or size found
>>>> Camera configuration adjusted
>>>> [6:52:41.157176136] [28926] DEBUG CameraSensor camera_sensor.cpp:560 'ov13850 1-0010': No supported format or size found
>>>> [6:52:41.157245844] [28926] INFO Camera camera.cpp:890 configuring streams: (0) 4416x3312-NV12
>>>> ...
>>>> [6:52:41.159387000] [28927] INFO RkISP1 rkisp1.cpp:585 Sensor format 4224x3136 is not supported by the ISP (maximum: 4032x3024), trying to re-configure to a smaller sensor format
>>>> [6:52:41.159552958] [28927] DEBUG RkISP1 rkisp1.cpp:609 Sensor configured with 2112x1568-SBGGR10_1X10
>>>> [6:52:41.159752749] [28927] DEBUG RkISP1 rkisp1.cpp:620 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
>>>> [6:52:41.159883123] [28927] DEBUG RkISP1 rkisp1.cpp:626 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:52:41.160129289] [28927] DEBUG RkISP1 rkisp1.cpp:638 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:52:41.160322079] [28927] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:52:41.160451870] [28927] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 4416x3312-YUYV8_2X8
>>>> [6:52:41.160574370] [28927] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 4416x3312-YUYV8_1_5X8
>>>>
>>>> 4. LIBCAMERA_LOG_LEVELS=0 cam -c 1 --capture=3 -s width=40,height=30,pixelformat=NV12,role=video
>>>>
>>>> [6:53:43.547487714] [28930] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
>>>> [6:53:43.547902462] [28930] INFO Camera camera.cpp:890 configuring streams: (0) 40x30-NV12
>>>> ...
>>>> [6:53:43.550449033] [28931] DEBUG RkISP1 rkisp1.cpp:609 Sensor configured with 2112x1568-SBGGR10_1X10
>>>> [6:53:43.550650282] [28931] DEBUG RkISP1 rkisp1.cpp:620 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
>>>> [6:53:43.550773657] [28931] DEBUG RkISP1 rkisp1.cpp:626 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:53:43.550932323] [28931] DEBUG RkISP1 rkisp1.cpp:638 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:53:43.551103822] [28931] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:53:43.551222530] [28931] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 40x30-YUYV8_2X8
>>>> [6:53:43.551334237] [28931] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 40x30-YUYV8_1_5X8
>>>>
>>>> 5. LIBCAMERA_LOG_LEVELS=0 cam -c 1 --capture=3 -s width=3450,height=2456,pixelformat=NV12,role=video
>>>>
>>>> [6:54:43.212517584] [28933] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
>>>> [6:54:43.212946040] [28933] INFO Camera camera.cpp:890 configuring streams: (0) 3450x2456-NV12
>>>> ...
>>>> [6:54:43.215050739] [28934] INFO RkISP1 rkisp1.cpp:585 Sensor format 4224x3136 is not supported by the ISP (maximum: 4032x3024), trying to re-configure to a smaller sensor format
>>>> [6:54:43.215213779] [28934] DEBUG RkISP1 rkisp1.cpp:609 Sensor configured with 2112x1568-SBGGR10_1X10
>>>> [6:54:43.215416487] [28934] DEBUG RkISP1 rkisp1.cpp:620 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
>>>> [6:54:43.215545986] [28934] DEBUG RkISP1 rkisp1.cpp:626 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:54:43.215710485] [28934] DEBUG RkISP1 rkisp1.cpp:638 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:54:43.215891026] [28934] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>>>> [6:54:43.216104233] [28934] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 3450x2456-YUYV8_2X8
>>>> [6:54:43.216246274] [28934] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 3450x2456-YUYV8_1_5X8
>>>>
>>>> ---
>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 67 ++++++++++++++++++++++++
>>>> 1 file changed, 67 insertions(+)
>>>>
>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> index e85979a7..55b1a10d 100644
>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> @@ -163,6 +163,8 @@ private:
>>>> void paramReady(FrameBuffer *buffer);
>>>> void statReady(FrameBuffer *buffer);
>>>> void frameStart(uint32_t sequence);
>>>> + int findBestFitFormat(V4L2SubdeviceFormat *format,
>>>> + CameraSensor *sensor);
>>>> int allocateBuffers(Camera *camera);
>>>> int freeBuffers(Camera *camera);
>>>> @@ -551,6 +553,67 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>>>> return config;
>>>> }
>>>> +/**
>>>> + * \brief Find the closest possible match to the desired configuration format,
>>>> + * that is valid on the RkISP1.
>>>> + * \param[out] format The configuration format for the image sensor
>>>> + * \param[in] sensor The camera sensor object
>>>> + * \return 0 on success, -1 on failure
>>>> + */
>>>> +int PipelineHandlerRkISP1::findBestFitFormat(
>>>> + V4L2SubdeviceFormat *format, CameraSensor *sensor)
>>>> +{
>>>> + V4L2Subdevice::Formats ispFormats = isp_->formats(0);
>>>> + if (ispFormats.empty()) {
>>>> + LOG(RkISP1, Error) << "Unable to fetch ISP formats.";
>>>> + return -1;
>>>> + }
>>>> + /*
>>>> + * The maximum resolution is identical for all media bus codes on
>>>> + * the RkISP1 isp entity. Therefore take the first available resolution.
>>>> + */
>>>> + Size ispMaximum = ispFormats.begin()->second[0].max;
>>>> +
>>>> + if (ispMaximum.width < format->size.width ||
>>>> + ispMaximum.height < format->size.height) {
>>>> + Size maxSize;
>>>> + LOG(RkISP1, Info) << "Sensor format " << format->size.toString()
>>>> + << " is not supported by the ISP (maximum: "
>>>> + << ispMaximum.toString() << "), trying to "
>>>> + << "re-configure to a smaller sensor format";
>>>> +
>>>> + for (const Size &size : sensor->sizes()) {
>>
>> The sizes are sorted in increasing order and you want to get the max size
>> so it make sense to iterate it in reverse and break once a valid size is found.
>
> That is a good idea, I will give this a shot.
>
>>
>>>> + if (size.width > ispMaximum.width ||
>>>> + size.height > ispMaximum.height)
>>>> + continue;
>>>> + maxSize = std::max(maxSize, size);
>>>> + }
>>
>> I think you can add this loop to the validate function just before the
>> sensor->getFormat call.
>> Then the 'maxSize' can be the maxSize sent to the sensor->getFormat
>
> I really tried to make this work, but the problem is I don't have access
> to the isp sub device within the scope of the validate method. Because
> validate is a method of RkISP1CameraConfiguration and configure is a
> method of PipelineHandlerRkISP1.
From what I understand, in the 'configure' stage we should already
know how to configure the sensor. So searching for a valid size should
be done in the 'validate'. I might be wrong though. Better ask
libcamera people. I think you can hard-code the max resolution
of pad 0 of the isp entity. The supported bus formats are already
hard coded in the call to sensor->getFormat probably for the same reason.
Maybe it means we should change the design somehow so we do have access
to the isp entity from 'validate'?
Thanks,
Dafna
>
>>
>> Thanks,
>> Dafna
>
> Greetings,
> Sebastian
>
>>
>>>> + if (maxSize == Size(0, 0)) {
>>>> + LOG(RkISP1, Error) << "No avail. sensor resolution <= "
>>>> + << format->toString();
>>>> + return -1;
>>>> + }
>>>> + format->size = maxSize;
>>>> + }
>>>> +
>>>> + auto mbusCodeMatch = ispFormats.find(format->mbus_code);
>>>> + if (mbusCodeMatch == ispFormats.end()) {
>>>> + format->mbus_code = 0;
>>>> + for (unsigned int mbusCode : sensor->mbusCodes()) {
>>>> + mbusCodeMatch = ispFormats.find(mbusCode);
>>>> + if (mbusCodeMatch != ispFormats.end()) {
>>>> + format->mbus_code = mbusCode;
>>>> + break;
>>>> + }
>>>> + }
>>>> + if (format->mbus_code == 0) {
>>>> + LOG(RkISP1, Error) << "No valid sensor mbus-code found";
>>>> + return -1;
>>>> + }
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>>>> {
>>>> RkISP1CameraConfiguration *config =
>>>> @@ -570,6 +633,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>>>> V4L2SubdeviceFormat format = config->sensorFormat();
>>>> LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString();
>>>> + ret = findBestFitFormat(&format, sensor);
>>>> + if (ret < 0)
>>>> + return -EINVAL;
>>>> +
>>>> ret = sensor->setFormat(&format);
>>>> if (ret < 0)
>>>> return ret;
>>>>
More information about the libcamera-devel
mailing list