Update StreamTextureProxy to accept a base::Closure

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 bf5add330c6a6d261843508eb01baff44f78f1ed..47151f3d5a25d817639e222b28d75104282672eb 100644
--- a/content/renderer/media/android/webmediaplayer_android.cc
+++ b/content/renderer/media/android/webmediaplayer_android.cc
@@ -1252,8 +1252,7 @@ void WebMediaPlayerAndroid::SetVideoFrameProviderClient(
   // Set the callback target when a frame is produced. Need to do this before
   // StopUsingProvider to ensure we really stop using the client.
   if (stream_texture_proxy_) {

[watk, 2016/07/29 19:03:16]

For consistency we don't use braces for single line statements inside ifs

[tguilbert, 2016/08/01 22:05:57]

Done.

-    stream_texture_proxy_->BindToLoop(stream_id_, client,
-                                      compositor_task_runner_);
+    UpdateStreamTextureProxyCallback(client);
   }
 
   if (video_frame_provider_client_ && video_frame_provider_client_ != client)
@@ -1310,6 +1309,26 @@ void WebMediaPlayerAndroid::RemoveSurfaceTextureAndProxy() {
                           (hasVideo() || IsHLSStream());
 }
 
+void WebMediaPlayerAndroid::UpdateStreamTextureProxyCallback(
+    cc::VideoFrameProvider::Client* client) {
+  base::Closure frame_received_cb = base::Closure();

[watk, 2016/07/29 19:03:16]

You can remove the "= base::Closure();" 

[tguilbert, 2016/08/01 22:05:57]

Done.

+
+  if (client) {
+    // Unretained is safe here because:
+    //  - |client| will remain alive, until we call |client_|->setWebLayer(NULL)
+    //  in the destructor.
+    //  - We call SetVideoFrameProviderClient(NULL) before calling
+    //  |client_|->setWebLayer(NULL) in the destructor.
+    //  - The Closure is protected by a lock in StreamTextureProxy.

[watk, 2016/07/29 19:03:16]

I would say the proof can be stated slightly more directly:
- |client| is valid until we receive a call to
SetVideoFrameProviderClient(nullptr)
- SetVideoFrameProviderClient(nullptr) clears proxy's callback guaranteeing it
will no longer be run.

[tguilbert, 2016/08/01 22:05:57]

Nicely said. Thank you!

+    frame_received_cb =
+        base::Bind(&cc::VideoFrameProvider::Client::DidReceiveFrame,
+                   base::Unretained(client));
+  }
+
+  stream_texture_proxy_->BindToLoop(stream_id_, frame_received_cb,

[watk, 2016/07/29 19:03:16]

Same question as above. Is it thread safe to be accessing stream_texture_proxy_
and stream_id_ without locking?

[tguilbert, 2016/08/01 22:05:57]

I have not gone through the mental exercise of proving the thread safety/lack
thereof. I think that while it is theoretically not thread safe, in practice it
is not causing any problems.

As I mentioned above, I am inclined to not make any other changes ATM.

[watk, 2016/08/01 22:30:03]

sgtm with leaving as is

+                                    compositor_task_runner_);
+}
+
 void WebMediaPlayerAndroid::TryCreateStreamTextureProxyIfNeeded() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   // Already created.
@@ -1329,8 +1348,7 @@ void WebMediaPlayerAndroid::TryCreateStreamTextureProxyIfNeeded() {
     DoCreateStreamTexture();
     ReallocateVideoFrame();
     if (video_frame_provider_client_) {

[watk, 2016/07/29 19:03:16]

no braces

[tguilbert, 2016/08/01 22:05:57]

Done.

-      stream_texture_proxy_->BindToLoop(
-          stream_id_, video_frame_provider_client_, compositor_task_runner_);
+      UpdateStreamTextureProxyCallback(video_frame_provider_client_);
     }
   }
 }