search

AprilRobotics / apriltag

AprilTag is a visual fiducial system popular for robotics research.
https://april.eecs.umich.edu/software/apriltag
Other
1.56k stars 535 forks source link

apriltag_quad_thresh: Prevent conversion from NaN to int #332

Closed peci1 closed 4 months ago

peci1 commented 4 months ago

If the user supplied quad_decimate value of 0.0, it works in most places, but in apriltag_quad_thresh, it led to undefined behavior after dividing by this value.

The symptoms are the triciker that on x86/GCC, the undefined behavior leads to -INT_MAX, which gets corrected to 3 pixels by the following lines of code. However, on arm64/GCC, it results in +INT_MAX, which does not get corrected and leads to a running detector which will never detect anything.

peci1 commented 4 months ago

I (now) know that setting quad_decimate to 0.0 is wrong (or at least doesn't make much sense), but the library doesn't check it and the symptoms were so weird I had to provide this fix.

peci1 commented 4 months ago

Okay, I've updated this PR to use the consistent approach. I don't think there are any more inconsistencies left in the codebase, but you should rather verify that.

So the current behavior is that any value that is 1.0 or lower practically disables the scaling.

peci1 commented 4 months ago

Squashed.

christian-rauch commented 4 months ago

Squashed.

The commit is still using the old commit message that does not make sense any more with the squashed diff. Can you update the commit message to something that fits the diff better?

peci1 commented 4 months ago

It seemed to me the commit message still applies. I kept everything that was there and added one more paragraph explaining that also other values lower than 1.0 are affected by this commit.

If you want something else, please, propose the commit message.

christian-rauch commented 4 months ago

It seemed to me the commit message still applies. I kept everything that was there and added one more paragraph explaining that also other values lower than 1.0 are affected by this commit.

The commit message is still talking about division-by-zero and the effect on different target architectures for GCC. But I think this is not what this commit is about. Yes, it prevents division-by-zero but mainly it aligns the behaviour for the quad_decimate value range across the code.

If you want something else, please, propose the commit message.

I would probably write something like "prevent using decimate for scale smaller 1" or "prevent upscaling of min tag width" as the summary and then just refer to how this aligns with the behaviour in the remaining usages of quad_decimate.

peci1 commented 4 months ago

Ok, what about now?

christian-rauch commented 4 months ago

Ok, what about now?

Thanks!