Update StreamTextureProxy to accept a base::Closure

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 <stddef.h>
 
 #include <limits>
 
@@ skipping 1233 matching lines @@
   }
 }
 
 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).
 
   // 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)
     video_frame_provider_client_->StopUsingProvider();
   video_frame_provider_client_ = client;

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

I'm not sure how this was ever thread safe, if this can be called on either the
compositor thread or render thread and it writes to a variable without a lock..
Not sure if a bug or not.

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

My guess is that it is being used in a safe way, without being thread safe in of
itself. I don't think this method gets called more than once during
initialization and once or twice during teardown.

Because WMPA is on its way out and because this CL doesn't make things worse, I
am inclined to not add extra locks and checks.

 }
 
 void WebMediaPlayerAndroid::SetCurrentFrameInternal(
     scoped_refptr<media::VideoFrame>& video_frame) {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   base::AutoLock auto_lock(current_frame_lock_);
   current_frame_ = video_frame;
 }
 
 bool WebMediaPlayerAndroid::UpdateCurrentFrame(base::TimeTicks deadline_min,
@@ skipping 31 matching lines @@
     texture_id_ = 0;
     texture_mailbox_ = gpu::Mailbox();
     stream_id_ = 0;
   }
   stream_texture_proxy_.reset();
   needs_establish_peer_ = !needs_external_surface_ && !is_remote_ &&
                           !is_fullscreen_ &&
                           (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.
   if (stream_texture_proxy_)
     return;
 
   // No factory to create proxy.
   if (!stream_texture_factory_.get())
     return;
 
   // Not needed for hole punching.
   if (!needs_establish_peer_)
     return;
 
   stream_texture_proxy_.reset(stream_texture_factory_->CreateProxy());
   if (stream_texture_proxy_) {
     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_);
     }
   }
 }
 
 void WebMediaPlayerAndroid::EstablishSurfaceTexturePeer() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   if (!stream_texture_proxy_)
     return;
 
   if (stream_texture_factory_.get() && stream_id_)
@@ skipping 318 matching lines @@
     result = PREDICTION_RESULT_PATH_BASED_WAS_BETTER;
   } else if (is_hls_url == is_hls) {
     result = PREDICTION_RESULT_URL_BASED_WAS_BETTER;
   }
   UMA_HISTOGRAM_ENUMERATION(
       "Media.Android.IsHttpLiveStreamingMediaPredictionResult",
       result, PREDICTION_RESULT_MAX);
 }
 
 }  // namespace content