[Android WebView] Clean up global GL resource pt.1

[Android WebView] Fix video in software mode

First ensure that main thread offscreen context is created after
on-screen context by keeping count of the number of compositors
that are initialized for hardware draw. Also release the compositor
thread offscreen context when the last compositor releases hardware
draw. Freeing the main thread context is still a TODO.

In software mode, still create StreamTextureFactory, but skip creating
StreamTextureProxy. WebMediaPlayerAndroid already works in this
mode, and it just means in-line video does not work in this case.
But video playback is still successful and user is able to enter fullscreen
to get working playback.

BUG=285349
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221843

[boliu, 2013-09-04 18:57:29]

Fixes the crash in b/10584901, but not everything.

After this, if the UI thread loses the race, then instead of crashing, the video
just will not play. Still need to teach video player to into full screen on play
in that case.

[no sievers, 2013-09-04 22:27:08]

lgtm

https://codereview.chromium.org/23618031/diff/1/content/browser/android/in_pr...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/23618031/diff/1/content/browser/android/in_pr...
content/browser/android/in_process/synchronous_compositor_impl.cc:167:
base::AutoLock lock(offscreen_context_for_compositor_thread_lock_);
Does this also need to check if we actually have a shared context, i.e.
num_hardware_compositors > 0?
Or should it at least DCHECK()?

https://codereview.chromium.org/23618031/diff/1/content/browser/android/in_pr...
content/browser/android/in_process/synchronous_compositor_impl.cc:205: unsigned
int num_hardware_compositor_;
nit: num_hardware_compositors

[boliu, 2013-09-04 23:05:57]

https://codereview.chromium.org/23618031/diff/1/content/browser/android/in_pr...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/23618031/diff/1/content/browser/android/in_pr...
content/browser/android/in_process/synchronous_compositor_impl.cc:167:
base::AutoLock lock(offscreen_context_for_compositor_thread_lock_);
On 2013/09/04 22:27:09, sievers wrote:
> Does this also need to check if we actually have a shared context, i.e.
> num_hardware_compositors > 0?
> Or should it at least DCHECK()?

Aha, yes, I did give this some thought. cc should never ask for the unless
hardware is initialized (ie has gl_renderer), by that time, we must already have
a compositor context, so it's fine.

I would have added DCHECK except we also call this from InitializeHwDraw, where
we need to call this before knowing if initialization is going to be successful,
so for the first initialization, num_hardware_compositor will be 0. Didn't want
to change things too much for merging to m30...

https://codereview.chromium.org/23618031/diff/1/content/browser/android/in_pr...
content/browser/android/in_process/synchronous_compositor_impl.cc:205: unsigned
int num_hardware_compositor_;
On 2013/09/04 22:27:09, sievers wrote:
> nit: num_hardware_compositors

Done.

[commit-bot: I haz the power, 2013-09-04 23:06:35]

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

[commit-bot: I haz the power, 2013-09-04 23:27:36]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[boliu, 2013-09-05 00:45:49]

Hmm, can't land this as is. Breaks video tests upstream, since it only has
software mode. Sigh...

[boliu, 2013-09-05 01:56:44]

Ok, here's what I think is happening with testMediaPlaybackWithoutUserGesture
upstream. +Mikhail and Michael for fyi.

Before this patch, we try to create the context, but immediately blocks the main
thread since makeCurrent will hang forever in software mode. The test apparently
didn't care at all and ran just fine (!)

With PS2, we do not create the context, which means video player fails, and test
timesout. This is fine.

With PS3, we still create the video player, but it just won't draw inline in
software mode, and will not block main thread. When running locally, it's
hitting a DCHECK failure in ContentViewCoreImpl::GetViewAndroid from
PowerSaveBlocker that never happened before (!!) hence why I suspected that
previously the test just deadlocked main thread forever and never got there.

So we need to properly implement video in software mode actually works. Idea in
PS3 is create the factory without context, but not create the proxy. Video
player seems to work just fine in this case.

Still need one step further to auto-fullscreen on play in software mode. And
maybe even dynamically create the proxy and make in-line work after initializing
hardware draw, but I'm ahead of myself...

And there goes my hope of merging this to m30.

[boliu, 2013-09-05 05:51:33]

On 2013/09/05 01:56:44, boliu wrote:
> With PS3, we still create the video player, but it just won't draw inline in
> software mode, and will not block main thread. When running locally, it's
> hitting a DCHECK failure in ContentViewCoreImpl::GetViewAndroid from
> PowerSaveBlocker that never happened before (!!) hence why I suspected that
> previously the test just deadlocked main thread forever and never got there.

Interesting, PS3 failed the DCHECK as I expected, but did not turn bot red, only
made a warning on the androidwebview instrumentation test. Slightly relieved
that the DCHECK failure is not a blocker for this.

[boliu, 2013-09-05 19:13:34]

Daniel, can you take another look at additional changes in PS3? Again, idea is
we still create the factory, but not the proxy, and webmediaplayer_android
handles that case beautifully already.

