Clean up WebMediaPlayerAndroid needs_establish_peer_

Clean up WebMediaPlayerAndroid needs_establish_peer_

needs_establish_peer_ has been used to skip actual estalishing a surface
texture peer based on many conditions. Not all places that modify
actually checks all these conditions so there are bound to be some bugs.

This CL aims to tease out all these different conditions and put them in
a single function EstablishSurfaceTexturePeerIfNeeded. This function
should be called any time these conditions are changed. Makes for much
saner reading.

BUG=412578
Committed: https://crrev.com/701cb640efb326aa107b5b0c25dcf91766fe765f
Cr-Commit-Position: refs/heads/master@{#294068}

[boliu, 2014-09-09 00:26:30]

boliu@chromium.org changed reviewers:
+ avayvod@chromium.org, qinmin@chromium.org, ycheo@chromium.org

[boliu, 2014-09-09 00:26:30]

PTAL

Clean up of this class is waaaaaayyyyy overdue.

[boliu, 2014-09-09 01:00:17]

cc+ igsolla

[ycheo (away), 2014-09-09 06:38:54]

LGTM.

Thanks for handling this.

BTW, I'll update the test result 2 days later, since Korea is thanks-giving
holiday by Wednesday.

[qinmin, 2014-09-09 06:39:19]

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

https://codereview.chromium.org/557593002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:166:
needs_external_surface_ = true;
why set this to true? if the video is not EME, no external surface is needed.
As mentioned by the comments above, it is not determined whether a stream
texture is needed or not.

https://codereview.chromium.org/557593002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:825: //
EstablishSurfaceTexturePeerIfNeeded() will not get called. As a result, the
when play is called, isn't updatePlayingState() always call into
EstablishSurfaceTexturePeerIfNeeded()?

https://codereview.chromium.org/557593002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:882: peer_established_
= false;
Sorry, I was confused with this call and the
FullscreenController::DidEnterFullscreen() earlier.

your change will cause racing issue. If EnterFullscreen() is called and we don't
set peer_established_, then later on a play() call will find that it should
establish the surface texture peer. So now the fullscreen surface will compete
with the surface texture in the browser process to bind to the media player.

https://codereview.chromium.org/557593002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:886: //
|needs_external_surface_| is always false on non-TV devices.
why removing the if statement? And why not removing the comment?

https://codereview.chromium.org/557593002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:1224: if
(stream_texture_proxy_) {
why we remove the if condition? if we use external surface, we don't need to
create stream texture for now

https://codereview.chromium.org/557593002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:1264:
EstablishSurfaceTexturePeerIfNeeded();
UpdatePlayingState() can be called by 4 different functions, and only play()
actually requires the surface texture to be established. So why not simply put
it in play()?

[boliu, 2014-09-09 21:17:04]

The changes we made during our pair programming session is the diff between PS2
and PS3.

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

https://codereview.chromium.org/557593002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:166:
needs_external_surface_ = true;
On 2014/09/09 06:39:18, qinmin wrote:
> why set this to true? if the video is not EME, no external surface is needed.
> As mentioned by the comments above, it is not determined whether a stream
> texture is needed or not.

Done.

https://codereview.chromium.org/557593002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:825: //
EstablishSurfaceTexturePeerIfNeeded() will not get called. As a result, the
On 2014/09/09 06:39:18, qinmin wrote:
> when play is called, isn't updatePlayingState() always call into
> EstablishSurfaceTexturePeerIfNeeded()?

Done.

https://codereview.chromium.org/557593002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:882: peer_established_
= false;
On 2014/09/09 06:39:18, qinmin wrote:
> Sorry, I was confused with this call and the
> FullscreenController::DidEnterFullscreen() earlier.
> 
> your change will cause racing issue. If EnterFullscreen() is called and we
don't
> set peer_established_, then later on a play() call will find that it should
> establish the surface texture peer. So now the fullscreen surface will compete
> with the surface texture in the browser process to bind to the media player.

Done.

https://codereview.chromium.org/557593002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:886: //
|needs_external_surface_| is always false on non-TV devices.
On 2014/09/09 06:39:18, qinmin wrote:
> why removing the if statement? And why not removing the comment?

Done.

https://codereview.chromium.org/557593002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:1224: if
(stream_texture_proxy_) {
On 2014/09/09 06:39:18, qinmin wrote:
> why we remove the if condition? if we use external surface, we don't need to
> create stream texture for now

Done.

https://codereview.chromium.org/557593002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:1264:
EstablishSurfaceTexturePeerIfNeeded();
On 2014/09/09 06:39:18, qinmin wrote:
> UpdatePlayingState() can be called by 4 different functions, and only play()
> actually requires the surface texture to be established. So why not simply put
> it in play()?

Because contract is any states change should lead to calling
EstablishSurfaceTexturePeerIfNeeded. So I also put it in enterFullScreen.

[qinmin, 2014-09-09 21:34:22]

then why not OnDidExitFullscreen()?

> Because contract is any states change should lead to calling
> EstablishSurfaceTexturePeerIfNeeded. So I also put it in enterFullScreen.

[qinmin, 2014-09-09 21:35:54]

sorry, i mean OnConnectedToRemoteDevice()

On 2014/09/09 21:34:22, qinmin wrote:
> then why not OnDidExitFullscreen()?
> 
> > Because contract is any states change should lead to calling
> > EstablishSurfaceTexturePeerIfNeeded. So I also put it in enterFullScreen.

[boliu, 2014-09-09 21:37:32]

On 2014/09/09 21:35:54, qinmin wrote:
> sorry, i mean OnConnectedToRemoteDevice()
> 
> On 2014/09/09 21:34:22, qinmin wrote:
> > then why not OnDidExitFullscreen()?
> > 
> > > Because contract is any states change should lead to calling
> > > EstablishSurfaceTexturePeerIfNeeded. So I also put it in enterFullScreen.

That's a mistake. Added. Let me know if you find more.

The only exception I think are Release where it only unsets peer_established_,
but doesn't immdiately want to re-establish. Those have comments already.

[qinmin, 2014-09-09 21:38:40]

On 2014/09/09 21:37:32, boliu wrote:
> On 2014/09/09 21:35:54, qinmin wrote:
> > sorry, i mean OnConnectedToRemoteDevice()
> > 
> > On 2014/09/09 21:34:22, qinmin wrote:
> > > then why not OnDidExitFullscreen()?
> > > 
> > > > Because contract is any states change should lead to calling
> > > > EstablishSurfaceTexturePeerIfNeeded. So I also put it in
enterFullScreen.
> 
> That's a mistake. Added. Let me know if you find more.
> 
> The only exception I think are Release where it only unsets peer_established_,
> but doesn't immdiately want to re-establish. Those have comments already.

what about OnPlaybackComplete()

[boliu, 2014-09-09 21:42:03]

On 2014/09/09 21:38:40, qinmin wrote:
> On 2014/09/09 21:37:32, boliu wrote:
> > On 2014/09/09 21:35:54, qinmin wrote:
> > > sorry, i mean OnConnectedToRemoteDevice()
> > > 
> > > On 2014/09/09 21:34:22, qinmin wrote:
> > > > then why not OnDidExitFullscreen()?
> > > > 
> > > > > Because contract is any states change should lead to calling
> > > > > EstablishSurfaceTexturePeerIfNeeded. So I also put it in
> enterFullScreen.
> > 
> > That's a mistake. Added. Let me know if you find more.
> > 
> > The only exception I think are Release where it only unsets
peer_established_,
> > but doesn't immdiately want to re-establish. Those have comments already.
> 
> what about OnPlaybackComplete()

Can it call UpdatePlayingState(false)?

[boliu, 2014-09-09 22:01:22]

On 2014/09/09 21:42:03, boliu wrote:
> On 2014/09/09 21:38:40, qinmin wrote:
> > On 2014/09/09 21:37:32, boliu wrote:
> > > On 2014/09/09 21:35:54, qinmin wrote:
> > > > sorry, i mean OnConnectedToRemoteDevice()
> > > > 
> > > > On 2014/09/09 21:34:22, qinmin wrote:
> > > > > then why not OnDidExitFullscreen()?
> > > > > 
> > > > > > Because contract is any states change should lead to calling
> > > > > > EstablishSurfaceTexturePeerIfNeeded. So I also put it in
> > enterFullScreen.
> > > 
> > > That's a mistake. Added. Let me know if you find more.
> > > 
> > > The only exception I think are Release where it only unsets
> peer_established_,
> > > but doesn't immdiately want to re-establish. Those have comments already.
> > 
> > what about OnPlaybackComplete()
> 
> Can it call UpdatePlayingState(false)?

Removed it as discussed

[qinmin, 2014-09-09 22:13:42]

https://codereview.chromium.org/557593002/diff/60001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/557593002/diff/60001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.h:270: // conditions
chances and it is ok to establish peer.
s/chances/change/

https://codereview.chromium.org/557593002/diff/60001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.h:408: bool
peer_established_;
this is a poor variable name, we never know whether peer is established or not.
When we set this to true, we just send an IPC, and no idea whether browser side
will establish the peer or not. And the comments on the variable is not fixed.

[boliu, 2014-09-09 22:35:14]

https://codereview.chromium.org/557593002/diff/60001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/557593002/diff/60001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.h:270: // conditions
chances and it is ok to establish peer.
On 2014/09/09 22:13:41, qinmin wrote:
> s/chances/change/

Done.

https://codereview.chromium.org/557593002/diff/60001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.h:408: bool
peer_established_;
On 2014/09/09 22:13:41, qinmin wrote:
> this is a poor variable name, we never know whether peer is established or
not.
> When we set this to true, we just send an IPC, and no idea whether browser
side
> will establish the peer or not. And the comments on the variable is not fixed.

Renamed it back to needs_establish_peer_ as I can't think of a better name.
Which means every single condition is inverted.

Added more comment on how this should be used.

[qinmin, 2014-09-09 22:44:38]

lgtm % nits

https://codereview.chromium.org/557593002/diff/100001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/557593002/diff/100001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.h:408: // This should be
unset in EstablishSurfaceTexturePeerIfNeeded only, and set
nit:s/EstablishSurfaceTexturePeerIfNeeded/EstablishSurfaceTexturePeerIfNeeded()/

https://codereview.chromium.org/557593002/diff/100001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.h:414: // This is a helper
state used in EstablishSurfaceTexturePeerIfNeeded.
nit:s/EstablishSurfaceTexturePeerIfNeeded/EstablishSurfaceTexturePeerIfNeeded()/

[boliu, 2014-09-09 22:57:43]

The CQ bit was checked by boliu@chromium.org

[boliu, 2014-09-09 23:01:52]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2014-09-09 23:06:34]

nits fixed, submitting

Thanks for the review, pair programming, and heated debates :p

[boliu, 2014-09-09 23:06:41]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-09-09 23:25:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/557593002/120001

[commit-bot: I haz the power, 2014-09-10 00:35:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 00:35:04]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[boliu, 2014-09-10 01:14:02]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-09-10 01:27:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/557593002/120001

[commit-bot: I haz the power, 2014-09-10 01:35:22]

Committed patchset #7 (id:120001) as 55362f76228fb9fce2019b4cfbd042b1636014d5

[commit-bot: I haz the power, 2014-09-10 03:57:29]

Patchset 7 (id:??) landed as
https://crrev.com/701cb640efb326aa107b5b0c25dcf91766fe765f
Cr-Commit-Position: refs/heads/master@{#294068}

[boliu, 2014-09-11 00:23:09]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/562803002/ by boliu@chromium.org.

The reason for reverting is: Suspected to break tough_video_cases. See
crbug.com/412897 for details.

BUG=412897.

[ycheo (away), 2014-09-11 07:59:40]

On 2014/09/11 00:23:09, boliu wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
> https://codereview.chromium.org/562803002/ by mailto:boliu@chromium.org.
> 
> The reason for reverting is: Suspected to break tough_video_cases. See
> crbug.com/412897 for details.
> 
> BUG=412897.

FYI, it's OK that playing MSE and MSE/EME videos in WebView.