From 2bd4d61b9b8c838fb7dc81e7d6f513c4a4e00c76 Mon Sep 17 00:00:00 2001 From: ibaker Date: Thu, 12 Mar 2020 12:41:08 +0000 Subject: [PATCH] Use a Multiset for reference counting in MediaSourceEventDispatcher This avoids duplicate events being dispatched to object foo if addListener(foo) is called more than once. Part of issue:#6765 PiperOrigin-RevId: 300529733 --- .../exoplayer2/util/CopyOnWriteMultiset.java | 139 ++++++++++++++++++ .../util/CopyOnWriteMultisetTest.java | 110 ++++++++++++++ .../source/MediaSourceEventListener.java | 6 +- .../util/MediaSourceEventDispatcher.java | 29 +++- 4 files changed, 276 insertions(+), 8 deletions(-) create mode 100644 library/common/src/main/java/com/google/android/exoplayer2/util/CopyOnWriteMultiset.java create mode 100644 library/common/src/test/java/com/google/android/exoplayer2/util/CopyOnWriteMultisetTest.java diff --git a/library/common/src/main/java/com/google/android/exoplayer2/util/CopyOnWriteMultiset.java b/library/common/src/main/java/com/google/android/exoplayer2/util/CopyOnWriteMultiset.java new file mode 100644 index 0000000000..e8eb0d0df9 --- /dev/null +++ b/library/common/src/main/java/com/google/android/exoplayer2/util/CopyOnWriteMultiset.java @@ -0,0 +1,139 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package com.google.android.exoplayer2.util; + +import androidx.annotation.GuardedBy; +import androidx.annotation.Nullable; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.Set; + +/** + * An unordered collection of elements that allows duplicates, but also allows access to a set of + * unique elements. + * + *

