Remove function level static initializers media library initialization.

Remove function level static initializers media library initialization.

Moves media library initialization into a lazy initializer and dedups a
bunch of code.  This will also be the eventual home for global media
initialization routines (YUV and VectorMath).

BUG=224662
TEST=compiles, media library works.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198559

[DaleCurtis, 2013-05-02 23:57:23]

WDYT?

[scherkus (not reviewing), 2013-05-03 17:44:43]

https://codereview.chromium.org/14891002/diff/1002/media/base/media_android.cc
File media/base/media_android.cc (right):

https://codereview.chromium.org/14891002/diff/1002/media/base/media_android.c...
media/base/media_android.cc:17: bool for_testing) {
this is the only place where |for_testing| is used, which makes me wonder about
whether the RegisterJni() code should really be here or whether it was placed
here out of convenience


if we need to call this once for running unit tests then a better place would be
inside media/base/run_all_unittests.cc

[DaleCurtis, 2013-05-03 17:49:26]

+nilesh who added the code in https://codereview.chromium.org/13467037

Nilesh: Should this code instead go in media/base/run_all_unittests.cc ?

[DaleCurtis, 2013-05-03 17:49:41]

argh, actually +nilesh this time.

[DaleCurtis, 2013-05-03 18:03:40]

Thinking about this more, I suspect it needs to be added elsewhere if we remove
it here since the run_all_unittests code doesn't get executed everywhere we use
InitializeMediaLibraryForTesting().

https://code.google.com/p/chromium/codesearch#search/&q=InitializeMediaLibrar...

[scherkus (not reviewing), 2013-05-03 18:13:41]

On 2013/05/03 18:03:40, DaleCurtis wrote:
> Thinking about this more, I suspect it needs to be added elsewhere if we
remove
> it here since the run_all_unittests code doesn't get executed everywhere we
use
> InitializeMediaLibraryForTesting().
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=InitializeMediaLibrar...

I think that's fine.

FWIW, these aren't used on Android so you don't need to worry about them:
media/tools/seek_tester/seek_tester.cc
media/tools/player_x11/player_x11.cc
media/tools/demuxer_bench/demuxer_bench.cc

This one has a nice section where it calls all the appropriate RegisterJni()
calls in Initialize():
content/public/test/content_test_suite_base.cc

That leaves cc_unittests. Currently they don't seem to call any other
RegisterJni() calls. It's unclear to me whether they actually need to call
media's RegisterJni() in this case or need FFmpeg libraries loaded for running
on desktop:
cc/trees/layer_tree_host_unittest_context.cc

[DaleCurtis, 2013-05-03 18:18:51]

Hmm, and cc_tests.gyp includes:

https://code.google.com/p/chromium/codesearch#chromium/src/testing/android/na...

Which seems to run JNI init via:

https://code.google.com/p/chromium/codesearch#chromium/src/testing/android/na...

We also include that target, so I wonder if this is still even necessary
anymore?

[scherkus (not reviewing), 2013-05-03 18:23:04]

On 2013/05/03 18:18:51, DaleCurtis wrote:
> Hmm, and cc_tests.gyp includes:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/android/na...
> 
> Which seems to run JNI init via:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/android/na...
> 
> We also include that target, so I wonder if this is still even necessary
> anymore?

That calls base::android::RegisterJni(). The question is whether cc needs to
call media::RegisterJni()
http://src.chromium.org/viewvc/chrome/trunk/src/media/base/android/media_jni_...

[DaleCurtis, 2013-05-03 18:28:49]

Ah, I didn't notice there were two methods. Thanks!

[DaleCurtis, 2013-05-03 20:34:37]

Given that cc/ only uses VideoFrame() I believe the call there is a remnant of
when it used to require FFmpeg.  I've removed it under this assumption and we'll
see what the trybots say.

[scherkus (not reviewing), 2013-05-03 20:40:47]

On 2013/05/03 20:34:37, DaleCurtis wrote:
> Given that cc/ only uses VideoFrame() I believe the call there is a remnant of
> when it used to require FFmpeg.  I've removed it under this assumption and
we'll
> see what the trybots say.

lgtm

we'll see if content/public/test/content_test_suite_base.cc needs
media::RegisterJni() (I'm not sure if our media content tests run on android)

[DaleCurtis, 2013-05-03 21:58:46]

Failed content_unittests on Android, so I'll add the code there.

[DaleCurtis, 2013-05-03 22:38:39]

+phajdan.jr for content/test OWNERs stamp.

[Paweł Hajdan Jr., 2013-05-03 23:04:09]

content/public/test LGTM

[nilesh, 2013-05-04 10:04:22]

On 2013/05/03 17:49:26, DaleCurtis wrote:
> +nilesh who added the code in https://codereview.chromium.org/13467037
> 
> Nilesh: Should this code instead go in media/base/run_all_unittests.cc ?

Yes, TestSuite::Init is a better place for this. Thanks.

[nilesh, 2013-05-04 10:07:43]

LGTM

[DaleCurtis, 2013-05-06 18:03:52]

Thanks for the review everyone.  CQ'ing.

[commit-bot: I haz the power, 2013-05-06 18:04:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/14891002/28001

[commit-bot: I haz the power, 2013-05-06 18:04:28]

Failed to apply patch for media/base/run_all_unittests.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file media/base/run_all_unittests.cc
  Hunk #1 FAILED at 3.
  Hunk #2 succeeded at 18 (offset 2 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
media/base/run_all_unittests.cc.rej

Patch:       media/base/run_all_unittests.cc
Index: media/base/run_all_unittests.cc
diff --git a/media/base/run_all_unittests.cc b/media/base/run_all_unittests.cc
index
ea4b7dddd55dfa2ed87132e9051bbb61fc375bb3..ed0501fec21d3986d37feff8529cccdacaf74a76
100644
--- a/media/base/run_all_unittests.cc
+++ b/media/base/run_all_unittests.cc
@@ -3,8 +3,14 @@
 // found in the LICENSE file.
 
 #include "base/test/test_suite.h"
+#include "build/build_config.h"
 #include "media/base/media.h"
 
+#if defined(OS_ANDROID)
+#include "base/android/jni_android.h"
+#include "media/base/android/media_jni_registrar.h"
+#endif
+
 class TestSuiteNoAtExit : public base::TestSuite {
  public:
   TestSuiteNoAtExit(int argc, char** argv) : TestSuite(argc, argv) {}
@@ -16,6 +22,13 @@ class TestSuiteNoAtExit : public base::TestSuite {
 void TestSuiteNoAtExit::Initialize() {
   // Run TestSuite::Initialize first so that logging is initialized.
   base::TestSuite::Initialize();
+
+#if defined(OS_ANDROID)
+  // Register JNI bindings for android.
+  JNIEnv* env = base::android::AttachCurrentThread();
+  media::RegisterJni(env);
+#endif
+
   // Run this here instead of main() to ensure an AtExitManager is already
   // present.
   media::InitializeMediaLibraryForTesting();

[DaleCurtis, 2013-05-06 18:20:51]

Rebased for recent non-conflicting vp9 changes.

[commit-bot: I haz the power, 2013-05-06 18:21:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/14891002/42001

[commit-bot: I haz the power, 2013-05-06 22:55:27]

Change committed as 198559