From ee33d64bf82f3781ab826863f69d089d0a7400da Mon Sep 17 00:00:00 2001 From: Chong Zhang Date: Thu, 8 Aug 2019 14:26:43 -0700 Subject: [PATCH] Report video battery on/off from mediaserver We can't track video stats accurately if it's reported from app side MediaCodec. If the app dies, video stats get stuck in On state forever. This can be easily triggered by forcefully kill and app that uses MediaCodec from app side (instead of through mediaserver's Recorder or Player service), eg. launch GoogleCamera app and switch to Video tab, and kill it from adb shell. In order to track MediaCodec usage from apps we need to move the battery stats reporting from MediaCodec into ResourceManager. bug: 138381810 test: 1. test if app uses MediaCodec directly and it dies off, the video off is received: launch GoogleCamera app, swipe to Video tab, "dumpsys batterystats --history" and see +video; now adb shell kill GoogleCamera, dumpsys should show -video. 2. test app that uses MediaCodec through mediaserver: use Chrome (which uses MediaPlayer) to play some website with video, kills Chrome from adb shell, and check -video is received. In anycase it shouldn't stuck in On state. 3. ResourceManagerService_test Change-Id: I164b31681c4e72e8cce02342641dbec14b8df374 (cherry picked from commit 47db2ff19c3ceb2d3b39ff9315a463984e55dec1) --- media/libmedia/IResourceManagerService.cpp | 5 ++- .../include/media/IResourceManagerService.h | 1 + media/libmedia/include/media/MediaResource.h | 3 +- media/libstagefright/MediaCodec.cpp | 33 +++++++------------ .../include/media/stagefright/MediaCodec.h | 4 +-- .../ResourceManagerService.cpp | 24 +++++++++++--- .../ResourceManagerService.h | 3 ++ .../test/ResourceManagerService_test.cpp | 24 +++++++++----- 8 files changed, 58 insertions(+), 39 deletions(-) diff --git a/media/libmedia/IResourceManagerService.cpp b/media/libmedia/IResourceManagerService.cpp index 9724fc1a12..00f9b88aad 100644 --- a/media/libmedia/IResourceManagerService.cpp +++ b/media/libmedia/IResourceManagerService.cpp @@ -72,12 +72,14 @@ class BpResourceManagerService : public BpInterface virtual void addResource( int pid, + int uid, int64_t clientId, const sp client, const Vector &resources) { Parcel data, reply; data.writeInterfaceToken(IResourceManagerService::getInterfaceDescriptor()); data.writeInt32(pid); + data.writeInt32(uid); data.writeInt64(clientId); data.writeStrongBinder(IInterface::asBinder(client)); writeToParcel(&data, resources); @@ -129,6 +131,7 @@ status_t BnResourceManagerService::onTransact( case ADD_RESOURCE: { CHECK_INTERFACE(IResourceManagerService, data, reply); int pid = data.readInt32(); + int uid = data.readInt32(); int64_t clientId = data.readInt64(); sp client( interface_cast(data.readStrongBinder())); @@ -137,7 +140,7 @@ status_t BnResourceManagerService::onTransact( } Vector resources; readFromParcel(data, &resources); - addResource(pid, clientId, client, resources); + addResource(pid, uid, clientId, client, resources); return NO_ERROR; } break; diff --git a/media/libmedia/include/media/IResourceManagerService.h b/media/libmedia/include/media/IResourceManagerService.h index 1e4f6dea74..404519b2a6 100644 --- a/media/libmedia/include/media/IResourceManagerService.h +++ b/media/libmedia/include/media/IResourceManagerService.h @@ -39,6 +39,7 @@ class IResourceManagerService: public IInterface virtual void addResource( int pid, + int uid, int64_t clientId, const sp client, const Vector &resources) = 0; diff --git a/media/libmedia/include/media/MediaResource.h b/media/libmedia/include/media/MediaResource.h index e1fdb9be3f..10d0e3b1b1 100644 --- a/media/libmedia/include/media/MediaResource.h +++ b/media/libmedia/include/media/MediaResource.h @@ -31,12 +31,13 @@ class MediaResource { kNonSecureCodec, kGraphicMemory, kCpuBoost, + kBattery, }; enum SubType { kUnspecifiedSubType = 0, kAudioCodec, - kVideoCodec + kVideoCodec, }; MediaResource(); diff --git a/media/libstagefright/MediaCodec.cpp b/media/libstagefright/MediaCodec.cpp index f579e9de6b..a6a856c3f8 100644 --- a/media/libstagefright/MediaCodec.cpp +++ b/media/libstagefright/MediaCodec.cpp @@ -57,7 +57,6 @@ #include #include #include -#include #include #include @@ -166,8 +165,9 @@ struct ResourceManagerClient : public BnResourceManagerClient { DISALLOW_EVIL_CONSTRUCTORS(ResourceManagerClient); }; -MediaCodec::ResourceManagerServiceProxy::ResourceManagerServiceProxy(pid_t pid) - : mPid(pid) { +MediaCodec::ResourceManagerServiceProxy::ResourceManagerServiceProxy( + pid_t pid, uid_t uid) + : mPid(pid), mUid(uid) { if (mPid == MediaCodec::kNoPid) { mPid = IPCThreadState::self()->getCallingPid(); } @@ -204,7 +204,7 @@ void MediaCodec::ResourceManagerServiceProxy::addResource( if (mService == NULL) { return; } - mService->addResource(mPid, clientId, client, resources); + mService->addResource(mPid, mUid, clientId, client, resources); } void MediaCodec::ResourceManagerServiceProxy::removeResource(int64_t clientId) { @@ -517,8 +517,6 @@ MediaCodec::MediaCodec(const sp &looper, pid_t pid, uid_t uid) mStickyError(OK), mSoftRenderer(NULL), mAnalyticsItem(NULL), - mResourceManagerClient(new ResourceManagerClient(this)), - mResourceManagerService(new ResourceManagerServiceProxy(pid)), mBatteryStatNotified(false), mIsVideo(false), mVideoWidth(0), @@ -537,6 +535,8 @@ MediaCodec::MediaCodec(const sp &looper, pid_t pid, uid_t uid) } else { mUid = uid; } + mResourceManagerClient = new ResourceManagerClient(this); + mResourceManagerService = new ResourceManagerServiceProxy(pid, mUid); initAnalyticsItem(); } @@ -1977,6 +1977,11 @@ void MediaCodec::onMessageReceived(const sp &msg) { if (mIsVideo) { // audio codec is currently ignored. addResource(resourceType, MediaResource::kVideoCodec, 1); + // TODO: track battery on/off by actual queueing/dequeueing + // For now, keep existing behavior and request battery on/off + // together with codec init/uninit. We'll improve the tracking + // later by adding/removing this based on queue/dequeue timing. + addResource(MediaResource::kBattery, MediaResource::kVideoCodec, 1); } (new AMessage)->postReply(mReplyID); @@ -3126,8 +3131,6 @@ void MediaCodec::setState(State newState) { mState = newState; cancelPendingDequeueOperations(); - - updateBatteryStat(); } void MediaCodec::returnBuffersToCodec(bool isReclaim) { @@ -3631,20 +3634,6 @@ status_t MediaCodec::amendOutputFormatWithCodecSpecificData( return OK; } -void MediaCodec::updateBatteryStat() { - if (!mIsVideo) { - return; - } - - if (mState == CONFIGURED && !mBatteryStatNotified) { - BatteryNotifier::getInstance().noteStartVideo(mUid); - mBatteryStatNotified = true; - } else if (mState == UNINITIALIZED && mBatteryStatNotified) { - BatteryNotifier::getInstance().noteStopVideo(mUid); - mBatteryStatNotified = false; - } -} - std::string MediaCodec::stateString(State state) { const char *rval = NULL; char rawbuffer[16]; // room for "%d" diff --git a/media/libstagefright/include/media/stagefright/MediaCodec.h b/media/libstagefright/include/media/stagefright/MediaCodec.h index 89cca63dc6..0218a88af7 100644 --- a/media/libstagefright/include/media/stagefright/MediaCodec.h +++ b/media/libstagefright/include/media/stagefright/MediaCodec.h @@ -283,7 +283,7 @@ struct MediaCodec : public AHandler { }; struct ResourceManagerServiceProxy : public IBinder::DeathRecipient { - ResourceManagerServiceProxy(pid_t pid); + ResourceManagerServiceProxy(pid_t pid, uid_t uid); ~ResourceManagerServiceProxy(); void init(); @@ -304,6 +304,7 @@ struct MediaCodec : public AHandler { Mutex mLock; sp mService; pid_t mPid; + uid_t mUid; }; State mState; @@ -425,7 +426,6 @@ struct MediaCodec : public AHandler { status_t onSetParameters(const sp ¶ms); status_t amendOutputFormatWithCodecSpecificData(const sp &buffer); - void updateBatteryStat(); bool isExecuting() const; uint64_t getGraphicBufferSize(); diff --git a/services/mediaresourcemanager/ResourceManagerService.cpp b/services/mediaresourcemanager/ResourceManagerService.cpp index 28bfd3ff51..117a2114e1 100644 --- a/services/mediaresourcemanager/ResourceManagerService.cpp +++ b/services/mediaresourcemanager/ResourceManagerService.cpp @@ -21,8 +21,11 @@ #include #include +#include #include #include +#include +#include #include #include #include @@ -31,8 +34,7 @@ #include "ResourceManagerService.h" #include "ServiceLog.h" -#include "mediautils/SchedulingPolicyService.h" -#include + namespace android { namespace { @@ -101,6 +103,7 @@ static ResourceInfos& getResourceInfosForEdit( } static ResourceInfo& getResourceInfoForEdit( + uid_t uid, int64_t clientId, const sp& client, ResourceInfos& infos) { @@ -110,9 +113,11 @@ static ResourceInfo& getResourceInfoForEdit( } } 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); } @@ -204,7 +209,9 @@ ResourceManagerService::ResourceManagerService(sp processI mServiceLog(new ServiceLog()), mSupportsMultipleSecureCodecs(true), mSupportsSecureWithNonSecureCodec(true), - mCpuBoostCount(0) {} + mCpuBoostCount(0) { + BatteryNotifier::getInstance().noteResetVideo(); +} ResourceManagerService::~ResourceManagerService() {} @@ -226,6 +233,7 @@ void ResourceManagerService::config(const Vector &policies) void ResourceManagerService::addResource( int pid, + int uid, int64_t clientId, const sp client, const Vector &resources) { @@ -239,7 +247,7 @@ void ResourceManagerService::addResource( return; } ResourceInfos& infos = getResourceInfosForEdit(pid, mMap); - ResourceInfo& info = getResourceInfoForEdit(clientId, client, infos); + ResourceInfo& info = getResourceInfoForEdit(uid, clientId, client, infos); // TODO: do the merge instead of append. info.resources.appendVector(resources); @@ -253,6 +261,11 @@ void ResourceManagerService::addResource( 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); } } if (info.deathNotifier == nullptr) { @@ -291,6 +304,9 @@ void ResourceManagerService::removeResource(int pid, int64_t clientId, bool chec 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; diff --git a/services/mediaresourcemanager/ResourceManagerService.h b/services/mediaresourcemanager/ResourceManagerService.h index 82d2a0b369..741ef73c61 100644 --- a/services/mediaresourcemanager/ResourceManagerService.h +++ b/services/mediaresourcemanager/ResourceManagerService.h @@ -35,10 +35,12 @@ struct ProcessInfoInterface; struct ResourceInfo { int64_t clientId; + uid_t uid; sp client; sp deathNotifier; Vector resources; bool cpuBoost; + bool batteryNoted; }; typedef Vector ResourceInfos; @@ -61,6 +63,7 @@ class ResourceManagerService virtual void addResource( int pid, + int uid, int64_t clientId, const sp client, const Vector &resources); diff --git a/services/mediaresourcemanager/test/ResourceManagerService_test.cpp b/services/mediaresourcemanager/test/ResourceManagerService_test.cpp index ed0b0efa63..8a3987ace8 100644 --- a/services/mediaresourcemanager/test/ResourceManagerService_test.cpp +++ b/services/mediaresourcemanager/test/ResourceManagerService_test.cpp @@ -86,7 +86,10 @@ struct TestClient : public BnResourceManagerClient { }; static const int kTestPid1 = 30; +static const int kTestUid1 = 1010; + static const int kTestPid2 = 20; +static const int kTestUid2 = 1011; static const int kLowPriorityPid = 40; static const int kMidPriorityPid = 25; @@ -115,8 +118,11 @@ class ResourceManagerServiceTest : public ::testing::Test { return true; } - static void expectEqResourceInfo(const ResourceInfo &info, sp client, + static void expectEqResourceInfo(const ResourceInfo &info, + int uid, + sp client, const Vector &resources) { + EXPECT_EQ(uid, info.uid); EXPECT_EQ(client, info.client); EXPECT_TRUE(isEqualResources(resources, info.resources)); } @@ -153,24 +159,24 @@ class ResourceManagerServiceTest : public ::testing::Test { // kTestPid1 mTestClient1 Vector resources1; resources1.push_back(MediaResource(MediaResource::kSecureCodec, 1)); - mService->addResource(kTestPid1, getId(mTestClient1), mTestClient1, resources1); + mService->addResource(kTestPid1, kTestUid1, getId(mTestClient1), mTestClient1, resources1); resources1.push_back(MediaResource(MediaResource::kGraphicMemory, 200)); Vector resources11; resources11.push_back(MediaResource(MediaResource::kGraphicMemory, 200)); - mService->addResource(kTestPid1, getId(mTestClient1), mTestClient1, resources11); + mService->addResource(kTestPid1, kTestUid1, getId(mTestClient1), mTestClient1, resources11); // kTestPid2 mTestClient2 Vector resources2; resources2.push_back(MediaResource(MediaResource::kNonSecureCodec, 1)); resources2.push_back(MediaResource(MediaResource::kGraphicMemory, 300)); - mService->addResource(kTestPid2, getId(mTestClient2), mTestClient2, resources2); + mService->addResource(kTestPid2, kTestUid2, getId(mTestClient2), mTestClient2, resources2); // kTestPid2 mTestClient3 Vector resources3; - mService->addResource(kTestPid2, getId(mTestClient3), mTestClient3, resources3); + mService->addResource(kTestPid2, kTestUid2, getId(mTestClient3), mTestClient3, resources3); resources3.push_back(MediaResource(MediaResource::kSecureCodec, 1)); resources3.push_back(MediaResource(MediaResource::kGraphicMemory, 100)); - mService->addResource(kTestPid2, getId(mTestClient3), mTestClient3, resources3); + mService->addResource(kTestPid2, kTestUid2, getId(mTestClient3), mTestClient3, resources3); const PidResourceInfosMap &map = mService->mMap; EXPECT_EQ(2u, map.size()); @@ -178,14 +184,14 @@ class ResourceManagerServiceTest : public ::testing::Test { ASSERT_GE(index1, 0); const ResourceInfos &infos1 = map[index1]; EXPECT_EQ(1u, infos1.size()); - expectEqResourceInfo(infos1[0], mTestClient1, resources1); + expectEqResourceInfo(infos1[0], 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], mTestClient2, resources2); - expectEqResourceInfo(infos2[1], mTestClient3, resources3); + expectEqResourceInfo(infos2[0], kTestUid2, mTestClient2, resources2); + expectEqResourceInfo(infos2[1], kTestUid2, mTestClient3, resources3); } void testConfig() {