This class is thread-safe using the same method as {@link + * java.util.concurrent.CopyOnWriteArrayList}. Mutation methods cause the underlying data to be + * copied. {@link #elementSet()} and {@link #iterator()} return snapshots that are unaffected by + * subsequent mutations. + * + *

Iterating directly on this class reveals duplicate elements. Unique elements can be accessed + * via {@link #elementSet()}. Iteration order for both of these is not defined. + * + * @param The type of element being stored. + */ +public final class CopyOnWriteMultiset implements Iterable { + + private final Object lock; + + @GuardedBy("lock") + private final Map elementCounts; + + @GuardedBy("lock") + private Set elementSet; + + @GuardedBy("lock") + private List elements; + + public CopyOnWriteMultiset() { + lock = new Object(); + elementCounts = new HashMap<>(); + elementSet = Collections.emptySet(); + elements = Collections.emptyList(); + } + + /** + * Adds {@code element} to the multiset. + * + * @param element The element to be added. + */ + public void add(E element) { + synchronized (lock) { + List elements = new ArrayList<>(this.elements); + elements.add(element); + this.elements = Collections.unmodifiableList(elements); + + @Nullable Integer count = elementCounts.get(element); + if (count == null) { + Set elementSet = new HashSet<>(this.elementSet); + elementSet.add(element); + this.elementSet = Collections.unmodifiableSet(elementSet); + } + elementCounts.put(element, count != null ? count + 1 : 1); + } + } + + /** + * Removes {@code element} from the multiset. + * + * @param element The element to be removed. + */ + public void remove(E element) { + synchronized (lock) { + @Nullable Integer count = elementCounts.get(element); + if (count == null) { + return; + } + + List elements = new ArrayList<>(this.elements); + elements.remove(element); + this.elements = Collections.unmodifiableList(elements); + + if (count == 1) { + elementCounts.remove(element); + Set elementSet = new HashSet<>(this.elementSet); + elementSet.remove(element); + this.elementSet = Collections.unmodifiableSet(elementSet); + } else { + elementCounts.put(element, count - 1); + } + } + } + + /** + * Returns a snapshot of the unique elements currently in this multiset. + * + *

Changes to the underlying multiset are not reflected in the returned value. + * + * @return An unmodifiable set containing the unique elements in this multiset. + */ + public Set elementSet() { + synchronized (lock) { + return elementSet; + } + } + + /** + * Returns an iterator over a snapshot of all the elements currently in this multiset (including + * duplicates). + * + *

Changes to the underlying multiset are not reflected in the returned value. + * + * @return An unmodifiable iterator over all the elements in this multiset (including duplicates). + */ + @Override + public Iterator iterator() { + synchronized (lock) { + return elements.iterator(); + } + } +} diff --git a/library/common/src/test/java/com/google/android/exoplayer2/util/CopyOnWriteMultisetTest.java b/library/common/src/test/java/com/google/android/exoplayer2/util/CopyOnWriteMultisetTest.java new file mode 100644 index 0000000000..92e4124a6a --- /dev/null +++ b/library/common/src/test/java/com/google/android/exoplayer2/util/CopyOnWriteMultisetTest.java @@ -0,0 +1,110 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package com.google.android.exoplayer2.util; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +import androidx.test.ext.junit.runners.AndroidJUnit4; +import java.util.Iterator; +import java.util.Set; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** Tests for {@link CopyOnWriteMultiset}. */ +@RunWith(AndroidJUnit4.class) +public final class CopyOnWriteMultisetTest { + + @Test + public void multipleEqualObjectsCountedAsExpected() { + String item1 = "a string"; + String item2 = "a string"; + String item3 = "different string"; + + CopyOnWriteMultiset multiset = new CopyOnWriteMultiset<>(); + + multiset.add(item1); + multiset.add(item2); + multiset.add(item3); + + assertThat(multiset).containsExactly("a string", "a string", "different string"); + assertThat(multiset.elementSet()).containsExactly("a string", "different string"); + } + + @Test + public void removingObjectDecrementsCount() { + String item1 = "a string"; + String item2 = "a string"; + String item3 = "different string"; + + CopyOnWriteMultiset multiset = new CopyOnWriteMultiset<>(); + + multiset.add(item1); + multiset.add(item2); + multiset.add(item3); + + multiset.remove("a string"); + + assertThat(multiset).containsExactly("a string", "different string"); + assertThat(multiset.elementSet()).containsExactly("a string", "different string"); + } + + @Test + public void removingLastObjectRemovesCompletely() { + String item1 = "a string"; + String item2 = "a string"; + String item3 = "different string"; + + CopyOnWriteMultiset multiset = new CopyOnWriteMultiset<>(); + + multiset.add(item1); + multiset.add(item2); + multiset.add(item3); + + multiset.remove("different string"); + + assertThat(multiset).containsExactly("a string", "a string"); + assertThat(multiset.elementSet()).containsExactly("a string"); + } + + @Test + public void removingNonexistentElementSucceeds() { + CopyOnWriteMultiset multiset = new CopyOnWriteMultiset<>(); + + multiset.remove("a string"); + } + + @Test + public void modifyingIteratorFails() { + CopyOnWriteMultiset multiset = new CopyOnWriteMultiset<>(); + multiset.add("a string"); + + Iterator iterator = multiset.iterator(); + + assertThrows(UnsupportedOperationException.class, iterator::remove); + } + + @Test + public void modifyingElementSetFails() { + CopyOnWriteMultiset multiset = new CopyOnWriteMultiset<>(); + multiset.add("a string"); + + Set elementSet = multiset.elementSet(); + + assertThrows(UnsupportedOperationException.class, () -> elementSet.remove("a string")); + } +} diff --git a/library/core/src/main/java/com/google/android/exoplayer2/source/MediaSourceEventListener.java b/library/core/src/main/java/com/google/android/exoplayer2/source/MediaSourceEventListener.java index 00536f374f..70892ee972 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/source/MediaSourceEventListener.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/source/MediaSourceEventListener.java @@ -23,12 +23,12 @@ import com.google.android.exoplayer2.Player; import com.google.android.exoplayer2.source.MediaSource.MediaPeriodId; import com.google.android.exoplayer2.upstream.DataSpec; import com.google.android.exoplayer2.util.Assertions; +import com.google.android.exoplayer2.util.CopyOnWriteMultiset; import com.google.android.exoplayer2.util.MediaSourceEventDispatcher; import java.io.IOException; import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.concurrent.CopyOnWriteArrayList; /** Interface for callbacks to be notified of {@link MediaSource} events. */ public interface MediaSourceEventListener { @@ -173,8 +173,8 @@ public interface MediaSourceEventListener { super(); } - public EventDispatcher( - CopyOnWriteArrayList listeners, + private EventDispatcher( + CopyOnWriteMultiset listeners, int windowIndex, @Nullable MediaPeriodId mediaPeriodId, long mediaTimeOffsetMs) { diff --git a/library/core/src/main/java/com/google/android/exoplayer2/util/MediaSourceEventDispatcher.java b/library/core/src/main/java/com/google/android/exoplayer2/util/MediaSourceEventDispatcher.java index abd5815090..3b9086f4e6 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/util/MediaSourceEventDispatcher.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/util/MediaSourceEventDispatcher.java @@ -21,7 +21,6 @@ import androidx.annotation.CheckResult; import androidx.annotation.Nullable; import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.source.MediaSource.MediaPeriodId; -import java.util.concurrent.CopyOnWriteArrayList; /** * Event dispatcher which forwards events to a list of registered listeners. @@ -51,21 +50,21 @@ public class MediaSourceEventDispatcher { @Nullable public final MediaPeriodId mediaPeriodId; // TODO: Make these private when MediaSourceEventListener.EventDispatcher is deleted. - protected final CopyOnWriteArrayList listenerAndHandlers; + protected final CopyOnWriteMultiset listenerAndHandlers; // TODO: Define exactly what this means, and check it's always set correctly. protected final long mediaTimeOffsetMs; /** Creates an event dispatcher. */ public MediaSourceEventDispatcher() { this( - /* listenerAndHandlers= */ new CopyOnWriteArrayList<>(), + /* listenerAndHandlers= */ new CopyOnWriteMultiset<>(), /* windowIndex= */ 0, /* mediaPeriodId= */ null, /* mediaTimeOffsetMs= */ 0); } protected MediaSourceEventDispatcher( - CopyOnWriteArrayList listenerAndHandlers, + CopyOnWriteMultiset listenerAndHandlers, int windowIndex, @Nullable MediaPeriodId mediaPeriodId, long mediaTimeOffsetMs) { @@ -119,7 +118,7 @@ public class MediaSourceEventDispatcher { /** Dispatches {@code event} to all registered listeners of type {@code listenerClass}. */ @SuppressWarnings("unchecked") // The cast is gated with listenerClass.isInstance() public void dispatch(EventWithPeriodId event, Class listenerClass) { - for (ListenerAndHandler listenerAndHandler : listenerAndHandlers) { + for (ListenerAndHandler listenerAndHandler : listenerAndHandlers.elementSet()) { if (listenerClass.isInstance(listenerAndHandler.listener)) { postOrRun( listenerAndHandler.handler, @@ -151,5 +150,25 @@ public class MediaSourceEventDispatcher { this.handler = handler; this.listener = listener; } + + @Override + public boolean equals(@Nullable Object o) { + if (this == o) { + return true; + } + if (!(o instanceof ListenerAndHandler)) { + return false; + } + + // We deliberately only consider listener (and not handler) in equals() and hashcode() + // because the handler used to process the callbacks is an implementation detail. + ListenerAndHandler that = (ListenerAndHandler) o; + return listener.equals(that.listener); + } + + @Override + public int hashCode() { + return listener.hashCode(); + } } }