Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

imgproc: fix fixedpoint #11395

Merged
merged 2 commits into from Apr 26, 2018
Merged

imgproc: fix fixedpoint #11395

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
469dc6a
imgproc: fix fixed point build
alalek Apr 25, 2018
9166e60
imgproc: fixed fixedpoint coding style
alalek Apr 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
75 changes: 53 additions & 22 deletions modules/imgproc/src/fixedpoint.inl.hpp
View file
Open in desktop
Expand Up @@ -55,9 +55,11 @@ class fixedpoint64
CV_ALWAYS_INLINE fixedpoint64 operator * (const int32_t& val2) const { return operator *(fixedpoint64(val2)); }
CV_ALWAYS_INLINE fixedpoint64 operator * (const fixedpoint64& val2) const
{
uint64_t uval = (uint64_t)((val ^ (val >> 63)) - (val >> 63));
uint64_t umul = (uint64_t)((val2.val ^ (val2.val >> 63)) - (val2.val >> 63));
int64_t ressign = (val >> 63) ^ (val2.val >> 63);
bool sign_val = val < 0;
bool sign_mul = val2.val < 0;
uint64_t uval = sign_val ? (uint64_t)(-val) : (uint64_t)val;
uint64_t umul = sign_mul ? (uint64_t)(-val2.val) : (uint64_t)val2.val;
bool ressign = sign_val ^ sign_mul;

uint64_t sh0 = fixedround((uval & 0xFFFFFFFF) * (umul & 0xFFFFFFFF));
uint64_t sh1_0 = (uval >> 32) * (umul & 0xFFFFFFFF);
Expand All @@ -67,33 +69,37 @@ class fixedpoint64
uint64_t val0_h = (sh2 & 0xFFFFFFFF) + (sh1_0 >> 32) + (sh1_1 >> 32) + (val0_l >> 32);
val0_l &= 0xFFFFFFFF;

if ( (sh2 >> 32) || (val0_h >> ressign ? 32 : 31) )
return (ressign ? ~(int64_t)0x7FFFFFFFFFFFFFFF : (int64_t)0x7FFFFFFFFFFFFFFF);
if (sh2 > CV_BIG_INT(0x7FFFFFFF) || val0_h > CV_BIG_INT(0x7FFFFFFF))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be if (sh2 > CV_BIG_UINT(0xFFFFFFFF) || val0_h > (ressign ? CV_BIG_UINT(0x80000000) : CV_BIG_INT(0x7FFFFFFF)))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final result should be the same: saturated underflow - 0x8000'0000'0000'0000 .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. Get the idea. The check is fine

return (int64_t)(ressign ? CV_BIG_UINT(0x8000000000000000) : CV_BIG_INT(0x7FFFFFFFFFFFFFFF));

if (ressign)
{
val0_l = (~val0_l + 1) & 0xFFFFFFFF;
val0_h = val0_l ? ~val0_h : (~val0_h + 1);
return -(int64_t)(val0_h << 32 | val0_l);
}
return (int64_t)(val0_h << 32 | val0_l);
}
CV_ALWAYS_INLINE fixedpoint64 operator + (const fixedpoint64& val2) const
{
int64_t res = val + val2.val;
return ((val ^ res) & (val2.val ^ res)) >> 63 ? ~(res & ~0x7FFFFFFFFFFFFFFF) : res;
return (int64_t)(((val ^ res) & (val2.val ^ res)) < 0 ? ~(res & CV_BIG_UINT(0x8000000000000000)) : res);
}
CV_ALWAYS_INLINE fixedpoint64 operator - (const fixedpoint64& val2) const
{
int64_t res = val - val2.val;
return ((val ^ val2.val) & (val ^ res)) >> 63 ? ~(res & ~0x7FFFFFFFFFFFFFFF) : res;
return (int64_t)(((val ^ val2.val) & (val ^ res)) < 0 ? ~(res & CV_BIG_UINT(0x8000000000000000)) : res);
}
CV_ALWAYS_INLINE fixedpoint64 operator >> (int n) const { return fixedpoint64(val >> n); }
CV_ALWAYS_INLINE fixedpoint64 operator << (int n) const { return fixedpoint64(val << n); }
CV_ALWAYS_INLINE bool operator == (const fixedpoint64& val2) const { return val == val2.val; }
template <typename ET>
CV_ALWAYS_INLINE operator ET() const { return cv::saturate_cast<ET>((int64_t)fixedround((uint64_t)val) >> fixedShift); }
CV_ALWAYS_INLINE ET saturate_cast() const { return cv::saturate_cast<ET>((int64_t)fixedround((uint64_t)val) >> fixedShift); }
CV_ALWAYS_INLINE operator double() const { return (double)val / (1LL << fixedShift); }
CV_ALWAYS_INLINE operator float() const { return (float)val / (1LL << fixedShift); }
CV_ALWAYS_INLINE operator uint8_t() const { return saturate_cast<uint8_t>(); }
CV_ALWAYS_INLINE operator int8_t() const { return saturate_cast<int8_t>(); }
CV_ALWAYS_INLINE operator uint16_t() const { return saturate_cast<uint16_t>(); }
CV_ALWAYS_INLINE operator int16_t() const { return saturate_cast<int16_t>(); }
CV_ALWAYS_INLINE operator int32_t() const { return saturate_cast<int32_t>(); }
Copy link
Contributor

