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

content/renderer/media/android/webmediaplayer_android.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/android/webmediaplayer_android.h"
 
 #include <limits>
 
 #include "base/android/build_info.h"
 #include "base/bind.h"
@@ skipping 136 matching lines @@
       is_remote_(false),
       media_log_(media_log),
       web_cdm_(NULL),
       allow_stored_credentials_(false),
       is_local_resource_(false),
       weak_factory_(this) {
   DCHECK(player_manager_);
   DCHECK(cdm_manager_);
 
   DCHECK(main_thread_checker_.CalledOnValidThread());
+  stream_texture_factory_->AddObserver(this);
 
   player_id_ = player_manager_->RegisterMediaPlayer(this);
 
 #if defined(VIDEO_HOLE)
   force_use_overlay_embedded_video_ = CommandLine::ForCurrentProcess()->
       HasSwitch(switches::kForceUseOverlayEmbeddedVideo);
   if (force_use_overlay_embedded_video_ ||
     player_manager_->ShouldUseVideoOverlayForEmbeddedEncryptedVideo()) {
     // Defer stream texture creation until we are sure it's necessary.
     needs_establish_peer_ = false;
@@ skipping 21 matching lines @@
     stream_id_ = 0;
   }
 
   {
     base::AutoLock auto_lock(current_frame_lock_);
     current_frame_ = NULL;
   }
 
   if (player_type_ == MEDIA_PLAYER_TYPE_MEDIA_SOURCE && delegate_)
     delegate_->PlayerGone(this);
+
+  stream_texture_factory_->RemoveObserver(this);
 }
 
 void WebMediaPlayerAndroid::load(LoadType load_type,
                                  const blink::WebURL& url,
                                  CORSMode cors_mode) {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   media::ReportMediaSchemeUma(GURL(url));
 
   switch (load_type) {
     case LoadTypeURL:
@@ skipping 980 matching lines @@
         base::TimeDelta(),
         VideoFrame::ReadPixelsCB());
     SetCurrentFrameInternal(new_frame);
   }
 }
 
 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;
 
   // Set the callback target when a frame is produced.
   if (stream_texture_proxy_) {
     stream_texture_proxy_->SetClient(client);
     // If client exists, the compositor thread calls it. At that time,

[no sievers, 2014/09/11 22:52:33]

This is really just based on probably what the current state of the code is, and
contradicts the general comment in video_frame_provider.h ("May be called from
any thread") :(

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

Yep. It's a requirement that main thread is blocked when this is called on the
compositor thread. It says so a few lines above.

I'll update the header.

[boliu, 2014/09/11 22:57:35]

Ehh, update in a follow up since that's in cc. Or maybe fix this once and for
all somehow...

[no sievers, 2014/09/11 23:06:24]

I was more worried about SetVideoFrameProviderClient() getting called on the
main thread. The 'if (client' check etc. looks dubious. Well, not a problem of
this patch really.

     // stream_id_, needs_external_surface_, is_remote_ can be accessed because
     // the main thread is blocked.
     if (client && !stream_texture_proxy_initialized_ && stream_id_ &&
         !needs_external_surface_ && !is_remote_) {
       stream_texture_proxy_->BindToCurrentThread(stream_id_);
       stream_texture_proxy_initialized_ = true;
     }
   }
 }
 
@@ skipping 11 matching lines @@
     video_frame = current_frame_;
   }
 
   return video_frame;
 }
 
 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.
   if (stream_texture_proxy_)
     return;
 
   // No factory to create proxy.
   if (!stream_texture_factory_)
     return;
 
   stream_texture_proxy_.reset(stream_texture_factory_->CreateProxy());
   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() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   if (!stream_texture_proxy_)
     return;
 
   if (stream_texture_factory_.get() && stream_id_)
     stream_texture_factory_->EstablishPeer(stream_id_, player_id_);
 
@@ skipping 504 matching lines @@
 
 bool WebMediaPlayerAndroid::IsHLSStream() const {
   std::string mime;
   GURL url = redirected_url_.is_empty() ? url_ : redirected_url_;
   if (!net::GetMimeTypeFromFile(base::FilePath(url.path()), &mime))
     return false;
   return !mime.compare("application/x-mpegurl");
 }
 
 }  // namespace content