Enable deferred rendering strategy for Android WebView for Spitzer

media/gpu/android_video_decode_accelerator.cc

 // Copyright (c) 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 "media/gpu/android_video_decode_accelerator.h"
 
 #include <stddef.h>
 
 #include <memory>
 
@@ skipping 512 matching lines @@
   auto gles_decoder = get_gles2_decoder_cb_.Run();
   if (!gles_decoder) {
     LOG(ERROR) << "Failed to get gles2 decoder instance.";
     return false;
   }
 
   const gpu::GpuPreferences& gpu_preferences =
       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 {
     DVLOG(1) << __FUNCTION__ << ", using copy back strategy.";
     strategy_.reset(new AndroidCopyingBackingStrategy(this));

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

No way to get into this branch now. I hope that means we can delete the copying
strategy :)

   }
 
   if (!make_context_current_cb_.Run()) {
     LOG(ERROR) << "Failed to make this decoder's GL context current.";
     return false;
   }
 
   if (g_avda_timer.Pointer()->AllocateSurface(config_.surface_id, this)) {
     // We have succesfully owned the surface, so finish initialization now.
     return InitializeStrategy();
@@ skipping 1110 matching lines @@
 
   if (should_be_running)
     g_avda_timer.Pointer()->StartTimer(this);
   else
     g_avda_timer.Pointer()->StopTimer(this);
 }
 
 // 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
 VideoDecodeAccelerator::Capabilities
 AndroidVideoDecodeAccelerator::GetCapabilities(
     const gpu::GpuPreferences& gpu_preferences) {
   Capabilities capabilities;
   SupportedProfiles& profiles = capabilities.supported_profiles;
 
   // Only support VP8 on Android versions where we don't have to synchronously
   // tear down the MediaCodec on surface destruction because VP8 requires
   // us to completely drain the decoder before releasing it, which is difficult
   // and time consuming to do while the surface is being destroyed.
@@ skipping 43 matching lines @@
     // software fallback for H264 on Android anyway.
     profile.max_resolution.SetSize(3840, 2160);
     profiles.push_back(profile);
   }
 
   capabilities.flags =
       VideoDecodeAccelerator::Capabilities::SUPPORTS_DEFERRED_INITIALIZATION;
   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;
     }
   }
 
   return capabilities;
 }
 
 }  // namespace media