[PATCH] libcamera: matrix: Fix compilation error in inverse() function

[Barnabás Pőcze]

Hi

2025. 05. 22. 15:02 keltezéssel, Laurent Pinchart írta:
> On Thu, May 22, 2025 at 02:53:57PM +0200, Barnabás Pőcze wrote:
>> 2025. 05. 22. 14:42 keltezéssel, Laurent Pinchart írta:
>>> Some gcc versions report uninitialized variable usage:
>>>
>>> In member function ‘constexpr T& libcamera::Span<T, 4294967295>::operator[](size_type) const [with T = unsigned int]’,
>>>       inlined from ‘void libcamera::matrixInvert(Span<const T>, Span<T, 4294967295>, unsigned int, Span<T, 4294967295>, Span<unsigned int>)::MatrixAccessor::swap(unsigned int, unsigned int) [with T = float]’ at ../../src/libcamera/matrix.cpp:194:13,
>>>       inlined from ‘bool libcamera::matrixInvert(Span<const T>, Span<T, 4294967295>, unsigned int, Span<T, 4294967295>, Span<unsigned int>) [with T = float]’ at ../../src/libcamera/matrix.cpp:255:14:
>>> ../../include/libcamera/base/span.h:362:76: error: ‘row’ may be used uninitialized [-Werror=maybe-uninitialized]
>>>     362 |         constexpr reference operator[](size_type idx) const { return data()[idx]; }
>>>         |                                                                      ~~~~~~^
>>> ../../src/libcamera/matrix.cpp: In function ‘bool libcamera::matrixInvert(Span<const T>, Span<T, 4294967295>, unsigned int, Span<T, 4294967295>, Span<unsigned int>) [with T = float]’:
>>> ../../src/libcamera/matrix.cpp:232:30: note: ‘row’ was declared here
>>>     232 |                 unsigned int row;
>>>         |                              ^~~
>>>
>>> This is a false positive. Fix it by initializing the variable when
>>> declaring it.
>>>
>>> Fixes: 6287ceff5aba ("libcamera: matrix: Add inverse() function")
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>>    src/libcamera/matrix.cpp | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/libcamera/matrix.cpp b/src/libcamera/matrix.cpp
>>> index ed22263b58f8..b7c07e896538 100644
>>> --- a/src/libcamera/matrix.cpp
>>> +++ b/src/libcamera/matrix.cpp
>>> @@ -229,7 +229,7 @@ bool matrixInvert(Span<const T> dataIn, Span<T> dataOut, unsigned int dim,
>>>    		 * Locate the next pivot. To improve numerical stability, use
>>>    		 * the row with the largest value in the pivot's column.
>>>    		 */
>>> -		unsigned int row;
>>> +		unsigned int row = pivot;
>>
>> I am wondering if it could be set to `dim`, -1, etc. initially, and the below `if`
>> could be modified to check `row >= dim`, etc. That would avoid the value equality
>> check, which I think is a good thing.
> 
> I've considered something similar. When trying to see if it was really a
> false positive, I wondered what would happen when all pivot elements are
> 0, and then realized it means that the matrix isn't invertible. I've
> decided to keep the value check, as I think it carries that meaning
> better than the row check.
> 
> Is there another reason you think a row check would be better ?

Nothing concrete, but I find that floating points always need extra consideration,
especially regarding equality. I think it should work with 0, as it is currently,
but still, it's better not to have to even entertain the possibility of signalling
nans, subnormals, etc.

Disregarding the above, this patch fixes the compilation error for me as well.

Tested-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>


Regards,
Barnabás Pőcze


> 
>>>    		T maxValue{ 0 };
>>>    
>>>    		for (unsigned int i = pivot; i < dim; ++i) {
>>>
>>> base-commit: efdbe3969841e342c30dfdced38b6ad9ad55dccf
>