From 7885d3f5c6bc6190bd4784cf41d42330ab6f9d6e Mon Sep 17 00:00:00 2001 From: kwiberg Date: Tue, 25 Apr 2017 12:35:07 -0700 Subject: [PATCH] Add SafeMin() and SafeMax(), which accept args of different types Specifically, they handle all combinations of two integer and two floating-point arguments by picking a result type that is guaranteed to be able to hold the result. This means callers no longer have to deal with potentially dangerous casting to make all the arguments have the same type, like they have to with std::min() and std::max(). Also, they're constexpr. Mostly for illustrative purposes, this CL replaces a few std::min() and std::max() calls with SafeMin() and SafeMax(). BUG=webrtc:7459 Review-Url: https://codereview.webrtc.org/2810483002 Cr-Commit-Position: refs/heads/master@{#17869} --- webrtc/base/BUILD.gn | 2 + webrtc/base/safe_minmax.h | 188 ++++++++++++++++++ webrtc/base/safe_minmax_unittest.cc | 141 +++++++++++++ webrtc/common_video/h264/sps_vui_rewriter.cc | 4 +- webrtc/modules/audio_coding/neteq/merge.cc | 6 +- .../audio_processing_unittest.cc | 3 +- .../aimd_rate_control.cc | 5 +- 7 files changed, 342 insertions(+), 7 deletions(-) create mode 100644 webrtc/base/safe_minmax.h create mode 100644 webrtc/base/safe_minmax_unittest.cc diff --git a/webrtc/base/BUILD.gn b/webrtc/base/BUILD.gn index 6014809661..a3ee03f4ba 100644 --- a/webrtc/base/BUILD.gn +++ b/webrtc/base/BUILD.gn @@ -165,6 +165,7 @@ rtc_static_library("rtc_base_approved") { "safe_compare.h", "safe_conversions.h", "safe_conversions_impl.h", + "safe_minmax.h", "sanitizer.h", "scoped_ref_ptr.h", "string_to_number.cc", @@ -813,6 +814,7 @@ if (rtc_include_tests) { "ratetracker_unittest.cc", "refcountedobject_unittest.cc", "safe_compare_unittest.cc", + "safe_minmax_unittest.cc", "string_to_number_unittest.cc", "stringencode_unittest.cc", "stringutils_unittest.cc", diff --git a/webrtc/base/safe_minmax.h b/webrtc/base/safe_minmax.h new file mode 100644 index 0000000000..8d1fc8776f --- /dev/null +++ b/webrtc/base/safe_minmax.h @@ -0,0 +1,188 @@ +/* + * Copyright 2017 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +// Minimum and maximum +// =================== +// +// rtc::SafeMin(x, y) +// rtc::SafeMax(x, y) +// +// Accept two arguments of either any two integral or any two floating-point +// types, and return the smaller and larger value, respectively, with no +// truncation or wrap-around. If only one of the input types is statically +// guaranteed to be able to represent the result, the return type is that type; +// if either one would do, the result type is the smaller type. (One of these +// two cases always applies.) +// +// (The case with one floating-point and one integral type is not allowed, +// because the floating-point type will have greater range, but may not have +// sufficient precision to represent the integer value exactly.) +// +// Requesting a specific return type +// ================================= +// +// Both functions allow callers to explicitly specify the return type as a +// template parameter, overriding the default return type. E.g. +// +// rtc::SafeMin(x, y) // returns an int +// +// If the requested type is statically guaranteed to be able to represent the +// result, then everything's fine, and the return type is as requested. But if +// the requested type is too small, a static_assert is triggered. + +#ifndef WEBRTC_BASE_SAFE_MINMAX_H_ +#define WEBRTC_BASE_SAFE_MINMAX_H_ + +#include +#include + +#include "webrtc/base/checks.h" +#include "webrtc/base/safe_compare.h" +#include "webrtc/base/type_traits.h" + +namespace rtc { + +namespace safe_minmax_impl { + +// Make the range of a type available via something other than a constexpr +// function, to work around MSVC limitations. See +// https://blogs.msdn.microsoft.com/vcblog/2015/12/02/partial-support-for-expression-sfinae-in-vs-2015-update-1/ +template +struct Limits { + static constexpr T lowest = std::numeric_limits::lowest(); + static constexpr T max = std::numeric_limits::max(); +}; + +template ::value> +struct UnderlyingType; + +template +struct UnderlyingType { + using type = T; +}; + +template +struct UnderlyingType { + using type = typename std::underlying_type::type; +}; + +// Given two types T1 and T2, find types that can hold the smallest (in +// ::min_t) and the largest (in ::max_t) of the two values. +template ::value, + bool int2 = IsIntlike::value> +struct MType { + static_assert(int1 == int2, + "You may not mix integral and floating-point arguments"); +}; + +// Specialization for when neither type is integral (and therefore presumably +// floating-point). +template +struct MType { + using min_t = typename std::common_type::type; + static_assert(std::is_same::value || + std::is_same::value, + ""); + + using max_t = typename std::common_type::type; + static_assert(std::is_same::value || + std::is_same::value, + ""); +}; + +// Specialization for when both types are integral. +template +struct MType { + // The type with the lowest minimum value. In case of a tie, the type with + // the lowest maximum value. In case that too is a tie, the types have the + // same range, and we arbitrarily pick T1. + using min_t = typename std::conditional< + safe_cmp::Lt(Limits::lowest, Limits::lowest), + T1, + typename std::conditional< + safe_cmp::Gt(Limits::lowest, Limits::lowest), + T2, + typename std::conditional::max, + Limits::max), + T1, + T2>::type>::type>::type; + static_assert(std::is_same::value || + std::is_same::value, + ""); + + // The type with the highest maximum value. In case of a tie, the types have + // the same range (because in C++, integer types with the same maximum also + // have the same minimum). + static_assert(safe_cmp::Ne(Limits::max, Limits::max) || + safe_cmp::Eq(Limits::lowest, Limits::lowest), + "integer types with the same max should have the same min"); + using max_t = typename std:: + conditional::max, Limits::max), T1, T2>::type; + static_assert(std::is_same::value || + std::is_same::value, + ""); +}; + +// A dummy type that we pass around at compile time but never actually use. +// Declared but not defined. +struct DefaultType; + +// ::type is A, except we fall back to B if A is DefaultType. We static_assert +// that the chosen type can hold all values that B can hold. +template +struct TypeOr { + using type = typename std:: + conditional::value, B, A>::type; + static_assert(safe_cmp::Le(Limits::lowest, Limits::lowest) && + safe_cmp::Ge(Limits::max, Limits::max), + "The specified type isn't large enough"); + static_assert(IsIntlike::value == IsIntlike::value && + std::is_floating_point::value == + std::is_floating_point::value, + "float<->int conversions not allowed"); +}; + +} // namespace safe_minmax_impl + +template ::min_t>::type>::type> +constexpr R2 SafeMin(T1 a, T2 b) { + static_assert(IsIntlike::value || std::is_floating_point::value, + "The first argument must be integral or floating-point"); + static_assert(IsIntlike::value || std::is_floating_point::value, + "The second argument must be integral or floating-point"); + return safe_cmp::Lt(a, b) ? static_cast(a) : static_cast(b); +} + +template ::max_t>::type>::type> +constexpr R2 SafeMax(T1 a, T2 b) { + static_assert(IsIntlike::value || std::is_floating_point::value, + "The first argument must be integral or floating-point"); + static_assert(IsIntlike::value || std::is_floating_point::value, + "The second argument must be integral or floating-point"); + return safe_cmp::Gt(a, b) ? static_cast(a) : static_cast(b); +} + +} // namespace rtc + +#endif // WEBRTC_BASE_SAFE_MINMAX_H_ diff --git a/webrtc/base/safe_minmax_unittest.cc b/webrtc/base/safe_minmax_unittest.cc new file mode 100644 index 0000000000..6fcbf00332 --- /dev/null +++ b/webrtc/base/safe_minmax_unittest.cc @@ -0,0 +1,141 @@ +/* + * Copyright 2017 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include +#include + +#include "webrtc/base/safe_minmax.h" +#include "webrtc/test/gtest.h" + +namespace rtc { + +namespace { + +// Functions that check that SafeMin() and SafeMax() return the specified type. +// The functions that end in "R" use an explicitly given return type. + +template +constexpr bool TypeCheckMinMax() { + return std::is_same(), std::declval())), + Tmin>::value && + std::is_same(), std::declval())), + Tmax>::value; +} + +template +constexpr bool TypeCheckMinR() { + return std::is_same< + decltype(SafeMin(std::declval(), std::declval())), R>::value; +} + +template +constexpr bool TypeCheckMaxR() { + return std::is_same< + decltype(SafeMax(std::declval(), std::declval())), R>::value; +} + +// clang-format off + +// SafeMin/SafeMax: Check that all combinations of signed/unsigned 8/64 bits +// give the correct default result type. +static_assert(TypeCheckMinMax< int8_t, int8_t, int8_t, int8_t>(), ""); +static_assert(TypeCheckMinMax< int8_t, uint8_t, int8_t, uint8_t>(), ""); +static_assert(TypeCheckMinMax< int8_t, int64_t, int64_t, int64_t>(), ""); +static_assert(TypeCheckMinMax< int8_t, uint64_t, int8_t, uint64_t>(), ""); +static_assert(TypeCheckMinMax< uint8_t, int8_t, int8_t, uint8_t>(), ""); +static_assert(TypeCheckMinMax< uint8_t, uint8_t, uint8_t, uint8_t>(), ""); +static_assert(TypeCheckMinMax< uint8_t, int64_t, int64_t, int64_t>(), ""); +static_assert(TypeCheckMinMax< uint8_t, uint64_t, uint8_t, uint64_t>(), ""); +static_assert(TypeCheckMinMax< int64_t, int8_t, int64_t, int64_t>(), ""); +static_assert(TypeCheckMinMax< int64_t, uint8_t, int64_t, int64_t>(), ""); +static_assert(TypeCheckMinMax< int64_t, int64_t, int64_t, int64_t>(), ""); +static_assert(TypeCheckMinMax< int64_t, uint64_t, int64_t, uint64_t>(), ""); +static_assert(TypeCheckMinMax(), ""); +static_assert(TypeCheckMinMax(), ""); +static_assert(TypeCheckMinMax(), ""); +static_assert(TypeCheckMinMax(), ""); + +// SafeMin/SafeMax: Check that we can use enum types. +enum DefaultE { kFoo = -17 }; +enum UInt8E : uint8_t { kBar = 17 }; +static_assert(TypeCheckMinMax(), ""); +static_assert(TypeCheckMinMax(), ""); +static_assert(TypeCheckMinMax(), ""); + +using ld = long double; + +// SafeMin/SafeMax: Check that all floating-point combinations give the +// correct result type. +static_assert(TypeCheckMinMax< float, float, float, float>(), ""); +static_assert(TypeCheckMinMax< float, double, double, double>(), ""); +static_assert(TypeCheckMinMax< float, ld, ld, ld>(), ""); +static_assert(TypeCheckMinMax(), ""); +static_assert(TypeCheckMinMax(), ""); +static_assert(TypeCheckMinMax(), ""); +static_assert(TypeCheckMinMax< ld, float, ld, ld>(), ""); +static_assert(TypeCheckMinMax< ld, double, ld, ld>(), ""); +static_assert(TypeCheckMinMax< ld, ld, ld, ld>(), ""); + +// clang-format on + +// SafeMin/SafeMax: Check some cases of explicitly specified return type. The +// commented-out lines give compilation errors due to the requested return type +// being too small or requiring an int<->float conversion. +static_assert(TypeCheckMinR(), ""); +// static_assert(TypeCheckMinR(), ""); +static_assert(TypeCheckMinR(), ""); +// static_assert(TypeCheckMaxR(), ""); +// static_assert(TypeCheckMaxR(), ""); +static_assert(TypeCheckMaxR(), ""); +// static_assert(TypeCheckMaxR(), ""); + +template +constexpr bool CheckMinMax(T1 a, T2 b, Tmin min, Tmax max) { + return TypeCheckMinMax() && SafeMin(a, b) == min && + SafeMax(a, b) == max; +} + +// SafeMin/SafeMax: Check a few values. +static_assert(CheckMinMax(int8_t{1}, int8_t{-1}, int8_t{-1}, int8_t{1}), ""); +static_assert(CheckMinMax(uint8_t{1}, int8_t{-1}, int8_t{-1}, uint8_t{1}), ""); +static_assert(CheckMinMax(uint8_t{5}, uint64_t{2}, uint8_t{2}, uint64_t{5}), + ""); +static_assert(CheckMinMax(std::numeric_limits::min(), + std::numeric_limits::max(), + std::numeric_limits::min(), + std::numeric_limits::max()), + ""); +static_assert(CheckMinMax(std::numeric_limits::min(), + std::numeric_limits::max(), + std::numeric_limits::min(), + int32_t{std::numeric_limits::max()}), + ""); +// static_assert(CheckMinMax(1.f, 2, 1.f, 2.f), ""); +static_assert(CheckMinMax(1.f, 0.0, 0.0, 1.0), ""); + +} // namespace + +// clang-format off + +// These functions aren't used in the tests, but it's useful to look at the +// compiler output for them, and verify that (1) the same-signedness *Safe +// functions result in exactly the same code as their *Ref counterparts, and +// that (2) the mixed-signedness *Safe functions have just a few extra +// arithmetic and logic instructions (but no extra control flow instructions). +int32_t TestMinRef( int32_t a, int32_t b) { return std::min(a, b); } +uint32_t TestMinRef( uint32_t a, uint32_t b) { return std::min(a, b); } +int32_t TestMinSafe( int32_t a, int32_t b) { return SafeMin(a, b); } +int32_t TestMinSafe( int32_t a, uint32_t b) { return SafeMin(a, b); } +int32_t TestMinSafe(uint32_t a, int32_t b) { return SafeMin(a, b); } +uint32_t TestMinSafe(uint32_t a, uint32_t b) { return SafeMin(a, b); } + +// clang-format on + +} // namespace rtc diff --git a/webrtc/common_video/h264/sps_vui_rewriter.cc b/webrtc/common_video/h264/sps_vui_rewriter.cc index 2e118d5233..534e75c865 100644 --- a/webrtc/common_video/h264/sps_vui_rewriter.cc +++ b/webrtc/common_video/h264/sps_vui_rewriter.cc @@ -18,6 +18,7 @@ #include "webrtc/base/bitbuffer.h" #include "webrtc/base/checks.h" #include "webrtc/base/logging.h" +#include "webrtc/base/safe_minmax.h" #include "webrtc/common_video/h264/h264_common.h" #include "webrtc/common_video/h264/sps_parser.h" @@ -351,8 +352,7 @@ bool CopyRemainingBits(rtc::BitBuffer* source, COPY_BITS(source, destination, bits_tmp, misaligned_bits); } while (source->RemainingBitCount() > 0) { - size_t count = std::min(static_cast(32u), - static_cast(source->RemainingBitCount())); + auto count = rtc::SafeMin(32u, source->RemainingBitCount()); COPY_BITS(source, destination, bits_tmp, count); } // TODO(noahric): The last byte could be all zeroes now, which we should just diff --git a/webrtc/modules/audio_coding/neteq/merge.cc b/webrtc/modules/audio_coding/neteq/merge.cc index 299682f60d..d50b66588a 100644 --- a/webrtc/modules/audio_coding/neteq/merge.cc +++ b/webrtc/modules/audio_coding/neteq/merge.cc @@ -16,6 +16,8 @@ #include // min, max #include +#include "webrtc/base/safe_conversions.h" +#include "webrtc/base/safe_minmax.h" #include "webrtc/common_audio/signal_processing/include/signal_processing_library.h" #include "webrtc/modules/audio_coding/neteq/audio_multi_vector.h" #include "webrtc/modules/audio_coding/neteq/cross_correlation.h" @@ -209,8 +211,8 @@ size_t Merge::GetExpandedSignal(size_t* old_length, size_t* expand_period) { int16_t Merge::SignalScaling(const int16_t* input, size_t input_length, const int16_t* expanded_signal) const { // Adjust muting factor if new vector is more or less of the BGN energy. - const size_t mod_input_length = - std::min(static_cast(64 * fs_mult_), input_length); + const auto mod_input_length = rtc::SafeMin( + 64 * rtc::dchecked_cast(fs_mult_), input_length); const int16_t expanded_max = WebRtcSpl_MaxAbsValueW16(expanded_signal, mod_input_length); int32_t factor = (expanded_max * expanded_max) / diff --git a/webrtc/modules/audio_processing/audio_processing_unittest.cc b/webrtc/modules/audio_processing/audio_processing_unittest.cc index 814eea9967..c56a57b4c3 100644 --- a/webrtc/modules/audio_processing/audio_processing_unittest.cc +++ b/webrtc/modules/audio_processing/audio_processing_unittest.cc @@ -21,6 +21,7 @@ #include "webrtc/base/gtest_prod_util.h" #include "webrtc/base/ignore_wundef.h" #include "webrtc/base/protobuf_utils.h" +#include "webrtc/base/safe_minmax.h" #include "webrtc/common_audio/include/audio_util.h" #include "webrtc/common_audio/resampler/include/push_resampler.h" #include "webrtc/common_audio/resampler/push_sinc_resampler.h" @@ -678,7 +679,7 @@ void ApmTest::ProcessDelayVerificationTest(int delay_ms, int system_delay_ms, // Calculate expected delay estimate and acceptable regions. Further, // limit them w.r.t. AEC delay estimation support. const size_t samples_per_ms = - std::min(static_cast(16), frame_->samples_per_channel_ / 10); + rtc::SafeMin(16u, frame_->samples_per_channel_ / 10); int expected_median = std::min(std::max(delay_ms - system_delay_ms, delay_min), delay_max); int expected_median_high = std::min( diff --git a/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc b/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc index 71415c77a8..e645090b35 100644 --- a/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc +++ b/webrtc/modules/remote_bitrate_estimator/aimd_rate_control.cc @@ -15,6 +15,7 @@ #include #include "webrtc/base/checks.h" +#include "webrtc/base/safe_minmax.h" #include "webrtc/modules/remote_bitrate_estimator/overuse_detector.h" #include "webrtc/modules/remote_bitrate_estimator/include/bwe_defines.h" @@ -250,8 +251,8 @@ uint32_t AimdRateControl::MultiplicativeRateIncrease( int64_t now_ms, int64_t last_ms, uint32_t current_bitrate_bps) const { double alpha = 1.08; if (last_ms > -1) { - int time_since_last_update_ms = std::min(static_cast(now_ms - last_ms), - 1000); + auto time_since_last_update_ms = + rtc::SafeMin(now_ms - last_ms, 1000); alpha = pow(alpha, time_since_last_update_ms / 1000.0); } uint32_t multiplicative_increase_bps = std::max(