I'm not seeing the "full screen on play" behavior in old webview, so that part
is not implemented and waiting for confirmation that we don't need it.

[joth, 2013-09-05 20:47:12]

I defer to you & Daniel on overall threading correctness on this..

https://codereview.chromium.org/23618031/diff/21001/content/browser/android/i...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/23618031/diff/21001/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_impl.cc:150: if
(!can_create_context ||
splicing the can_create_context into both expressions makes it hard to follow
the possible different paths here.

what you really want is a goto. (or moreover, extract a function).

As it is, maybe

bool fail = !CanCreateMainThreadContext();
if (!fail) {
  if (!offscreen_context_for_main_thread_.get() ||
offscreen_context_for_main_thread_->DestroyedOnMainThread()) {
  offscreen_context_for_main_thread_ = create...
  fail = !offscreen_context_for_main_thread_.get() ||
!offscreen_context_for_main_thread_->BindToCurrentThread();
}
}

if (fail) {
  offscreen_context_for_main_thread_ = NULL;
  wrapped_gl_context_for_main_thread_ = NULL;
}

https://codereview.chromium.org/23618031/diff/21001/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_impl.cc:207:
base::Lock num_hardware_compositor_lock_;
comment that num_hardware_compositors_ is always updated on the UI thread, but
maybe read on another thread.

(also should have a comment here or in the release function to acknowledge
possible race in reading this while the last compositor is closing)

[boliu, 2013-09-05 23:21:25]

https://codereview.chromium.org/23618031/diff/21001/content/browser/android/i...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/23618031/diff/21001/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_impl.cc:150: if
(!can_create_context ||
On 2013/09/05 20:47:12, joth wrote:
> splicing the can_create_context into both expressions makes it hard to follow
> the possible different paths here.
> 
> what you really want is a goto. (or moreover, extract a function).
> 
> As it is, maybe
> 
> bool fail = !CanCreateMainThreadContext();
> if (!fail) {
>   if (!offscreen_context_for_main_thread_.get() ||
> offscreen_context_for_main_thread_->DestroyedOnMainThread()) {
>   offscreen_context_for_main_thread_ = create...
>   fail = !offscreen_context_for_main_thread_.get() ||
> !offscreen_context_for_main_thread_->BindToCurrentThread();
> }
> }
> 
> if (fail) {
>   offscreen_context_for_main_thread_ = NULL;
>   wrapped_gl_context_for_main_thread_ = NULL;
> }

Done.

https://codereview.chromium.org/23618031/diff/21001/content/browser/android/i...
content/browser/android/in_process/synchronous_compositor_impl.cc:207:
base::Lock num_hardware_compositor_lock_;
On 2013/09/05 20:47:12, joth wrote:
> comment that num_hardware_compositors_ is always updated on the UI thread, but
> maybe read on another thread.
> 
> (also should have a comment here or in the release function to acknowledge
> possible race in reading this while the last compositor is closing)

Both done

[joth, 2013-09-05 23:24:14]

lgtm

[no sievers, 2013-09-06 01:50:34]

https://codereview.chromium.org/23618031/diff/36001/content/renderer/media/an...
File
content/renderer/media/android/stream_texture_factory_android_synchronous_impl.cc
(right):

https://codereview.chromium.org/23618031/diff/36001/content/renderer/media/an...
content/renderer/media/android/stream_texture_factory_android_synchronous_impl.cc:153:
if (context_provider_ &&
Can we avoid allowing context_provider_ == NULL in here by making WebMediaPlayer
handle proxy creation failure correctly?
Then we could also imply auto-fullscreen from that.

[boliu, 2013-09-06 02:59:26]

+qinmin for media change

https://codereview.chromium.org/23618031/diff/36001/content/renderer/media/an...
File
content/renderer/media/android/stream_texture_factory_android_synchronous_impl.cc
(right):

https://codereview.chromium.org/23618031/diff/36001/content/renderer/media/an...
content/renderer/media/android/stream_texture_factory_android_synchronous_impl.cc:153:
if (context_provider_ &&
On 2013/09/06 01:50:34, sievers wrote:
> Can we avoid allowing context_provider_ == NULL in here by making
WebMediaPlayer
> handle proxy creation failure correctly?
> Then we could also imply auto-fullscreen from that.

Done.

[qinmin, 2013-09-06 03:23:07]

lgtm

[qinmin, 2013-09-06 03:23:12]

lgtm

[no sievers, 2013-09-06 03:46:20]

lgtm

[boliu, 2013-09-06 03:51:50]

+jamesr for render_view_impl

[boliu, 2013-09-06 21:19:06]

+piman for render_view_impl, since James appears to be busy today

[piman, 2013-09-06 21:23:44]

lgtm

[commit-bot: I haz the power, 2013-09-06 21:27:38]

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

[commit-bot: I haz the power, 2013-09-07 01:02:14]

Change committed as 221843