commit dba4fc7e5995f501da0ff329806fc9a8031ee4f9
parent 34a4e384fc01a450ac011c7a66fc75f5aabc95b7
Author: Chun-Min Chang <chun.m.chang@gmail.com>
Date: Mon, 15 Dec 2025 20:42:59 +0000
Bug 2005833 - SimpleMap: Enforce key uniqueness and optimize removal r=media-playback-reviewers,padenot
Previously, `SimpleMap` performed unconditional insertions without checking for
duplicate keys. This patch enforces key uniqueness by preventing the insertion
of duplicates.
Guaranteing uniqueness allows the use of `UnorderedRemoveElementAt` instead of
`RemoveElementAt`. This improves removal performance, as preserving relative
entry order is no longer required.
Additionally, this patch adds a `Count` method to retrieve the number of items
in the map and introduces gtests to verify these behavior changes.
Differential Revision: https://phabricator.services.mozilla.com/D276307
Diffstat:
7 files changed, 152 insertions(+), 14 deletions(-)
diff --git a/dom/media/gmp/ChromiumCDMChild.cpp b/dom/media/gmp/ChromiumCDMChild.cpp
@@ -743,6 +743,7 @@ mozilla::ipc::IPCResult ChromiumCDMChild::RecvDecryptAndDecodeFrame(
// CDM's decoder reorders to ensure frames output are in presentation order.
// So we need to store the durations of the frames input, and retrieve them
// on output.
+ MOZ_DIAGNOSTIC_ASSERT(!mFrameDurations.Contains(aBuffer.mTimestamp()));
mFrameDurations.Insert(aBuffer.mTimestamp(), aBuffer.mDuration());
cdm::InputBuffer_2 input = {};
diff --git a/dom/media/gtest/TestSimpleMap.cpp b/dom/media/gtest/TestSimpleMap.cpp
@@ -0,0 +1,111 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "SimpleMap.h"
+#include "gtest/gtest.h"
+#include "nsString.h"
+
+namespace mozilla {
+
+TEST(SimpleMapTest, Insert)
+{
+ SimpleMap<int, nsCString> map;
+
+ // Insert new key-value pair
+ ASSERT_TRUE(map.Insert(1, "first"_ns));
+ ASSERT_EQ(map.Count(), size_t(1));
+
+ // Try to insert duplicate key - should fail
+ ASSERT_FALSE(map.Insert(1, "second"_ns));
+ ASSERT_EQ(map.Count(), size_t(1)); // Count should still be 1
+
+ // Verify original value is preserved
+ Maybe<nsCString> taken = map.Take(1);
+ ASSERT_TRUE(taken.isSome());
+ ASSERT_TRUE(taken.ref().EqualsLiteral("first"));
+ ASSERT_EQ(map.Count(), size_t(0));
+
+ // Verify key is removed
+ Maybe<nsCString> shouldBeNothing = map.Take(1);
+ ASSERT_TRUE(shouldBeNothing.isNothing());
+}
+
+TEST(SimpleMapTest, Find)
+{
+ SimpleMap<int, nsCString> map;
+
+ ASSERT_FALSE(map.Contains(1));
+ ASSERT_TRUE(map.Insert(1, "one"_ns));
+ ASSERT_TRUE(map.Contains(1));
+
+ nsCString value;
+ ASSERT_TRUE(map.Find(1, value));
+ ASSERT_TRUE(value.EqualsLiteral("one"));
+ ASSERT_FALSE(map.Contains(1)); // Find also removes the element.
+ ASSERT_FALSE(map.Find(2, value));
+}
+
+TEST(SimpleMapTest, Take)
+{
+ SimpleMap<int, nsCString> map;
+
+ ASSERT_EQ(map.Count(), size_t(0));
+ map.Insert(1, "one"_ns);
+ ASSERT_EQ(map.Count(), size_t(1));
+ map.Insert(2, "two"_ns);
+ ASSERT_EQ(map.Count(), size_t(2));
+
+ Maybe<nsCString> taken = map.Take(1);
+ ASSERT_TRUE(taken.isSome());
+ ASSERT_TRUE(taken.ref().EqualsLiteral("one"));
+ ASSERT_FALSE(map.Contains(1));
+ ASSERT_TRUE(map.Contains(2));
+ ASSERT_EQ(map.Count(), size_t(1));
+
+ Maybe<nsCString> notTaken = map.Take(3);
+ ASSERT_TRUE(notTaken.isNothing());
+}
+
+TEST(SimpleMapTest, Clear)
+{
+ SimpleMap<int, nsCString> map;
+
+ ASSERT_EQ(map.Count(), size_t(0));
+
+ map.Insert(1, "one"_ns);
+ map.Insert(2, "two"_ns);
+
+ ASSERT_EQ(map.Count(), size_t(2));
+ ASSERT_TRUE(map.Contains(1));
+ ASSERT_TRUE(map.Contains(2));
+
+ map.Clear();
+ ASSERT_FALSE(map.Contains(1));
+ ASSERT_FALSE(map.Contains(2));
+ ASSERT_EQ(map.Count(), size_t(0));
+}
+
+TEST(SimpleMapTest, ForEach)
+{
+ SimpleMap<int, nsCString> map;
+
+ map.Insert(1, "one"_ns);
+ map.Insert(2, "two"_ns);
+ map.Insert(3, "three"_ns);
+ Maybe<nsCString> taken = map.Take(1);
+ ASSERT_TRUE(taken.isSome());
+ ASSERT_TRUE(taken.ref().EqualsLiteral("one"));
+ ASSERT_EQ(map.Count(), size_t(2));
+
+ nsTArray<int> keys;
+ map.ForEach(
+ [&](int key, const nsCString& value) { keys.AppendElement(key); });
+
+ // The order of iteration is not guaranteed.
+ ASSERT_EQ(keys.Length(), size_t(2));
+ ASSERT_TRUE(keys.Contains(2) && keys.Contains(3));
+}
+
+} // namespace mozilla
diff --git a/dom/media/gtest/moz.build b/dom/media/gtest/moz.build
@@ -64,6 +64,7 @@ UNIFIED_SOURCES += [
"TestOggWriter.cpp",
"TestOpusParser.cpp",
"TestRust.cpp",
+ "TestSimpleMap.cpp",
"TestTimeUnit.cpp",
"TestVideoFrameContainer.cpp",
"TestVideoSegment.cpp",
diff --git a/dom/media/platforms/SimpleMap.h b/dom/media/platforms/SimpleMap.h
@@ -47,19 +47,22 @@ class SimpleMap {
// Check if aKey is in the map.
bool Contains(const K& aKey) {
- struct Comparator {
- bool Equals(const ElementType& aElement, const K& aKey) const {
- return aElement.first == aKey;
- }
- };
Policy guard(mLock);
- return mMap.Contains(aKey, Comparator());
+ return FindIndex(aKey).isSome();
}
+
// Insert Key and Value pair at the end of our map.
- void Insert(const K& aKey, const V& aValue) {
+ // Returns true if the insertion succeeded, or false if the key already
+ // exists.
+ bool Insert(const K& aKey, const V& aValue) {
Policy guard(mLock);
+ if (FindIndex(aKey).isSome()) {
+ return false;
+ }
mMap.AppendElement(std::make_pair(aKey, aValue));
+ return true;
}
+
// Sets aValue matching aKey and remove it from the map if found.
// The element returned is the first one found.
// Returns true if found, false otherwise.
@@ -70,24 +73,24 @@ class SimpleMap {
}
return false;
}
+
// Take the value matching aKey and remove it from the map if found.
Maybe<V> Take(const K& aKey) {
Policy guard(mLock);
- for (uint32_t i = 0; i < mMap.Length(); i++) {
- ElementType& element = mMap[i];
- if (element.first == aKey) {
- Maybe<V> value = Some(element.second);
- mMap.RemoveElementAt(i);
- return value;
- }
+ if (Maybe<size_t> index = FindIndex(aKey)) {
+ Maybe<V> value = Some(std::move(mMap[*index].second));
+ mMap.UnorderedRemoveElementAt(*index);
+ return value;
}
return Nothing();
}
+
// Remove all elements of the map.
void Clear() {
Policy guard(mLock);
mMap.Clear();
}
+
// Iterate through all elements of the map and call the function F.
template <typename F>
void ForEach(F&& aCallback) {
@@ -97,7 +100,24 @@ class SimpleMap {
}
}
+ // Return the number of elements in the map.
+ size_t Count() {
+ Policy guard(mLock);
+ return mMap.Length();
+ }
+
private:
+ // Return the index of the first element matching aKey, or Nothing() if not
+ // found.
+ Maybe<size_t> FindIndex(const K& aKey) const {
+ for (size_t i = 0; i < mMap.Length(); ++i) {
+ if (mMap[i].first == aKey) {
+ return Some(i);
+ }
+ }
+ return Nothing();
+ }
+
typename Policy::PolicyLock mLock;
MapType mMap;
};
diff --git a/dom/media/platforms/android/RemoteDataDecoder.cpp b/dom/media/platforms/android/RemoteDataDecoder.cpp
@@ -301,6 +301,8 @@ class RemoteVideoDecoder final : public RemoteDataDecoder {
InputInfo info(aSample->mDuration.ToMicroseconds(), config->mImage,
config->mDisplay);
+ MOZ_DIAGNOSTIC_ASSERT(
+ !mInputInfos.Contains(aSample->mTime.ToMicroseconds()));
mInputInfos.Insert(aSample->mTime.ToMicroseconds(), info);
return RemoteDataDecoder::Decode(aSample);
}
diff --git a/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.h b/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.h
@@ -336,6 +336,7 @@ class FFmpegVideoDecoder<LIBAV_VER>
// after decoding. As such we instead use a map using the given ts as key
// that we will retrieve later. The map will have a typical size of 16
// entry.
+ MOZ_DIAGNOSTIC_ASSERT(!mInputInfo.Contains(GetSampleInputKey(aSample)));
mInputInfo.Insert(GetSampleInputKey(aSample), InputInfo(aSample));
}
diff --git a/dom/media/platforms/ffmpeg/FFmpegVideoEncoder.cpp b/dom/media/platforms/ffmpeg/FFmpegVideoEncoder.cpp
@@ -617,6 +617,7 @@ Result<MediaDataEncoder::EncodedData, MediaResult> FFmpegVideoEncoder<
// Provide fake pts, see header file.
if (mConfig.mCodec == CodecType::AV1) {
mFrame->pts = mFakePts;
+ MOZ_DIAGNOSTIC_ASSERT(!mPtsMap.Contains(mFakePts));
mPtsMap.Insert(mFakePts, aSample->mTime.ToMicroseconds());
mFakePts += aSample->mDuration.ToMicroseconds();
mCurrentFramePts = aSample->mTime.ToMicroseconds();
@@ -627,6 +628,7 @@ Result<MediaDataEncoder::EncodedData, MediaResult> FFmpegVideoEncoder<
mFrame->duration = aSample->mDuration.ToMicroseconds();
# else
// Save duration in the time_base unit.
+ MOZ_DIAGNOSTIC_ASSERT(!mDurationMap.Contains(mFrame->pts));
mDurationMap.Insert(mFrame->pts, aSample->mDuration.ToMicroseconds());
# endif
Duration(mFrame) = aSample->mDuration.ToMicroseconds();
