Expand nuplayer mutex for mediametrics management
refactor some mutex for how nuplayer sets up mediametrics data.
expanded locking to eliminate a couple race conditions.
Bug: 151644303
Bug: 151643722
Test: poc attached to bugs
Merged-In: I75f29a6254c5eab5d4f524ee7a7ef59f93a0b405
Merged-In: Ia2e68ef616e249a6e8746b9068f22bd208a0ffc8
Merged-In: I1e9bbcd67a1510f70fad66e8ef77f529008e248a
Change-Id: I9c473e47e7eb8dce24399bcb24f82a3d2d583983
(cherry picked from commit 8220d3f72c42befa948737ae4bd6b3fcc5fb2927)
diff --git a/media/libmediaplayerservice/nuplayer/NuPlayerDriver.cpp b/media/libmediaplayerservice/nuplayer/NuPlayerDriver.cpp
index dc29761..a512a0b 100644
--- a/media/libmediaplayerservice/nuplayer/NuPlayerDriver.cpp
+++ b/media/libmediaplayerservice/nuplayer/NuPlayerDriver.cpp
@@ -98,6 +98,7 @@
updateMetrics("destructor");
logMetrics("destructor");
+ Mutex::Autolock autoLock(mMetricsLock);
if (mAnalyticsItem != NULL) {
delete mAnalyticsItem;
mAnalyticsItem = NULL;
@@ -521,12 +522,34 @@
if (where == NULL) {
where = "unknown";
}
- ALOGV("updateMetrics(%p) from %s at state %d", this, where, mState);
+ ALOGD("updateMetrics(%p) from %s at state %d", this, where, mState);
- // gather the final stats for this record
+ // gather the final track statistics for this record
Vector<sp<AMessage>> trackStats;
mPlayer->getStats(&trackStats);
+ // getDuration() uses mLock
+ int duration_ms = -1;
+ getDuration(&duration_ms);
+
+ int64_t playingTimeUs;
+ {
+ Mutex::Autolock autoLock(mLock);
+
+ playingTimeUs = mPlayingTimeUs;
+ }
+
+ // finish the rest of the gathering holding mMetricsLock;
+ // to avoid any races within mediametrics machinery
+ Mutex::Autolock autoLock(mMetricsLock);
+
+ mAnalyticsItem->setInt64(kPlayerDuration, duration_ms);
+
+ // these update under mLock, we'll read them under mLock
+ mAnalyticsItem->setInt64(kPlayerPlaying, (playingTimeUs+500)/1000 );
+
+ mAnalyticsItem->setCString(kPlayerDataSourceType, mPlayer->getDataSourceType());
+
if (trackStats.size() > 0) {
for (size_t i = 0; i < trackStats.size(); ++i) {
const sp<AMessage> &stats = trackStats.itemAt(i);
@@ -567,17 +590,6 @@
}
}
}
-
- // always provide duration and playing time, even if they have 0/unknown values.
-
- // getDuration() uses mLock for mutex -- careful where we use it.
- int duration_ms = -1;
- getDuration(&duration_ms);
- mAnalyticsItem->setInt64(kPlayerDuration, duration_ms);
-
- mAnalyticsItem->setInt64(kPlayerPlaying, (mPlayingTimeUs+500)/1000 );
-
- mAnalyticsItem->setCString(kPlayerDataSourceType, mPlayer->getDataSourceType());
}
@@ -587,6 +599,9 @@
}
ALOGV("logMetrics(%p) from %s at state %d", this, where, mState);
+ // make sure that the stats are stable while we're writing them.
+ Mutex::Autolock autoLock(mMetricsLock);
+
if (mAnalyticsItem == NULL || mAnalyticsItem->isEnabled() == false) {
return;
}
@@ -738,6 +753,9 @@
// mtrX -- a play on 'metrics' (not matrix)
// gather current info all together, parcel it, and send it back
updateMetrics("api");
+
+ // make sure that the stats are static while we're writing to the parcel
+ Mutex::Autolock autoLock(mMetricsLock);
mAnalyticsItem->writeToParcel(reply);
return OK;
}
@@ -952,9 +970,12 @@
// ext1 is our primary 'error type' value. Only add ext2 when non-zero.
// [test against msg is due to fall through from previous switch value]
if (msg == MEDIA_ERROR) {
- mAnalyticsItem->setInt32(kPlayerError, ext1);
- if (ext2 != 0) {
- mAnalyticsItem->setInt32(kPlayerErrorCode, ext2);
+ Mutex::Autolock autoLock(mMetricsLock);
+ if (mAnalyticsItem != NULL) {
+ mAnalyticsItem->setInt32(kPlayerError, ext1);
+ if (ext2 != 0) {
+ mAnalyticsItem->setInt32(kPlayerErrorCode, ext2);
+ }
}
}
mAtEOS = true;
diff --git a/media/libmediaplayerservice/nuplayer/NuPlayerDriver.h b/media/libmediaplayerservice/nuplayer/NuPlayerDriver.h
index d0cf1dd..327f21b 100644
--- a/media/libmediaplayerservice/nuplayer/NuPlayerDriver.h
+++ b/media/libmediaplayerservice/nuplayer/NuPlayerDriver.h
@@ -133,6 +133,7 @@
MediaAnalyticsItem *mAnalyticsItem;
uid_t mClientUid;
+ mutable Mutex mMetricsLock;
bool mAtEOS;
bool mLooping;
