Implement PPAPI VideoDecode out-of-process support

webkit/plugins/ppapi/ppb_video_decoder_impl.cc

Index: webkit/plugins/ppapi/ppb_video_decoder_impl.cc
diff --git a/webkit/plugins/ppapi/ppb_video_decoder_impl.cc b/webkit/plugins/ppapi/ppb_video_decoder_impl.cc
index 562fe3e6510ea8164e284734385c74f8e094a6dc..610169c986ae54262c7d356b51f05cdcab9c11d7 100644
--- a/webkit/plugins/ppapi/ppb_video_decoder_impl.cc
+++ b/webkit/plugins/ppapi/ppb_video_decoder_impl.cc
@@ -34,11 +34,7 @@ namespace ppapi {
 
 PPB_VideoDecoder_Impl::PPB_VideoDecoder_Impl(PluginInstance* instance)
     : Resource(instance),
-      callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
-      context3d_id_(0),
-      flush_callback_(PP_BlockUntilComplete()),
-      reset_callback_(PP_BlockUntilComplete()),
-      gles2_impl_(NULL) {
+      VideoDecoderImpl(true) {
   ppp_videodecoder_ =
       static_cast<const PPP_VideoDecoder_Dev*>(instance->module()->
           GetPluginInterface(PPP_VIDEODECODER_DEV_INTERFACE));
@@ -64,17 +60,18 @@ PP_Resource PPB_VideoDecoder_Impl::Create(PluginInstance* instance,
 
 bool PPB_VideoDecoder_Impl::Init(PP_Resource context3d_id,
                                  const PP_VideoConfigElement* config) {
-  if (!instance() || !context3d_id || !config)
+  if(!instance() || !::ppapi::VideoDecoderImpl::Init(context3d_id, config))

[Ami GONE FROM CHROMIUM, 2011/08/02 00:49:08]

How can instance() be NULL, do you know?

[vrk (LEFT CHROMIUM), 2011/08/03 22:05:22]

It appears it can't be -- when ResourceCreationImpl calls
PPB_VideoDecoder_Impl::Create, it passes as an instance its own instance_ field,
ignoring what is passed into it? And that instance_ field has been vetted via
another method in ResourceCreationImpl. Tentatively removing the check...

brettw: does this sound right?

     return false;
 
-  EnterResourceNoLock<PPB_Context3D_API> enter(context3d_id, true);
-  if (enter.failed())
+  EnterResourceNoLock<PPB_Context3D_API> enter_context(context3d_id, true);
+  DCHECK(enter_context.succeeded());

[Ami GONE FROM CHROMIUM, 2011/08/02 00:49:08]

I'm not entirely sure about this; it's only correct b/c the super-class Init()
would have returned false if its copy of this failed, but that seems brittle
without at least a comment documenting the dependency.

[vrk (LEFT CHROMIUM), 2011/08/03 19:04:30]

Agreed. To also address piman's concern w/ the double map lookup, I changed Init
to take a PPB_Context3D_API* in addition to PP_Resource (can't drop the
PP_Resource so that we keep resource tracking). Let me know what you think!

+
+  std::vector<int32> copied;
+  if (!CopyConfigsToVector(config, &copied))
     return false;
-  PPB_Context3D_Impl* context3d =
-      static_cast<PPB_Context3D_Impl*>(enter.object());
 
-  context3d_id_ = context3d_id;
-  ResourceTracker::Get()->AddRefResource(context3d_id_);
+  PPB_Context3D_Impl* context3d =
+      static_cast<PPB_Context3D_Impl*>(enter_context.object());
 
   int command_buffer_route_id =
       context3d->platform_context()->GetCommandBufferRouteId();
@@ -82,26 +79,9 @@ bool PPB_VideoDecoder_Impl::Init(PP_Resource context3d_id,
     return false;
   platform_video_decoder_ = instance()->delegate()->CreateVideoDecoder(
       this, command_buffer_route_id);
-
-  gles2_impl_ = context3d->gles2_impl();
-
   if (!platform_video_decoder_)
     return false;
 
-  std::vector<uint32> copied;
-  // TODO(fischman,vrk): this is completely broken in that it fails to account
-  // for the semantic distinction between keys and values; it is certainly
-  // possible for a value to show up as 0, and that shouldn't terminate the
-  // config vector.  Only a *key* of 0 should do so.
-  // TODO(vrk): This is assuming PP_VideoAttributeDictionary and
-  // VideoAttributeKey have identical enum values. There is no compiler
-  // assert to guarantee this. We either need to add such asserts or
-  // merge PP_VideoAttributeDictionary and VideoAttributeKey.
-  for (const PP_VideoConfigElement* current = config;
-       *current != PP_VIDEOATTR_DICTIONARY_TERMINATOR; ++current) {
-    copied.push_back(static_cast<uint32>(*current));
-  }
-
   FlushCommandBuffer();
   return platform_video_decoder_->Initialize(copied);
 }
@@ -120,8 +100,7 @@ int32_t PPB_VideoDecoder_Impl::Decode(
   media::BitstreamBuffer decode_buffer(bitstream_buffer->id,
                                        buffer->shared_memory()->handle(),
                                        static_cast<size_t>(buffer->size()));
-  CHECK(bitstream_buffer_callbacks_.insert(std::make_pair(
-      bitstream_buffer->id, callback)).second);
+  AddBitstreamBufferCallback(bitstream_buffer->id, callback);
 
   FlushCommandBuffer();
   platform_video_decoder_->Decode(decode_buffer);
@@ -162,7 +141,7 @@ int32_t PPB_VideoDecoder_Impl::Flush(PP_CompletionCallback callback) {
 
   // Store the callback to be called when Flush() is done.
   // TODO(fischman,vrk): consider implications of already-outstanding callback.

[Ami GONE FROM CHROMIUM, 2011/08/02 00:49:08]

This TODO (and its sibling below) don't belong here anymore.  If anywhere, they
should probably go in the shared impl's header above the declaration of the
Set*Callback() methods.

[vrk (LEFT CHROMIUM), 2011/08/03 19:04:30]

Done.

-  flush_callback_ = callback;
+  SetFlushCallback(callback);
 
   FlushCommandBuffer();
   platform_video_decoder_->Flush();
@@ -175,7 +154,7 @@ int32_t PPB_VideoDecoder_Impl::Reset(PP_CompletionCallback callback) {
 
   // Store the callback to be called when Reset() is done.
   // TODO(fischman,vrk): consider implications of already-outstanding callback.
-  reset_callback_ = callback;
+  SetResetCallback(callback);
 
   FlushCommandBuffer();
   platform_video_decoder_->Reset();
@@ -188,11 +167,10 @@ void PPB_VideoDecoder_Impl::Destroy() {
 
   FlushCommandBuffer();
   platform_video_decoder_->Destroy();
-  gles2_impl_ = NULL;
-  if (context3d_id_)
-    ResourceTracker::Get()->UnrefResource(context3d_id_);
   platform_video_decoder_ = NULL;
   ppp_videodecoder_ = NULL;
+
+  ::ppapi::VideoDecoderImpl::Destroy();

[Ami GONE FROM CHROMIUM, 2011/08/02 00:49:08]

Is it obvious why this belongs after the 2 =NULL's above, as opposed to before
them?

[vrk (LEFT CHROMIUM), 2011/08/03 19:04:30]

Talked in person, done.

 }
 
 void PPB_VideoDecoder_Impl::ProvidePictureBuffers(
@@ -251,42 +229,21 @@ void PPB_VideoDecoder_Impl::NotifyError(
 }
 
 void PPB_VideoDecoder_Impl::NotifyResetDone() {
-  if (reset_callback_.func == NULL)
-    return;
-
-  // Call the callback that was stored to be called when Reset is done.
-  PP_RunAndClearCompletionCallback(&reset_callback_, PP_OK);
+  RunResetCallback();
 }
 
 void PPB_VideoDecoder_Impl::NotifyEndOfBitstreamBuffer(
     int32 bitstream_buffer_id) {
-  CallbackById::iterator it =
-      bitstream_buffer_callbacks_.find(bitstream_buffer_id);
-  DCHECK(it != bitstream_buffer_callbacks_.end());
-  PP_CompletionCallback cc = it->second;
-  bitstream_buffer_callbacks_.erase(it);
-  PP_RunCompletionCallback(&cc, PP_OK);
+  RunBitstreamBufferCallback(bitstream_buffer_id);
 }
 
 void PPB_VideoDecoder_Impl::NotifyFlushDone() {
-  if (flush_callback_.func == NULL)
-    return;
-
-  // Call the callback that was stored to be called when Flush is done.
-  PP_RunAndClearCompletionCallback(&flush_callback_, PP_OK);
+  RunFlushCallback();
 }
 
 void PPB_VideoDecoder_Impl::NotifyInitializeDone() {
   NOTREACHED() << "PlatformVideoDecoder::Initialize() is synchronous!";
 }
 
-void PPB_VideoDecoder_Impl::FlushCommandBuffer() {
-  // For the out-of-process case, |gles2_impl_| will be NULL in the renderer
-  // process. The VideoDecoder_Proxy is charged with the responsibility of
-  // doing this Flush() in the analogous places in the plugin process.
-  if (gles2_impl_)
-    gles2_impl_->Flush();
-}
-
 }  // namespace ppapi
 }  // namespace webkit