Set the texture to cleared in VideoDecodeAccelerator::PictureReady.

content/common/gpu/media/gpu_video_decode_accelerator.cc

 // Copyright (c) 2012 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/common/gpu/media/gpu_video_decode_accelerator.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 162 matching lines @@
   // Notify client that picture buffer is now unused.
   if (!Send(new AcceleratedVideoDecoderHostMsg_DismissPictureBuffer(
           host_route_id_, picture_buffer_id))) {
     DLOG(ERROR) << "Send(AcceleratedVideoDecoderHostMsg_DismissPictureBuffer) "
                 << "failed";
   }
 }
 
 void GpuVideoDecodeAccelerator::PictureReady(
     const media::Picture& picture) {
+  // VDA may call PictureReady on IO thread. SetTextureCleared should run on
+  // the child thread. VDA is responsible to call PictureReady on the child
+  // thread when a picture buffer is delivered the first time.
+  if (child_message_loop_->BelongsToCurrentThread())

[piman, 2013/09/26 16:50:01]

nit: needs brackets per style guide.

[wuchengli, 2013/09/30 16:11:21]

Done.

+    if (!SetTextureCleared(picture))
+      return;

[piman, 2013/09/26 16:50:01]

Is there a way to DCHECK that we don't get here on the IO thread without the
texture having been set cleared? For example check the list that you keep? You'd
need a lock, but only in debug, so that would be ok I think.

[wuchengli, 2013/09/30 16:11:21]

Done.

+
   if (!Send(new AcceleratedVideoDecoderHostMsg_PictureReady(
           host_route_id_,
           picture.picture_buffer_id(),
           picture.bitstream_buffer_id()))) {
     DLOG(ERROR) << "Send(AcceleratedVideoDecoderHostMsg_PictureReady) failed";
   }
 }
 
 void GpuVideoDecodeAccelerator::NotifyError(
     media::VideoDecodeAccelerator::Error error) {
@@ skipping 127 matching lines @@
     }
     gpu::gles2::Texture* info = texture_ref->texture();
     if (info->target() != texture_target_) {
       DLOG(ERROR) << "Texture target mismatch for texture id "
                   << texture_ids[i];
       NotifyError(media::VideoDecodeAccelerator::INVALID_ARGUMENT);
       return;
     }
     if (texture_target_ == GL_TEXTURE_EXTERNAL_OES) {
       // GL_TEXTURE_EXTERNAL_OES textures have their dimensions defined by the
-      // underlying EGLImage.  Use |texture_dimensions_| for this size.  The
-      // textures cannot be rendered to or cleared, so we set |cleared| true to
-      // skip clearing.
+      // underlying EGLImage.  Use |texture_dimensions_| for this size.
       texture_manager->SetLevelInfo(texture_ref,
                                     GL_TEXTURE_EXTERNAL_OES,
                                     0,
                                     0,
                                     texture_dimensions_.width(),
                                     texture_dimensions_.height(),
                                     1,
                                     0,
                                     0,
                                     0,
-                                    true);
+                                    false);
     } else {
       // For other targets, texture dimensions should already be defined.
       GLsizei width = 0, height = 0;
       info->GetLevelSize(texture_target_, 0, &width, &height);
       if (width != texture_dimensions_.width() ||
           height != texture_dimensions_.height()) {
         DLOG(ERROR) << "Size mismatch for texture id " << texture_ids[i];
         NotifyError(media::VideoDecodeAccelerator::INVALID_ARGUMENT);
         return;
       }
     }
-    if (!texture_manager->ClearRenderableLevels(command_decoder, texture_ref)) {
-      DLOG(ERROR) << "Failed to Clear texture id " << texture_ids[i];
-      NotifyError(media::VideoDecodeAccelerator::PLATFORM_FAILURE);
-      return;
-    }
     uint32 service_texture_id;
     if (!command_decoder->GetServiceTextureId(
             texture_ids[i], &service_texture_id)) {
       DLOG(ERROR) << "Failed to translate texture!";
       NotifyError(media::VideoDecodeAccelerator::PLATFORM_FAILURE);
       return;
     }
     buffers.push_back(media::PictureBuffer(
         buffer_ids[i], texture_dimensions_, service_texture_id));
   }
+  buffer_ids_ = buffer_ids;
+  texture_ids_ = texture_ids;
   video_decode_accelerator_->AssignPictureBuffers(buffers);
 }
 
 void GpuVideoDecodeAccelerator::OnReusePictureBuffer(
     int32 picture_buffer_id) {
   DCHECK(video_decode_accelerator_.get());
   video_decode_accelerator_->ReusePictureBuffer(picture_buffer_id);
 }
 
 void GpuVideoDecodeAccelerator::OnFlush() {
@@ skipping 59 matching lines @@
 }
 
 bool GpuVideoDecodeAccelerator::Send(IPC::Message* message) {
   DCHECK(stub_);
   if (filter_.get() && io_message_loop_->BelongsToCurrentThread())
     return filter_->SendOnIOThread(message);
   DCHECK(child_message_loop_->BelongsToCurrentThread());
   return stub_->channel()->Send(message);
 }
 
+bool GpuVideoDecodeAccelerator::SetTextureCleared(
+    const media::Picture& picture) {
+  DCHECK(child_message_loop_->BelongsToCurrentThread());
+  uint32 i;
+  for (i = 0; i < buffer_ids_.size(); i++)

[piman, 2013/09/26 16:50:01]

nit: needs brackets per style guide

[wuchengli, 2013/09/30 16:11:21]

Done. Code was changed.

+    if (picture.picture_buffer_id() == buffer_ids_[i])
+      break;
+  if (i >= buffer_ids_.size())
+    return true;
+
+  gpu::gles2::TextureManager* texture_manager =
+      stub_->decoder()->GetContextGroup()->texture_manager();
+  gpu::gles2::TextureRef* texture_ref = texture_manager->GetTexture(
+      texture_ids_[i]);

[piman, 2013/09/26 16:50:01]

Can you, instead, keep a scoped_refptr<TextureRef> when you first lookup the
texture in OnAssignPictureBuffers?
It is possible that by the time this executes, the original texture has been
deleted, and another one took its place on the same id. You don't want to touch
the new one.

[wuchengli, 2013/09/30 16:11:21]

Done.

+  if (!texture_ref) {
+    DLOG(ERROR) << "Failed to find texture id " << texture_ids_[i];
+    NotifyError(media::VideoDecodeAccelerator::PLATFORM_FAILURE);
+    return false;
+  }
+  gpu::gles2::Texture* info = texture_ref->texture();
+  if (info->target() != texture_target_) {
+    DLOG(ERROR) << "Texture target mismatch for texture id " << texture_ids_[i];
+    NotifyError(media::VideoDecodeAccelerator::PLATFORM_FAILURE);
+    return false;
+  }
+  texture_manager->SetLevelCleared(texture_ref, info->target(), 0, true);
+  buffer_ids_.erase(buffer_ids_.begin() + i);
+  texture_ids_.erase(texture_ids_.begin() + i);

[Ami GONE FROM CHROMIUM, 2013/09/26 16:21:56]

Would require less book-keeping as a vector of pairs (buffer/texture ids) or a
map from buffer id to texture id, FWIW.

[piman, 2013/09/26 16:50:01]

+1 on map, given the linear lookup above.

[wuchengli, 2013/09/30 16:11:21]

Done. Changed to a map.

+  return true;
+}
+
 }  // namespace content