Recover from timing problem at exit of video-fullscreen

Recover from timing problem at exit of video-fullscreen

Background:
When exiting video fullscreen, UI thread sends two
IPC-messages the player's (renderer) thread.

1. MediaPlayerMsg_DidExitFullscreen is sent to tell
player to EstablishSurfaceTexturePeer() for inline
video. needs_establish_peer_ gets true.

2. SynchronousCompositorFactoryImpl posts a
callback to player's ResetStreamTextureProxy().

In *almost* all cases (1) happens before (2).

Problem:
When (2) happens before (1), (2) does not create a
new proxy because needs_establish_peer_ has not yet
been set. (1)'s EstablishSurfaceTexturePeer() fails
because there is no proxy.

Solution:
Let (1) do TryCreateStreamTextureProxyIfNeeded()
before EstablishSurfaceTexturePeer().

BUG=504800
TEST=See bug

Committed: https://crrev.com/93f9419bc378a6c311832c8463685f8a2b6c6ea0
Cr-Commit-Position: refs/heads/master@{#338027}

[Hugo Holgersson, 2015-06-26 13:09:01]

hugo.holgersson@sonymobile.com changed reviewers:
+ qinmin@chromium.org

[Hugo Holgersson, 2015-06-26 13:21:53]

@qinmin, I found this timing problem. PTAL. 

@boliu on CC, FYI, because of ResetStreamTextureProxy() we get this timing
problem. Is it intentional to recreate (OpenGL) context when fullscreen exits?
Is it dangerous to reuse the context the player had before fullscreen?

[qinmin, 2015-06-27 23:56:53]

On 2015/06/26 13:21:53, Hugo Holgersson wrote:
> @qinmin, I found this timing problem. PTAL. 
> 
> @boliu on CC, FYI, because of ResetStreamTextureProxy() we get this timing
> problem. Is it intentional to recreate (OpenGL) context when fullscreen exits?
> Is it dangerous to reuse the context the player had before fullscreen?

I am not familiar with the webview requirement, but why exiting fullscreen cause
us to post a task calling ResetStreamTextureProxy()?

[Hugo Holgersson, 2015-06-29 11:47:27]

On 2015/06/27 23:56:53, qinmin wrote:
> On 2015/06/26 13:21:53, Hugo Holgersson wrote:
> > @qinmin, I found this timing problem. PTAL. 
> > 
> > @boliu on CC, FYI, because of ResetStreamTextureProxy() we get this timing
> > problem. Is it intentional to recreate (OpenGL) context when fullscreen
exits?
> > Is it dangerous to reuse the context the player had before fullscreen?
> 
> I am not familiar with the webview requirement, but why exiting fullscreen
cause
> us to post a task calling ResetStreamTextureProxy()?

@boliu, do you have an idea? I will be out of office this week, so I cannot dig
deeper into this at the moment.

[qinmin, 2015-07-04 17:11:34]

qinmin@chromium.org changed reviewers:
+ boliu@chromium.org

[qinmin, 2015-07-04 17:11:35]

+boliu for webview

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

uhh, this external ResetStreamTextureProxy thing is webview-only. it's a big
hack to avoid some deadlock situations.

The real long term solution is to rewrite the video context to use the gpu
thread, which involves each frame, and avoid the deadlocks altogether.

Short term, you can try adding a delay to posting RestoreContextOnMainThread and
see if that helps. Let's avoid adding more hacks onto webmediaplayer_android,
which is shared between chrome and webview

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
content/renderer/media/android/webmediaplayer_android.cc:952:
TryCreateStreamTextureProxyIfNeeded();
how would this help if the problem is needs_establish_peer_ is false?

[Hugo Holgersson, 2015-07-06 20:53:00]

On 2015/07/06 18:34:20, boliu wrote:
> uhh, this external ResetStreamTextureProxy thing is webview-only. it's a big
> hack to avoid some deadlock situations.
> 
> The real long term solution is to rewrite the video context to use the gpu
> thread, which involves each frame, and avoid the deadlocks altogether.
>
Ok!
> 
> Short term, you can try adding a delay to posting RestoreContextOnMainThread
and
> see if that helps. Let's avoid adding more hacks onto webmediaplayer_android,
> which is shared between chrome and webview.
I think a delay would "help", but delays are hacks? :) This CL allows
WebMediaPlayerAndroid to handle the events in any order.

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

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
content/renderer/media/android/webmediaplayer_android.cc:952:
TryCreateStreamTextureProxyIfNeeded();
On 2015/07/06 18:34:20, boliu wrote:
> how would this help if the problem is needs_establish_peer_ is false?

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]

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
content/renderer/media/android/webmediaplayer_android.cc:952:
TryCreateStreamTextureProxyIfNeeded();
On 2015/07/06 20:53:53, Hugo Holgersson wrote:
> On 2015/07/06 18:34:20, boliu wrote:
> > how would this help if the problem is needs_establish_peer_ is false?
> 
> 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.

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

