[libcamera-devel] [PATCH v4] pipeline-rkisp1-Fix-sensor-ISP-format-mismatch
Sebastian Fricke
sebastian.fricke at posteo.net
Tue Feb 16 06:00:12 CET 2021
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.
>
>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