From 0c1891e714eba2fa3acbc47338cb010c061f49b7 Mon Sep 17 00:00:00 2001 From: Peiyong Lin Date: Fri, 28 Jan 2022 23:04:17 +0000 Subject: [PATCH] Address API review feedback. Avoid using nested callback classes, move the Executor to register function and update the Javadoc to emphasize unregister call is always expected even when a task is already destoryed. Bug: b/216628683 Test: atest TaskFpsCallbackTest Change-Id: I801898d082a6eeb6a6f991e3765795a270943d42 --- core/api/system-current.txt | 11 ++-- core/java/android/view/IWindowManager.aidl | 10 ++-- core/java/android/view/WindowManager.java | 11 ++-- core/java/android/view/WindowManagerImpl.java | 56 +++++++++++++++++-- ...ackListener.aidl => ITaskFpsCallback.aidl} | 2 +- core/java/android/window/TaskFpsCallback.java | 41 +++----------- .../android/window/TaskFpsCallbackTest.java | 11 ++-- .../server/wm/TaskFpsCallbackController.java | 36 ++++++------ .../server/wm/WindowManagerService.java | 10 ++-- ...id_server_wm_TaskFpsCallbackController.cpp | 4 +- 10 files changed, 105 insertions(+), 87 deletions(-) rename core/java/android/window/{IOnFpsCallbackListener.aidl => ITaskFpsCallback.aidl} (95%) diff --git a/core/api/system-current.txt b/core/api/system-current.txt index ab38d12e593bd..a97b2609ee57c 100644 --- a/core/api/system-current.txt +++ b/core/api/system-current.txt @@ -15593,7 +15593,7 @@ package android.view { public interface WindowManager extends android.view.ViewManager { method @RequiresPermission(android.Manifest.permission.RESTRICTED_VR_ACCESS) public android.graphics.Region getCurrentImeTouchRegion(); - method public default void registerTaskFpsCallback(@IntRange(from=0) int, @NonNull android.window.TaskFpsCallback); + method public default void registerTaskFpsCallback(@IntRange(from=0) int, @NonNull java.util.concurrent.Executor, @NonNull android.window.TaskFpsCallback); method public default void unregisterTaskFpsCallback(@NonNull android.window.TaskFpsCallback); } @@ -16185,12 +16185,9 @@ package android.webkit { package android.window { - public final class TaskFpsCallback { - ctor public TaskFpsCallback(@NonNull java.util.concurrent.Executor, @NonNull android.window.TaskFpsCallback.OnFpsCallbackListener); - } - - public static interface TaskFpsCallback.OnFpsCallbackListener { - method public void onFpsReported(float); + public abstract class TaskFpsCallback { + ctor public TaskFpsCallback(); + method public abstract void onFpsReported(float); } } diff --git a/core/java/android/view/IWindowManager.aidl b/core/java/android/view/IWindowManager.aidl index 36baa04471762..ce21086931da0 100644 --- a/core/java/android/view/IWindowManager.aidl +++ b/core/java/android/view/IWindowManager.aidl @@ -65,7 +65,7 @@ import android.view.WindowManager; import android.view.SurfaceControl; import android.view.displayhash.DisplayHash; import android.view.displayhash.VerifiedDisplayHash; -import android.window.IOnFpsCallbackListener; +import android.window.ITaskFpsCallback; /** * System private interface to the window manager. @@ -926,21 +926,21 @@ interface IWindowManager * registered, the registered callback will not be unregistered until * {@link unregisterTaskFpsCallback()} is called * @param taskId task id of the task. - * @param listener listener to be registered. + * @param callback callback to be registered. * * @hide */ - void registerTaskFpsCallback(in int taskId, in IOnFpsCallbackListener listener); + void registerTaskFpsCallback(in int taskId, in ITaskFpsCallback callback); /** * Unregisters the frame rate per second count callback which was registered with * {@link #registerTaskFpsCallback(int,TaskFpsCallback)}. * - * @param listener listener to be unregistered. + * @param callback callback to be unregistered. * * @hide */ - void unregisterTaskFpsCallback(in IOnFpsCallbackListener listener); + void unregisterTaskFpsCallback(in ITaskFpsCallback listener); /** * Take a snapshot using the same path that's used for Recents. This is used for Testing only. diff --git a/core/java/android/view/WindowManager.java b/core/java/android/view/WindowManager.java index 771d40bdf6550..1c2704632e51a 100644 --- a/core/java/android/view/WindowManager.java +++ b/core/java/android/view/WindowManager.java @@ -4865,21 +4865,24 @@ public interface WindowManager extends ViewManager { * Registers the frame rate per second count callback for one given task ID. * Each callback can only register for receiving FPS callback for one task id until unregister * is called. If there's no task associated with the given task id, - * {@link IllegalArgumentException} will be thrown. If a task id destroyed after a callback is - * registered, the registered callback will not be unregistered until - * {@link #unregisterTaskFpsCallback(TaskFpsCallback))} is called + * {@link IllegalArgumentException} will be thrown. Registered callbacks should always be + * unregistered via {@link #unregisterTaskFpsCallback(TaskFpsCallback)} + * even when the task id has been destroyed. + * * @param taskId task id of the task. + * @param executor Executor to execute the callback. * @param callback callback to be registered. * * @hide */ @SystemApi default void registerTaskFpsCallback(@IntRange(from = 0) int taskId, + @NonNull Executor executor, @NonNull TaskFpsCallback callback) {} /** * Unregisters the frame rate per second count callback which was registered with - * {@link #registerTaskFpsCallback(int,TaskFpsCallback)}. + * {@link #registerTaskFpsCallback(Executor, int, TaskFpsCallback)}. * * @param callback callback to be unregistered. * diff --git a/core/java/android/view/WindowManagerImpl.java b/core/java/android/view/WindowManagerImpl.java index f4353eb5b397f..20cdad42b4cc3 100644 --- a/core/java/android/view/WindowManagerImpl.java +++ b/core/java/android/view/WindowManagerImpl.java @@ -39,14 +39,18 @@ import android.os.Bundle; import android.os.IBinder; import android.os.RemoteException; import android.os.StrictMode; +import android.window.ITaskFpsCallback; import android.window.TaskFpsCallback; import android.window.WindowContext; import android.window.WindowProvider; +import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.os.IResultReceiver; +import java.util.ArrayList; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Set; import java.util.concurrent.Executor; @@ -99,6 +103,10 @@ public final class WindowManagerImpl implements WindowManager { @Nullable private final IBinder mWindowContextToken; + @GuardedBy("mOnFpsCallbackListenerProxies") + private final ArrayList mOnFpsCallbackListenerProxies = + new ArrayList<>(); + public WindowManagerImpl(Context context) { this(context, null /* parentWindow */, null /* clientToken */); } @@ -424,20 +432,56 @@ public final class WindowManagerImpl implements WindowManager { } @Override - public void registerTaskFpsCallback(@IntRange(from = 0) int taskId, TaskFpsCallback callback) { + public void registerTaskFpsCallback(@IntRange(from = 0) int taskId, @NonNull Executor executor, + TaskFpsCallback callback) { + final OnFpsCallbackListenerProxy onFpsCallbackListenerProxy = + new OnFpsCallbackListenerProxy(executor, callback); try { WindowManagerGlobal.getWindowManagerService().registerTaskFpsCallback( - taskId, callback.getListener()); + taskId, onFpsCallbackListenerProxy); } catch (RemoteException e) { + throw e.rethrowFromSystemServer(); + } + synchronized (mOnFpsCallbackListenerProxies) { + mOnFpsCallbackListenerProxies.add(onFpsCallbackListenerProxy); } } @Override public void unregisterTaskFpsCallback(TaskFpsCallback callback) { - try { - WindowManagerGlobal.getWindowManagerService().unregisterTaskFpsCallback( - callback.getListener()); - } catch (RemoteException e) { + synchronized (mOnFpsCallbackListenerProxies) { + final Iterator iterator = + mOnFpsCallbackListenerProxies.iterator(); + while (iterator.hasNext()) { + final OnFpsCallbackListenerProxy proxy = iterator.next(); + if (proxy.mCallback == callback) { + try { + WindowManagerGlobal.getWindowManagerService() + .unregisterTaskFpsCallback(proxy); + } catch (RemoteException e) { + throw e.rethrowFromSystemServer(); + } + iterator.remove(); + } + } + } + } + + private static class OnFpsCallbackListenerProxy + extends ITaskFpsCallback.Stub { + private final Executor mExecutor; + private final TaskFpsCallback mCallback; + + private OnFpsCallbackListenerProxy(Executor executor, TaskFpsCallback callback) { + mExecutor = executor; + mCallback = callback; + } + + @Override + public void onFpsReported(float fps) { + mExecutor.execute(() -> { + mCallback.onFpsReported(fps); + }); } } diff --git a/core/java/android/window/IOnFpsCallbackListener.aidl b/core/java/android/window/ITaskFpsCallback.aidl similarity index 95% rename from core/java/android/window/IOnFpsCallbackListener.aidl rename to core/java/android/window/ITaskFpsCallback.aidl index 3091df3b23a32..aee403e98f967 100644 --- a/core/java/android/window/IOnFpsCallbackListener.aidl +++ b/core/java/android/window/ITaskFpsCallback.aidl @@ -19,7 +19,7 @@ package android.window; /** * @hide */ -oneway interface IOnFpsCallbackListener { +oneway interface ITaskFpsCallback { /** * Reports the fps from the registered task diff --git a/core/java/android/window/TaskFpsCallback.java b/core/java/android/window/TaskFpsCallback.java index a8e01b6df4b84..19b9f28614f41 100644 --- a/core/java/android/window/TaskFpsCallback.java +++ b/core/java/android/window/TaskFpsCallback.java @@ -21,8 +21,6 @@ import android.annotation.NonNull; import android.annotation.SystemApi; import android.os.RemoteException; -import java.util.concurrent.Executor; - /** * Callback for sampling the frames per second for a task and its children. * This should only be used by a system component that needs to listen to a task's @@ -30,44 +28,19 @@ import java.util.concurrent.Executor; * Otherwise, ASurfaceTransaction_OnComplete callbacks should be used. * * Each callback can only register for receiving FPS report for one task id until - * {@link WindowManager#unregister()} is called. + * {@link WindowManager#unregisterTaskFpsCallback()} is called. * * @hide */ @SystemApi -public final class TaskFpsCallback { +public abstract class TaskFpsCallback { /** - * Listener interface to receive frame per second of a task. + * Reports the fps from the registered task + * @param fps The frame per second of the task that has the registered task id + * and its children. */ - public interface OnFpsCallbackListener { - /** - * Reports the fps from the registered task - * @param fps The frame per second of the task that has the registered task id - * and its children. - */ - void onFpsReported(float fps); - } - - private final IOnFpsCallbackListener mListener; - - public TaskFpsCallback(@NonNull Executor executor, @NonNull OnFpsCallbackListener listener) { - mListener = new IOnFpsCallbackListener.Stub() { - @Override - public void onFpsReported(float fps) { - executor.execute(() -> { - listener.onFpsReported(fps); - }); - } - }; - } - - /** - * @hide - */ - public IOnFpsCallbackListener getListener() { - return mListener; - } + public abstract void onFpsReported(float fps); /** * Dispatch the collected sample. @@ -76,7 +49,7 @@ public final class TaskFpsCallback { */ @BinderThread private static void dispatchOnFpsReported( - @NonNull IOnFpsCallbackListener listener, float fps) { + @NonNull ITaskFpsCallback listener, float fps) { try { listener.onFpsReported(fps); } catch (RemoteException e) { diff --git a/core/tests/coretests/src/android/window/TaskFpsCallbackTest.java b/core/tests/coretests/src/android/window/TaskFpsCallbackTest.java index bf508db56852e..2dadb20d8f2cb 100644 --- a/core/tests/coretests/src/android/window/TaskFpsCallbackTest.java +++ b/core/tests/coretests/src/android/window/TaskFpsCallbackTest.java @@ -53,14 +53,15 @@ public class TaskFpsCallbackTest { @Test public void testRegisterAndUnregister() { - final TaskFpsCallback.OnFpsCallbackListener listener = fps -> { - // Ignore + final TaskFpsCallback callback = new TaskFpsCallback() { + @Override + public void onFpsReported(float fps) { + // Ignore + } }; - final TaskFpsCallback callback = new TaskFpsCallback(Runnable::run, listener); - final List tasks = mActivityTaskManager.getTasks(1); assertEquals(tasks.size(), 1); - mWindowManager.registerTaskFpsCallback(tasks.get(0).taskId, callback); + mWindowManager.registerTaskFpsCallback(tasks.get(0).taskId, Runnable::run, callback); mWindowManager.unregisterTaskFpsCallback(callback); } } diff --git a/services/core/java/com/android/server/wm/TaskFpsCallbackController.java b/services/core/java/com/android/server/wm/TaskFpsCallbackController.java index d9dc9aa9e5e2b..c099628438900 100644 --- a/services/core/java/com/android/server/wm/TaskFpsCallbackController.java +++ b/services/core/java/com/android/server/wm/TaskFpsCallbackController.java @@ -19,51 +19,51 @@ package com.android.server.wm; import android.content.Context; import android.os.IBinder; import android.os.RemoteException; -import android.window.IOnFpsCallbackListener; +import android.window.ITaskFpsCallback; import java.util.HashMap; final class TaskFpsCallbackController { private final Context mContext; - private final HashMap mTaskFpsCallbackListeners; - private final HashMap mDeathRecipients; + private final HashMap mTaskFpsCallbacks; + private final HashMap mDeathRecipients; TaskFpsCallbackController(Context context) { mContext = context; - mTaskFpsCallbackListeners = new HashMap<>(); + mTaskFpsCallbacks = new HashMap<>(); mDeathRecipients = new HashMap<>(); } - void registerCallback(int taskId, IOnFpsCallbackListener listener) { - if (mTaskFpsCallbackListeners.containsKey(listener)) { + void registerListener(int taskId, ITaskFpsCallback callback) { + if (mTaskFpsCallbacks.containsKey(callback)) { return; } - final long nativeListener = nativeRegister(listener, taskId); - mTaskFpsCallbackListeners.put(listener, nativeListener); + final long nativeListener = nativeRegister(callback, taskId); + mTaskFpsCallbacks.put(callback, nativeListener); - final IBinder.DeathRecipient deathRecipient = () -> unregisterCallback(listener); + final IBinder.DeathRecipient deathRecipient = () -> unregisterListener(callback); try { - listener.asBinder().linkToDeath(deathRecipient, 0); - mDeathRecipients.put(listener, deathRecipient); + callback.asBinder().linkToDeath(deathRecipient, 0); + mDeathRecipients.put(callback, deathRecipient); } catch (RemoteException e) { // ignore } } - void unregisterCallback(IOnFpsCallbackListener listener) { - if (!mTaskFpsCallbackListeners.containsKey(listener)) { + void unregisterListener(ITaskFpsCallback callback) { + if (!mTaskFpsCallbacks.containsKey(callback)) { return; } - listener.asBinder().unlinkToDeath(mDeathRecipients.get(listener), 0); - mDeathRecipients.remove(listener); + callback.asBinder().unlinkToDeath(mDeathRecipients.get(callback), 0); + mDeathRecipients.remove(callback); - nativeUnregister(mTaskFpsCallbackListeners.get(listener)); - mTaskFpsCallbackListeners.remove(listener); + nativeUnregister(mTaskFpsCallbacks.get(callback)); + mTaskFpsCallbacks.remove(callback); } - private static native long nativeRegister(IOnFpsCallbackListener listener, int taskId); + private static native long nativeRegister(ITaskFpsCallback callback, int taskId); private static native void nativeUnregister(long ptr); } diff --git a/services/core/java/com/android/server/wm/WindowManagerService.java b/services/core/java/com/android/server/wm/WindowManagerService.java index 4d1bc22f24376..74e1c6ffa21eb 100644 --- a/services/core/java/com/android/server/wm/WindowManagerService.java +++ b/services/core/java/com/android/server/wm/WindowManagerService.java @@ -283,7 +283,7 @@ import android.view.WindowManagerPolicyConstants.PointerEventListener; import android.view.displayhash.DisplayHash; import android.view.displayhash.VerifiedDisplayHash; import android.window.ClientWindowFrames; -import android.window.IOnFpsCallbackListener; +import android.window.ITaskFpsCallback; import android.window.TaskSnapshot; import com.android.internal.R; @@ -8875,7 +8875,7 @@ public class WindowManagerService extends IWindowManager.Stub @Override @RequiresPermission(Manifest.permission.ACCESS_FPS_COUNTER) public void registerTaskFpsCallback(@IntRange(from = 0) int taskId, - IOnFpsCallbackListener listener) { + ITaskFpsCallback callback) { if (mContext.checkCallingOrSelfPermission(Manifest.permission.ACCESS_FPS_COUNTER) != PackageManager.PERMISSION_GRANTED) { final int pid = Binder.getCallingPid(); @@ -8887,12 +8887,12 @@ public class WindowManagerService extends IWindowManager.Stub throw new IllegalArgumentException("no task with taskId: " + taskId); } - mTaskFpsCallbackController.registerCallback(taskId, listener); + mTaskFpsCallbackController.registerListener(taskId, callback); } @Override @RequiresPermission(Manifest.permission.ACCESS_FPS_COUNTER) - public void unregisterTaskFpsCallback(IOnFpsCallbackListener listener) { + public void unregisterTaskFpsCallback(ITaskFpsCallback callback) { if (mContext.checkCallingOrSelfPermission(Manifest.permission.ACCESS_FPS_COUNTER) != PackageManager.PERMISSION_GRANTED) { final int pid = Binder.getCallingPid(); @@ -8900,7 +8900,7 @@ public class WindowManagerService extends IWindowManager.Stub + ", must have permission " + Manifest.permission.ACCESS_FPS_COUNTER); } - mTaskFpsCallbackController.unregisterCallback(listener); + mTaskFpsCallbackController.unregisterListener(callback); } @Override diff --git a/services/core/jni/com_android_server_wm_TaskFpsCallbackController.cpp b/services/core/jni/com_android_server_wm_TaskFpsCallbackController.cpp index 0202306fc3951..0a60e0df156f3 100644 --- a/services/core/jni/com_android_server_wm_TaskFpsCallbackController.cpp +++ b/services/core/jni/com_android_server_wm_TaskFpsCallbackController.cpp @@ -99,7 +99,7 @@ void nativeUnregister(JNIEnv* env, jclass clazz, jlong ptr) { static const JNINativeMethod gMethods[] = { /* name, signature, funcPtr */ - {"nativeRegister", "(Landroid/window/IOnFpsCallbackListener;I)J", (void*)nativeRegister}, + {"nativeRegister", "(Landroid/window/ITaskFpsCallback;I)J", (void*)nativeRegister}, {"nativeUnregister", "(J)V", (void*)nativeUnregister}}; } // namespace @@ -113,7 +113,7 @@ int register_com_android_server_wm_TaskFpsCallbackController(JNIEnv* env) { gCallbackClassInfo.mClass = MakeGlobalRefOrDie(env, clazz); gCallbackClassInfo.mDispatchOnFpsReported = env->GetStaticMethodID(clazz, "dispatchOnFpsReported", - "(Landroid/window/IOnFpsCallbackListener;F)V"); + "(Landroid/window/ITaskFpsCallback;F)V"); return 0; }