This is undefined behavior, according to specification.
BUG=chromium:661133
Review-Url: https://codereview.webrtc.org/2500953003
Cr-Commit-Position: refs/heads/master@{#15078}
The values in question are supposed to be able to be negative.
BUG=chromium:653448
Review-Url: https://codereview.webrtc.org/2387333005
Cr-Commit-Position: refs/heads/master@{#14573}
A left shift by 10 was assumed to never overflow, since "[s]imulation
of the 25 files shows that maximum value in the vector gain_lo_hiQ17[]
is 441344, which means that it is log2((2^31)/441344) = 12.2 shifting
bits from saturation." However, a fuzzer test succeeded in provoking
an overflow, which we ignore in this CL on the theory that only
"abnormal" inputs cause overflow.
Also had to replace a "foo << 1" with "foo * (1 << 1)" in
WEBRTC_SPL_MUL_16_32_RSFT15 because foo could be negative; this
problem showed up as soon as I'd asked UBSan to ignore the overflow
discussed above.
BUG=chromium:615819
Review-Url: https://codereview.webrtc.org/2314413002
Cr-Commit-Position: refs/heads/master@{#14162}
The sample uses are from when I debugged bug 617124. The change in neteq_network_stats_unittest.cc is a fix for a minor unrelated bug found by the try bots when I tried to land this CL (a test was passing uninitialized packet data to NetEq).
BUG=chromium:617124
Review-Url: https://codereview.webrtc.org/2293893002
Cr-Commit-Position: refs/heads/master@{#14034}
We have RTC_CHECK and RTC_DCHECK for C now, so we should use it. It's
one fewer difference between our C and C++ code.
NOPRESUBMIT=true
Review-Url: https://codereview.webrtc.org/2274083002
Cr-Commit-Position: refs/heads/master@{#13930}
Using __builtin_clz on gcc/clang, and a fallback implementation
otherwise.
Also redefine WebRtcSpl_GetSizeInBits(x) as simply 32 - clz32(x).
BUG=chromium:601787
Review-Url: https://codereview.webrtc.org/2014023002
Cr-Commit-Position: refs/heads/master@{#13014}
Of course, functions called WebRtcSpl_AddSatW32 and WebRtcSpl_SubSatW32 are supposed to handle overflow gracefully, and they probably did. But since the overflow handling depended on undefined behavior, a sufficiently smart optimizing compiler would have realized that it could just ignore the possibility of overflow and omit all the overflow handling code, leaving only the unadorned addition or subtraction.
Also, the new implementations, unlike the old ones, result in branch-free code (tested with clang 3.9 with -O2).
BUG=chromium:601728
Review-Url: https://codereview.webrtc.org/2002523002
Cr-Commit-Position: refs/heads/master@{#12846}
I've settled on replacing x << n with x * (1 << n); this gets rid of
the "left shift of negative value" warning, but will still trigger
undefined behavior if the multiplication overflows. It also still
looks like a left shift, which is good for the readability of the
fixed-point code.
(The compiler is smart enough to recognize that the
multiplication+shift is just a shift, for both variable and constant
shift amounts, so the generated code should not change.)
BUG=chromium:603491
Review-Url: https://codereview.webrtc.org/1989803002
Cr-Commit-Position: refs/heads/master@{#12845}
Chrome does not detect NEON instruction set at runtime in WebRTC code starting
with M50, which is now in Stable. Remove support for runtime detection for
simplicity.
The only remaining piece of Chrome that will continue to depend on runtime
detection is /net, where devices with _broken_ neon support are also detected,
and it is not configurable via GYP/GN.
BUG=522035
NOPRESUBMIT=true
Review-Url: https://codereview.webrtc.org/1955413003
Cr-Commit-Position: refs/heads/master@{#12778}
Reason for revert:
There seems an error made in this patch.
Hi Henrik,
I think the bit shift returned by CrossCorrelationWithAutoShift may be wrongly used by DotProduct.
We'd better revert this CL.
Doing another fix (and future fixes) will be paintful. I will work on a easy-to-modify bitexactness test first.
Original issue's description:
> Avoiding overflow in cross correlation in NetEq.
>
> BUG=
TBR=henrik.lundin@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=
Review-Url: https://codereview.webrtc.org/1925053002
Cr-Commit-Position: refs/heads/master@{#12543}
When the input to WebRtcSpl_Sqrt was the maximum negative value
(-2147483648), the calculations would overflow. This is now solved by
nudging this particular input value one step.
BUG=webrtc:5512
Review URL: https://codereview.webrtc.org/1685743003
Cr-Commit-Position: refs/heads/master@{#11631}
Macro incorrectly displays DISABLED_ON_ANDROID in test names for
parameterized tests under --gtest_list_tests, causing tests to be
disabled on all platforms since they contain the DISABLED_ prefix rather
than their expanded variants.
This expands the macro variants to inline if they're disabled or not,
and removes building some tests under configurations where they should
fail, instead of building them but disabling them by default.
The change also removes gtest_disable.h as an unused include from many
other files.
BUG=webrtc:5387, webrtc:5400
R=kjellander@webrtc.org, phoglund@webrtc.orgTBR=henrik.lundin@webrtc.org
Review URL: https://codereview.webrtc.org/1547343002 .
Cr-Commit-Position: refs/heads/master@{#11150}
This will hurt Linux x64 perf, but we think that's a compiler bug and we're
willing to take the hit for the better clarity of the code sans cast as well as
the better Windows perf. Hopefully eventually the compiler will improve.
BUG=504813
TEST=none
TBR=andrew
Review URL: https://codereview.webrtc.org/1215053002
Cr-Commit-Position: refs/heads/master@{#9516}
Most of commit cb180976dd0e9672cde4523d87b5f4857478b5e9 (which reverted
commit 83ad33a8aed1fb00e422b6abd33c3e8942821c24) was already re-landed. This relands the rest, including modifications by kwiberg to hopefully avoid regressing performance.
In a subsequent change I will see if removing the int16_t cast in this modified version still causes perf problems.
BUG=499241
TEST=none
TBR=andrew
Review URL: https://codereview.webrtc.org/1181693005
Cr-Commit-Position: refs/heads/master@{#9487}
This reverts portions of commit cb180976dd0e9672cde4523d87b5f4857478b5e9, which
reverted commit 83ad33a8aed1fb00e422b6abd33c3e8942821c24. Specifically, the
files in webrtc/common_audio/ are relanded.
The original commit message is below:
Upconvert various types to int.
Per comments from HL/kwiberg on https://webrtc-codereview.appspot.com/42569004 , when there is existing usage of mixed types (int16_t, int, etc.), we'd prefer to standardize on larger types like int and phase out use of int16_t.
Specifically, "Using int16 just because we're sure all reasonable values will fit in 16 bits isn't usually meaningful in C."
This converts some existing uses of int16_t (and, in a few cases, other types such as uint16_t) to int (or, in a few places, int32_t). Other locations will be converted to size_t in a separate change.
BUG=none
TBR=andrew
Review URL: https://codereview.webrtc.org/1184613003
Cr-Commit-Position: refs/heads/master@{#9425}
This includes changes like:
* Attempt to break lines at better positions
* Use "override" in more places, don't use "virtual" with it
* Use {} where the body is more than one line
* Make declaration and definition arg names match
* Eliminate unused code
* EXPECT_EQ(expected, actual) (but use (actual, expected) for e.g. _GT)
* Correct #include order
* Use anonymous namespaces in preference to "static" for file-scoping
* Eliminate unnecessary casts
* Update reference code in comments of ARM assembly sources to match actual current C code
* Fix indenting to be more style-guide compliant
* Use arraysize() in more places
* Use bool instead of int for "boolean" values (0/1)
* Shorten and simplify code
* Spaces around operators
* 80 column limit
* Use const more consistently
* Space goes after '*' in type name, not before
* Remove unnecessary return values
* Use "(var == const)", not "(const == var)"
* Spelling
* Prefer true, typed constants to "enum hack" constants
* Avoid "virtual" on non-overridden functions
* ASSERT(x == y) -> ASSERT_EQ(y, x)
BUG=none
R=andrew@webrtc.org, asapersson@webrtc.org, henrika@webrtc.org, juberti@webrtc.org, kjellander@webrtc.org, kwiberg@webrtc.org
Review URL: https://codereview.webrtc.org/1172163004
Cr-Commit-Position: refs/heads/master@{#9420}
This makes a variety of small changes to synchronize bits of code using different types, remove useless code or casts, and add explicit casts in some places previously doing implicit ones. For example:
* Change a few type declarations to better match how the majority of code uses those objects.
* Eliminate "< 0" check for unsigned values.
* Replace "(float)sin(x)", where |x| is also a float, with "sinf(x)", and similar.
* Add casts to uint32_t in many places timestamps were used and the existing code stored signed values into the unsigned objects.
* Remove downcasts when the results would be passed to a larger type, e.g. calling "foo((int16_t)x)" with an int |x| when foo() takes an int instead of an int16_t.
* Similarly, add casts when passing a larger type to a function taking a smaller one.
* Add casts to int16_t when doing something like "int16_t = int16_t + int16_t" as the "+" operation would implicitly upconvert to int, and similar.
* Use "false" instead of "0" for setting a bool.
* Shift a few temp types when doing a multi-stage calculation involving typecasts, so as to put the most logical/semantically correct type possible into the temps. For example, when doing "int foo = int + int; size_t bar = (size_t)foo + size_t;", we might change |foo| to a size_t and move the cast if it makes more sense for |foo| to be represented as a size_t.
BUG=none
R=andrew@webrtc.org, asapersson@webrtc.org, henrika@webrtc.org, juberti@webrtc.org, kwiberg@webrtc.org
TBR=andrew, asapersson, henrika
Review URL: https://codereview.webrtc.org/1168753002
Cr-Commit-Position: refs/heads/master@{#9419}
This makes some behaviorally-invariant changes to make certain code that
currently only works correctly with signed types work safely regardless of the
signedness of the types in question. This is preparation for a future change
that will convert a variety of types to size_t.
There are also some formatting changes (e.g. converting "enum hack" usage to real consts) to make it simpler to just change "int" to "size_t" in the future to change the types of those constants.
BUG=none
R=andrew@webrtc.org, juberti@webrtc.org, kwiberg@webrtc.org
TBR=ajm
Review URL: https://codereview.webrtc.org/1174813003
Cr-Commit-Position: refs/heads/master@{#9413}
Per comments from HL/kwiberg on https://webrtc-codereview.appspot.com/42569004 , when there is existing usage of mixed types (int16_t, int, etc.), we'd prefer to standardize on larger types like int and phase out use of int16_t.
Specifically, "Using int16 just because we're sure all reasonable values will fit in 16 bits isn't usually meaningful in C."
This converts some existing uses of int16_t (and, in a few cases, other types such as uint16_t) to int (or, in a few places, int32_t). Other locations will be converted to size_t in a separate change.
BUG=none
R=andrew@webrtc.org, kwiberg@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/54629004
Cr-Commit-Position: refs/heads/master@{#9405}
Merge WEBRTC_ARCH_ARM64_NEON and WEBRTC_ARCH_ARM_NEON into one
WEBRTC_HAS_NEON.
Replace WEBRTC_DETECT_ARM_NEON by WEBRTC_DETECT_NEON.
Replace WEBRTC_ARCH_ARM by WEBRTC_ARCH_ARM64 for arm64 cpu.
BUG=4002
R=andrew@webrtc.org, jridges@masque.com, kjellander@webrtc.org
Change-Id: I870a4d0682b80633b671c9aab733153f6d95a980
Review URL: https://webrtc-codereview.appspot.com/49309004
Cr-Commit-Position: refs/heads/master@{#9228}
The macro is in C defined as
#define WEBRTC_SPL_MUL_16_16(a, b) \
((int32_t) (((int16_t)(a)) * ((int16_t)(b))))
(For definitions on ARMv7 and MIPS, see common_audio/signal_processing/include/spl_inl_{armv7,mips}.h)
Also includes
- style changes
- replaced pointer operations with direct element access
BUG=3348,3353
TESTED=locally on Linux and trybots
R=kwiberg@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/48949004
Cr-Commit-Position: refs/heads/master@{#9075}
We currently hit asserts in AECM where the output of WebRtcSpl_NormW16() on armv7 is incorrect.
I've verified that it outputs -17 for negative values. Internally that means that clz returns 0 after a two's complement operation on a int16_t.
There is a mismatch between the int16_t input and otherwise 32 bit assumptions. Explicitly casting to int32_t makes the two's complement do the correct thing.
The CL also extends the unit tests by running through a larger set of values.
BUG=4486
TESTED=locally on Android Nexus 7 and trybots
R=aluebs@webrtc.org, kwiberg@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/49549004
Cr-Commit-Position: refs/heads/master@{#8897}
The macro is defined as
#define WEBRTC_SPL_MUL_16_16_RSFT(a, b, c) \
(WEBRTC_SPL_MUL_16_16(a, b) >> (c))
where the latter macro is in C defined as
#define WEBRTC_SPL_MUL_16_16(a, b) \
((int32_t) (((int16_t)(a)) * ((int16_t)(b))))
(For definitions on ARMv7 and MIPS, see common_audio/signal_processing/include/spl_inl_{armv7,mips}.h)
The replacement consists of
- avoiding casts to int16_t if inputs already are int16_t
- adding explicit cast to <type> if result is assigned to <type> (other than int or int32_t)
- minor cleanups like remove of unnecessary parentheses and style changes
BUG=3348, 3353
TESTED=locally on Linux for both fixed and floating point and trybots
R=kwiberg@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/49499004
Cr-Commit-Position: refs/heads/master@{#8853}
The modification only uses the unique part of the WebRtcSpl_MaxAbsValue
function. Pass Spltest.MinMaxOperationTest conformance test on both
ARMv7 and ARM64. And the single function performance is similar with
original assembly version on different platforms. If not specified, the
code is compiled by GCC 4.6. The result is the "X version / C version"
ratio, and the less is better.
| run 100k times | cortex-a7 | cortex-a15 |
| use C as the base on each | (1.2Ghz) | (1.7Ghz) |
| CPU target | | |
|----------------------------+-----------+------------|
| Neon asm | 32% | 15% |
| Neon intrinsics (GCC 4.6) | 36% | 37% |
| Neon intrinsics (GCC 4.8) | 35% | 18% |
BUG=3580
R=andrew@webrtc.org, jridges@masque.com
Change-Id: Ia2f6822ec58774b401cc440b6751a97e540b5048
Review URL: https://webrtc-codereview.appspot.com/30109004
git-svn-id: http://webrtc.googlecode.com/svn/trunk@7803 4adac7df-926f-26a2-2b94-8c16560cd09d
Implemented the 3 bands splitting filter bank by:
1. Upsample by 4/3.
2. Split twice into 2 bands.
3. Discard upper most band, because it is empty anyway.
A unittest was also implemented:
1. Generate a signal from presence or absence of sine waves of different frequencies.
2. Split into 3 bands and check their presence or absence.
3. Recombine the bands.
4. Calculate delay (as it is an IIR it depends on frequency).
5. Check that the cross correlation of input and output is high enough at that delay.
BUG=webrtc:3146
R=andrew@webrtc.org, bjornv@webrtc.org, kwiberg@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/31029004
git-svn-id: http://webrtc.googlecode.com/svn/trunk@7754 4adac7df-926f-26a2-2b94-8c16560cd09d
