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

gpu/command_buffer/common/mailbox.h

Index: gpu/command_buffer/common/mailbox.h
diff --git a/gpu/command_buffer/common/mailbox.h b/gpu/command_buffer/common/mailbox.h
index 938167f5cf8142b078cd32af506f1d10b4d384f9..a03594f1253de0b674e92c3014b827baddd4119f 100644
--- a/gpu/command_buffer/common/mailbox.h
+++ b/gpu/command_buffer/common/mailbox.h
@@ -7,6 +7,8 @@
 
 #include <string.h>
 
+#include "base/callback_forward.h"
+#include "base/memory/scoped_ptr.h"
 #include "gpu/command_buffer/common/types.h"
 #include "gpu/gpu_export.h"
 
@@ -23,6 +25,24 @@ struct GPU_EXPORT Mailbox {
   }
 };
 
+class GPU_EXPORT MailboxHolder {

[piman, 2013/12/06 23:29:42]

Should this be a struct? It's a value type, copyable and IPC-able, and all
accessors are trivial. We usually use structs for these kind, making it
explicit.

[sheu, 2013/12/07 01:03:50]

Sure.

+ public:
+  typedef base::Callback<void(scoped_ptr<MailboxHolder>)> ReleaseCallback;

[danakj, 2013/12/06 22:37:03]

At least in cc, we also include a "is_lost" boolean in the release callback, and
we don't bother to pass back the target and mailbox id as the release-ee already
had that info. I think you should stick with just the simple struct for now.

[sheu, 2013/12/06 22:54:12]

That's cc::ReleaseCallback (and cc::SingleReleaseCallback) you're referring to? 
They're cc::-specific, so I'm not feeling the need to let their definitions
influence how we define the callback in gpu::.

Anyways, I pass in the entire struct so we don't have to change all the callback
definitions later if more state is desired.  I'm not particularly set on this
though, so if you'd like to see a simpler callback definition I'll do that.  I'm
going to assume something like:

base::Callback<void(uint32 sync_point)>

[piman, 2013/12/06 23:29:42]

I'm pretty meh about defining this here.
Among all the existing users of MailboxHolder there's only one that uses this
callback. I'm not convinced there is value in defining one true way of releasing
a mailbox (if this one-true-way doesn't apply to most existing cases), it seems
more work to try to fit all the use cases into a single callback model.

[sheu, 2013/12/07 01:03:50]

I'm getting more users of the callback soon.  Right now, there's
media::VideoFrame; the desktop/tab capture path will use it too.  See:

https://chromiumcodereview.appspot.com/83793004/

[piman, 2013/12/07 01:32:34]

Right, but like I said, this doesn't fit all use case, it really depends on the
local framework. Also the callback really has nothing to do with the GPU process
protocol. It just doesn't belong here as the "one true way" of returning a
mailbox.
Truth is, VideoFrame's callback should also have the is_lost bool (right now we
drop it on the floor because we can't really do anything with it anyway). Then
it'll have to migrate off this callback. And maybe the other ones you're
planning on adding, too.

[sheu, 2013/12/07 01:44:15]

This isn't really inherent to the gpu::MailboxHolder, I agree.  What I see
though is that there are already two places that have the pattern of having a
callback to be run when a Mailbox is done with:

* cc::ReleaseCallback (and cc::SingleReleaseCallback)
* media::VideoFrame::MailboxHolder::TextureNoLongerNeededCallback

I'm going to add a third one to the video capture pipeline.  And this one
shouldn't take a cc::-specific type like ReleaseCallback.  My concern then is to
avoid a third almost-identical type definition, so if you have any better ideas,
I'm game.

+
+  MailboxHolder();
+  MailboxHolder(const gpu::Mailbox& mailbox, uint32 target, uint32 sync_point);
+
+  const gpu::Mailbox& mailbox() const { return mailbox_; }
+  uint32 target() const { return target_; }
+  uint32 sync_point() const { return sync_point_; }
+  void set_sync_point(uint32 sync_point) { sync_point_ = sync_point; }

[danakj, 2013/12/06 22:37:03]

Can you do the set only if the |sync_point| != 0?

[sheu, 2013/12/06 22:54:12]

We use this also to replace cc::TextureMailbox::ResetSyncPoint(), by explicitly
setting to 0.

In any case, I'm not sure why there would be a reason for setting only if
|sync_point| != 0 ?

+
+ private:
+  gpu::Mailbox mailbox_;
+  uint32 target_;
+  uint32 sync_point_;
+};
+
 }  // namespace gpu
 
 #endif  // GPU_COMMAND_BUFFER_MAILBOX_H_