Add support for combining and remove individual resource

- Fix ResourceManagerSerivce handling of adding same-type
resource in multiple calls, combine entries with the same
<type, subtype>. This is not required in existing usage,
bug could be required if we allow same type to be added
incrementally (eg. memory usage).

- Add api for removing some resources. Currently we can
add resources in multiple calls, but removeResource always
remove all resources of that client. We need this to toggle
battery stat On/Off based on actual usage of the codec.

- Add unit tests for the above.

bug: 138381810
test: ResourceManagerService_test; CTS ResourceManangerTest.

Change-Id: Icdba6c6b4c517755291668c52764169aac3071ea

media/libmedia/IResourceManagerService.cpp

@@ -32,6 +32,7 @@ enum {
     CONFIG = IBinder::FIRST_CALL_TRANSACTION,
     ADD_RESOURCE,
     REMOVE_RESOURCE,
+    REMOVE_CLIENT,
     RECLAIM_RESOURCE,
 };
 
@@ -87,15 +88,25 @@ public:
         remote()->transact(ADD_RESOURCE, data, &reply);
     }
 
-    virtual void removeResource(int pid, int64_t clientId) {
+    virtual void removeResource(int pid, int64_t clientId, const Vector<MediaResource> &resources) {
         Parcel data, reply;
         data.writeInterfaceToken(IResourceManagerService::getInterfaceDescriptor());
         data.writeInt32(pid);
         data.writeInt64(clientId);
+        writeToParcel(&data, resources);
 
         remote()->transact(REMOVE_RESOURCE, data, &reply);
     }
 
+    virtual void removeClient(int pid, int64_t clientId) {
+        Parcel data, reply;
+        data.writeInterfaceToken(IResourceManagerService::getInterfaceDescriptor());
+        data.writeInt32(pid);
+        data.writeInt64(clientId);
+
+        remote()->transact(REMOVE_CLIENT, data, &reply);
+    }
+
     virtual bool reclaimResource(int callingPid, const Vector<MediaResource> &resources) {
         Parcel data, reply;
         data.writeInterfaceToken(IResourceManagerService::getInterfaceDescriptor());
@@ -148,7 +159,17 @@ status_t BnResourceManagerService::onTransact(
             CHECK_INTERFACE(IResourceManagerService, data, reply);
             int pid = data.readInt32();
             int64_t clientId = data.readInt64();
-            removeResource(pid, clientId);
+            Vector<MediaResource> resources;
+            readFromParcel(data, &resources);
+            removeResource(pid, clientId, resources);
+            return NO_ERROR;
+        } break;
+
+        case REMOVE_CLIENT: {
+            CHECK_INTERFACE(IResourceManagerService, data, reply);
+            int pid = data.readInt32();
+            int64_t clientId = data.readInt64();
+            removeClient(pid, clientId);
             return NO_ERROR;
         } break;
 

media/libmedia/include/media/IResourceManagerService.h

@@ -44,7 +44,10 @@ public:
             const sp<IResourceManagerClient> client,
             const Vector<MediaResource> &resources) = 0;
 
-    virtual void removeResource(int pid, int64_t clientId) = 0;
+    virtual void removeResource(int pid, int64_t clientId,
+            const Vector<MediaResource> &resources) = 0;
+
+    virtual void removeClient(int pid, int64_t clientId) = 0;
 
     virtual bool reclaimResource(
             int callingPid,

media/libmedia/include/media/MediaResource.h

@@ -63,6 +63,8 @@ inline static const char *asString(MediaResource::Type i, const char *def = "??"
         case MediaResource::kSecureCodec:    return "secure-codec";
         case MediaResource::kNonSecureCodec: return "non-secure-codec";
         case MediaResource::kGraphicMemory:  return "graphic-memory";
+        case MediaResource::kCpuBoost:       return "cpu-boost";
+        case MediaResource::kBattery:        return "battery";
         default:                             return def;
     }
 }

media/libstagefright/MediaCodec.cpp

@@ -212,7 +212,7 @@ void MediaCodec::ResourceManagerServiceProxy::removeResource(int64_t clientId) {
     if (mService == NULL) {
         return;
     }
-    mService->removeResource(mPid, clientId);
+    mService->removeClient(mPid, clientId);
 }
 
 bool MediaCodec::ResourceManagerServiceProxy::reclaimResource(

services/mediaresourcemanager/ResourceManagerService.cpp

@@ -71,9 +71,9 @@ static String8 getString(const Vector<T> &items) {
     return itemsStr;
 }
 
-static bool hasResourceType(MediaResource::Type type, const Vector<MediaResource>& resources) {
-    for (size_t i = 0; i < resources.size(); ++i) {
-        if (resources[i].mType == type) {
+static bool hasResourceType(MediaResource::Type type, const ResourceList& resources) {
+    for (auto it = resources.begin(); it != resources.end(); it++) {
+        if (it->second.mType == type) {
             return true;
         }
     }
@@ -107,19 +107,18 @@ static ResourceInfo& getResourceInfoForEdit(
         int64_t clientId,
         const sp<IResourceManagerClient>& client,
         ResourceInfos& infos) {
-    for (size_t i = 0; i < infos.size(); ++i) {
-        if (infos[i].clientId == clientId) {
-            return infos.editItemAt(i);
-        }
+    ssize_t index = infos.indexOfKey(clientId);
+
+    if (index < 0) {
+        ResourceInfo info;
+        info.uid = uid;
+        info.clientId = clientId;
+        info.client = client;
+
+        index = infos.add(clientId, info);
     }
-    ResourceInfo info;
-    info.uid = uid;
-    info.clientId = clientId;
-    info.client = client;
-    info.cpuBoost = false;
-    info.batteryNoted = false;
-    infos.push_back(info);
-    return infos.editItemAt(infos.size() - 1);
+
+    return infos.editValueAt(index);
 }
 
 static void notifyResourceGranted(int pid, const Vector<MediaResource> &resources) {
@@ -186,10 +185,10 @@ status_t ResourceManagerService::dump(int fd, const Vector<String16>& /* args */
             snprintf(buffer, SIZE, "        Name: %s\n", infos[j].client->getName().string());
             result.append(buffer);
 
-            Vector<MediaResource> resources = infos[j].resources;
+            const ResourceList &resources = infos[j].resources;
             result.append("        Resources:\n");
-            for (size_t k = 0; k < resources.size(); ++k) {
-                snprintf(buffer, SIZE, "          %s\n", resources[k].toString().string());
+            for (auto it = resources.begin(); it != resources.end(); it++) {
+                snprintf(buffer, SIZE, "          %s\n", it->second.toString().string());
                 result.append(buffer);
             }
         }
@@ -231,6 +230,38 @@ void ResourceManagerService::config(const Vector<MediaResourcePolicy> &policies)
     }
 }
 
+void ResourceManagerService::onFirstAdded(
+        const MediaResource& resource, const ResourceInfo& clientInfo) {
+    // first time added
+    if (resource.mType == MediaResource::kCpuBoost
+     && resource.mSubType == MediaResource::kUnspecifiedSubType) {
+        // Request it on every new instance of kCpuBoost, as the media.codec
+        // could have died, if we only do it the first time subsequent instances
+        // never gets the boost.
+        if (requestCpusetBoost(true, this) != OK) {
+            ALOGW("couldn't request cpuset boost");
+        }
+        mCpuBoostCount++;
+    } else if (resource.mType == MediaResource::kBattery
+            && resource.mSubType == MediaResource::kVideoCodec) {
+        BatteryNotifier::getInstance().noteStartVideo(clientInfo.uid);
+    }
+}
+
+void ResourceManagerService::onLastRemoved(
+        const MediaResource& resource, const ResourceInfo& clientInfo) {
+    if (resource.mType == MediaResource::kCpuBoost
+            && resource.mSubType == MediaResource::kUnspecifiedSubType
+            && mCpuBoostCount > 0) {
+        if (--mCpuBoostCount == 0) {
+            requestCpusetBoost(false, this);
+        }
+    } else if (resource.mType == MediaResource::kBattery
+            && resource.mSubType == MediaResource::kVideoCodec) {
+        BatteryNotifier::getInstance().noteStopVideo(clientInfo.uid);
+    }
+}
+
 void ResourceManagerService::addResource(
         int pid,
         int uid,
@@ -248,24 +279,14 @@ void ResourceManagerService::addResource(
     }
     ResourceInfos& infos = getResourceInfosForEdit(pid, mMap);
     ResourceInfo& info = getResourceInfoForEdit(uid, clientId, client, infos);
-    // TODO: do the merge instead of append.
-    info.resources.appendVector(resources);
 
     for (size_t i = 0; i < resources.size(); ++i) {
-        if (resources[i].mType == MediaResource::kCpuBoost && !info.cpuBoost) {
-            info.cpuBoost = true;
-            // Request it on every new instance of kCpuBoost, as the media.codec
-            // could have died, if we only do it the first time subsequent instances
-            // never gets the boost.
-            if (requestCpusetBoost(true, this) != OK) {
-                ALOGW("couldn't request cpuset boost");
-            }
-            mCpuBoostCount++;
-        } else if (resources[i].mType == MediaResource::kBattery
-                && resources[i].mSubType == MediaResource::kVideoCodec
-                && !info.batteryNoted) {
-            info.batteryNoted = true;
-            BatteryNotifier::getInstance().noteStartVideo(info.uid);
+        const auto resType = std::make_pair(resources[i].mType, resources[i].mSubType);
+        if (info.resources.find(resType) == info.resources.end()) {
+            onFirstAdded(resources[i], info);
+            info.resources[resType] = resources[i];
+        } else {
+            info.resources[resType].mValue += resources[i].mValue;
         }
     }
     if (info.deathNotifier == nullptr) {
@@ -275,7 +296,48 @@ void ResourceManagerService::addResource(
     notifyResourceGranted(pid, resources);
 }
 
-void ResourceManagerService::removeResource(int pid, int64_t clientId) {
+void ResourceManagerService::removeResource(int pid, int64_t clientId,
+        const Vector<MediaResource> &resources) {
+    String8 log = String8::format("removeResource(pid %d, clientId %lld, resources %s)",
+            pid, (long long) clientId, getString(resources).string());
+    mServiceLog->add(log);
+
+    Mutex::Autolock lock(mLock);
+    if (!mProcessInfo->isValidPid(pid)) {
+        ALOGE("Rejected removeResource call with invalid pid.");
+        return;
+    }
+    ssize_t index = mMap.indexOfKey(pid);
+    if (index < 0) {
+        ALOGV("removeResource: didn't find pid %d for clientId %lld", pid, (long long) clientId);
+        return;
+    }
+    ResourceInfos &infos = mMap.editValueAt(index);
+
+    index = infos.indexOfKey(clientId);
+    if (index < 0) {
+        ALOGV("removeResource: didn't find clientId %lld", (long long) clientId);
+        return;
+    }
+
+    ResourceInfo &info = infos.editValueAt(index);
+
+    for (size_t i = 0; i < resources.size(); ++i) {
+        const auto resType = std::make_pair(resources[i].mType, resources[i].mSubType);
+        // ignore if we don't have it
+        if (info.resources.find(resType) != info.resources.end()) {
+            MediaResource &resource = info.resources[resType];
+            if (resource.mValue > resources[i].mValue) {
+                resource.mValue -= resources[i].mValue;
+            } else {
+                onLastRemoved(resources[i], info);
+                info.resources.erase(resType);
+            }
+        }
+    }
+}
+
+void ResourceManagerService::removeClient(int pid, int64_t clientId) {
     removeResource(pid, clientId, true);
 }
 
@@ -295,27 +357,22 @@ void ResourceManagerService::removeResource(int pid, int64_t clientId, bool chec
         ALOGV("removeResource: didn't find pid %d for clientId %lld", pid, (long long) clientId);
         return;
     }
-    bool found = false;
     ResourceInfos &infos = mMap.editValueAt(index);
-    for (size_t j = 0; j < infos.size(); ++j) {
-        if (infos[j].clientId == clientId) {
-            if (infos[j].cpuBoost && mCpuBoostCount > 0) {
-                if (--mCpuBoostCount == 0) {
-                    requestCpusetBoost(false, this);
-                }
-            }
-            if (infos[j].batteryNoted) {
-                BatteryNotifier::getInstance().noteStopVideo(infos[j].uid);
-            }
-            IInterface::asBinder(infos[j].client)->unlinkToDeath(infos[j].deathNotifier);
-            j = infos.removeAt(j);
-            found = true;
-            break;
-        }
+
+    index = infos.indexOfKey(clientId);
+    if (index < 0) {
+        ALOGV("removeResource: didn't find clientId %lld", (long long) clientId);
+        return;
     }
-    if (!found) {
-        ALOGV("didn't find client");
+
+    const ResourceInfo &info = infos[index];
+    for (auto it = info.resources.begin(); it != info.resources.end(); it++) {
+        onLastRemoved(it->second, info);
     }
+
+    IInterface::asBinder(info.client)->unlinkToDeath(info.deathNotifier);
+
+    infos.removeItemsAt(index);
 }
 
 void ResourceManagerService::getClientForResource_l(
@@ -426,7 +483,7 @@ bool ResourceManagerService::reclaimResource(
             ResourceInfos &infos = mMap.editValueAt(i);
             for (size_t j = 0; j < infos.size();) {
                 if (infos[j].client == failedClient) {
-                    j = infos.removeAt(j);
+                    j = infos.removeItemsAt(j);
                     found = true;
                 } else {
                     ++j;
@@ -554,11 +611,12 @@ bool ResourceManagerService::getBiggestClient_l(
     uint64_t largestValue = 0;
     const ResourceInfos &infos = mMap.valueAt(index);
     for (size_t i = 0; i < infos.size(); ++i) {
-        Vector<MediaResource> resources = infos[i].resources;
-        for (size_t j = 0; j < resources.size(); ++j) {
-            if (resources[j].mType == type) {
-                if (resources[j].mValue > largestValue) {
-                    largestValue = resources[j].mValue;
+        const ResourceList &resources = infos[i].resources;
+        for (auto it = resources.begin(); it != resources.end(); it++) {
+            const MediaResource &resource = it->second;
+            if (resource.mType == type) {
+                if (resource.mValue > largestValue) {
+                    largestValue = resource.mValue;
                     clientTemp = infos[i].client;
                 }
             }

services/mediaresourcemanager/ResourceManagerService.h

@@ -33,17 +33,17 @@ namespace android {
 class ServiceLog;
 struct ProcessInfoInterface;
 
+typedef std::map<std::pair<MediaResource::Type, MediaResource::SubType>, MediaResource> ResourceList;
 struct ResourceInfo {
     int64_t clientId;
     uid_t uid;
     sp<IResourceManagerClient> client;
     sp<IBinder::DeathRecipient> deathNotifier;
-    Vector<MediaResource> resources;
-    bool cpuBoost;
-    bool batteryNoted;
+    ResourceList resources;
 };
 
-typedef Vector<ResourceInfo> ResourceInfos;
+// TODO: convert these to std::map
+typedef KeyedVector<int64_t, ResourceInfo> ResourceInfos;
 typedef KeyedVector<int, ResourceInfos> PidResourceInfosMap;
 
 class ResourceManagerService
@@ -68,7 +68,10 @@ public:
             const sp<IResourceManagerClient> client,
             const Vector<MediaResource> &resources);
 
-    virtual void removeResource(int pid, int64_t clientId);
+    virtual void removeResource(int pid, int64_t clientId,
+            const Vector<MediaResource> &resources);
+
+    virtual void removeClient(int pid, int64_t clientId);
 
     // Tries to reclaim resource from processes with lower priority than the calling process
     // according to the requested resources.
@@ -110,6 +113,9 @@ private:
     void getClientForResource_l(
         int callingPid, const MediaResource *res, Vector<sp<IResourceManagerClient>> *clients);
 
+    void onFirstAdded(const MediaResource& res, const ResourceInfo& clientInfo);
+    void onLastRemoved(const MediaResource& res, const ResourceInfo& clientInfo);
+
     mutable Mutex mLock;
     sp<ProcessInfoInterface> mProcessInfo;
     sp<ServiceLog> mServiceLog;

services/mediaresourcemanager/test/ResourceManagerService_test.cpp

@@ -58,7 +58,7 @@ struct TestClient : public BnResourceManagerClient {
 
     virtual bool reclaimResource() {
         sp<IResourceManagerClient> client(this);
-        mService->removeResource(mPid, (int64_t) client.get());
+        mService->removeClient(mPid, (int64_t) client.get());
         mReclaimed = true;
         return true;
     }
@@ -106,16 +106,14 @@ public:
 
 protected:
     static bool isEqualResources(const Vector<MediaResource> &resources1,
-            const Vector<MediaResource> &resources2) {
-        if (resources1.size() != resources2.size()) {
-            return false;
-        }
+            const ResourceList &resources2) {
+        // convert resource1 to ResourceList
+        ResourceList r1;
         for (size_t i = 0; i < resources1.size(); ++i) {
-            if (resources1[i] != resources2[i]) {
-                return false;
-            }
+            const auto resType = std::make_pair(resources1[i].mType, resources1[i].mSubType);
+            r1[resType] = resources1[i];
         }
-        return true;
+        return r1 == resources2;
     }
 
     static void expectEqResourceInfo(const ResourceInfo &info,
@@ -184,14 +182,14 @@ protected:
         ASSERT_GE(index1, 0);
         const ResourceInfos &infos1 = map[index1];
         EXPECT_EQ(1u, infos1.size());
-        expectEqResourceInfo(infos1[0], kTestUid1, mTestClient1, resources1);
+        expectEqResourceInfo(infos1.valueFor(getId(mTestClient1)), kTestUid1, mTestClient1, resources1);
 
         ssize_t index2 = map.indexOfKey(kTestPid2);
         ASSERT_GE(index2, 0);
         const ResourceInfos &infos2 = map[index2];
         EXPECT_EQ(2u, infos2.size());
-        expectEqResourceInfo(infos2[0], kTestUid2, mTestClient2, resources2);
-        expectEqResourceInfo(infos2[1], kTestUid2, mTestClient3, resources3);
+        expectEqResourceInfo(infos2.valueFor(getId(mTestClient2)), kTestUid2, mTestClient2, resources2);
+        expectEqResourceInfo(infos2.valueFor(getId(mTestClient3)), kTestUid2, mTestClient3, resources3);
     }
 
     void testConfig() {
@@ -225,10 +223,84 @@ protected:
         EXPECT_TRUE(mService->mSupportsSecureWithNonSecureCodec);
     }
 
+    void testCombineResource() {
+        // kTestPid1 mTestClient1
+        Vector<MediaResource> resources1;
+        resources1.push_back(MediaResource(MediaResource::kSecureCodec, 1));
+        mService->addResource(kTestPid1, kTestUid1, getId(mTestClient1), mTestClient1, resources1);
+
+        Vector<MediaResource> resources11;
+        resources11.push_back(MediaResource(MediaResource::kGraphicMemory, 200));
+        mService->addResource(kTestPid1, kTestUid1, getId(mTestClient1), mTestClient1, resources11);
+
+        const PidResourceInfosMap &map = mService->mMap;
+        EXPECT_EQ(1u, map.size());
+        ssize_t index1 = map.indexOfKey(kTestPid1);
+        ASSERT_GE(index1, 0);
+        const ResourceInfos &infos1 = map[index1];
+        EXPECT_EQ(1u, infos1.size());
+
+        // test adding existing types to combine values
+        resources1.push_back(MediaResource(MediaResource::kGraphicMemory, 100));
+        mService->addResource(kTestPid1, kTestUid1, getId(mTestClient1), mTestClient1, resources1);
+
+        Vector<MediaResource> expected;
+        expected.push_back(MediaResource(MediaResource::kSecureCodec, 2));
+        expected.push_back(MediaResource(MediaResource::kGraphicMemory, 300));
+        expectEqResourceInfo(infos1.valueFor(getId(mTestClient1)), kTestUid1, mTestClient1, expected);
+
+        // test adding new types (including types that differs only in subType)
+        resources11.push_back(MediaResource(MediaResource::kNonSecureCodec, 1));
+        resources11.push_back(MediaResource(MediaResource::kSecureCodec, MediaResource::kVideoCodec, 1));
+        mService->addResource(kTestPid1, kTestUid1, getId(mTestClient1), mTestClient1, resources11);
+
+        expected.clear();
+        expected.push_back(MediaResource(MediaResource::kSecureCodec, 2));
+        expected.push_back(MediaResource(MediaResource::kNonSecureCodec, 1));
+        expected.push_back(MediaResource(MediaResource::kSecureCodec, MediaResource::kVideoCodec, 1));
+        expected.push_back(MediaResource(MediaResource::kGraphicMemory, 500));
+        expectEqResourceInfo(infos1.valueFor(getId(mTestClient1)), kTestUid1, mTestClient1, expected);
+    }
+
     void testRemoveResource() {
+        // kTestPid1 mTestClient1
+        Vector<MediaResource> resources1;
+        resources1.push_back(MediaResource(MediaResource::kSecureCodec, 1));
+        mService->addResource(kTestPid1, kTestUid1, getId(mTestClient1), mTestClient1, resources1);
+
+        Vector<MediaResource> resources11;
+        resources11.push_back(MediaResource(MediaResource::kGraphicMemory, 200));
+        mService->addResource(kTestPid1, kTestUid1, getId(mTestClient1), mTestClient1, resources11);
+
+        const PidResourceInfosMap &map = mService->mMap;
+        EXPECT_EQ(1u, map.size());
+        ssize_t index1 = map.indexOfKey(kTestPid1);
+        ASSERT_GE(index1, 0);
+        const ResourceInfos &infos1 = map[index1];
+        EXPECT_EQ(1u, infos1.size());
+
+        // test partial removal
+        resources11.editItemAt(0).mValue = 100;
+        mService->removeResource(kTestPid1, getId(mTestClient1), resources11);
+
+        Vector<MediaResource> expected;
+        expected.push_back(MediaResource(MediaResource::kSecureCodec, 1));
+        expected.push_back(MediaResource(MediaResource::kGraphicMemory, 100));
+        expectEqResourceInfo(infos1.valueFor(getId(mTestClient1)), kTestUid1, mTestClient1, expected);
+
+        // test complete removal with overshoot value
+        resources11.editItemAt(0).mValue = 1000;
+        mService->removeResource(kTestPid1, getId(mTestClient1), resources11);
+
+        expected.clear();
+        expected.push_back(MediaResource(MediaResource::kSecureCodec, 1));
+        expectEqResourceInfo(infos1.valueFor(getId(mTestClient1)), kTestUid1, mTestClient1, expected);
+    }
+
+    void testRemoveClient() {
         addResource();
 
-        mService->removeResource(kTestPid2, getId(mTestClient2));
+        mService->removeClient(kTestPid2, getId(mTestClient2));
 
         const PidResourceInfosMap &map = mService->mMap;
         EXPECT_EQ(2u, map.size());
@@ -237,6 +309,7 @@ protected:
         EXPECT_EQ(1u, infos1.size());
         EXPECT_EQ(1u, infos2.size());
         // mTestClient2 has been removed.
+        // (OK to use infos2[0] as there is only 1 entry)
         EXPECT_EQ(mTestClient3, infos2[0].client);
     }
 
@@ -252,6 +325,7 @@ protected:
         EXPECT_TRUE(mService->getAllClients_l(kHighPriorityPid, type, &clients));
 
         EXPECT_EQ(2u, clients.size());
+        // (OK to require ordering in clients[], as the pid map is sorted)
         EXPECT_EQ(mTestClient3, clients[0]);
         EXPECT_EQ(mTestClient1, clients[1]);
     }
@@ -444,7 +518,7 @@ protected:
             verifyClients(true /* c1 */, false /* c2 */, false /* c3 */);
 
             // clean up client 3 which still left
-            mService->removeResource(kTestPid2, getId(mTestClient3));
+            mService->removeClient(kTestPid2, getId(mTestClient3));
         }
     }
 
@@ -518,10 +592,18 @@ TEST_F(ResourceManagerServiceTest, addResource) {
     addResource();
 }
 
+TEST_F(ResourceManagerServiceTest, combineResource) {
+    testCombineResource();
+}
+
 TEST_F(ResourceManagerServiceTest, removeResource) {
     testRemoveResource();
 }
 
+TEST_F(ResourceManagerServiceTest, removeClient) {
+    testRemoveClient();
+}
+
 TEST_F(ResourceManagerServiceTest, reclaimResource) {
     testReclaimResourceSecure();
     testReclaimResourceNonSecure();