From 9edbfa974aeab851065655e09e8c1accf51a009c Mon Sep 17 00:00:00 2001 From: ibaker Date: Tue, 3 Oct 2023 03:48:15 -0700 Subject: [PATCH] Fix `Util.scaleLargeValue/Timestamp` to handle negative numbers #minor-release PiperOrigin-RevId: 570337535 --- .../androidx/media3/common/util/Util.java | 5 +- .../UtilScaleLargeValueParameterizedTest.java | 145 ++++++++++++------ 2 files changed, 101 insertions(+), 49 deletions(-) diff --git a/libraries/common/src/main/java/androidx/media3/common/util/Util.java b/libraries/common/src/main/java/androidx/media3/common/util/Util.java index fa0e776ce1..b3c963ccab 100644 --- a/libraries/common/src/main/java/androidx/media3/common/util/Util.java +++ b/libraries/common/src/main/java/androidx/media3/common/util/Util.java @@ -1840,12 +1840,13 @@ public final class Util { // Directly multiplying value and multiplier will overflow a long, so we try and cancel // with GCD and try directly multiplying again below. If that still overflows we fall // through to floating point arithmetic. - long gcdOfMultiplierAndDivisor = LongMath.gcd(multiplier, divisor); + long gcdOfMultiplierAndDivisor = LongMath.gcd(Math.abs(multiplier), Math.abs(divisor)); long simplifiedMultiplier = LongMath.divide(multiplier, gcdOfMultiplierAndDivisor, RoundingMode.UNNECESSARY); long simplifiedDivisor = LongMath.divide(divisor, gcdOfMultiplierAndDivisor, RoundingMode.UNNECESSARY); - long gcdOfValueAndSimplifiedDivisor = LongMath.gcd(value, simplifiedDivisor); + long gcdOfValueAndSimplifiedDivisor = + LongMath.gcd(Math.abs(value), Math.abs(simplifiedDivisor)); long simplifiedValue = LongMath.divide(value, gcdOfValueAndSimplifiedDivisor, RoundingMode.UNNECESSARY); simplifiedDivisor = diff --git a/libraries/common/src/test/java/androidx/media3/common/util/UtilScaleLargeValueParameterizedTest.java b/libraries/common/src/test/java/androidx/media3/common/util/UtilScaleLargeValueParameterizedTest.java index 65be0c828a..84ec683dd9 100644 --- a/libraries/common/src/test/java/androidx/media3/common/util/UtilScaleLargeValueParameterizedTest.java +++ b/libraries/common/src/test/java/androidx/media3/common/util/UtilScaleLargeValueParameterizedTest.java @@ -18,11 +18,14 @@ package androidx.media3.common.util; import static androidx.media3.common.util.Assertions.checkState; import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; +import static org.junit.Assume.assumeFalse; import static org.junit.Assume.assumeTrue; import com.google.common.collect.ImmutableList; import com.google.common.math.LongMath; import java.math.RoundingMode; +import java.util.ArrayList; +import java.util.List; import org.junit.Test; import org.junit.runner.RunWith; import org.robolectric.ParameterizedRobolectricTestRunner; @@ -45,54 +48,92 @@ public class UtilScaleLargeValueParameterizedTest { @Parameters(name = "{0}") public static ImmutableList implementations() { - return ImmutableList.of( - new Object[] {"single-value", (ScaleLargeValueFn) Util::scaleLargeValue}, - new Object[] { - "list", - (ScaleLargeValueFn) - (value, multiplier, divisor, roundingMode) -> - Util.scaleLargeValues(ImmutableList.of(value), multiplier, divisor, roundingMode)[ - 0] - }, - new Object[] { - "array-in-place", - (ScaleLargeValueFn) + ImmutableList implementations = + ImmutableList.of( + new Implementation("single-value", Util::scaleLargeValue), + new Implementation( + "list", + (value, multiplier, divisor, roundingMode) -> + Util.scaleLargeValues( + ImmutableList.of(value), multiplier, divisor, roundingMode)[0]), + new Implementation( + "array-in-place", + (value, multiplier, divisor, roundingMode) -> { + long[] values = new long[] {value}; + Util.scaleLargeValuesInPlace(values, multiplier, divisor, roundingMode); + return values[0]; + }), + new Implementation( + "single-timestamp", + (long timestamp, long multiplier, long divisor, RoundingMode roundingMode) -> { + assumeTrue( + roundingMode == RoundingMode.UNNECESSARY + || roundingMode == RoundingMode.FLOOR); + return Util.scaleLargeTimestamp(timestamp, multiplier, divisor); + }), + new Implementation( + "timestamp-list", + (timestamp, multiplier, divisor, roundingMode) -> { + assumeTrue( + roundingMode == RoundingMode.UNNECESSARY + || roundingMode == RoundingMode.FLOOR); + return Util.scaleLargeTimestamps( + ImmutableList.of(timestamp), multiplier, divisor)[0]; + }), + new Implementation( + "timestamp-array-in-place", + (timestamp, multiplier, divisor, roundingMode) -> { + assumeTrue( + roundingMode == RoundingMode.UNNECESSARY + || roundingMode == RoundingMode.FLOOR); + long[] timestamps = new long[] {timestamp}; + Util.scaleLargeTimestampsInPlace(timestamps, multiplier, divisor); + return timestamps[0]; + })); + + List implementationsWithNegativeCases = new ArrayList<>(); + for (Implementation implementation : implementations) { + implementationsWithNegativeCases.add(implementation); + implementationsWithNegativeCases.add( + new Implementation( + implementation.name + "-negative-value", (value, multiplier, divisor, roundingMode) -> { - long[] values = new long[] {value}; - Util.scaleLargeValuesInPlace(values, multiplier, divisor, roundingMode); - return values[0]; - } - }, - new Object[] { - "single-timestamp", - (ScaleLargeValueFn) - (long timestamp, long multiplier, long divisor, RoundingMode roundingMode) -> { - assumeTrue( - roundingMode == RoundingMode.UNNECESSARY || roundingMode == RoundingMode.FLOOR); - return Util.scaleLargeTimestamp(timestamp, multiplier, divisor); - } - }, - new Object[] { - "timestamp-list", - (ScaleLargeValueFn) - (timestamp, multiplier, divisor, roundingMode) -> { - assumeTrue( - roundingMode == RoundingMode.UNNECESSARY || roundingMode == RoundingMode.FLOOR); - return Util.scaleLargeTimestamps(ImmutableList.of(timestamp), multiplier, divisor)[ - 0]; - } - }, - new Object[] { - "timestamp-array-in-place", - (ScaleLargeValueFn) - (timestamp, multiplier, divisor, roundingMode) -> { - assumeTrue( - roundingMode == RoundingMode.UNNECESSARY || roundingMode == RoundingMode.FLOOR); - long[] timestamps = new long[] {timestamp}; - Util.scaleLargeTimestampsInPlace(timestamps, multiplier, divisor); - return timestamps[0]; - } - }); + assumeTrue(roundingMode == RoundingMode.UNNECESSARY); + long result = + implementation.scaleFn.scaleLargeValue( + -value, multiplier, divisor, roundingMode); + assumeFalse(result == Long.MIN_VALUE || result == Long.MAX_VALUE); + return -result; + })); + implementationsWithNegativeCases.add( + new Implementation( + implementation.name + "-negative-multiplier", + (value, multiplier, divisor, roundingMode) -> { + assumeTrue(roundingMode == RoundingMode.UNNECESSARY); + long result = + implementation.scaleFn.scaleLargeValue( + value, -multiplier, divisor, roundingMode); + assumeFalse(result == Long.MIN_VALUE || result == Long.MAX_VALUE); + return -result; + })); + implementationsWithNegativeCases.add( + new Implementation( + implementation.name + "-negative-divisor", + (value, multiplier, divisor, roundingMode) -> { + assumeTrue(roundingMode == RoundingMode.UNNECESSARY); + long result = + implementation.scaleFn.scaleLargeValue( + value, multiplier, -divisor, roundingMode); + assumeFalse(result == Long.MIN_VALUE || result == Long.MAX_VALUE); + return -result; + })); + } + + ImmutableList.Builder implementationsAsObjectArray = ImmutableList.builder(); + for (Implementation implementation : implementationsWithNegativeCases) { + implementationsAsObjectArray.add(new Object[] {implementation.name, implementation.scaleFn}); + } + return implementationsAsObjectArray.build(); } // Every parameter has to be assigned to a field, even if it's only used to name the test. @@ -277,6 +318,16 @@ public class UtilScaleLargeValueParameterizedTest { assertThat(result).isEqualTo(Long.MAX_VALUE); } + private static final class Implementation { + public final String name; + public final ScaleLargeValueFn scaleFn; + + private Implementation(String name, ScaleLargeValueFn scaleFn) { + this.name = name; + this.scaleFn = scaleFn; + } + } + private interface ScaleLargeValueFn { long scaleLargeValue(long value, long multiplier, long divisor, RoundingMode roundingMode); }