> 
> 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.

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

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
content/renderer/media/android/webmediaplayer_android.cc:952:
TryCreateStreamTextureProxyIfNeeded();
On 2015/07/06 21:05:48, boliu wrote:
> On 2015/07/06 20:53:53, Hugo Holgersson wrote:
> > On 2015/07/06 18:34:20, boliu wrote:
> > > how would this help if the problem is needs_establish_peer_ is false?
> > 
> > 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.
> 
> Ahh, ok. Then the more correct fix is to create a new proxy from
> RestoreContextOnMainThread then.
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.

> The StreamTextureFactoryContextObserver method
> should be renamed to something else, to not conflict with existing calls to
> ResetStreamTextureProxy
> 
> > 
> > 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 23:33:21]

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
content/renderer/media/android/webmediaplayer_android.cc:952:
TryCreateStreamTextureProxyIfNeeded();
On 2015/07/06 22:30:46, Hugo Holgersson wrote:
> On 2015/07/06 21:05:48, boliu wrote:
> > On 2015/07/06 20:53:53, Hugo Holgersson wrote:
> > > On 2015/07/06 18:34:20, boliu wrote:
> > > > how would this help if the problem is needs_establish_peer_ is false?
> > > 
> > > 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.
> > 
> > Ahh, ok. Then the more correct fix is to create a new proxy from
> > RestoreContextOnMainThread then.
> 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.

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?

> 
> > The StreamTextureFactoryContextObserver method
> > should be renamed to something else, to not conflict with existing calls to
> > ResetStreamTextureProxy
> > 
> > > 
> > > 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.
> > 
>

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

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
content/renderer/media/android/webmediaplayer_android.cc:952:
TryCreateStreamTextureProxyIfNeeded();
On 2015/07/06 23:33:21, boliu wrote:
> On 2015/07/06 22:30:46, Hugo Holgersson wrote:
> > On 2015/07/06 21:05:48, boliu wrote:
> > > On 2015/07/06 20:53:53, Hugo Holgersson wrote:
> > > > On 2015/07/06 18:34:20, boliu wrote:
> > > > > how would this help if the problem is needs_establish_peer_ is false?
> > > > 
> > > > 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.
> > > 
> > > Ahh, ok. Then the more correct fix is to create a new proxy from
> > > RestoreContextOnMainThread then.
> > 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.
> 
> 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?
> 

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.

> > 
> > > The StreamTextureFactoryContextObserver method
> > > should be renamed to something else, to not conflict with existing calls
to
> > > ResetStreamTextureProxy
> > > 
> > > > 
> > > > 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-08 15:26:29]

This looks ok. Deferring to min for review though

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1212823003/diff/1/content/renderer/media/andr...
content/renderer/media/android/webmediaplayer_android.cc:952:
TryCreateStreamTextureProxyIfNeeded();
On 2015/07/08 13:23:50, Hugo Holgersson wrote:
> On 2015/07/06 23:33:21, boliu wrote:
> > On 2015/07/06 22:30:46, Hugo Holgersson wrote:
> > > On 2015/07/06 21:05:48, boliu wrote:
> > > > On 2015/07/06 20:53:53, Hugo Holgersson wrote:
> > > > > On 2015/07/06 18:34:20, boliu wrote:
> > > > > > how would this help if the problem is needs_establish_peer_ is
false?
> > > > > 
> > > > > 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.
> > > > 
> > > > Ahh, ok. Then the more correct fix is to create a new proxy from
> > > > RestoreContextOnMainThread then.
> > > 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.
> > 
> > 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?
> > 
> 
> If that method ignores needs_establish_peer_ it might create a new proxy that
is
> not needed?

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

> 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.
> 
> > > 
> > > > The StreamTextureFactoryContextObserver method
> > > > should be renamed to something else, to not conflict with existing calls
> to
> > > > ResetStreamTextureProxy
> > > > 
> > > > > 
> > > > > 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.
> > > > 
> > > 
> > 
>

[qinmin, 2015-07-08 21:20:45]

lgtm

[Hugo Holgersson, 2015-07-09 12:27:58]

The CQ bit was checked by hugo.holgersson@sonymobile.com

[Hugo Holgersson, 2015-07-09 12:27:58]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/1212823003/#ps40001
(title: "Rebased and removed trailing blank line")

[commit-bot: I haz the power, 2015-07-09 12:28:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1212823003/40001

[commit-bot: I haz the power, 2015-07-09 13:17:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-09 13:18:20]

Patchset 3 (id:??) landed as
https://crrev.com/93f9419bc378a6c311832c8463685f8a2b6c6ea0
Cr-Commit-Position: refs/heads/master@{#338027}