[libcamera-devel] [PATCH v2 0/1] pipeline-rkisp1-Fix-sensor-ISP-format-mismatch

Fri Nov 20 14:40:43 CET 2020

Hi Sebastian,

Thanks for your patch.

On 11/20/20 9:45 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 exceeded the maximum viable
> resolution of the ISP.
> This patch fixes the issue by first setting the ISP, checking if the
> given resolution was taken and searching for another viable format that
> is lower or equal to the ISP maximum.

This description could be in the commit message, so we can save in the
history the reasons for the change.

> 
> ---
> 
> Changelog:
> 
> 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
> 
> [8:05:49.668381732] [9973] DEBUG Camera camera.cpp:771 streams configuration: (0) 1920x1920-NV12
> [8:05:49.669108269] [9973]  INFO Camera camera.cpp:830 configuring streams: (0) 1920x1920-NV12
> ...
> [8:05:49.670965005] [9974] DEBUG RkISP1 rkisp1.cpp:687 Configuring sensor with 4224x3136-SBGGR10_1X10
> [8:05:49.671126004] [9974] DEBUG RkISP1 rkisp1.cpp:694 ISP configured with 4032x3024-SBGGR10_1X10
> [8:05:49.671233045] [9974]  INFO RkISP1 rkisp1.cpp:697 Configured resolution is greater than the maximum resolution for the ISP, trying to re-configure to a smaller valid sensor format
> [8:05:49.671378294] [9974] DEBUG RkISP1 rkisp1.cpp:716 ISP re-configured with 2112x1568-SBGGR10_1X10
> [8:05:49.671510710] [9974] DEBUG RkISP1 rkisp1.cpp:724 Sensor configured with 2112x1568-SBGGR10_1X10
> [8:05:49.671676667] [9974] DEBUG RkISP1 rkisp1.cpp:731 ISP input pad configured with 2112x1568-SBGGR10_1X10

There are two prints here for the same ISP pad, please see my comments on 1/1.

