From fb092d3b720923efaf0f9d4f4c16c57da6d657ac Mon Sep 17 00:00:00 2001 From: Chong Zhang Date: Mon, 12 Aug 2019 09:45:44 -0700 Subject: [PATCH] Add support for combining and remove individual resource - Fix ResourceManagerSerivce handling of adding same-type resource in multiple calls, combine entries with the same . 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 | 25 ++- .../include/media/IResourceManagerService.h | 5 +- media/libmedia/include/media/MediaResource.h | 2 + media/libstagefright/MediaCodec.cpp | 2 +- .../ResourceManagerService.cpp | 176 ++++++++++++------ .../ResourceManagerService.h | 16 +- .../test/ResourceManagerService_test.cpp | 110 +++++++++-- 7 files changed, 254 insertions(+), 82 deletions(-) diff --git a/media/libmedia/IResourceManagerService.cpp b/media/libmedia/IResourceManagerService.cpp index 00f9b88aad..f8a0a143e1 100644 --- a/media/libmedia/IResourceManagerService.cpp +++ b/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 @@ class BpResourceManagerService : public BpInterface remote()->transact(ADD_RESOURCE, data, &reply); } - virtual void removeResource(int pid, int64_t clientId) { + virtual void removeResource(int pid, int64_t clientId, const Vector &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 &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 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; diff --git a/media/libmedia/include/media/IResourceManagerService.h b/media/libmedia/include/media/IResourceManagerService.h index 404519b2a6..8992f8bc15 100644 --- a/media/libmedia/include/media/IResourceManagerService.h +++ b/media/libmedia/include/media/IResourceManagerService.h @@ -44,7 +44,10 @@ class IResourceManagerService: public IInterface const sp client, const Vector &resources) = 0; - virtual void removeResource(int pid, int64_t clientId) = 0; + virtual void removeResource(int pid, int64_t clientId, + const Vector &resources) = 0; + + virtual void removeClient(int pid, int64_t clientId) = 0; virtual bool reclaimResource( int callingPid, diff --git a/media/libmedia/include/media/MediaResource.h b/media/libmedia/include/media/MediaResource.h index 10d0e3b1b1..10a07bb639 100644 --- a/media/libmedia/include/media/MediaResource.h +++ b/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; } } diff --git a/media/libstagefright/MediaCodec.cpp b/media/libstagefright/MediaCodec.cpp index a6a856c3f8..4b52591aea 100644 --- a/media/libstagefright/MediaCodec.cpp +++ b/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( diff --git a/services/mediaresourcemanager/ResourceManagerService.cpp b/services/mediaresourcemanager/ResourceManagerService.cpp index 117a2114e1..5a52b3d5dc 100644 --- a/services/mediaresourcemanager/ResourceManagerService.cpp +++ b/services/mediaresourcemanager/ResourceManagerService.cpp @@ -71,9 +71,9 @@ static String8 getString(const Vector &items) { return itemsStr; } -static bool hasResourceType(MediaResource::Type type, const Vector& 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& 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 &resources) { @@ -186,10 +185,10 @@ status_t ResourceManagerService::dump(int fd, const Vector& /* args */ snprintf(buffer, SIZE, " Name: %s\n", infos[j].client->getName().string()); result.append(buffer); - Vector 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 &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 &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 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; } } diff --git a/services/mediaresourcemanager/ResourceManagerService.h b/services/mediaresourcemanager/ResourceManagerService.h index 741ef73c61..b9147ffbbb 100644 --- a/services/mediaresourcemanager/ResourceManagerService.h +++ b/services/mediaresourcemanager/ResourceManagerService.h @@ -33,17 +33,17 @@ namespace android { class ServiceLog; struct ProcessInfoInterface; +typedef std::map, MediaResource> ResourceList; struct ResourceInfo { int64_t clientId; uid_t uid; sp client; sp deathNotifier; - Vector resources; - bool cpuBoost; - bool batteryNoted; + ResourceList resources; }; -typedef Vector ResourceInfos; +// TODO: convert these to std::map +typedef KeyedVector ResourceInfos; typedef KeyedVector PidResourceInfosMap; class ResourceManagerService @@ -68,7 +68,10 @@ class ResourceManagerService const sp client, const Vector &resources); - virtual void removeResource(int pid, int64_t clientId); + virtual void removeResource(int pid, int64_t clientId, + const Vector &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 @@ class ResourceManagerService void getClientForResource_l( int callingPid, const MediaResource *res, Vector> *clients); + void onFirstAdded(const MediaResource& res, const ResourceInfo& clientInfo); + void onLastRemoved(const MediaResource& res, const ResourceInfo& clientInfo); + mutable Mutex mLock; sp mProcessInfo; sp mServiceLog; diff --git a/services/mediaresourcemanager/test/ResourceManagerService_test.cpp b/services/mediaresourcemanager/test/ResourceManagerService_test.cpp index 8a3987ace8..be592f537a 100644 --- a/services/mediaresourcemanager/test/ResourceManagerService_test.cpp +++ b/services/mediaresourcemanager/test/ResourceManagerService_test.cpp @@ -58,7 +58,7 @@ struct TestClient : public BnResourceManagerClient { virtual bool reclaimResource() { sp client(this); - mService->removeResource(mPid, (int64_t) client.get()); + mService->removeClient(mPid, (int64_t) client.get()); mReclaimed = true; return true; } @@ -106,16 +106,14 @@ class ResourceManagerServiceTest : public ::testing::Test { protected: static bool isEqualResources(const Vector &resources1, - const Vector &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 @@ class ResourceManagerServiceTest : public ::testing::Test { 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 @@ class ResourceManagerServiceTest : public ::testing::Test { EXPECT_TRUE(mService->mSupportsSecureWithNonSecureCodec); } + void testCombineResource() { + // kTestPid1 mTestClient1 + Vector resources1; + resources1.push_back(MediaResource(MediaResource::kSecureCodec, 1)); + mService->addResource(kTestPid1, kTestUid1, getId(mTestClient1), mTestClient1, resources1); + + Vector 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 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 resources1; + resources1.push_back(MediaResource(MediaResource::kSecureCodec, 1)); + mService->addResource(kTestPid1, kTestUid1, getId(mTestClient1), mTestClient1, resources1); + + Vector 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 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 @@ class ResourceManagerServiceTest : public ::testing::Test { 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 @@ class ResourceManagerServiceTest : public ::testing::Test { 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 @@ class ResourceManagerServiceTest : public ::testing::Test { 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();