Switch from build-time to run-time flags for Project Spitzer.

Contributed by dalecurtis@chromium.org. This puts the desktop media playback pipeline behind a runtime flag for Android; previously this was a combination of build-time and run-time flags to avoid introducing a binary size increase before we were ready for experiments. This change will result in a ~480kb apk size increase on Android that has been approved via Chrome Eng Review and klobag@ from the Chrome for Android team. http://crbug.com/570711 tracks this size increase and plans to reduce it in the future. It paves the way for unifying our playback stacks across platforms and brings previously-missing features and improved security to Android playback. More technical details can be found in the linked bug below and design doc: https://goo.gl/qC3OuL BUG=507834, 570711, 570762 TEST=builds with gn/gyp work with unified path. TBR=grt@chromium.org,dpranke@chromium.org,liberato@chromium.org,creis@chromium.org Review URL: https://codereview.chromium.org/1541513002 Cr-Commit-Position: refs/heads/master@{#366208}
Denger-Network · Dec 18, 2015 · fa2d9d4 · fa2d9d4
1 parent b20ddc2
commit fa2d9d4
Show file tree

Hide file tree

Showing 16 changed files with 79 additions and 89 deletions.
diff --git a/BUILD.gn b/BUILD.gn
@@ -457,7 +457,7 @@ group("both_gn_and_gyp") {
       }
     }
 
-    if (media_use_ffmpeg) {
+    if (media_use_ffmpeg && !is_android) {
       deps += [ "//media:ffmpeg_regression_tests" ]
     }
 

diff --git a/DEPS b/DEPS
@@ -184,7 +184,7 @@ deps = {
    Var('chromium_git') + '/webm/libvpx.git' + '@' +  'ecb8dff7682da7a42330ddd18c307a66aca7c875',
 
   'src/third_party/ffmpeg':
-   Var('chromium_git') + '/chromium/third_party/ffmpeg.git' + '@' + '8b946dec70ca7985098525d0e9dccc8daa42ef47',
+   Var('chromium_git') + '/chromium/third_party/ffmpeg.git' + '@' + 'a3d30daebf9e75fcd16fc401b069525467a89530',
 
   'src/third_party/libjingle/source/talk':
     Var('chromium_git') + '/external/webrtc/trunk/talk.git' + '@' + 'b7037034bc843dd1e7ff8a3b40439f8ead2dd823', # commit position 11031

diff --git a/build/common.gypi b/build/common.gypi
@@ -1882,7 +1882,6 @@
       }],
       ['chromecast==1', {
         'enable_mpeg2ts_stream_parser%': 1,
-        'ffmpeg_branding%': 'ChromeOS',
         'use_custom_freetype%': 0,
         'use_playready%': 0,
         'conditions': [
@@ -1891,6 +1890,11 @@
             'arm_tune%': 'cortex-a9',
             'arm_thumb%': 1,
           }],
+          # TODO(dalecurtis): What audio codecs does Chromecast want here?  Sort
+          # out and add configs if necessary.  http://crbug.com/570754
+          ['OS!="android"', {
+            'ffmpeg_branding%': 'ChromeOS',
+          }],
         ],
       }],
       ['OS=="linux"', {

diff --git a/build/gn_migration.gypi b/build/gn_migration.gypi
@@ -374,7 +374,7 @@
             '../ui/views/examples/examples.gyp:views_examples_with_content_exe',
           ],
         }],
