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

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/bind.h"
 #include "base/command_line.h"
@@ skipping 48 matching lines @@
 // Prefix for histograms related to Encrypted Media Extensions.
 const char* kMediaEme = "Media.EME.";
 }  // namespace
 
 namespace content {
 
 // static
 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(
     blink::WebFrame* frame,
     blink::WebMediaPlayerClient* client,
     base::WeakPtr<WebMediaPlayerDelegate> delegate,
     RendererMediaPlayerManager* manager,
     StreamTextureFactory* factory,
     const scoped_refptr<base::MessageLoopProxy>& media_loop,
     media::MediaLog* media_log)
@@ skipping 905 matching lines @@
                         bitmap.getPixels());
   }
 
   gpu::Mailbox texture_mailbox;
   context->genMailboxCHROMIUM(texture_mailbox.name);
   context->produceTextureCHROMIUM(texture_target, texture_mailbox.name);
   context->flush();
   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);
 }
 
 void WebMediaPlayerAndroid::ReallocateVideoFrame() {
   if (needs_external_surface_) {
     // VideoFrame::CreateHoleFrame is only defined under VIDEO_HOLE.
 #if defined(VIDEO_HOLE)
     if (!natural_size_.isEmpty()) {
       scoped_refptr<VideoFrame> new_frame =
           VideoFrame::CreateHoleFrame(natural_size_);
       SetCurrentFrameInternal(new_frame);
       // Force the client to grab the hole frame.
       client_->repaint();
     }
 #else
     NOTIMPLEMENTED() << "Hole punching not supported without VIDEO_HOLE flag";
 #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);
   }
 }
 
 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_)
     video_frame_provider_client_->StopUsingProvider();
@@ skipping 370 matching lines @@
     media::Demuxer* demuxer,
     const base::Closure& destroy_demuxer_cb) {
   DCHECK(!demuxer);
   media_stream_client_ = media_stream_client;
   demuxer_ = demuxer;
   destroy_demuxer_cb_ = destroy_demuxer_cb;
   return true;
 }
 #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;
 }
 
 void WebMediaPlayerAndroid::enterFullscreen() {
   if (manager_->CanEnterFullscreen(frame_)) {
     manager_->EnterFullscreen(player_id_);
     SetNeedsEstablishPeer(false);
   }
 }
 
 void WebMediaPlayerAndroid::exitFullscreen() {
   manager_->ExitFullscreen(player_id_);
 }
 
 bool WebMediaPlayerAndroid::canEnterFullscreen() const {
   return manager_->CanEnterFullscreen(frame_);
 }
 
 }  // namespace content