@Turim Turim Apr 26, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it seems somewhat insymmetrical: int8_t/uint8_t, int16_t/uint16_t, int32_t/(void)
(and +1 egg to the bucket: despite of internal implementation, there are operators up to int32 (why that upper limit?), to say).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

insymmetrical

This set is correlated with supported cv::Mat types.

IMHO, The worst problem here is low tests coverage for this file.

CV_ALWAYS_INLINE bool isZero() { return val == 0; }
static CV_ALWAYS_INLINE fixedpoint64 zero() { return fixedpoint64(); }
static CV_ALWAYS_INLINE fixedpoint64 one() { return fixedpoint64((int64_t)(1LL << fixedShift)); }
Expand Down Expand Up @@ -133,15 +139,15 @@ class ufixedpoint64
uint64_t val0_h = (sh2 & 0xFFFFFFFF) + (sh1_0 >> 32) + (sh1_1 >> 32) + (val0_l >> 32);
val0_l &= 0xFFFFFFFF;

if ((sh2 >> 32) || (val0_h >> 32))
return ((uint64_t)0xFFFFFFFFFFFFFFFF);
if (sh2 > CV_BIG_INT(0xFFFFFFFF) || val0_h > CV_BIG_INT(0xFFFFFFFF))
return (uint64_t)CV_BIG_UINT(0xFFFFFFFFFFFFFFFF);

