Add gpu::MailboxHolder to hold state for a gpu::Mailbox

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 be5b6dca522b4f7ee531cb89eecaa63dd7b4644e..42534962990f9ec12b58258e5d74d2a2251ee3b2 100644
--- a/content/renderer/media/android/webmediaplayer_android.cc
+++ b/content/renderer/media/android/webmediaplayer_android.cc
@@ -66,12 +66,12 @@ namespace content {
 void WebMediaPlayerAndroid::OnReleaseRemotePlaybackTexture(
     const scoped_refptr<base::MessageLoopProxy>& main_loop,
     const base::WeakPtr<WebMediaPlayerAndroid>& player,
-    uint32 sync_point) {
+    scoped_ptr<gpu::MailboxHolder> mailbox_holder) {

[danakj, 2014/01/06 20:13:42]

I'm a little uncertain about returning a scoped_ptr here for a couple reasons:

1) It's not a pattern used anywhere else we've used mailboxes
2) The underlying mailbox is actually ref-counted, and the original creator of
the mailbox is already holding onto a reference to the mailbox with a texture
id. So this scoped_ptr isn't really matching any lifetime of the underlying
mailbox data.

That said I'm not an owner here so I won't block this change either if y'all
think it's appropriate.

[sheu, 2014/01/06 22:44:44]

Well, it's scoped_ptr-ing the MailboxHolder, not the Mailbox itself.  I think of
the Mailbox as the primitive that gets passed around/copied as necessary, and
the MailboxHolder being wrapping also some more state to go with it.  The former
doesn't need to be scoped_ptr; the latter should.

[danakj, 2014/01/06 23:28:33]

I understand, but I don't think scoped_ptr is adding value for you here in the
callback. It's not like there is only one place that has access to the contents
of the mailbox. The site that created the VideoFrame still has a texture id
which refs the mailbox, so this isn't an ownership transfer of the mailbox
itself. Further, they don't need to receive back the mailbox name or the target,
they only need the sync point. The scoped_ptr just feels like a bit of a lie to
me, destroying the scoped_ptr held object does not do anything at all.

[sheu, 2014/01/07 01:41:00]

I think the ownership transfer semantics are really enforcing the idea that the
syncpoint is serializing all accesses on the mailbox.  If
DoReleaseRemotePlaybackTexture takes just a sync_point and not the whole
MailboxHolder struct, then it can't update the MailboxHolder's sync point
afterwards.  This is alright as long as the the implication of the release
callback is that the MailboxHolder is destroyed immediately after the callback
is run, i.e. the callback does not need to continue the "chain" of syncpoints.

This implication is safe for now since with this change
VideoFrame::WrapNativeTexture takes ownership of the MailboxHolder passed to it.
 If it were not the case, then it's possible that the caller of
VideoFrame::WrapNativeTexture could keep a copy of the MailboxHolder passed to
the VideoFrame, and not be expecting that the VideoFrame destructor calls a
callback that waits on the mailbox but does not update its syncpoint.  So I can
remove the ownership semantics here, as long as I don't remove them from
VideoFrame::WrapNativeTexture.

   main_loop->PostTask(
       FROM_HERE,
       base::Bind(&WebMediaPlayerAndroid::DoReleaseRemotePlaybackTexture,
                  player,
-                 sync_point));
+                 base::Passed(&mailbox_holder)));
 }
 
 WebMediaPlayerAndroid::WebMediaPlayerAndroid(
@@ -997,19 +997,16 @@ void WebMediaPlayerAndroid::DrawRemotePlaybackIcon() {
   unsigned texture_mailbox_sync_point = context->insertSyncPoint();
 
   scoped_refptr<VideoFrame> new_frame = VideoFrame::WrapNativeTexture(
-      make_scoped_ptr(new VideoFrame::MailboxHolder(
-          texture_mailbox,
-          texture_mailbox_sync_point,
-          base::Bind(&WebMediaPlayerAndroid::OnReleaseRemotePlaybackTexture,
-                     main_loop_,
-                     weak_factory_.GetWeakPtr()))),
-      texture_target,
+      make_scoped_ptr(new gpu::MailboxHolder(
+          texture_mailbox, texture_target, texture_mailbox_sync_point)),
+      base::Bind(&WebMediaPlayerAndroid::OnReleaseRemotePlaybackTexture,
+                 main_loop_,
+                 weak_factory_.GetWeakPtr()),
       canvas_size /* coded_size */,
       gfx::Rect(canvas_size) /* visible_rect */,
       canvas_size /* natural_size */,
       base::TimeDelta() /* timestamp */,
-      VideoFrame::ReadPixelsCB(),
-      base::Closure() /* no_longer_needed_cb */);
+      VideoFrame::ReadPixelsCB());
   SetCurrentFrameInternal(new_frame);
 }
 
@@ -1029,17 +1026,15 @@ void WebMediaPlayerAndroid::ReallocateVideoFrame() {
 #endif  // defined(VIDEO_HOLE)
   } else if (!is_remote_ && texture_id_) {
     scoped_refptr<VideoFrame> new_frame = VideoFrame::WrapNativeTexture(
-        make_scoped_ptr(new VideoFrame::MailboxHolder(
-            texture_mailbox_,
-            texture_mailbox_sync_point_,
-            VideoFrame::MailboxHolder::TextureNoLongerNeededCallback())),
-        kGLTextureExternalOES,
+        make_scoped_ptr(new gpu::MailboxHolder(texture_mailbox_,
+                                               kGLTextureExternalOES,
+                                               texture_mailbox_sync_point_)),
+        media::VideoFrame::ReleaseMailboxCB(),
         natural_size_,
         gfx::Rect(natural_size_),
         natural_size_,
         base::TimeDelta(),
-        VideoFrame::ReadPixelsCB(),
-        base::Closure());
+        VideoFrame::ReadPixelsCB());
     SetCurrentFrameInternal(new_frame);
   }
 }
@@ -1430,15 +1425,16 @@ bool WebMediaPlayerAndroid::InjectMediaStream(
 }
 #endif
 
-void WebMediaPlayerAndroid::DoReleaseRemotePlaybackTexture(uint32 sync_point) {
+void WebMediaPlayerAndroid::DoReleaseRemotePlaybackTexture(
+    scoped_ptr<gpu::MailboxHolder> mailbox_holder) {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   DCHECK(remote_playback_texture_id_);
 
   blink::WebGraphicsContext3D* context =
       stream_texture_factory_->Context3d();
 
-  if (sync_point)
-    context->waitSyncPoint(sync_point);
+  if (mailbox_holder->sync_point)
+    context->waitSyncPoint(mailbox_holder->sync_point);
   context->deleteTexture(remote_playback_texture_id_);
   remote_playback_texture_id_ = 0;
 }