-        ['media_use_ffmpeg==1', {
+        ['media_use_ffmpeg==1 and OS!="android"', {
           'dependencies': [
             '../media/media.gyp:ffmpeg_regression_tests',
           ],

diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd
@@ -6108,7 +6108,7 @@ Keep your key file in a safe place. You will need it to create new versions of y
           Enable the "stale-while-revalidate" cache directive
         </message>
         <message name="IDS_FLAGS_ENABLE_STALE_WHILE_REVALIDATE_DESCRIPTION" desc="Description of the flag to enable the &quot;stale-while-revalidate&quot; cache directive. This message is intended for web developers who are familiar with technical terms. &quot;Cache-Control: stale-while-revalidate&quot; is the literal string sent by the HTTP server, and so should be untranslated where possible. If necessary for translation it can be split into the HTTP header name part &quot;Cache-Control&quot; and the value part &quot;stale-while-revalidate&quot;. &quot;directive&quot; here is an instruction sent by an HTTP server to indicate what caching behaviour should be used. The directive is typically configured by the server administrator. &quot;servers&quot; here refers to HTTP server software. &quot;resources&quot; here means individual files served by HTTP servers and cached by the browser. &quot;be revalidated&quot; means &quot;be checked with the HTTP server whether or not the browser has the latest version&quot;. &quot;in the background&quot; here means without making the user wait. &quot;latency&quot; here refers to the time the user waits for a web page to be loaded or become usable.">
-          Enable the experimental implementation of the "Cache-Control: stale-while-revalidate" directive. This permits servers to specify that some resources may be revalidated in the background to improve latency. 
+          Enable the experimental implementation of the "Cache-Control: stale-while-revalidate" directive. This permits servers to specify that some resources may be revalidated in the background to improve latency.
         </message>
       </if>
       <message name="IDS_FLAGS_ENABLE_NAVIGATION_TRACING" desc="Name of the flag to enable navigation tracing">
@@ -6581,11 +6581,11 @@ Keep your key file in a safe place. You will need it to create new versions of y
       <message name="IDS_SUPERVISED_USER_SAFESITES_ONLINE_CHECK_ONLY" desc="Description for the choice to enable only the online check in the supervised user SafeSites filtering feature.">
         Online check only
       </message>
-      <message name="IDS_FLAGS_NEW_VIDEO_RENDERER_NAME" desc="Title for the flag for the new video rendering path for video elements.">
-        New video rendering path for video elements
+      <message name="IDS_FLAGS_ENABLE_UNIFIED_MEDIA_PIPELINE_NAME" desc="Title for the flag to enable the unified media pipeline on Android.">
+        Enables the unified media pipeline on Android.
       </message>
-      <message name="IDS_FLAGS_NEW_VIDEO_RENDERER_DESCRIPTION" desc="Description for the flag for the new video rendering path for video elements.">
-        New compositor driven video rendering path for video elements.
+      <message name="IDS_FLAGS_ENABLE_UNIFIED_MEDIA_PIPELINE_DESCRIPTION" desc="Description for the flag to enable the unified media pipeline on Android.">
+        Enables the unified (Android and desktop) media pipeline on Android.
       </message>
       <message name="IDS_FLAGS_SCROLL_TOP_LEFT_INTEROP_NAME" desc="Title for the flag to enable scrollTop interoperability">
         Document scrolling element interoperability.

diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
@@ -2080,6 +2080,12 @@ const FeatureEntry kFeatureEntries[] = {
      IDS_FLAGS_EXPERIMENTAL_FRAMEWORK_DESCRIPTION,
      kOsAll,
      FEATURE_VALUE_TYPE(features::kExperimentalFramework)},
+#if defined(OS_ANDROID)
+    {"enable-unified-media-pipeline",
+     IDS_FLAGS_ENABLE_UNIFIED_MEDIA_PIPELINE_NAME,
+     IDS_FLAGS_ENABLE_UNIFIED_MEDIA_PIPELINE_DESCRIPTION, kOsAndroid,
+     SINGLE_VALUE_TYPE(switches::kEnableUnifiedMediaPipeline)},
+#endif  // OS_ANDROID
     // NOTE: Adding new command-line switches requires adding corresponding
     // entries to enum "LoginCustomFlags" in histograms.xml. See note in
     // histograms.xml and don't forget to run AboutFlagsHistogramTest unit test.

diff --git a/content/common/gpu/media/android_video_decode_accelerator.cc b/content/common/gpu/media/android_video_decode_accelerator.cc
@@ -43,7 +43,6 @@ enum { kNumPictureBuffers = media::limits::kMaxVideoFrames + 1 };
 // NotifyEndOfBitstreamBuffer() before getting output from the bitstream.
 enum { kMaxBitstreamsNotifiedInAdvance = 32 };
 
-#if defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
 // MediaCodec is only guaranteed to support baseline, but some devices may
 // support others. Advertise support for all H264 profiles and let the
 // MediaCodec fail when decoding if it's not actually supported. It's assumed
@@ -61,7 +60,6 @@ static const media::VideoCodecProfile kSupportedH264Profiles[] = {
   media::H264PROFILE_STEREOHIGH,
   media::H264PROFILE_MULTIVIEWHIGH
 };
-#endif
 
 // Because MediaCodec is thread-hostile (must be poked on a single thread) and
 // has no callback mechanism (b/11990118), we must drive it by polling for
@@ -123,11 +121,9 @@ bool AndroidVideoDecodeAccelerator::Initialize(const Config& config,
   codec_ = VideoCodecProfileToVideoCodec(config.profile);
   is_encrypted_ = config.is_encrypted;
 
-  bool profile_supported = codec_ == media::kCodecVP8;
-#if defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
-  profile_supported |=
-      (codec_ == media::kCodecVP9 || codec_ == media::kCodecH264);
-#endif
+  bool profile_supported = codec_ == media::kCodecVP8 ||
+                           codec_ == media::kCodecVP9 ||
+                           codec_ == media::kCodecH264;
 
   if (!profile_supported) {
     LOG(ERROR) << "Unsupported profile: " << config.profile;
@@ -674,12 +670,8 @@ void AndroidVideoDecodeAccelerator::ManageTimer(bool did_work) {
 
 // static
 bool AndroidVideoDecodeAccelerator::UseDeferredRenderingStrategy() {
-#if defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableUnifiedMediaPipeline);
-#endif
-
-  return false;
 }
 
 // static
@@ -697,7 +689,6 @@ AndroidVideoDecodeAccelerator::GetCapabilities() {
     profiles.push_back(profile);
   }
 
-#if defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
   if (!media::VideoCodecBridge::IsKnownUnaccelerated(
           media::kCodecVP9, media::MEDIA_CODEC_DECODER)) {
     SupportedProfile profile;
@@ -717,7 +708,6 @@ AndroidVideoDecodeAccelerator::GetCapabilities() {
     profile.max_resolution.SetSize(3840, 2160);
     profiles.push_back(profile);
   }
-#endif
 
   if (UseDeferredRenderingStrategy()) {
     capabilities.flags = media::VideoDecodeAccelerator::Capabilities::

diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
@@ -213,7 +213,7 @@
 #include "mojo/public/cpp/bindings/interface_request.h"
 #endif
 
-#if defined(ENABLE_MOJO_MEDIA) && !defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
+#if defined(ENABLE_MOJO_MEDIA) && !defined(OS_ANDROID)
 #include "media/mojo/services/mojo_renderer_factory.h"
 #else
 #include "media/renderers/default_renderer_factory.h"
@@ -552,15 +552,13 @@ CommonNavigationParams MakeCommonNavigationParams(
       base::TimeTicks::Now());
 }
 
-#if !defined(OS_ANDROID) || defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
 media::Context3D GetSharedMainThreadContext3D() {
   cc::ContextProvider* provider =
       RenderThreadImpl::current()->SharedMainThreadContextProvider().get();
   if (!provider)
     return media::Context3D();
   return media::Context3D(provider->ContextGL(), provider->GrContext());
 }
-#endif
 
 bool IsReload(FrameMsg_Navigate_Type::Value navigation_type) {
   return navigation_type == FrameMsg_Navigate_Type::RELOAD ||
@@ -2291,16 +2289,11 @@ blink::WebMediaPlayer* RenderFrameImpl::createMediaPlayer(
 
   RenderThreadImpl* render_thread = RenderThreadImpl::current();
 
-#if defined(OS_ANDROID) && !defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
-  scoped_refptr<media::RestartableAudioRendererSink> audio_renderer_sink;
-  media::WebMediaPlayerParams::Context3DCB context_3d_cb;
-#else
   scoped_refptr<media::RestartableAudioRendererSink> audio_renderer_sink =
       render_thread->GetAudioRendererMixerManager()->CreateInput(
           routing_id_, sink_id.utf8(), frame->securityOrigin());
   media::WebMediaPlayerParams::Context3DCB context_3d_cb =
       base::Bind(&GetSharedMainThreadContext3D);
-#endif  // defined(OS_ANDROID) && !defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
 
   scoped_refptr<media::MediaLog> media_log(new RenderMediaLog());
   media::WebMediaPlayerParams params(
@@ -2315,18 +2308,16 @@ blink::WebMediaPlayer* RenderFrameImpl::createMediaPlayer(
                  base::Unretained(blink::mainThreadIsolate())),
       GetMediaPermission(), initial_cdm);
 
-// TODO(xhwang, watk): Find a better way to specify these ifdef conditions.
-#if defined(OS_ANDROID) && !defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
-  return CreateAndroidWebMediaPlayer(client, encrypted_client, params);
-#else
-#if defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
+#if defined(OS_ANDROID)
   if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableUnifiedMediaPipeline)) {
+    // TODO(sandersd): This check should be grown to include HLS and blacklist
+    // checks.  http://crbug.com/516765
     return CreateAndroidWebMediaPlayer(client, encrypted_client, params);
   }
-#endif  // defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
+#endif  // defined(OS_ANDROID)
 
-#if defined(ENABLE_MOJO_MEDIA) && !defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
+#if defined(ENABLE_MOJO_MEDIA) && !defined(OS_ANDROID)
   scoped_ptr<media::RendererFactory> media_renderer_factory(
       new media::MojoRendererFactory(GetMediaServiceFactory()));
 #else
@@ -2339,17 +2330,14 @@ blink::WebMediaPlayer* RenderFrameImpl::createMediaPlayer(
         media_log, render_thread->GetGpuFactories(),
         *render_thread->GetAudioHardwareConfig()));
   }
-#endif  // defined(ENABLE_MOJO_MEDIA) &&
-        // !defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
+#endif  // defined(ENABLE_MOJO_MEDIA) && !defined(OS_ANDROID)
 
-  if (!url_index_.get() || url_index_->frame() != frame) {
+  if (!url_index_.get() || url_index_->frame() != frame)
     url_index_.reset(new media::UrlIndex(frame));
-  }
 
   return new media::WebMediaPlayerImpl(
       frame, client, encrypted_client, GetWebMediaPlayerDelegate()->AsWeakPtr(),
       media_renderer_factory.Pass(), GetCdmFactory(), url_index_, params);
-#endif  // defined(OS_ANDROID) && !defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
 }
 
 blink::WebMediaSession* RenderFrameImpl::createMediaSession() {

diff --git a/media/BUILD.gn b/media/BUILD.gn
@@ -57,10 +57,7 @@ config("media_dependent_config") {
     ]
   }
   if (media_use_ffmpeg && is_android) {
-    defines += [
-      "ENABLE_MEDIA_PIPELINE_ON_ANDROID",
-      "DISABLE_FFMPEG_VIDEO_DECODERS",
-    ]
+    defines += [ "DISABLE_FFMPEG_VIDEO_DECODERS" ]
   }
 }
 
@@ -718,19 +715,22 @@ test("media_unittests") {
   if (media_use_ffmpeg) {
     sources += [
       "ffmpeg/ffmpeg_common_unittest.cc",
-      "filters/audio_decoder_unittest.cc",
-      "filters/ffmpeg_demuxer_unittest.cc",
+      "filters/blocking_url_protocol_unittest.cc",
       "filters/ffmpeg_glue_unittest.cc",
+      "filters/in_memory_url_protocol_unittest.cc",
     ]
 
-    # Even if FFmpeg is enabled we do not want these files on Android.
-    # TODO(watk): Refactor tests that could be made to run on Android.
     if (!is_android) {
       sources += [
+        # These tests are confused by Android always having proprietary
+        # codecs enabled, but ffmpeg_branding=Chromium.  These should be
+        # fixed, http://crbug.com/570762.
+        "filters/audio_decoder_unittest.cc",
         "filters/audio_file_reader_unittest.cc",
-        "filters/blocking_url_protocol_unittest.cc",
+        "filters/ffmpeg_demuxer_unittest.cc",
+
+        # FFmpeg on Android does not include video decoders.
         "filters/ffmpeg_video_decoder_unittest.cc",
-        "filters/in_memory_url_protocol_unittest.cc",
       ]
     }
   }
@@ -881,7 +881,9 @@ component("shared_memory_support") {
   ]
 }
 
-if (media_use_ffmpeg) {
+# TODO(watk): Refactor tests that could be made to run on Android. See
+# http://crbug.com/570762
+if (media_use_ffmpeg && !is_android) {
   test("ffmpeg_regression_tests") {
     sources = [
       "base/run_all_unittests.cc",

diff --git a/media/base/BUILD.gn b/media/base/BUILD.gn
@@ -390,6 +390,7 @@ source_set("unittests") {
     "callback_holder_unittest.cc",
     "channel_mixer_unittest.cc",
     "channel_mixing_matrix_unittest.cc",
+    "container_names_unittest.cc",
     "data_buffer_unittest.cc",
     "decoder_buffer_queue_unittest.cc",
     "decoder_buffer_unittest.cc",
@@ -443,9 +444,7 @@ source_set("unittests") {
     ]
   }
 
-  if (!is_android) {
-    sources += [ "container_names_unittest.cc" ]
-  } else {
+  if (is_android) {
     deps += [ "//ui/gl" ]
   }
 

diff --git a/media/media.gyp b/media/media.gyp
@@ -748,20 +748,13 @@
               'defines': [
                 # On Android, FFmpeg is built without video decoders. We only
                 # support hardware video decoding.
-                'ENABLE_MEDIA_PIPELINE_ON_ANDROID',
                 'DISABLE_FFMPEG_VIDEO_DECODERS',
               ],
               'direct_dependent_settings': {
                 'defines': [
-                  'ENABLE_MEDIA_PIPELINE_ON_ANDROID',
                   'DISABLE_FFMPEG_VIDEO_DECODERS',
                 ],
               },
-            }, {  # media_use_ffmpeg == 0
-              'sources!': [
-                'filters/opus_audio_decoder.cc',
-                'filters/opus_audio_decoder.h',
-              ],
             }],
           ],
         }],
@@ -1355,18 +1348,22 @@
           ],
         }],
         # Even if FFmpeg is enabled on Android we don't want these.
-        # TODO(watk): Refactor tests that could be made to run on Android.
+        # TODO(watk): Refactor tests that could be made to run on Android. See
+        # http://crbug.com/570762
         ['media_use_ffmpeg==0 or OS=="android"', {
           'sources!': [
             'base/audio_video_metadata_extractor_unittest.cc',
-            'base/container_names_unittest.cc',
             'base/media_file_checker_unittest.cc',
-            'filters/audio_file_reader_unittest.cc',
-            'filters/blocking_url_protocol_unittest.cc',
             'filters/ffmpeg_video_decoder_unittest.cc',
-            'filters/in_memory_url_protocol_unittest.cc',
             'test/pipeline_integration_test.cc',
             'test/pipeline_integration_test_base.cc',
+
+            # These tests are confused by Android always having proprietary
+            # codecs enabled, but ffmpeg_branding=Chromium. These should be
+            # fixed, see http://crbug.com/570762.
+            'filters/audio_decoder_unittest.cc',
+            'filters/audio_file_reader_unittest.cc',
+            'filters/ffmpeg_demuxer_unittest.cc',
           ],
         }],
 
@@ -1481,6 +1478,15 @@
             'USE_NEON'
           ],
         }],
+        ['OS=="android" or media_use_ffmpeg==0', {
+          # TODO(watk): Refactor tests that could be made to run on Android.
+          # See http://crbug.com/570762
+          'sources!': [
+            'base/demuxer_perftest.cc',
+            'test/pipeline_integration_perftest.cc',
+            'test/pipeline_integration_test_base.cc',
+          ],
+        }],
         ['OS=="android"', {
           'dependencies': [
             '../testing/android/native_test.gyp:native_test_native_code',
@@ -1491,12 +1497,6 @@
           'dependencies': [
             '../third_party/ffmpeg/ffmpeg.gyp:ffmpeg',
           ],
-        }, {  # media_use_ffmpeg==0
-          'sources!': [
-            'base/demuxer_perftest.cc',
-            'test/pipeline_integration_perftest.cc',
-            'test/pipeline_integration_test_base.cc',
-          ],
         }],
       ],
     },
@@ -1988,7 +1988,9 @@
         ],
       ],
     }],
-    ['media_use_ffmpeg==1', {
+    # TODO(watk): Refactor tests that could be made to run on Android. See
+    # http://crbug.com/570762
+    ['media_use_ffmpeg==1 and OS!="android"', {
       'targets': [
         {
           # GN version: //media:ffmpeg_regression_tests