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();