commit d1fdf3a85be12a846088a22dde27482e1d467f32
author Tyler Gunn <tgunn@google.com> Tue Nov 05 15:47:58 2019 -0800
committer Tyler Gunn <tgunn@google.com> Tue Nov 12 10:43:09 2019 -0800
tree 34993aa26430bb591d0037df0dea74779f167456
parent e8a5b8a76d30c21067a5ce391b15e1da750eb84c

Cleanup video provider binder code.

1. Ensure we always unlink for death from the VideoCallImpl.
2. Ensure we compare the binders in the Call instance properly so that
we are not always replacing the VideoCallImpl used.
3. Ensure we cleanup the VideoCallImpl when its being removed.

Test: Manual testing of video calls for regressions.
Bug: 121156974
Change-Id: I440e8fb4ab9a5051ee02358933c495de736e2bd0

telecomm/java/android/telecom/Call.java

diff --git a/telecomm/java/android/telecom/Call.java b/telecomm/java/android/telecom/Call.java
index 60290e3..3b36364 100644
--- a/telecomm/java/android/telecom/Call.java
+++ b/telecomm/java/android/telecom/Call.java
@@ -25,8 +25,11 @@
 import android.os.Build;
 import android.os.Bundle;
 import android.os.Handler;
+import android.os.IBinder;
 import android.os.ParcelFileDescriptor;
 
+import com.android.internal.telecom.IVideoProvider;
+
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.OutputStreamWriter;
@@ -2130,13 +2133,22 @@
             cannedTextResponsesChanged = true;
         }
 
-        VideoCallImpl newVideoCallImpl = parcelableCall.getVideoCallImpl(mCallingPackage,
-                mTargetSdkVersion);
-        boolean videoCallChanged = parcelableCall.isVideoCallProviderChanged() &&
-                !Objects.equals(mVideoCallImpl, newVideoCallImpl);
+        IVideoProvider previousVideoProvider = mVideoCallImpl == null ? null :
+                mVideoCallImpl.getVideoProvider();
+        IVideoProvider newVideoProvider = parcelableCall.getVideoProvider();
+
+        // parcelableCall.isVideoCallProviderChanged is only true when we have a video provider
+        // specified; so we should check if the actual IVideoProvider changes as well.
+        boolean videoCallChanged = parcelableCall.isVideoCallProviderChanged()
+                && !Objects.equals(previousVideoProvider, newVideoProvider);
         if (videoCallChanged) {
-            mVideoCallImpl = newVideoCallImpl;
+            if (mVideoCallImpl != null) {
+                mVideoCallImpl.destroy();
+            }
+            mVideoCallImpl = parcelableCall.isVideoCallProviderChanged() ?
+                    parcelableCall.getVideoCallImpl(mCallingPackage, mTargetSdkVersion) : null;
         }
+
         if (mVideoCallImpl != null) {
             mVideoCallImpl.setVideoState(getDetails().getVideoState());
         }

telecomm/java/android/telecom/ParcelableCall.java

diff --git a/telecomm/java/android/telecom/ParcelableCall.java b/telecomm/java/android/telecom/ParcelableCall.java
index aa50991..fdc3243 100644
--- a/telecomm/java/android/telecom/ParcelableCall.java
+++ b/telecomm/java/android/telecom/ParcelableCall.java
@@ -21,6 +21,7 @@
 import android.net.Uri;
 import android.os.Build;
 import android.os.Bundle;
+import android.os.IBinder;
 import android.os.Parcel;
 import android.os.Parcelable;
 import android.os.RemoteException;
@@ -225,6 +226,10 @@
         return mVideoCall;
     }
 
+    public IVideoProvider getVideoProvider() {
+        return mVideoCallProvider;
+    }
+
     public boolean getIsRttCallChanged() {
         return mIsRttCallChanged;
     }

telecomm/java/android/telecom/VideoCallImpl.java

diff --git a/telecomm/java/android/telecom/VideoCallImpl.java b/telecomm/java/android/telecom/VideoCallImpl.java
index cb74012..4a1aa0a 100644
--- a/telecomm/java/android/telecom/VideoCallImpl.java
+++ b/telecomm/java/android/telecom/VideoCallImpl.java
@@ -32,6 +32,8 @@
 import com.android.internal.telecom.IVideoCallback;
 import com.android.internal.telecom.IVideoProvider;
 
+import java.util.NoSuchElementException;
+
 /**
  * Implementation of a Video Call, which allows InCallUi to communicate commands to the underlying
  * {@link Connection.VideoProvider}, and direct callbacks from the
@@ -53,7 +55,11 @@
     private IBinder.DeathRecipient mDeathRecipient = new IBinder.DeathRecipient() {
         @Override
         public void binderDied() {
-            mVideoProvider.asBinder().unlinkToDeath(this, 0);
+            try {
+                mVideoProvider.asBinder().unlinkToDeath(this, 0);
+            } catch (NoSuchElementException nse) {
+                // Already unlinked in destroy below.
+            }
         }
     };
 
@@ -222,6 +228,11 @@
     @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P, trackingBug = 127403196)
     public void destroy() {
         unregisterCallback(mCallback);
+        try {
+            mVideoProvider.asBinder().unlinkToDeath(mDeathRecipient, 0);
+        } catch (NoSuchElementException nse) {
+            // Already unlinked in binderDied above.
+        }
     }
 
     /** {@inheritDoc} */
@@ -353,4 +364,12 @@
     public void setVideoState(int videoState) {
         mVideoState = videoState;
     }
+
+    /**
+     * Get the video provider binder.
+     * @return the video provider binder.
+     */
+    public IVideoProvider getVideoProvider() {
+        return mVideoProvider;
+    }
 }