Enable deferred rendering strategy for Android WebView for Spitzer

media/gpu/android_video_decode_accelerator.cc

Index: media/gpu/android_video_decode_accelerator.cc
diff --git a/media/gpu/android_video_decode_accelerator.cc b/media/gpu/android_video_decode_accelerator.cc
index 8e00a4f1dff3024a2e1af983dd4de43f66a6dfd8..495719ea5e6852f81f3affecf3a7a145197a3c08 100644
--- a/media/gpu/android_video_decode_accelerator.cc
+++ b/media/gpu/android_video_decode_accelerator.cc
@@ -530,9 +530,6 @@ bool AndroidVideoDecodeAccelerator::Initialize(const Config& config,
       gles_decoder->GetContextGroup()->gpu_preferences();
 
   if (UseDeferredRenderingStrategy(gpu_preferences)) {
-    // TODO(liberato, watk): Figure out what we want to do about zero copy for
-    // fullscreen external SurfaceView in WebView.  http://crbug.com/582170.
-    DCHECK(!gles_decoder->GetContextGroup()->mailbox_manager()->UsesSync());
     DVLOG(1) << __FUNCTION__ << ", using deferred rendering strategy.";
     strategy_.reset(new AndroidDeferredRenderingBackingStrategy(this));
   } else {
@@ -1668,9 +1665,14 @@ void AndroidVideoDecodeAccelerator::ManageTimer(bool did_work) {
 // static
 bool AndroidVideoDecodeAccelerator::UseDeferredRenderingStrategy(
     const gpu::GpuPreferences& gpu_preferences) {
-  // TODO(liberato, watk): Figure out what we want to do about zero copy for
-  // fullscreen external SurfaceView in WebView.  http://crbug.com/582170.
-  return !gpu_preferences.enable_threaded_texture_mailboxes;
+  return true;

[watk, 2016/06/24 17:54:46]

Delete this now?

[liberato (no reviews please), 2016/06/24 17:59:13]

i think holding onto it still has value.  once we go a quarter without using it
internally, then yes :)

+}
+
+// static
+bool AndroidVideoDecodeAccelerator::RequiresTextureCopy(
+    const gpu::GpuPreferences& gpu_preferences) {
+  // http://crbug.com/582170
+  return gpu_preferences.enable_threaded_texture_mailboxes;

[liberato (no reviews please), 2016/06/24 17:15:45]

i think that it should either (a) return false if we're using the deferred
strategy, or (b) the function name should qualify that it's only applicable to
the deferred strategy.

[Tima Vaisburd, 2016/06/24 17:22:54]

I think only (b) is correct: we always use deferred strategy, for WebView it
does require texture copy and for Chrome it does not. Or am I missing something?

[liberato (no reviews please), 2016/06/24 17:28:32]

either a or b works.  if we (a) keep the current name, then we should be sure
that it returns false if UseDeferred...() returns false, since that's what the
name (and comments) say.  the texture copy is not required for the copying
strategy.

either (a) or (b) is fine with me.  as it is, this will return true even if
UseDeferred selects the copying strategy.  that's unexpected behavior given the
function name and comments.

[Tima Vaisburd, 2016/06/24 17:42:22]

I see. I renamed the method to UseTextureCopyForDeferredStrategy

 }
 
 // static
@@ -1737,8 +1739,13 @@ AndroidVideoDecodeAccelerator::GetCapabilities(
   if (UseDeferredRenderingStrategy(gpu_preferences)) {
     capabilities.flags |= VideoDecodeAccelerator::Capabilities::
         NEEDS_ALL_PICTURE_BUFFERS_TO_DECODE;
-    if (MediaCodecUtil::IsSurfaceViewOutputSupported()) {
-      capabilities.flags |= VideoDecodeAccelerator::Capabilities::
+    if (RequiresTextureCopy(gpu_preferences)) {
+      capabilities.flags |=
+          media::VideoDecodeAccelerator::Capabilities::REQUIRES_TEXTURE_COPY;
+    } else if (media::MediaCodecUtil::IsSurfaceViewOutputSupported()) {
+      // Fullscreen external SurfaceView is disabled for WenView.

[watk, 2016/06/24 17:54:46]

s/wenview/webview

[Tima Vaisburd, 2016/06/24 18:12:37]

Done.

+      // http://crbug.com/582170
+      capabilities.flags |= media::VideoDecodeAccelerator::Capabilities::
           SUPPORTS_EXTERNAL_OUTPUT_SURFACE;
     }
   }