Create "stream texture proxy" before "stream texture"

Create "stream texture proxy" before "stream texture"

Background: WebMediaPlayerAndroid's constructor does not know
video meta data. Meta data is received in OnVideoSizeChanged().
If ShouldUseVideoOverlayForEmbeddedEncryptedVideo answers true
WebMediaPlayerAndroid's constructor postpones stream texture
creation until OnVideoSizeChanged().

Problem: When constructor sets needs_establish_peer_ = false,
TryCreateStreamTextureProxyIfNeeded() does not create a proxy.
Without a proxy OnVideoSizeChanged() cannot create a stream
texture.

Solution: Once we get meta data, create proxy AND texture.

BUG=433833
TEST=Manual,
 1. Play a Youtube clip (no black frame).
 2. Play a purchased movie at Youtube (no black frame).

Committed: https://crrev.com/5e431c15487b8e9f749810435eb0ef491af0737e
Cr-Commit-Position: refs/heads/master@{#304995}

[Hugo Holgersson, 2014-11-17 15:55:57]

hugo.holgersson@sonymobile.com changed reviewers:
+ boliu@chromium.org, gunsch@chromium.org, qinmin@chromium.org,
xhwang@chromium.org, ycheo@chromium.org

[boliu, 2014-11-17 16:16:55]

Please do not add so many reviewers. You only need qinmin@ who's the owner of
this code

[qinmin, 2014-11-17 19:06:45]

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

https://codereview.chromium.org/730103002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:168:
player_manager_->ShouldUseVideoOverlayForEmbeddedEncryptedVideo()) {
If this is true, needs_establish_peer_ will be set to false.
But SetNeedsEstablishPeer(true) will be called if the video is not EME in
OnVideoSizeChanged(). And in that case, we will immediately create the
stream_texture, and call ReallocateVideoFrame(), how come we will still see the
black frame?

[Hugo Holgersson, 2014-11-18 10:14:20]

On 2014/11/17 19:06:45, qinmin wrote:
>
https://codereview.chromium.org/730103002/diff/1/content/renderer/media/andro...
> File content/renderer/media/android/webmediaplayer_android.cc (left):
> 
>
https://codereview.chromium.org/730103002/diff/1/content/renderer/media/andro...
> content/renderer/media/android/webmediaplayer_android.cc:168:
> player_manager_->ShouldUseVideoOverlayForEmbeddedEncryptedVideo()) {
> If this is true, needs_establish_peer_ will be set to false.
> But SetNeedsEstablishPeer(true) will be called if the video is not EME in
> OnVideoSizeChanged(). And in that case, we will immediately create the
> stream_texture, and call ReallocateVideoFrame(), how come we will still see
the
> black frame?

SetNeedsEstablishPeer(true), in OnVideoSizeChanged(), is called only if we have
a stream_texture_proxy_.

We got no stream_texture_proxy_ because TryCreateStreamTextureProxyIfNeeded()
returns if (!needs_establish_peer_).

[qinmin, 2014-11-18 18:55:37]

On 2014/11/18 10:14:20, Hugo Holgersson wrote:
> On 2014/11/17 19:06:45, qinmin wrote:
> >
>
https://codereview.chromium.org/730103002/diff/1/content/renderer/media/andro...
> > File content/renderer/media/android/webmediaplayer_android.cc (left):
> > 
> >
>
https://codereview.chromium.org/730103002/diff/1/content/renderer/media/andro...
> > content/renderer/media/android/webmediaplayer_android.cc:168:
> > player_manager_->ShouldUseVideoOverlayForEmbeddedEncryptedVideo()) {
> > If this is true, needs_establish_peer_ will be set to false.
> > But SetNeedsEstablishPeer(true) will be called if the video is not EME in
> > OnVideoSizeChanged(). And in that case, we will immediately create the
> > stream_texture, and call ReallocateVideoFrame(), how come we will still see
> the
> > black frame?
> 
> SetNeedsEstablishPeer(true), in OnVideoSizeChanged(), is called only if we
have
> a stream_texture_proxy_.
> 
> We got no stream_texture_proxy_ because TryCreateStreamTextureProxyIfNeeded()
> returns if (!needs_establish_peer_).

I see. Seems the logic in 853-857 is wrong due to recent refactoring.
TryCreateStreamTextureProxyIfNeeded() will either create the stream_id and
stream_texture_proxy_ at the same time, or fail to create both of them.
So the correct fix is to change that code piece to  
else if (!stream_texture_proxy_) {
  // Do deferred stream texture creation finally.
  SetNeedsEstablishPeer(true);
  TryCreateStreamTextureProxyIfNeeded();
}

[Hugo Holgersson, 2014-11-19 12:25:38]

On 2014/11/18 18:55:37, qinmin wrote:
> On 2014/11/18 10:14:20, Hugo Holgersson wrote:
> > On 2014/11/17 19:06:45, qinmin wrote:
> > >
> >
>
https://codereview.chromium.org/730103002/diff/1/content/renderer/media/andro...
> > > File content/renderer/media/android/webmediaplayer_android.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/730103002/diff/1/content/renderer/media/andro...
> > > content/renderer/media/android/webmediaplayer_android.cc:168:
> > > player_manager_->ShouldUseVideoOverlayForEmbeddedEncryptedVideo()) {
> > > If this is true, needs_establish_peer_ will be set to false.
> > > But SetNeedsEstablishPeer(true) will be called if the video is not EME in
> > > OnVideoSizeChanged(). And in that case, we will immediately create the
> > > stream_texture, and call ReallocateVideoFrame(), how come we will still
see
> > the
> > > black frame?
> > 
> > SetNeedsEstablishPeer(true), in OnVideoSizeChanged(), is called only if we
> have
> > a stream_texture_proxy_.
> > 
> > We got no stream_texture_proxy_ because
TryCreateStreamTextureProxyIfNeeded()
> > returns if (!needs_establish_peer_).
> 
> I see. Seems the logic in 853-857 is wrong due to recent refactoring.
> TryCreateStreamTextureProxyIfNeeded() will either create the stream_id and
> stream_texture_proxy_ at the same time, or fail to create both of them.
> So the correct fix is to change that code piece to  
> else if (!stream_texture_proxy_) {
>   // Do deferred stream texture creation finally.
>   SetNeedsEstablishPeer(true);
>   TryCreateStreamTextureProxyIfNeeded();
> }

Yes! I uploaded your suggestion. I also rewrote description to fit the new
solution.

[qinmin, 2014-11-19 18:30:26]

lgtm

[Hugo Holgersson, 2014-11-20 08:16:16]

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

[commit-bot: I haz the power, 2014-11-20 08:16:35]

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

[commit-bot: I haz the power, 2014-11-20 09:03:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-20 09:04:44]

Patchset 2 (id:??) landed as
https://crrev.com/5e431c15487b8e9f749810435eb0ef491af0737e
Cr-Commit-Position: refs/heads/master@{#304995}