suspicious use of modulo operator

[lenoleno]

This is a code comment to https://github.com/ubports/qtubuntu-camera/blob/xenial/src/aalcameraserviceplugin.cpp#L131

    // Android's orientation means differently compared to QT's orientation.
    // On Android, it means "the angle that the camera image needs to be
    // rotated", but on QT, it means "the physical orientation of the camera
    // sensor". So, the value will have to be inverted.

    return (360 - orientation) % 360;

The TL;DR is that some dark corners of the modulo operator may not have been taken into account by the transformation applied. If I'm totally wrong, sorry for the noise!

The transformation applied to variable orientation seems to serve two purposes:

1. A logic inversion due to the fact that orientation in Android and Qt API has different semantics.
2. A translation between valid range of Android API and Qt API.

Let me make clear that I have no knowledge of either API and I have no broader view about the purpose of this code. But nevertheless the transformation applied looks suspicious. Here is why:

Here's a selection of hypothetical valid orientation angle ranges:

• 0 to 360 (guaranteed positive, guaranteed spread of 360 degrees),
• 0 to +inf (guaranteed positive)
• -180 to +180 (guaranteed spread)
• -inf to +inf (no limits)

The easiest case may be that both, Android API as well as Qt API use the range [0, 360). Then the only reason for the transformation applied is the different semantics. But then, the transformation 360 - orientation would be enough to translate orientation from Android to Qt API. The additional application of the modulo operator (360 - orientation) % 360 indicates that things may be more involved. This requires someone with insight into Android and Qt API to clarify. I cannot comment on this any further. Except, that I think a code comment documenting orientation source and target range would be useful.

Second, let's see if we got the maths right. For the remainder, I assume that target range is [0, 360) hinted to by the trailing modulo operator and that source range is not [0, 360) as the modulo operator would be redundant in that case. Additionally, I assume that source range doesn't contain negative values either, due to complete lack of any mentioning of this in code comments. So I assume source range is [0, +inf).

For input orientation values 0, 1, 2, …, 359, the transformation maps to 0, 359, 358, …, 1. And input orientation 360 maps to 0 again. OK. But what is the result of 361 fed to the transformation (360 - orientation) % 360? 360 - 361 equals -1. -1 % 360 equals … no, not 359. It's -1. While we wanted to clip target orientation angles to the range [0, 360), we get negative results. BAAMM! This is one of the dark corners similar to floor operator behaviour for negative arguments that I always have to look up … and then check with the given compiler. (I tested this with clickable on Ubuntu 18.04 x64. I did not compile the camera app, since I'm a beginner with anything clickable.)

The solution is simple, after the transformation one needs to check for negative result and add another 360 degrees in that case. As I said, I don't know what the correct transformation should look like due to complete ignorance of Android and Qt API. But, in light of some current camera rotation issues, maybe this reasoning helps solving any one of them. Or does this matter at all?

Best!

[peat-psuwit]

Well, % 360 is used to catch the case when orientation is zero. In that case, if modulo isn't used, we'll return 360 - 0 = 360, which doesn't make sense.

As far as I know, both ranges of Android's and Qt's value are {0, 90, 180, 270}. That is, only these 4 values are valid. The orientation is intended to tell how the camera is mounted inside the device, and I think no manufacturer is crazy enough to put the sensor diagonally.

So, I think it's OK to leave it as is. However, if someone else finds any information that suggests otherwise, feel free to raise that.

[lenoleno]

Sounds reasonable. I haven't been aware that here angle can only be a multiple of 90 degrees.

One other thing: As long as orientation is known to be from the set {0, 90, 180, 270} catching the zero orientation case by a conditional should be faster than employing the modulo operator. (Last time I did ARM assembler was two decades ago [on ARM3], but I hope things didn't change too much in this respect.)

Since ARM can do conditional execution (no need to branch and empty a pipeline), a compiler should translate

if (orientation != 0) {
    orientation = 360 - orientation
}

into something along the lines

CMP R0, #0
RSBNE R0, R0, #360

which consumes two cycles. (Assuming #360 is a valid immediate constant in ARM assembler, which I think it is. Valid constants are/were of the form x << n where x is an 8 bit value and n is an even shift. 360 = 90 << 2.) And perhaps even the zero test could be optimized away by a compiler, leaving us with just one cycle for the whole transformation. Doing an integer division (modulo) should be much slower. (Well, ARM3 didn't even have an integer division unit back then, which is why one got used to employ all possible tricks to avoid them. :-)

Since this is a camera app, we should aim for snappiness, wherever possible, no? Otherwise "all those moments" are at risk to be "lost in time like tears in rain." (RIP, Rutger Hauer!)

[peat-psuwit]

Hello,

Thank you for contributing to UBports. As part of project renaming and the effort to port Ubuntu Touch stack to Ubuntu 20.04, we're incrementally migrating repositories to GitLab.

Your issue is now migrated to:
https://gitlab.com/ubports/core/qtubuntu-camera/-/issues/7

Sorry for your inconvenience.