> [8:05:49.671787791] [9974] DEBUG RkISP1 rkisp1.cpp:735 Configuring ISP output pad with 2112x1568-YUYV8_2X8
> [8:05:49.671894540] [9974] DEBUG RkISP1 rkisp1.cpp:741 ISP output pad configured with 800x600-YUYV8_2X8
> [8:05:49.672107747] [9974] DEBUG RkISP1 rkisp1_path.cpp:120 Configured main resizer input pad with 800x600-YUYV8_2X8
> [8:05:49.672226163] [9974] DEBUG RkISP1 rkisp1_path.cpp:126 Configuring main resizer output pad with 1920x1920-YUYV8_2X8
> [8:05:49.672341662] [9974] DEBUG RkISP1 rkisp1_path.cpp:144 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
> 
> [8:07:05.725799205] [9977] DEBUG Camera camera.cpp:771 streams configuration: (0) 1920x1920-NV12
> [8:07:05.726242535] [9977]  INFO Camera camera.cpp:830 configuring streams: (0) 900x600-NV12
> ...
> [8:07:05.727718025] [9978] DEBUG RkISP1 rkisp1.cpp:687 Configuring sensor with 2112x1568-SBGGR10_1X10
> [8:07:05.727873482] [9978] DEBUG RkISP1 rkisp1.cpp:694 ISP configured with 2112x1568-SBGGR10_1X10
> [8:07:05.728056356] [9978] DEBUG RkISP1 rkisp1.cpp:724 Sensor configured with 2112x1568-SBGGR10_1X10
> [8:07:05.728233104] [9978] DEBUG RkISP1 rkisp1.cpp:731 ISP input pad configured with 2112x1568-SBGGR10_1X10
> [8:07:05.728340437] [9978] DEBUG RkISP1 rkisp1.cpp:735 Configuring ISP output pad with 2112x1568-YUYV8_2X8
> [8:07:05.728448353] [9978] DEBUG RkISP1 rkisp1.cpp:741 ISP output pad configured with 800x600-YUYV8_2X8
> [8:07:05.728567352] [9978] DEBUG RkISP1 rkisp1_path.cpp:120 Configured main resizer input pad with 800x600-YUYV8_2X8
> [8:07:05.728673809] [9978] DEBUG RkISP1 rkisp1_path.cpp:126 Configuring main resizer output pad with 900x600-YUYV8_2X8
> [8:07:05.728785517] [9978] DEBUG RkISP1 rkisp1_path.cpp:144 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
> 
> [8:08:02.372820979] [9981] DEBUG Camera camera.cpp:771 streams configuration: (0) 1920x1920-NV12
> [8:08:02.373280350] [9981] DEBUG RkISP1 rkisp1_path.cpp:100 Adjusting format from 4500x3500-NV12 to 4416x3312-NV12
> [8:08:02.373534098] [9981] DEBUG RkISP1 rkisp1_path.cpp:100 Adjusting format from 4500x3500-NV12 to 1920x1920-NV12
> [8:08:02.373767722] [9981] DEBUG RkISP1 rkisp1_path.cpp:100 Adjusting format from 4500x3500-NV12 to 4416x3312-NV12
> [8:08:02.374010095] [9981] DEBUG CameraSensor camera_sensor.cpp:408 'ov13850 1-0010': No supported format or size found
> Camera configuration adjusted
> [8:08:02.374449634] [9981] DEBUG CameraSensor camera_sensor.cpp:408 'ov13850 1-0010': No supported format or size found
> [8:08:02.374591091] [9981]  INFO Camera camera.cpp:830 configuring streams: (0) 4416x3312-NV12
> ...
> [8:08:02.376301704] [9982] DEBUG RkISP1 rkisp1.cpp:687 Configuring sensor with 4224x3136-0x0000
> [8:08:02.376469994] [9982] DEBUG RkISP1 rkisp1.cpp:694 ISP configured with 4032x3024-SRGGB10_1X10
> [8:08:02.376576160] [9982]  INFO RkISP1 rkisp1.cpp:697 Configured resolution is greater than the maximum resolution for the ISP, trying to re-configure to a smaller valid sensor format
> [8:08:02.376710909] [9982] DEBUG RkISP1 rkisp1.cpp:716 ISP re-configured with 2112x1568-SBGGR10_1X10
> [8:08:02.376839241] [9982] DEBUG RkISP1 rkisp1.cpp:724 Sensor configured with 2112x1568-SBGGR10_1X10
> [8:08:02.377001115] [9982] DEBUG RkISP1 rkisp1.cpp:731 ISP input pad configured with 2112x1568-SBGGR10_1X10
> [8:08:02.377104364] [9982] DEBUG RkISP1 rkisp1.cpp:735 Configuring ISP output pad with 2112x1568-YUYV8_2X8
> [8:08:02.377208780] [9982] DEBUG RkISP1 rkisp1.cpp:741 ISP output pad configured with 800x600-YUYV8_2X8
> [8:08:02.377324863] [9982] DEBUG RkISP1 rkisp1_path.cpp:120 Configured main resizer input pad with 800x600-YUYV8_2X8
> [8:08:02.377425779] [9982] DEBUG RkISP1 rkisp1_path.cpp:126 Configuring main resizer output pad with 4416x3312-YUYV8_2X8
> [8:08:02.377533695] [9982] DEBUG RkISP1 rkisp1_path.cpp:144 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
> 
> [8:10:41.140561164] [9989] DEBUG Camera camera.cpp:771 streams configuration: (0) 1920x1920-NV12
> [8:10:41.141348367] [9989]  INFO Camera camera.cpp:830 configuring streams: (0) 40x30-NV12
> ...
> [8:10:41.143016688] [9990] DEBUG RkISP1 rkisp1.cpp:687 Configuring sensor with 2112x1568-SBGGR10_1X10
> [8:10:41.143161062] [9990] DEBUG RkISP1 rkisp1.cpp:694 ISP configured with 2112x1568-SBGGR10_1X10
> [8:10:41.143283561] [9990] DEBUG RkISP1 rkisp1.cpp:724 Sensor configured with 2112x1568-SBGGR10_1X10
> [8:10:41.143442810] [9990] DEBUG RkISP1 rkisp1.cpp:731 ISP input pad configured with 2112x1568-SBGGR10_1X10
> [8:10:41.143544893] [9990] DEBUG RkISP1 rkisp1.cpp:735 Configuring ISP output pad with 2112x1568-YUYV8_2X8
> [8:10:41.143644934] [9990] DEBUG RkISP1 rkisp1.cpp:741 ISP output pad configured with 800x600-YUYV8_2X8
> [8:10:41.143756058] [9990] DEBUG RkISP1 rkisp1_path.cpp:120 Configured main resizer input pad with 800x600-YUYV8_2X8
> [8:10:41.143854640] [9990] DEBUG RkISP1 rkisp1_path.cpp:126 Configuring main resizer output pad with 40x30-YUYV8_2X8
> [8:10:41.143960223] [9990] DEBUG RkISP1 rkisp1_path.cpp:144 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
> 
> [8:11:40.321895811] [9992] DEBUG Camera camera.cpp:771 streams configuration: (0) 1920x1920-NV12
> [8:11:40.322697014] [9992]  INFO Camera camera.cpp:830 configuring streams: (0) 3450x2456-NV12
> ...
> [8:11:40.324431543] [9993] DEBUG RkISP1 rkisp1.cpp:687 Configuring sensor with 4224x3136-SBGGR10_1X10
> [8:11:40.324579125] [9993] DEBUG RkISP1 rkisp1.cpp:694 ISP configured with 4032x3024-SBGGR10_1X10
> [8:11:40.324680041] [9993]  INFO RkISP1 rkisp1.cpp:697 Configured resolution is greater than the maximum resolution for the ISP, trying to re-configure to a smaller valid sensor format
> [8:11:40.324815082] [9993] DEBUG RkISP1 rkisp1.cpp:716 ISP re-configured with 2112x1568-SBGGR10_1X10
> [8:11:40.324946039] [9993] DEBUG RkISP1 rkisp1.cpp:724 Sensor configured with 2112x1568-SBGGR10_1X10
> [8:11:40.325111121] [9993] DEBUG RkISP1 rkisp1.cpp:731 ISP input pad configured with 2112x1568-SBGGR10_1X10
> [8:11:40.325216121] [9993] DEBUG RkISP1 rkisp1.cpp:735 Configuring ISP output pad with 2112x1568-YUYV8_2X8
> [8:11:40.325323453] [9993] DEBUG RkISP1 rkisp1.cpp:741 ISP output pad configured with 800x600-YUYV8_2X8
> [8:11:40.325441286] [9993] DEBUG RkISP1 rkisp1_path.cpp:120 Configured main resizer input pad with 800x600-YUYV8_2X8
> [8:11:40.325547451] [9993] DEBUG RkISP1 rkisp1_path.cpp:126 Configuring main resizer output pad with 3450x2456-YUYV8_2X8
> [8:11:40.325660909] [9993] DEBUG RkISP1 rkisp1_path.cpp:144 Configured main resizer output pad with 3450x2456-YUYV8_1_5X8
> 

Just a tip, when sending a single patch, I usually don't
bother sending a cover letter, I would put all this information
after the 3 dashes "---" in the patch (what is written after those
3 dashes will be ignored when git am file.patch)

But maybe this is just my style, but I wanted to let you know
you have this option :)

Regards,
Helen

> -----
> 
> Sebastian Fricke (1):
>   pipeline: rkisp1: Fix sensor-ISP format mismatch
> 
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 46 ++++++++++++++++++++----
>  1 file changed, 40 insertions(+), 6 deletions(-)
> 