Correct issues with VideoProvider discovered via CTS tests.
While authoring the CTS tests I discovered a few issues with the
VideoProvider.
1. The VideoProvider's default handler should be on the main Looper,
similar to other Telecom components.
2. When calling IVideoCallbacks, the try/catch for RemoteExceptions was
around the for-loop which iterates over the various VideoCallbacks. If
one was to fail, the other VideoCallbacks would not be called.
3. The mVideoCallbacks hashmap should use a ConcurrentHashMap; in CTS
tests I ran into a few concurrency exceptions related to the various
InCallServices registering callbacks while other methods on the provider
were being called.
Bug: 21802841
Change-Id: Ib0d46daf03554309044e9efaa991a15cb2c4b46b
diff --git a/telecomm/java/android/telecom/Connection.java b/telecomm/java/android/telecom/Connection.java
index 9a63aa3..91566f8 100644
--- a/telecomm/java/android/telecom/Connection.java
+++ b/telecomm/java/android/telecom/Connection.java
@@ -448,8 +448,13 @@
/**
* Stores a list of the video callbacks, keyed by IBinder.
+ *
+ * ConcurrentHashMap constructor params: 8 is initial table size, 0.9f is
+ * load factor before resizing, 1 means we only expect a single thread to
+ * access the map so make only a single shard
*/
- private HashMap<IBinder, IVideoCallback> mVideoCallbacks = new HashMap<>();
+ private ConcurrentHashMap<IBinder, IVideoCallback> mVideoCallbacks =
+ new ConcurrentHashMap<IBinder, IVideoCallback>(8, 0.9f, 1);
/**
* Default handler used to consolidate binder method calls onto a single thread.
@@ -470,12 +475,16 @@
IBinder binder = (IBinder) msg.obj;
IVideoCallback callback = IVideoCallback.Stub
.asInterface((IBinder) msg.obj);
+ if (callback == null) {
+ Log.w(this, "addVideoProvider - skipped; callback is null.");
+ break;
+ }
+
if (mVideoCallbacks.containsKey(binder)) {
Log.i(this, "addVideoProvider - skipped; already present.");
break;
}
mVideoCallbacks.put(binder, callback);
- Log.i(this, "addVideoProvider "+ mVideoCallbacks.size());
break;
}
case MSG_REMOVE_VIDEO_CALLBACK: {
@@ -594,7 +603,7 @@
public VideoProvider() {
mBinder = new VideoProvider.VideoProviderBinder();
- mMessageHandler = new VideoProvider.VideoProviderHandler();
+ mMessageHandler = new VideoProvider.VideoProviderHandler(Looper.getMainLooper());
}
/**
@@ -763,11 +772,12 @@
*/
public void receiveSessionModifyRequest(VideoProfile videoProfile) {
if (mVideoCallbacks != null) {
- try {
- for (IVideoCallback callback : mVideoCallbacks.values()) {
+ for (IVideoCallback callback : mVideoCallbacks.values()) {
+ try {
callback.receiveSessionModifyRequest(videoProfile);
+ } catch (RemoteException ignored) {
+ Log.w(this, "receiveSessionModifyRequest callback failed", ignored);
}
- } catch (RemoteException ignored) {
}
}
}
@@ -793,12 +803,13 @@
public void receiveSessionModifyResponse(int status,
VideoProfile requestedProfile, VideoProfile responseProfile) {
if (mVideoCallbacks != null) {
- try {
- for (IVideoCallback callback : mVideoCallbacks.values()) {
+ for (IVideoCallback callback : mVideoCallbacks.values()) {
+ try {
callback.receiveSessionModifyResponse(status, requestedProfile,
responseProfile);
+ } catch (RemoteException ignored) {
+ Log.w(this, "receiveSessionModifyResponse callback failed", ignored);
}
- } catch (RemoteException ignored) {
}
}
}
@@ -819,11 +830,12 @@
*/
public void handleCallSessionEvent(int event) {
if (mVideoCallbacks != null) {
- try {
- for (IVideoCallback callback : mVideoCallbacks.values()) {
+ for (IVideoCallback callback : mVideoCallbacks.values()) {
+ try {
callback.handleCallSessionEvent(event);
+ } catch (RemoteException ignored) {
+ Log.w(this, "handleCallSessionEvent callback failed", ignored);
}
- } catch (RemoteException ignored) {
}
}
}
@@ -843,11 +855,12 @@
*/
public void changePeerDimensions(int width, int height) {
if (mVideoCallbacks != null) {
- try {
- for (IVideoCallback callback : mVideoCallbacks.values()) {
+ for (IVideoCallback callback : mVideoCallbacks.values()) {
+ try {
callback.changePeerDimensions(width, height);
+ } catch (RemoteException ignored) {
+ Log.w(this, "changePeerDimensions callback failed", ignored);
}
- } catch (RemoteException ignored) {
}
}
}
@@ -869,11 +882,12 @@
*/
public void setCallDataUsage(long dataUsage) {
if (mVideoCallbacks != null) {
- try {
- for (IVideoCallback callback : mVideoCallbacks.values()) {
+ for (IVideoCallback callback : mVideoCallbacks.values()) {
+ try {
callback.changeCallDataUsage(dataUsage);
+ } catch (RemoteException ignored) {
+ Log.w(this, "setCallDataUsage callback failed", ignored);
}
- } catch (RemoteException ignored) {
}
}
}
@@ -905,11 +919,12 @@
*/
public void changeCameraCapabilities(VideoProfile.CameraCapabilities cameraCapabilities) {
if (mVideoCallbacks != null) {
- try {
- for (IVideoCallback callback : mVideoCallbacks.values()) {
+ for (IVideoCallback callback : mVideoCallbacks.values()) {
+ try {
callback.changeCameraCapabilities(cameraCapabilities);
+ } catch (RemoteException ignored) {
+ Log.w(this, "changeCameraCapabilities callback failed", ignored);
}
- } catch (RemoteException ignored) {
}
}
}
@@ -929,11 +944,12 @@
*/
public void changeVideoQuality(int videoQuality) {
if (mVideoCallbacks != null) {
- try {
- for (IVideoCallback callback : mVideoCallbacks.values()) {
+ for (IVideoCallback callback : mVideoCallbacks.values()) {
+ try {
callback.changeVideoQuality(videoQuality);
+ } catch (RemoteException ignored) {
+ Log.w(this, "changeVideoQuality callback failed", ignored);
}
- } catch (RemoteException ignored) {
}
}
}