From 630fc01bf6644833c35747e1dfd5c781ab20c53b Mon Sep 17 00:00:00 2001 From: Amy Date: Thu, 20 Feb 2020 20:02:42 -0800 Subject: [PATCH] Fix issues in register/unregister client API implementation Issues include: 1. clientId array size checking too hard 2. missed use case validation 3. missed in use resources update when unregistering client(same in on binder dead, will be in separate CL) 4. Calling PID should have only be used when tvinputsession id is null 5. Adding resources list to client profile Test: atest TunerResourceManagerServiceTest Bug: 147380513 Change-Id: I09b4105659a491b3c07e3b737fe0617b83d24447 (cherry picked from commit 19b71275e0880b7963f842a1287173db816b40ab) --- .../tunerresourcemanager/ClientProfile.java | 32 +++++++ .../TunerResourceManagerService.java | 87 ++++++++++++------- .../UseCasePriorityHints.java | 6 +- .../TunerResourceManagerServiceTest.java | 42 +++++++++ .../UseCasePriorityHintsTest.java | 19 +++- 5 files changed, 149 insertions(+), 37 deletions(-) diff --git a/services/core/java/com/android/server/tv/tunerresourcemanager/ClientProfile.java b/services/core/java/com/android/server/tv/tunerresourcemanager/ClientProfile.java index 07c565a729bbe..4eff954f1ec88 100644 --- a/services/core/java/com/android/server/tv/tunerresourcemanager/ClientProfile.java +++ b/services/core/java/com/android/server/tv/tunerresourcemanager/ClientProfile.java @@ -15,6 +15,9 @@ */ package com.android.server.tv.tunerresourcemanager; +import java.util.ArrayList; +import java.util.List; + /** * A client profile object used by the Tuner Resource Manager to record the registered clients' * information. @@ -59,6 +62,11 @@ public final class ClientProfile { */ private int mNiceValue; + /** + * List of the frontend ids that are used by the current client. + */ + private List mUsingFrontendIds = new ArrayList<>(); + /** * Optional arbitrary priority value given by the client. * @@ -114,6 +122,30 @@ public final class ClientProfile { mNiceValue = niceValue; } + /** + * Set when the client starts to use a frontend. + * + * @param frontendId being used. + */ + public void useFrontend(int frontendId) { + mUsingFrontendIds.add(frontendId); + } + + public List getInUseFrontendIds() { + return mUsingFrontendIds; + } + + /** + * Called when the client released a frontend. + * + *

This could happen when client resource reclaimed. + * + * @param frontendId being released. + */ + public void releaseFrontend(int frontendId) { + mUsingFrontendIds.remove(frontendId); + } + @Override public String toString() { return "ClientProfile[id=" + this.mId + ", tvInputSessionId=" + this.mTvInputSessionId diff --git a/services/core/java/com/android/server/tv/tunerresourcemanager/TunerResourceManagerService.java b/services/core/java/com/android/server/tv/tunerresourcemanager/TunerResourceManagerService.java index 621f2b024e843..cb31a502ecfa0 100644 --- a/services/core/java/com/android/server/tv/tunerresourcemanager/TunerResourceManagerService.java +++ b/services/core/java/com/android/server/tv/tunerresourcemanager/TunerResourceManagerService.java @@ -28,6 +28,7 @@ import android.media.tv.tunerresourcemanager.TunerFrontendInfo; import android.media.tv.tunerresourcemanager.TunerFrontendRequest; import android.media.tv.tunerresourcemanager.TunerLnbRequest; import android.media.tv.tunerresourcemanager.TunerResourceManager; +import android.os.Binder; import android.os.RemoteException; import android.util.Log; import android.util.Slog; @@ -54,7 +55,7 @@ public class TunerResourceManagerService extends SystemService { // Array of the registered client profiles @VisibleForTesting private SparseArray mClientProfiles = new SparseArray<>(); private int mNextUnusedClientId = 0; - private List mReleasedClientId = new ArrayList(); + private List mRegisteredClientIds = new ArrayList(); // Array of the current available frontend resources @VisibleForTesting @@ -98,8 +99,12 @@ public class TunerResourceManagerService extends SystemService { throw new RemoteException("ResourceClientProfile can't be null"); } - if (clientId == null || clientId.length != 1) { - throw new RemoteException("clientId must be a size 1 array!"); + if (clientId == null) { + throw new RemoteException("clientId can't be null!"); + } + + if (!mPriorityCongfig.isDefinedUseCase(profile.getUseCase())) { + throw new RemoteException("Use undefined client use case:" + profile.getUseCase()); } synchronized (mLock) { @@ -108,20 +113,20 @@ public class TunerResourceManagerService extends SystemService { } @Override - public void unregisterClientProfile(int clientId) { - if (DEBUG) { - Slog.d(TAG, "unregisterClientProfile(clientId=" + clientId + ")"); + public void unregisterClientProfile(int clientId) throws RemoteException { + enforceAccessPermission(); + synchronized (mLock) { + if (!checkClientExists(clientId)) { + Slog.e(TAG, "Unregistering non exists client:" + clientId); + return; + } + unregisterClientProfileInternal(clientId); } - - mClientProfiles.remove(clientId); - mListeners.remove(clientId); - mReleasedClientId.add(clientId); } @Override public boolean updateClientPriority(int clientId, int priority, int niceValue) { enforceAccessPermission(); - synchronized (mLock) { return updateClientPriorityInternal(clientId, priority, niceValue); } @@ -243,24 +248,38 @@ public class TunerResourceManagerService extends SystemService { return; } // TODO tell if the client already exists - if (mReleasedClientId.isEmpty()) { - clientId[0] = mNextUnusedClientId++; - } else { - clientId[0] = mReleasedClientId.get(0); - mReleasedClientId.remove(0); - } + clientId[0] = mNextUnusedClientId++; - int callingPid = mManager.getClientPid(profile.getTvInputSessionId()); + int pid = profile.getTvInputSessionId() == null + ? Binder.getCallingPid() /*callingPid*/ + : mManager.getClientPid(profile.getTvInputSessionId()); /*tvAppId*/ ClientProfile clientProfile = new ClientProfile.Builder(clientId[0]) .tvInputSessionId(profile.getTvInputSessionId()) .useCase(profile.getUseCase()) - .processId(callingPid) + .processId(pid) .build(); - clientProfile.setPriority(getClientPriority(profile.getUseCase(), callingPid)); + clientProfile.setPriority(getClientPriority(profile.getUseCase(), pid)); mClientProfiles.append(clientId[0], clientProfile); mListeners.append(clientId[0], listener); + mRegisteredClientIds.add(clientId[0]); + } + + @VisibleForTesting + protected void unregisterClientProfileInternal(int clientId) { + if (DEBUG) { + Slog.d(TAG, "unregisterClientProfile(clientId=" + clientId + ")"); + } + for (int id : getClientProfile(clientId).getInUseFrontendIds()) { + getFrontendResource(id).removeOwner(); + for (int groupMemberId : getFrontendResource(id).getExclusiveGroupMemberFeIds()) { + getFrontendResource(groupMemberId).removeOwner(); + } + } + mClientProfiles.remove(clientId); + mListeners.remove(clientId); + mRegisteredClientIds.remove(clientId); } @VisibleForTesting @@ -355,12 +374,11 @@ public class TunerResourceManagerService extends SystemService { } frontendId[0] = TunerResourceManager.INVALID_FRONTEND_ID; - ClientProfile requestClient = getClientProfile(request.getClientId()); - if (requestClient == null) { - Slog.e(TAG, "Request from unregistered client. Id: " + request.getClientId()); + if (!checkClientExists(request.getClientId())) { + Slog.e(TAG, "Request frontend from unregistered client:" + request.getClientId()); return false; } - + ClientProfile requestClient = getClientProfile(request.getClientId()); int grantingFrontendId = -1; int inUseLowestPriorityFrId = -1; // Priority max value is 1000 @@ -393,7 +411,7 @@ public class TunerResourceManagerService extends SystemService { // Grant frontend when there is unused resource. if (grantingFrontendId > -1) { frontendId[0] = grantingFrontendId; - updateFrontendResourcesOnNewGrant(frontendId[0], request.getClientId()); + updateFrontendClientMappingOnNewGrant(frontendId[0], request.getClientId()); return true; } @@ -402,7 +420,7 @@ public class TunerResourceManagerService extends SystemService { if (inUseLowestPriorityFrId > -1 && (requestClient.getPriority() > currentLowestPriority)) { frontendId[0] = inUseLowestPriorityFrId; reclaimFrontendResource(getFrontendResource(frontendId[0]).getOwnerClientId()); - updateFrontendResourcesOnNewGrant(frontendId[0], request.getClientId()); + updateFrontendClientMappingOnNewGrant(frontendId[0], request.getClientId()); return true; } @@ -410,20 +428,20 @@ public class TunerResourceManagerService extends SystemService { } @VisibleForTesting - protected int getClientPriority(int useCase, int callingPid) { + protected int getClientPriority(int useCase, int pid) { if (DEBUG) { Slog.d(TAG, "getClientPriority useCase=" + useCase - + ", calling Pid=" + callingPid + ")"); + + ", pid=" + pid + ")"); } - if (isForeground(callingPid)) { + if (isForeground(pid)) { return mPriorityCongfig.getForegroundPriority(useCase); } return mPriorityCongfig.getBackgroundPriority(useCase); } @VisibleForTesting - protected boolean isForeground(int callingPid) { + protected boolean isForeground(int pid) { // TODO: how to get fg/bg information from pid return true; } @@ -439,11 +457,14 @@ public class TunerResourceManagerService extends SystemService { } } - private void updateFrontendResourcesOnNewGrant(int grantingId, int ownerClientId) { + private void updateFrontendClientMappingOnNewGrant(int grantingId, int ownerClientId) { FrontendResource grantingFrontend = getFrontendResource(grantingId); + ClientProfile ownerProfile = getClientProfile(ownerClientId); grantingFrontend.setOwner(ownerClientId); + ownerProfile.useFrontend(grantingId); for (int exclusiveGroupMember : grantingFrontend.getExclusiveGroupMemberFeIds()) { getFrontendResource(exclusiveGroupMember).setOwner(ownerClientId); + ownerProfile.useFrontend(exclusiveGroupMember); } } @@ -475,6 +496,10 @@ public class TunerResourceManagerService extends SystemService { return mFrontendResources; } + private boolean checkClientExists(int clientId) { + return mRegisteredClientIds.contains(clientId); + } + private void enforceAccessPermission() { getContext().enforceCallingOrSelfPermission( "android.permission.TUNER_RESOURCE_ACCESS", TAG); diff --git a/services/core/java/com/android/server/tv/tunerresourcemanager/UseCasePriorityHints.java b/services/core/java/com/android/server/tv/tunerresourcemanager/UseCasePriorityHints.java index e6d672d9acbc5..8c2de475b99bb 100644 --- a/services/core/java/com/android/server/tv/tunerresourcemanager/UseCasePriorityHints.java +++ b/services/core/java/com/android/server/tv/tunerresourcemanager/UseCasePriorityHints.java @@ -95,8 +95,10 @@ public class UseCasePriorityHints { } catch (XmlPullParserException e) { Slog.e(TAG, "Unable to parse vendor file: " + file, e); } - } else if (DEBUG) { - Slog.i(TAG, "no vendor priority configuration available. Using default priority"); + } else { + if (DEBUG) { + Slog.i(TAG, "no vendor priority configuration available. Using default priority"); + } addNewUseCasePriority(TvInputService.PRIORITY_HINT_USE_CASE_TYPE_BACKGROUND, 180, 100); addNewUseCasePriority(TvInputService.PRIORITY_HINT_USE_CASE_TYPE_SCAN, 450, 200); addNewUseCasePriority(TvInputService.PRIORITY_HINT_USE_CASE_TYPE_PLAYBACK, 480, 300); diff --git a/services/tests/servicestests/src/com/android/server/tv/tunerresourcemanager/TunerResourceManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/tv/tunerresourcemanager/TunerResourceManagerServiceTest.java index 72fbaef339a38..192c6fe4ab75f 100644 --- a/services/tests/servicestests/src/com/android/server/tv/tunerresourcemanager/TunerResourceManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/tv/tunerresourcemanager/TunerResourceManagerServiceTest.java @@ -452,4 +452,46 @@ public class TunerResourceManagerServiceTest { .get(infos[1].getId()).getOwnerClientId()).isEqualTo(clientId1[0]); assertThat(mReclaimingId).isEqualTo(clientId0[0]); } + + @Test + public void unregisterClientTest_usingFrontend() { + // Register client + ResourceClientProfile profile = new ResourceClientProfile("0" /*sessionId*/, + TvInputService.PRIORITY_HINT_USE_CASE_TYPE_PLAYBACK); + int[] clientId = new int[1]; + mTunerResourceManagerService.registerClientProfileInternal( + profile, null /*listener*/, clientId); + assertThat(clientId[0]).isNotEqualTo(TunerResourceManagerService.INVALID_CLIENT_ID); + + // Init frontend resources. + TunerFrontendInfo[] infos = new TunerFrontendInfo[2]; + infos[0] = + new TunerFrontendInfo(0 /*id*/, FrontendSettings.TYPE_DVBT, 1 /*exclusiveGroupId*/); + infos[1] = + new TunerFrontendInfo(1 /*id*/, FrontendSettings.TYPE_DVBS, 1 /*exclusiveGroupId*/); + mTunerResourceManagerService.setFrontendInfoListInternal(infos); + + TunerFrontendRequest request = + new TunerFrontendRequest(clientId[0] /*clientId*/, FrontendSettings.TYPE_DVBT); + int[] frontendId = new int[1]; + try { + assertThat(mTunerResourceManagerService.requestFrontendInternal(request, frontendId)) + .isTrue(); + } catch (RemoteException e) { + throw e.rethrowFromSystemServer(); + } + assertThat(frontendId[0]).isEqualTo(infos[0].getId()); + assertThat(mTunerResourceManagerService.getFrontendResources().get(infos[0].getId()) + .isInUse()).isTrue(); + assertThat(mTunerResourceManagerService.getFrontendResources().get(infos[1].getId()) + .isInUse()).isTrue(); + + // Unregister client when using frontend + mTunerResourceManagerService.unregisterClientProfileInternal(clientId[0]); + assertThat(mTunerResourceManagerService.getFrontendResources().get(infos[0].getId()) + .isInUse()).isFalse(); + assertThat(mTunerResourceManagerService.getFrontendResources().get(infos[1].getId()) + .isInUse()).isFalse(); + + } } diff --git a/services/tests/servicestests/src/com/android/server/tv/tunerresourcemanager/UseCasePriorityHintsTest.java b/services/tests/servicestests/src/com/android/server/tv/tunerresourcemanager/UseCasePriorityHintsTest.java index 8a75d5c2a3804..ab5665ba99fc2 100644 --- a/services/tests/servicestests/src/com/android/server/tv/tunerresourcemanager/UseCasePriorityHintsTest.java +++ b/services/tests/servicestests/src/com/android/server/tv/tunerresourcemanager/UseCasePriorityHintsTest.java @@ -58,17 +58,16 @@ public class UseCasePriorityHintsTest { @Before public void setUp() throws Exception { mPriorityHints = new UseCasePriorityHints(); - } - - @Test - public void parseTest_parseSampleXml() { try { mPriorityHints.parseInternal( new ByteArrayInputStream(mExampleXML.getBytes(StandardCharsets.UTF_8))); } catch (IOException | XmlPullParserException e) { Slog.e(TAG, "Error parse xml.", e); } + } + @Test + public void parseTest_parseSampleXml() { // Pre-defined foreground assertThat(mPriorityHints.getForegroundPriority( TvInputService.PRIORITY_HINT_USE_CASE_TYPE_BACKGROUND)).isEqualTo(180); @@ -97,4 +96,16 @@ public class UseCasePriorityHintsTest { assertThat(mPriorityHints.getForegroundPriority(1001)).isEqualTo(300); assertThat(mPriorityHints.getBackgroundPriority(1001)).isEqualTo(80); } + + @Test + public void isDefinedUseCaseTest_invalidUseCase() { + assertThat(mPriorityHints.isDefinedUseCase(1992)).isFalse(); + } + + @Test + public void isDefinedUseCaseTest_validUseCase() { + assertThat(mPriorityHints.isDefinedUseCase(1001)).isTrue(); + assertThat(mPriorityHints.isDefinedUseCase( + TvInputService.PRIORITY_HINT_USE_CASE_TYPE_RECORD)).isTrue(); + } }