Recover from timing problem at exit of video-fullscreen

content/renderer/media/android/webmediaplayer_android.cc

Index: content/renderer/media/android/webmediaplayer_android.cc
diff --git a/content/renderer/media/android/webmediaplayer_android.cc b/content/renderer/media/android/webmediaplayer_android.cc
index e4b6fb0d731f857fea72d99788efd564223bda66..f1597fbdad701e022d85beb7f6fa94f1a0ca250b 100644
--- a/content/renderer/media/android/webmediaplayer_android.cc
+++ b/content/renderer/media/android/webmediaplayer_android.cc
@@ -948,8 +948,10 @@ void WebMediaPlayerAndroid::OnDidExitFullscreen() {
     SetNeedsEstablishPeer(true);
   // We had the fullscreen surface connected to Android MediaPlayer,
   // so reconnect our surface texture for embedded playback.
-  if (!paused() && needs_establish_peer_)
+  if (!paused() && needs_establish_peer_) {
+    TryCreateStreamTextureProxyIfNeeded();

[boliu, 2015/07/06 18:34:20]

how would this help if the problem is needs_establish_peer_ is false?

[Hugo Holgersson, 2015/07/06 20:53:53]

At this point, here in OnDidExitFullscreen(), needs_establish_peer_ is true! It
is set at line 948.

The problem is that OnDidExitFullscreen()'s EstablishSurfaceTexturePeer() will
do nothing because it returns on !stream_texture_proxy_ (so inline video
freezes). There is no stream_texture_proxy_  because ResetStreamTextureProxy()
did stream_texture_proxy_.reset() but did not create a new one.

Most often, however, OnDidExitFullscreen() is queued before
ResetStreamTextureProxy(). Then everything is fine: ResetStreamTextureProxy()
resets AND recreates stream_texture_proxy because needs_establish_peer_ is set
true on line 948.

[boliu, 2015/07/06 21:05:48]

Ahh, ok. Then the more correct fix is to create a new proxy from
RestoreContextOnMainThread then. The StreamTextureFactoryContextObserver method
should be renamed to something else, to not conflict with existing calls to
ResetStreamTextureProxy

[Hugo Holgersson, 2015/07/06 22:30:46]

Hmm. I don't quite understand.
SynchronousCompositorFactoryImpl::RestoreContextOnMainThread() runs
WebMediaPlayerAndroid::ResetStreamTextureProxy() already... So
RestoreContextOnMainThread() *usually* creates a new proxy! Only it doesn't in
this CL's corner case because OnDidExitFullscreen() has not yet updated the
state variable needs_establish_peer_: if the player is still in fullscreen it
will not (and should not?) bother to create a new proxy. This CL creates the
proxy once fullscreen exits.

[boliu, 2015/07/06 23:33:21]

I meant RestoreContextOnMainThread will call a different method, that will do
essentially what ResetStreamTextureProxy does right now, except it ignores
needs_establish_peer_. I'm not sure if it breaks other cases though, like video
hole punching.

Otherwise, I have no better alternative than the current patch.

Min: thoughts?

[Hugo Holgersson, 2015/07/08 13:23:50]

If that method ignores needs_establish_peer_ it might create a new proxy that is
not needed?
1. If player is still in fullscreen (proxy is not needed until fullscreen
exits). 
2. For video hole, stream textures are not used so neither is the proxy.

That's why I think ResetStreamTextureProxy()'s current logic is good - but we
could rename it to RemoveStreamTextureProxyAndRecreateIfNeeded() to make this
semantics clearer?

In patch set #2 I factored out ResetStreamTextureProxy()'s "remove"-code into
RemoveSurfaceTextureAndProxy(). The new method decouples the player's "private"
need for ResetStreamTextureProxy() so we can simply delete
ResetStreamTextureProxy() once the long term solution (video-context on gpu
thread) is ready.

Patch set #2 just adds re-factoring, so we still need this line to solve the
bug. TryCreateStreamTextureProxyIfNeeded() already precedes
EstablishSurfaceTexturePeer() at 3 places: play(), OnVideoSizeChanged() and
ResetStreamTextureProxy(), so it just follows that praxis.
TryCreateStreamTextureProxyIfNeeded() directly returns if a new proxy isn't
needed, so there no penalty in the usual Chrome-case.

[boliu, 2015/07/08 15:26:29]

It could recreate only if there is already an existing proxy. But yeah, it's not
a very good solution

     EstablishSurfaceTexturePeer();
+  }
 
 #if defined(VIDEO_HOLE)
   if (!paused() && needs_external_surface_)