return val0_h << 32 | val0_l;
return (val0_h << 32 | val0_l);
}
CV_ALWAYS_INLINE ufixedpoint64 operator + (const ufixedpoint64& val2) const
{
uint64_t res = val + val2.val;
return (val > res) ? (uint64_t)0xFFFFFFFFFFFFFFFF : res;
return (uint64_t)((val > res) ? CV_BIG_UINT(0xFFFFFFFFFFFFFFFF) : res);
}
CV_ALWAYS_INLINE ufixedpoint64 operator - (const ufixedpoint64& val2) const
{
Expand All @@ -151,9 +157,14 @@ class ufixedpoint64
CV_ALWAYS_INLINE ufixedpoint64 operator << (int n) const { return ufixedpoint64(val << n); }
CV_ALWAYS_INLINE bool operator == (const ufixedpoint64& val2) const { return val == val2.val; }
template <typename ET>
CV_ALWAYS_INLINE operator ET() const { return cv::saturate_cast<ET>(fixedround(val) >> fixedShift); }
CV_ALWAYS_INLINE ET saturate_cast() const { return cv::saturate_cast<ET>(fixedround(val) >> fixedShift); }
CV_ALWAYS_INLINE operator double() const { return (double)val / (1LL << fixedShift); }
CV_ALWAYS_INLINE operator float() const { return (float)val / (1LL << fixedShift); }
CV_ALWAYS_INLINE operator uint8_t() const { return saturate_cast<uint8_t>(); }
CV_ALWAYS_INLINE operator int8_t() const { return saturate_cast<int8_t>(); }
CV_ALWAYS_INLINE operator uint16_t() const { return saturate_cast<uint16_t>(); }
CV_ALWAYS_INLINE operator int16_t() const { return saturate_cast<int16_t>(); }
CV_ALWAYS_INLINE operator int32_t() const { return saturate_cast<int32_t>(); }
CV_ALWAYS_INLINE bool isZero() { return val == 0; }
static CV_ALWAYS_INLINE ufixedpoint64 zero() { return ufixedpoint64(); }
static CV_ALWAYS_INLINE ufixedpoint64 one() { return ufixedpoint64((uint64_t)(1ULL << fixedShift)); }
Expand Down Expand Up @@ -187,21 +198,26 @@ class fixedpoint32
CV_ALWAYS_INLINE fixedpoint32 operator + (const fixedpoint32& val2) const
{
int32_t res = val + val2.val;
return ((val ^ res) & (val2.val ^ res)) >> 31 ? ~(res & ~0x7FFFFFFF) : res;
return (int64_t)((val ^ res) & (val2.val ^ res)) >> 31 ? ~(res & ~0x7FFFFFFF) : res;
}
CV_ALWAYS_INLINE fixedpoint32 operator - (const fixedpoint32& val2) const
{
int32_t res = val - val2.val;
return ((val ^ val2.val) & (val ^ res)) >> 31 ? ~(res & ~0x7FFFFFFF) : res;
return (int64_t)((val ^ val2.val) & (val ^ res)) >> 31 ? ~(res & ~0x7FFFFFFF) : res;
}
CV_ALWAYS_INLINE fixedpoint32 operator >> (int n) const { return fixedpoint32(val >> n); }
CV_ALWAYS_INLINE fixedpoint32 operator << (int n) const { return fixedpoint32(val << n); }
CV_ALWAYS_INLINE bool operator == (const fixedpoint32& val2) const { return val == val2.val; }
template <typename ET>
CV_ALWAYS_INLINE operator ET() const { return cv::saturate_cast<ET>((int32_t)fixedround((uint32_t)val) >> fixedShift); }
CV_ALWAYS_INLINE ET saturate_cast() const { return cv::saturate_cast<ET>((int32_t)fixedround((uint32_t)val) >> fixedShift); }
CV_ALWAYS_INLINE operator fixedpoint64() const { return (int64_t)val << (fixedpoint64::fixedShift - fixedShift); }
CV_ALWAYS_INLINE operator double() const { return (double)val / (1 << fixedShift); }
CV_ALWAYS_INLINE operator float() const { return (float)val / (1 << fixedShift); }
CV_ALWAYS_INLINE operator uint8_t() const { return saturate_cast<uint8_t>(); }
CV_ALWAYS_INLINE operator int8_t() const { return saturate_cast<int8_t>(); }
CV_ALWAYS_INLINE operator uint16_t() const { return saturate_cast<uint16_t>(); }
CV_ALWAYS_INLINE operator int16_t() const { return saturate_cast<int16_t>(); }
CV_ALWAYS_INLINE operator int32_t() const { return saturate_cast<int32_t>(); }
CV_ALWAYS_INLINE bool isZero() { return val == 0; }
static CV_ALWAYS_INLINE fixedpoint32 zero() { return fixedpoint32(); }
static CV_ALWAYS_INLINE fixedpoint32 one() { return fixedpoint32((1 << fixedShift)); }
Expand Down Expand Up @@ -242,10 +258,15 @@ class ufixedpoint32
CV_ALWAYS_INLINE ufixedpoint32 operator << (int n) const { return ufixedpoint32(val << n); }
CV_ALWAYS_INLINE bool operator == (const ufixedpoint32& val2) const { return val == val2.val; }
template <typename ET>
CV_ALWAYS_INLINE operator ET() const { return cv::saturate_cast<ET>(fixedround(val) >> fixedShift); }
CV_ALWAYS_INLINE ET saturate_cast() const { return cv::saturate_cast<ET>(fixedround(val) >> fixedShift); }
CV_ALWAYS_INLINE operator ufixedpoint64() const { return (uint64_t)val << (ufixedpoint64::fixedShift - fixedShift); }
CV_ALWAYS_INLINE operator double() const { return (double)val / (1 << fixedShift); }
CV_ALWAYS_INLINE operator float() const { return (float)val / (1 << fixedShift); }
CV_ALWAYS_INLINE operator uint8_t() const { return saturate_cast<uint8_t>(); }
CV_ALWAYS_INLINE operator int8_t() const { return saturate_cast<int8_t>(); }
CV_ALWAYS_INLINE operator uint16_t() const { return saturate_cast<uint16_t>(); }
CV_ALWAYS_INLINE operator int16_t() const { return saturate_cast<int16_t>(); }
CV_ALWAYS_INLINE operator int32_t() const { return saturate_cast<int32_t>(); }
CV_ALWAYS_INLINE bool isZero() { return val == 0; }
static CV_ALWAYS_INLINE ufixedpoint32 zero() { return ufixedpoint32(); }
static CV_ALWAYS_INLINE ufixedpoint32 one() { return ufixedpoint32((1U << fixedShift)); }
Expand Down Expand Up @@ -284,10 +305,15 @@ class fixedpoint16
CV_ALWAYS_INLINE fixedpoint16 operator << (int n) const { return fixedpoint16((int16_t)(val << n)); }
CV_ALWAYS_INLINE bool operator == (const fixedpoint16& val2) const { return val == val2.val; }
template <typename ET>
CV_ALWAYS_INLINE operator ET() const { return cv::saturate_cast<ET>((int16_t)fixedround((uint16_t)val) >> fixedShift); }
CV_ALWAYS_INLINE ET saturate_cast() const { return cv::saturate_cast<ET>((int16_t)fixedround((uint16_t)val) >> fixedShift); }
CV_ALWAYS_INLINE operator fixedpoint32() const { return (int32_t)val << (fixedpoint32::fixedShift - fixedShift); }
CV_ALWAYS_INLINE operator double() const { return (double)val / (1 << fixedShift); }
CV_ALWAYS_INLINE operator float() const { return (float)val / (1 << fixedShift); }
CV_ALWAYS_INLINE operator uint8_t() const { return saturate_cast<uint8_t>(); }
CV_ALWAYS_INLINE operator int8_t() const { return saturate_cast<int8_t>(); }
CV_ALWAYS_INLINE operator uint16_t() const { return saturate_cast<uint16_t>(); }
CV_ALWAYS_INLINE operator int16_t() const { return saturate_cast<int16_t>(); }
CV_ALWAYS_INLINE operator int32_t() const { return saturate_cast<int32_t>(); }
CV_ALWAYS_INLINE bool isZero() { return val == 0; }
static CV_ALWAYS_INLINE fixedpoint16 zero() { return fixedpoint16(); }
static CV_ALWAYS_INLINE fixedpoint16 one() { return fixedpoint16((int16_t)(1 << fixedShift)); }
Expand Down Expand Up @@ -324,15 +350,20 @@ class ufixedpoint16
CV_ALWAYS_INLINE ufixedpoint16 operator << (int n) const { return ufixedpoint16((uint16_t)(val << n)); }
CV_ALWAYS_INLINE bool operator == (const ufixedpoint16& val2) const { return val == val2.val; }
template <typename ET>
CV_ALWAYS_INLINE operator ET() const { return cv::saturate_cast<ET>(fixedround(val) >> fixedShift); }
CV_ALWAYS_INLINE ET saturate_cast() const { return cv::saturate_cast<ET>(fixedround(val) >> fixedShift); }
CV_ALWAYS_INLINE operator ufixedpoint32() const { return (uint32_t)val << (ufixedpoint32::fixedShift - fixedShift); }
CV_ALWAYS_INLINE operator double() const { return (double)val / (1 << fixedShift); }
CV_ALWAYS_INLINE operator float() const { return (float)val / (1 << fixedShift); }
CV_ALWAYS_INLINE operator uint8_t() const { return saturate_cast<uint8_t>(); }
CV_ALWAYS_INLINE operator int8_t() const { return saturate_cast<int8_t>(); }
CV_ALWAYS_INLINE operator uint16_t() const { return saturate_cast<uint16_t>(); }
CV_ALWAYS_INLINE operator int16_t() const { return saturate_cast<int16_t>(); }
CV_ALWAYS_INLINE operator int32_t() const { return saturate_cast<int32_t>(); }
CV_ALWAYS_INLINE bool isZero() { return val == 0; }
static CV_ALWAYS_INLINE ufixedpoint16 zero() { return ufixedpoint16(); }
static CV_ALWAYS_INLINE ufixedpoint16 one() { return ufixedpoint16((uint16_t)(1 << fixedShift)); }
};

}

#endif
#endif