Merge "Notify surface creation and surface change only after playback is requested" into tm-dev
diff --git a/src/com/android/providers/media/photopicker/ui/remotepreview/RemotePreviewHandler.java b/src/com/android/providers/media/photopicker/ui/remotepreview/RemotePreviewHandler.java
index cb0a151..ccf2885 100644
--- a/src/com/android/providers/media/photopicker/ui/remotepreview/RemotePreviewHandler.java
+++ b/src/com/android/providers/media/photopicker/ui/remotepreview/RemotePreviewHandler.java
@@ -94,7 +94,6 @@
*
* @param viewHolder {@link PreviewVideoHolder} for the media item under preview
* @param item {@link Item} to be previewed
- * @return true if the given {@link Item} can be previewed remotely, else false
*/
public void onViewAttachedToWindow(PreviewVideoHolder viewHolder, Item item) {
final RemotePreviewSession session = createRemotePreviewSession(item, viewHolder);
@@ -105,9 +104,6 @@
// anywhere else.
holder.removeCallback(mSurfaceHolderCallback);
holder.addCallback(mSurfaceHolderCallback);
-
- mCurrentPreviewState.item = item;
- mCurrentPreviewState.viewHolder = viewHolder;
}
/**
@@ -133,6 +129,9 @@
return false;
}
+ mCurrentPreviewState.item = item;
+ mCurrentPreviewState.viewHolder = session.getPreviewVideoHolder();
+
session.requestPlayMedia();
return true;
}
@@ -176,7 +175,7 @@
}
mSessionMap.put(holder, session);
- session.surfaceCreated(holder.getSurface());
+ session.surfaceCreated();
session.requestPlayMedia();
}
@@ -294,7 +293,7 @@
Surface surface = holder.getSurface();
RemotePreviewSession session = mSessionMap.get(holder);
- session.surfaceCreated(surface);
+ session.surfaceCreated();
}
@Override
diff --git a/src/com/android/providers/media/photopicker/ui/remotepreview/RemotePreviewSession.java b/src/com/android/providers/media/photopicker/ui/remotepreview/RemotePreviewSession.java
index 27df3c8..4cd7e8e 100644
--- a/src/com/android/providers/media/photopicker/ui/remotepreview/RemotePreviewSession.java
+++ b/src/com/android/providers/media/photopicker/ui/remotepreview/RemotePreviewSession.java
@@ -33,7 +33,6 @@
import android.os.RemoteException;
import android.provider.CloudMediaProvider.CloudMediaSurfaceStateChangedCallback.PlaybackState;
import android.util.Log;
-import android.view.Surface;
import android.view.View;
import android.view.accessibility.AccessibilityManager;
import android.view.accessibility.AccessibilityManager.AccessibilityStateChangeListener;
@@ -89,7 +88,9 @@
private final AccessibilityStateChangeListener mAccessibilityStateChangeListener =
this::updateAccessibilityState;
+ private SurfaceChangeData mSurfaceChangeData;
private boolean mIsSurfaceCreated = false;
+ private boolean mIsSurfaceCreationNotified = false;
private boolean mIsPlaybackRequested = false;
@PlaybackState
private int mCurrentPlaybackState = PLAYBACK_STATE_BUFFERING;
@@ -126,20 +127,36 @@
return mAuthority;
}
- void surfaceCreated(@NonNull Surface surface) {
+ @NonNull
+ PreviewVideoHolder getPreviewVideoHolder() {
+ return mPreviewVideoHolder;
+ }
+
+ void surfaceCreated() {
if (mIsSurfaceCreated) {
throw new IllegalStateException("Surface is already created.");
}
-
- if (surface == null) {
- throw new IllegalStateException("surfaceCreated() called with null surface.");
+ if (mIsSurfaceCreationNotified) {
+ throw new IllegalStateException(
+ "Surface creation has been already notified to SurfaceController.");
}
- try {
- mSurfaceController.onSurfaceCreated(mSurfaceId, surface, mMediaId);
- mIsSurfaceCreated = true;
- } catch (RemoteException e) {
- Log.e(TAG, "Failure in onSurfaceCreated().", e);
+ mIsSurfaceCreated = true;
+
+ // Notify surface creation only if playback has been already requested, else this will be
+ // done in requestPlayMedia() when playback is explicitly requested.
+ if (mIsPlaybackRequested) {
+ notifySurfaceCreated();
+ }
+ }
+
+ void surfaceChanged(int format, int width, int height) {
+ mSurfaceChangeData = new SurfaceChangeData(format, width, height);
+
+ // Notify surface change only if playback has been already requested, else this will be
+ // done in requestPlayMedia() when playback is explicitly requested.
+ if (mIsPlaybackRequested) {
+ notifySurfaceChanged();
}
}
@@ -148,8 +165,16 @@
throw new IllegalStateException("Surface is not created.");
}
+ mSurfaceChangeData = null;
+
tearDownUI();
+ if (!mIsSurfaceCreationNotified) {
+ // If we haven't notified surface creation yet, then no need to notify surface
+ // destruction either.
+ return;
+ }
+
try {
mSurfaceController.onSurfaceDestroyed(mSurfaceId);
} catch (RemoteException e) {
@@ -157,18 +182,6 @@
}
}
- void surfaceChanged(int format, int width, int height) {
- if (!mIsSurfaceCreated) {
- throw new IllegalStateException("Surface is not created.");
- }
-
- try {
- mSurfaceController.onSurfaceChanged(mSurfaceId, format, width, height);
- } catch (RemoteException e) {
- Log.e(TAG, "Failure in onSurfaceChanged().", e);
- }
- }
-
void requestPlayMedia() {
// When the user is at the first item in ViewPager, swiping further right would trigger the
// callback {@link ViewPager2.PageTransformer#transforPage(View, int)}, which would call
@@ -186,6 +199,15 @@
return;
}
+ // Now that playback has been requested, try to notify surface creation and surface change
+ // so that player can be prepared with the surface.
+ if (mIsSurfaceCreated) {
+ notifySurfaceCreated();
+ }
+ if (mSurfaceChangeData != null) {
+ notifySurfaceChanged();
+ }
+
mIsPlaybackRequested = true;
}
@@ -219,10 +241,54 @@
}
}
+ private void notifySurfaceCreated() {
+ if (!mIsSurfaceCreated) {
+ throw new IllegalStateException("Surface is not created.");
+ }
+ if (mIsSurfaceCreationNotified) {
+ throw new IllegalStateException(
+ "Surface creation has already been notified to SurfaceController.");
+ }
+
+ try {
+ mSurfaceController.onSurfaceCreated(mSurfaceId,
+ mPreviewVideoHolder.getSurfaceHolder().getSurface(), mMediaId);
+ mIsSurfaceCreationNotified = true;
+ } catch (RemoteException e) {
+ Log.e(TAG, "Failure in notifySurfaceCreated().", e);
+ }
+ }
+
+ private void notifySurfaceChanged() {
+ if (!mIsSurfaceCreated) {
+ throw new IllegalStateException("Surface is not created.");
+ }
+ if (!mIsSurfaceCreationNotified) {
+ throw new IllegalStateException(
+ "Surface creation has not been notified to SurfaceController.");
+ }
+
+ if (mSurfaceChangeData == null) {
+ throw new IllegalStateException("No surface change data present.");
+ }
+
+ try {
+ mSurfaceController.onSurfaceChanged(mSurfaceId, mSurfaceChangeData.getFormat(),
+ mSurfaceChangeData.getWidth(), mSurfaceChangeData.getHeight());
+ } catch (RemoteException e) {
+ Log.e(TAG, "Failure in notifySurfaceChanged().", e);
+ }
+ }
+
private void playMedia() {
if (!mIsSurfaceCreated) {
throw new IllegalStateException("Surface is not created.");
}
+ if (!mIsSurfaceCreationNotified) {
+ throw new IllegalStateException(
+ "Surface creation has not been notified to SurfaceController.");
+ }
+
if (mCurrentPlaybackState == PLAYBACK_STATE_STARTED) {
throw new IllegalStateException("Player is already playing.");
}
@@ -238,6 +304,11 @@
if (!mIsSurfaceCreated) {
throw new IllegalStateException("Surface is not created.");
}
+ if (!mIsSurfaceCreationNotified) {
+ throw new IllegalStateException(
+ "Surface creation has not been notified to SurfaceController.");
+ }
+
if (mCurrentPlaybackState != PLAYBACK_STATE_STARTED) {
throw new IllegalStateException("Player is not playing.");
}
@@ -269,11 +340,9 @@
}
private void initUI() {
- // We show both the player view and thumbnail view till the player is ready and we know the
- // media size, then we hide the thumbnail view. This works because in the layout, the
- // thumbnail view appears last in the FrameLayout container and will thus be shown over the
- // player view.
- mPreviewVideoHolder.getPlayerContainer().setVisibility(View.VISIBLE);
+ // We show the thumbnail view till the player is ready and when we know the
+ // media size, then we hide the thumbnail view.
+ mPreviewVideoHolder.getPlayerContainer().setVisibility(View.INVISIBLE);
mPreviewVideoHolder.getThumbnailView().setVisibility(View.VISIBLE);
mPreviewVideoHolder.getPlayerControlsRoot().setVisibility(View.GONE);
@@ -334,4 +403,29 @@
visible ? View.VISIBLE : View.GONE);
mPlayerControlsVisibilityStatus.setShouldShowPlayerControlsForNextItem(visible);
}
+
+ private static final class SurfaceChangeData {
+
+ private int mFormat;
+ private int mWidth;
+ private int mHeight;
+
+ SurfaceChangeData(int format, int width, int height) {
+ mFormat = format;
+ mWidth = width;
+ mHeight = height;
+ }
+
+ int getFormat() {
+ return mFormat;
+ }
+
+ int getWidth() {
+ return mWidth;
+ }
+
+ int getHeight() {
+ return mHeight;
+ }
+ }
}
diff --git a/tests/src/com/android/providers/media/photopicker/espresso/PreviewMultiSelectLongPressTest.java b/tests/src/com/android/providers/media/photopicker/espresso/PreviewMultiSelectLongPressTest.java
index 62c6a8a..de6ad97 100644
--- a/tests/src/com/android/providers/media/photopicker/espresso/PreviewMultiSelectLongPressTest.java
+++ b/tests/src/com/android/providers/media/photopicker/espresso/PreviewMultiSelectLongPressTest.java
@@ -101,9 +101,11 @@
try (ViewPager2IdlingResource idlingResource
= ViewPager2IdlingResource.register(mRule, PREVIEW_VIEW_PAGER_ID)) {
- // Verify video player is displayed
assertMultiSelectLongPressCommonLayoutMatches();
- onView(withId(R.id.preview_player_container)).check(matches(isDisplayed()));
+ // Verify thumbnail view is displayed
+ onView(withId(R.id.preview_video_image)).check(matches(isDisplayed()));
+ // TODO (b/232792753): Assert video player visibility using custom IdlingResource
+
// Verify no special format icon is previewed
onView(withId(PREVIEW_MOTION_PHOTO_ID)).check(doesNotExist());
onView(withId(PREVIEW_GIF_ID)).check(doesNotExist());
diff --git a/tests/src/com/android/providers/media/photopicker/espresso/PreviewMultiSelectTest.java b/tests/src/com/android/providers/media/photopicker/espresso/PreviewMultiSelectTest.java
index 0f07ed0..3920059 100644
--- a/tests/src/com/android/providers/media/photopicker/espresso/PreviewMultiSelectTest.java
+++ b/tests/src/com/android/providers/media/photopicker/espresso/PreviewMultiSelectTest.java
@@ -65,7 +65,7 @@
@RunWith(AndroidJUnit4ClassRunner.class)
public class PreviewMultiSelectTest extends PhotoPickerBaseTest {
- private static final int PLAYER_VIEW_ID = R.id.preview_player_container;
+ private static final int VIDEO_PREVIEW_THUMBNAIL_ID = R.id.preview_video_image;
@Rule
public ActivityScenarioRule<PhotoPickerTestActivity> mRule
@@ -207,7 +207,7 @@
// TODO(b/197083539): We don't check the video image to be visible or not because its
// visibility is time sensitive. Try waiting till player is ready and assert that video
// image is no more visible.
- onView(ViewPagerMatcher(PREVIEW_VIEW_PAGER_ID, PLAYER_VIEW_ID))
+ onView(ViewPagerMatcher(PREVIEW_VIEW_PAGER_ID, VIDEO_PREVIEW_THUMBNAIL_ID))
.check(matches(isDisplayed()));
// Verify no special format icon is previewed
assertSpecialFormatBadgeDoesNotExist();
@@ -313,16 +313,17 @@
// Verify that "View Selected" shows the video item, not the image item that was
// previewed earlier with preview on long press
- onView(ViewPagerMatcher(PREVIEW_VIEW_PAGER_ID, PLAYER_VIEW_ID))
+ onView(ViewPagerMatcher(PREVIEW_VIEW_PAGER_ID, VIDEO_PREVIEW_THUMBNAIL_ID))
.check(matches(isDisplayed()));
// Swipe and verify we don't preview the image item
swipeLeftAndWait(PREVIEW_VIEW_PAGER_ID);
- onView(ViewPagerMatcher(PREVIEW_VIEW_PAGER_ID, PLAYER_VIEW_ID))
+ onView(ViewPagerMatcher(PREVIEW_VIEW_PAGER_ID, VIDEO_PREVIEW_THUMBNAIL_ID))
.check(matches(isDisplayed()));
swipeRightAndWait(PREVIEW_VIEW_PAGER_ID);
- onView(ViewPagerMatcher(PREVIEW_VIEW_PAGER_ID, PLAYER_VIEW_ID))
+ onView(ViewPagerMatcher(PREVIEW_VIEW_PAGER_ID, VIDEO_PREVIEW_THUMBNAIL_ID))
.check(matches(isDisplayed()));
+ // TODO (b/232792753): Assert video player visibility using custom IdlingResource
}
}
diff --git a/tests/src/com/android/providers/media/photopicker/espresso/PreviewSingleSelectTest.java b/tests/src/com/android/providers/media/photopicker/espresso/PreviewSingleSelectTest.java
index f1ad43a..df11c29 100644
--- a/tests/src/com/android/providers/media/photopicker/espresso/PreviewSingleSelectTest.java
+++ b/tests/src/com/android/providers/media/photopicker/espresso/PreviewSingleSelectTest.java
@@ -134,9 +134,11 @@
try (ViewPager2IdlingResource idlingResource
= ViewPager2IdlingResource.register(mRule, PREVIEW_VIEW_PAGER_ID)) {
- // Verify video player is displayed
assertSingleSelectCommonLayoutMatches();
- onView(withId(R.id.preview_player_container)).check(matches(isDisplayed()));
+ // Verify thumbnail view is displayed
+ onView(withId(R.id.preview_video_image)).check(matches(isDisplayed()));
+ // TODO (b/232792753): Assert video player visibility using custom IdlingResource
+
// Verify no special format icon is previewed
onView(withId(PREVIEW_MOTION_PHOTO_ID)).check(doesNotExist());
onView(withId(PREVIEW_GIF_ID)).check(doesNotExist());