Theoretical assert when filtering resolutions on RkISP

[Jacopo Mondi]

Hi Quentin

On Fri, Apr 04, 2025 at 11:33:01AM +0200, Quentin Schulz wrote:
> Hi all,
>
> Looking at the code that triggered an assert when running an "old" kernel
> (5.10 for me but should happen until 6.4) on a Rockchip board (PX30
> based)[1] I think I've identified another way to trigger that assert (on
> recent kernels).
>
> I'm not sure it's reasonable to expect it to happen and I for sure do not
> know what to do, safely.
>
> So here goes nothing:
>
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp#n139
>
> """
> Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor)
> {
> 	auto iter = sensorSizesMap_.find(sensor);
> 	if (iter != sensorSizesMap_.end())
> 		return iter->second.back();
>
> 	std::vector<Size> &sizes = sensorSizesMap_[sensor];
> 	for (unsigned int code : sensor->mbusCodes()) {
> 		for (const Size &size : sensor->sizes(code)) {
> 			if (size.width > maxResolution_.width ||
> 			    size.height > maxResolution_.height)
> 				continue;
>
> 			sizes.push_back(size);
> 		}
> 	}
>
> 	/* Sort in increasing order and remove duplicates. */
> 	std::sort(sizes.begin(), sizes.end());
> 	auto last = std::unique(sizes.begin(), sizes.end());
> 	sizes.erase(last, sizes.end());
>
> 	return sizes.back();
> }
> """
>
> sensorSizesmap_ is an std::map<const CameraSensor *, std::vector<Size>>.
>
> On first call to this function for sensor X, the first part till the
> for-loop is "skipped" because the sensor isn't present in the map. Then with
>
> sensorSizesMap_[sensor];
>
> it creates a new entry in the map, c.f.
> https://en.cppreference.com/w/cpp/container/map/operator_at
>
> """
> Returns a reference to the value that is mapped to a key equivalent to key
> or x respectively, performing an insertion if such key does not already
> exist.
> """
>
> So now we have a new entry in the map but without anything in map->second
> (or rather an empty std::vector).
>
> Then we filter the sensor resolutions based on the min and max resolutions
> supported by the pipeline. However, it could (at least theoretically) be
> possible to have no possible subset of sensor resolution for the pipeline,
> in which case sizes vector is empty.

That's very unlikely, as it means someone has connected to the board a
sensor that won't work in combination with the isp

>
> I have no clue if that would trigger an issue in the three lines after the
> end of the for loop but it'll for sure trigger an assert in the return
> statement as sizes.back() will assert on !this->empty() that I got in [2].

Albeit theoretical, I think it's worth avoiding a potential assertion
in STL. You can return earlier in RkISP1Path::filterSensorResolution()
if sizes is empty, and check in the caller of filterSensorResolution
if the returned size isNUll() and bail out.

>
> [1]
> https://lists.libcamera.org/pipermail/libcamera-devel/2025-April/049691.html
> [2]
> https://lists.libcamera.org/pipermail/libcamera-devel/2025-April/049699.html
>
> An option to not trigger the assert (here) would be to remove the sensor
> from the map if no compatible resolution is found, and then return.... what
> exactly? It seems callers of filterSensorResolution() always expect a
> resolution to be found. Maybe 0 for maxResolution and (unsigned int)-1 for
> minResolution? Not sure if the rest of the code is able to handle that

You can return an empty size {} and check in the caller for isNull()

> though. We could also simply insert the same dummy max and min resolutions
> for the sensor instead of removing the sensor from the map (same outcome,
> but we then avoid running through the forloop every time someone asks to
> filter the resolutions for that sensor.
>
> Cheers,
> Quentin