MediaMetrics: Make submit one-way
Remove the unused session id to improve speed and clarity.
Test: dumpsys media.metrics sanity, mediametrics_tests
Change-Id: Ide5e9218811e110997fed19f43eb6a5fdc5812ec
diff --git a/services/mediaanalytics/MediaAnalyticsService.cpp b/services/mediaanalytics/MediaAnalyticsService.cpp
index f0cb9c1..1ed8b74 100644
--- a/services/mediaanalytics/MediaAnalyticsService.cpp
+++ b/services/mediaanalytics/MediaAnalyticsService.cpp
@@ -66,19 +66,8 @@
mItems.clear();
}
-MediaAnalyticsItem::SessionID_t MediaAnalyticsService::generateUniqueSessionID() {
- return ++mLastSessionID;
-}
-
-// TODO: consider removing forcenew.
-MediaAnalyticsItem::SessionID_t MediaAnalyticsService::submit(
- MediaAnalyticsItem *item, bool forcenew __unused)
+status_t MediaAnalyticsService::submitInternal(MediaAnalyticsItem *item, bool release)
{
- // fill in a sessionID if we do not yet have one
- if (item->getSessionID() <= MediaAnalyticsItem::SessionIDNone) {
- item->setSessionID(generateUniqueSessionID());
- }
-
// we control these, generally not trusting user input
nsecs_t now = systemTime(SYSTEM_TIME_REALTIME);
// round nsecs to seconds
@@ -136,8 +125,8 @@
// validate the record; we discard if we don't like it
if (isContentValid(item, isTrusted) == false) {
- delete item;
- return MediaAnalyticsItem::SessionIDInvalid;
+ if (release) delete item;
+ return PERMISSION_DENIED;
}
// XXX: if we have a sessionid in the new record, look to make
@@ -145,18 +134,17 @@
if (item->count() == 0) {
ALOGV("%s: dropping empty record...", __func__);
- delete item;
- return MediaAnalyticsItem::SessionIDInvalid;
+ if (release) delete item;
+ return BAD_VALUE;
}
// send to statsd
extern bool dump2Statsd(MediaAnalyticsItem *item); // extern hook
(void)dump2Statsd(item); // failure should be logged in function.
- // keep our copy
- const MediaAnalyticsItem::SessionID_t id = item->getSessionID();
+ if (!release) item = item->dup();
saveItem(item);
- return id;
+ return NO_ERROR;
}
status_t MediaAnalyticsService::dump(int fd, const Vector<String16>& args)
diff --git a/services/mediaanalytics/MediaAnalyticsService.h b/services/mediaanalytics/MediaAnalyticsService.h
index ed7b7b1..eb7d725 100644
--- a/services/mediaanalytics/MediaAnalyticsService.h
+++ b/services/mediaanalytics/MediaAnalyticsService.h
@@ -35,31 +35,28 @@
~MediaAnalyticsService() override;
/**
- * Submits the indicated record to the mediaanalytics service, where
- * it will be merged (if appropriate) with incomplete records that
- * share the same key and sessionid.
+ * Submits the indicated record to the mediaanalytics service.
*
* \param item the item to submit.
- * \param forcenew marks any matching incomplete record as complete before
- * inserting this new record.
- *
- * \return the sessionID associated with that item or
- * MediaAnalyticsItem::SessionIDInvalid on failure.
- *
- * BEWARE: When called directly on the service (not from the binder interface),
- * the caller surrenders ownership of item, MediaAnalyticsService will delete
- * even on error. The binder interface does not take ownership.
- * TODO: fix this inconsistency with the binder RPC interface.
+ * \return status failure, which is negative on binder transaction failure.
+ * As the transaction is one-way, remote failures will not be reported.
*/
- MediaAnalyticsItem::SessionID_t submit(MediaAnalyticsItem *item, bool forcenew) override;
+ status_t submit(MediaAnalyticsItem *item) override {
+ return submitInternal(item, false /* release */);
+ }
status_t dump(int fd, const Vector<String16>& args) override;
static constexpr const char * const kServiceName = "media.metrics";
+protected:
+
+ // Internal call where release is true if ownership of item is transferred
+ // to the service (that is, the service will eventually delete the item).
+ status_t submitInternal(MediaAnalyticsItem *item, bool release) override;
+
private:
void processExpirations();
- MediaAnalyticsItem::SessionID_t generateUniqueSessionID();
// input validation after arrival from client
static bool isContentValid(const MediaAnalyticsItem *item, bool isTrusted);
bool isRateLimited(MediaAnalyticsItem *) const;
@@ -86,8 +83,6 @@
const size_t mMaxRecordsExpiredAtOnce;
const int mDumpProtoDefault;
- std::atomic<MediaAnalyticsItem::SessionID_t> mLastSessionID{};
-
class UidInfo {
public:
void setPkgInfo(MediaAnalyticsItem *item, uid_t uid, bool setName, bool setVersion);
diff --git a/services/mediaanalytics/tests/mediametrics_tests.cpp b/services/mediaanalytics/tests/mediametrics_tests.cpp
index 794b2f0..ea7739b 100644
--- a/services/mediaanalytics/tests/mediametrics_tests.cpp
+++ b/services/mediaanalytics/tests/mediametrics_tests.cpp
@@ -30,26 +30,25 @@
sp mediaMetrics = new MediaAnalyticsService();
status_t status;
- // NOTE: submission of items to MediaMetrics releases ownership, even on error.
-
// random keys ignored when empty
- status = mediaMetrics->submit(MediaAnalyticsItem::create("random_key"), false);
- ASSERT_EQ(MediaAnalyticsItem::SessionIDInvalid, status);
+ std::unique_ptr<MediaAnalyticsItem> random_key(MediaAnalyticsItem::create("random_key"));
+ status = mediaMetrics->submit(random_key.get());
+ ASSERT_EQ(PERMISSION_DENIED, status);
// random keys ignored with data
- auto random_key = MediaAnalyticsItem::create("random_key");
random_key->setInt32("foo", 10);
- status = mediaMetrics->submit(random_key, false);
- ASSERT_EQ(MediaAnalyticsItem::SessionIDInvalid, status);
+ status = mediaMetrics->submit(random_key.get());
+ ASSERT_EQ(PERMISSION_DENIED, status);
// known keys ignored if empty
- status = mediaMetrics->submit(MediaAnalyticsItem::create("audiotrack"), false);
- ASSERT_EQ(MediaAnalyticsItem::SessionIDInvalid, status);
+ std::unique_ptr<MediaAnalyticsItem> audiotrack_key(MediaAnalyticsItem::create("audiotrack"));
+ status = mediaMetrics->submit(audiotrack_key.get());
+ ASSERT_EQ(BAD_VALUE, status);
- auto audiotrack = MediaAnalyticsItem::create("audiotrack");
- audiotrack->addInt32("foo", 10);
- status = mediaMetrics->submit(audiotrack, false);
- ASSERT_GT(status, MediaAnalyticsItem::SessionIDNone);
+ // known keys not ignored if not empty
+ audiotrack_key->addInt32("foo", 10);
+ status = mediaMetrics->submit(audiotrack_key.get());
+ ASSERT_EQ(NO_ERROR, status);
mediaMetrics->dump(fileno(stdout), {} /* args */);
}