work-in-progress patch to fix context lost black video

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 048994912aa7377092e315f14cfd0e39924acb5b..b914240fbb74282197d80b6f0ccb19ff5be958b5 100644
--- a/content/renderer/media/android/webmediaplayer_android.cc
+++ b/content/renderer/media/android/webmediaplayer_android.cc
@@ -154,6 +154,7 @@ WebMediaPlayerAndroid::WebMediaPlayerAndroid(
   DCHECK(cdm_manager_);
 
   DCHECK(main_thread_checker_.CalledOnValidThread());
+  stream_texture_factory_->AddObserver(this);
 
   player_id_ = player_manager_->RegisterMediaPlayer(this);
 
@@ -195,6 +196,8 @@ WebMediaPlayerAndroid::~WebMediaPlayerAndroid() {
 
   if (player_type_ == MEDIA_PLAYER_TYPE_MEDIA_SOURCE && delegate_)
     delegate_->PlayerGone(this);
+
+  stream_texture_factory_->RemoveObserver(this);
 }
 
 void WebMediaPlayerAndroid::load(LoadType load_type,
@@ -1195,7 +1198,7 @@ void WebMediaPlayerAndroid::SetVideoFrameProviderClient(
     cc::VideoFrameProvider::Client* client) {
   // This is called from both the main renderer thread and the compositor
   // thread (when the main thread is blocked).
-  if (video_frame_provider_client_)
+  if (video_frame_provider_client_ && video_frame_provider_client_ != client)
     video_frame_provider_client_->StopUsingProvider();
   video_frame_provider_client_ = client;
 
@@ -1234,6 +1237,25 @@ void WebMediaPlayerAndroid::PutCurrentFrame(
     const scoped_refptr<media::VideoFrame>& frame) {
 }
 
+void WebMediaPlayerAndroid::ResetStreamTextureProxy() {
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+
+  if (stream_id_) {
+    GLES2Interface* gl = stream_texture_factory_->ContextGL();
+    gl->DeleteTextures(1, &texture_id_);
+    texture_id_ = 0;
+    texture_mailbox_ = gpu::Mailbox();
+    stream_id_ = 0;
+  }
+  stream_texture_proxy_.reset();
+  needs_establish_peer_ =

[qinmin, 2014/09/11 23:26:41]

need to check fullscreen status

[boliu, 2014/09/12 16:38:25]

Done using player_manager_->IsInFullscreen(frame_)

+      !needs_external_surface_ && !is_remote_ && (hasVideo() || IsHLSStream());

[boliu, 2014/09/11 17:08:33]

Min, please double check this line. Unfortunately that clean up had to be
reverted because it really did break perf tests. Otherwise it would be a lot
easier..

+  stream_texture_proxy_initialized_ = false;
+
+  TryCreateStreamTextureProxyIfNeeded();
+  EstablishSurfaceTexturePeer();

[qinmin, 2014/09/11 23:26:41]

shouldn't this be:

if (needs_establish_peer_ && is_playing_)  EstablishSurfaceTexturePeer();

[boliu, 2014/09/12 16:38:25]

Yes done.

If only the refactor wasn't reverted......

+}
+
 void WebMediaPlayerAndroid::TryCreateStreamTextureProxyIfNeeded() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   // Already created.
@@ -1248,10 +1270,24 @@ void WebMediaPlayerAndroid::TryCreateStreamTextureProxyIfNeeded() {
   if (needs_establish_peer_ && stream_texture_proxy_) {
     DoCreateStreamTexture();
     ReallocateVideoFrame();
+
+    base::WaitableEvent completion(true, false);
+    RenderThreadImpl::current()->compositor_message_loop_proxy()->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &WebMediaPlayerAndroid::BindStreamTextureProxyOnCompositorThread,
+            base::Unretained(this),

[no sievers, 2014/09/11 22:28:34]

Do we need to do this or can we just invalidate and lazily bind to the thread
when GetCurrentFrame() happens?

[boliu, 2014/09/11 22:30:56]

That's fine in m37. But that code doesn't exist anymore in trunk.

+            &completion));
+    completion.Wait();

[qinmin, 2014/09/11 23:26:41]

blocking the main thread? I don't feel like this is ok, better check with gpu
folks on that.

[boliu, 2014/09/12 16:38:25]

Blink thread blocks on compositor thread all the time inside the compositor (eg
when SetVideoFrameProviderClient is called), so it is definitely allowed.

I'm not sure what your concern is here?

[qinmin, 2014/09/12 17:01:58]

SetVideoFrameProviderClient uses a lock inside, so it is very cheap.
Here you are posting a task, and let the main thread blocked. Are you 100% sure
the impl thread will never be waiting on the render thread before executing this
task?  And what is the impact of blocking main thread, any impacts on javascript
performance?

On 2014/09/12 16:38:25, boliu wrote:

[boliu, 2014/09/12 17:08:07]

By render thread, you mean blink main thread, right? I am 100% sure compositor
thread will never block on blink main thread. There is no deadlock issues here.

Sure blocking *might* be expensive, but the goal is to keep the compositor
thread lively so it's never expensive in practice. Compositor already does
exactly this on every single commit:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr...

[boliu, 2014/09/12 17:10:14]

Also I'm floating the idea that WMPA should do more blocking like this so to
make threading safety saner, and keep the promises like
SetVideoFrameProviderClient can be called on compositor thread at any time, not
just when it's blocked.

[qinmin, 2014/09/12 17:19:48]

But why cannot we just use a callback and don't block the render thread?

On 2014/09/12 17:08:07, boliu wrote:

[boliu, 2014/09/12 17:26:22]

Calling SetVideoFrameProviderClient without the main thread blocked is not
thread safe right now. Eg there is no lock protecting
video_frame_provider_client_, stream_texture_proxy_initialized_, etc

[qinmin, 2014/09/12 17:31:09]

I don't see that's a valid reason to block main thread.
we can just have a variable on main thread checking that a post task is in
progress, and don't access any of the method that has these thread_unsafe
variables in there

[boliu, 2014/09/12 17:36:34]

That is way too complex, it's a super invasive change, and easy for someone to
break in the future.

What do you propose you should do if those variables are needed on the main
thread while the post task is pending?

Also I'm not sure if it's legal to use the proxy before it's bound.

Compositor already blocks on every single frame. And this only happens on
context loss, which is supposedly a rare event to begin with.

   }
+}
 
-  if (stream_texture_proxy_ && video_frame_provider_client_)
-    stream_texture_proxy_->SetClient(video_frame_provider_client_);
+void WebMediaPlayerAndroid::BindStreamTextureProxyOnCompositorThread(
+    base::WaitableEvent* completion) {
+  if (video_frame_provider_client_) {
+    SetVideoFrameProviderClient(video_frame_provider_client_);

[qinmin, 2014/09/11 23:26:41]

nit: no {}

[boliu, 2014/09/12 16:38:25]

Done.

+  }
+  completion->Signal();
 }
 
 void WebMediaPlayerAndroid::EstablishSurfaceTexturePeer() {