Upstream: Media implementation for Android.

Upstream: Media implementation for Android.

- This CL makes media_unittests linked.
- The audio_track_output_stub_android.cc is in place of
  audio_track_output_android.cc because the Java environment is not
  avaliable yet.
- Also changed to dependence on '../third_party/ffmpeg/ffmpeg.gyp'
  only if os is not Android in media.gyp.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112739

[michaelbai, 2011-11-28 17:42:44]

Please help to review it.

[michaelbai, 2011-11-29 17:53:36]

Ping Reviewers

Would you like help to review this CL?

[John Grabowski, 2011-11-29 19:43:30]

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
File media/audio/android/audio_manager_android.h (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.h:10: class AudioManagerAndroid :
public AudioManagerBase {
Class level comment, always

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
File media/audio/android/audio_track_output_android.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_android.cc:15: static const int
kTimerIntervalInMilliseconds = 50;
prefer anon namespace to static vars

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
File media/audio/android/audio_track_output_stub_android.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_stub_android.cc:9: // downstream.
Add TODO(michaelbai) to be clear we intend to finish this.

http://codereview.chromium.org/8718014/diff/1/media/base/media_android.cc
File media/base/media_android.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/base/media_android.cc#new...
media/base/media_android.cc:4: 
Add file-level comments explaining what this is for.

http://codereview.chromium.org/8718014/diff/1/media/base/media_android.cc#new...
media/base/media_android.cc:11: // Android doesn't use ffmpeg. Return true to
make chrome happy
Complete sentences end in a period.
Be clear that "happy" is "good".  E.g. 

// Android doesn't use ffmpeg so we have nothing to do.

[scherkus (not reviewing), 2011-11-29 20:00:28]

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
File media/audio/android/audio_manager_android.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.cc:32: } else if (params.format ==
AudioParameters::AUDIO_PCM_LINEAR ||
nit: no need for else

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.cc:53:
AudioManagerAndroid::AudioManagerAndroid() {
nit: collapse empty methods that fit onto one line w/ {}

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.cc:56:
AudioManagerAndroid::~AudioManagerAndroid() {
nit: try to match function ordering inside of .h where it makes sense

in this case we generally put ctor/dtor at top

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.cc:60: //static
nit: one period between // and static

also we typically put statics at the top of the file before ctor/dtor

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
File media/audio/android/audio_manager_android.h (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.h:15: virtual void Init();
why do you need this method if the impl only calls AudioManagerBase::Init()?

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.h:18: virtual bool
HasAudioOutputDevices();
OVERRIDE on all overridden virtual methods

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
File media/audio/android/audio_track_output_android.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_android.cc:210: if (!!manager &&
params.IsValid())
what's up with the !! ?

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
File media/audio/android/audio_track_output_android.h (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_android.h:29: class StreamBuffer {
nit: forward declare this class before where you use it w/ data_buffer_ and
implement the class inside the .cc

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_android.h:55: virtual bool Open();
OVERRIDE

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_android.h:62: AudioSourceCallback*
source_callback() { return source_callback_; }
why do these methods need to be exposed?

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
File media/audio/android/audio_track_output_stub_android.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_stub_android.cc:9: // downstream.
is there a TODO or bug to reference here?

I want to understand what benefit we have for including the .h but not even a
fake implementation. Is the goal to catch compilation errors in the .h file?

http://codereview.chromium.org/8718014/diff/1/media/base/media_android.cc
File media/base/media_android.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/base/media_android.cc#new...
media/base/media_android.cc:11: // Android doesn't use ffmpeg. Return true to
make chrome happy
nit: I'd reword this as "Android doesn't require any additional media
libraries."

http://codereview.chromium.org/8718014/diff/1/media/base/media_android.cc#new...
media/base/media_android.cc:15: void InitializeMediaLibraryForTesting() {
nit: collapse empty methods to {}

http://codereview.chromium.org/8718014/diff/1/media/base/test_data_util.cc
File media/base/test_data_util.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/base/test_data_util.cc#ne...
media/base/test_data_util.cc:19: void ReadTestDataFile(const std::string& name,
scoped_array<uint8>* buffer,
which media unit tests require these methods but not ffmpeg?

perhaps you should be excluding those unit tests (and this file) as well if
they're not needed

http://codereview.chromium.org/8718014/diff/1/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/8718014/diff/1/media/media.gyp#newcode267
media/media.gyp:267: ['OS!="android"', {
nit: spaces around operators here + below

[Ami GONE FROM CHROMIUM, 2011-11-29 20:41:12]

drive-by

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
File media/audio/android/audio_track_output_android.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_android.cc:15: static const int
kTimerIntervalInMilliseconds = 50;
On 2011/11/29 19:43:31, John Grabowski wrote:
> prefer anon namespace to static vars

Not in media/ (style guide is neutral, and statics have tooling benefits).

[michaelbai, 2011-11-30 17:20:33]

PTAL

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
File media/audio/android/audio_manager_android.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.cc:32: } else if (params.format ==
AudioParameters::AUDIO_PCM_LINEAR ||
On 2011/11/29 20:00:28, scherkus wrote:
> nit: no need for else

Done.

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.cc:53:
AudioManagerAndroid::AudioManagerAndroid() {
On 2011/11/29 20:00:28, scherkus wrote:
> nit: collapse empty methods that fit onto one line w/ {}

Done.

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.cc:56:
AudioManagerAndroid::~AudioManagerAndroid() {
On 2011/11/29 20:00:28, scherkus wrote:
> nit: try to match function ordering inside of .h where it makes sense
> 
> in this case we generally put ctor/dtor at top

Done.

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.cc:60: //static
On 2011/11/29 20:00:28, scherkus wrote:
> nit: one period between // and static
> 
> also we typically put statics at the top of the file before ctor/dtor

Done.

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
File media/audio/android/audio_manager_android.h (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.h:10: class AudioManagerAndroid :
public AudioManagerBase {
On 2011/11/29 19:43:31, John Grabowski wrote:
> Class level comment, always

Done.

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.h:15: virtual void Init();
On 2011/11/29 20:00:28, scherkus wrote:
> why do you need this method if the impl only calls AudioManagerBase::Init()?

Done.

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_manag...
media/audio/android/audio_manager_android.h:18: virtual bool
HasAudioOutputDevices();
On 2011/11/29 20:00:28, scherkus wrote:
> OVERRIDE on all overridden virtual methods

Done.

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
File media/audio/android/audio_track_output_android.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_android.cc:210: if (!!manager &&
params.IsValid())
On 2011/11/29 20:00:28, scherkus wrote:
> what's up with the !! ?

Done.

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
File media/audio/android/audio_track_output_android.h (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_android.h:29: class StreamBuffer {
On 2011/11/29 20:00:28, scherkus wrote:
> nit: forward declare this class before where you use it w/ data_buffer_ and
> implement the class inside the .cc

As it is used with scoped_ptr<>, forward declare would not work.

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_android.h:55: virtual bool Open();
On 2011/11/29 20:00:28, scherkus wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_android.h:62: AudioSourceCallback*
source_callback() { return source_callback_; }
On 2011/11/29 20:00:28, scherkus wrote:
> why do these methods need to be exposed?

It seemed no one actually use it, removed

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
File media/audio/android/audio_track_output_stub_android.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_stub_android.cc:9: // downstream.
On 2011/11/29 20:00:28, scherkus wrote:
> is there a TODO or bug to reference here?
> 
> I want to understand what benefit we have for including the .h but not even a
> fake implementation. Is the goal to catch compilation errors in the .h file?


Our current goal is make the media tests linked, even it might not be run
correctly.

http://codereview.chromium.org/8718014/diff/1/media/base/media_android.cc
File media/base/media_android.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/base/media_android.cc#new...
media/base/media_android.cc:11: // Android doesn't use ffmpeg. Return true to
make chrome happy
On 2011/11/29 20:00:28, scherkus wrote:
> nit: I'd reword this as "Android doesn't require any additional media
> libraries."

Done.

http://codereview.chromium.org/8718014/diff/1/media/base/test_data_util.cc
File media/base/test_data_util.cc (right):

http://codereview.chromium.org/8718014/diff/1/media/base/test_data_util.cc#ne...
media/base/test_data_util.cc:19: void ReadTestDataFile(const std::string& name,
scoped_array<uint8>* buffer,
On 2011/11/29 20:00:28, scherkus wrote:
> which media unit tests require these methods but not ffmpeg?
> 
> perhaps you should be excluding those unit tests (and this file) as well if
> they're not needed

Removed this file

http://codereview.chromium.org/8718014/diff/1/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/8718014/diff/1/media/media.gyp#newcode267
media/media.gyp:267: ['OS!="android"', {
On 2011/11/29 20:00:28, scherkus wrote:
> nit: spaces around operators here + below

Done.

[scherkus (not reviewing), 2011-12-01 17:24:13]

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
File media/audio/android/audio_track_output_android.h (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_android.h:29: class StreamBuffer {
On 2011/11/30 17:20:34, michaelbai wrote:
> On 2011/11/29 20:00:28, scherkus wrote:
> > nit: forward declare this class before where you use it w/ data_buffer_ and
> > implement the class inside the .cc
> 
> As it is used with scoped_ptr<>, forward declare would not work.

scoped_ptr<> only needs the full definition of the class if it invokes the
ctor/dtor inside a complication unit

I don't see anything inside this + other files where that's the case, in which
case you can do the following:

// audio_track_output_android.h
class StreamBuffer;
scoped_ptr<StreamBuffer> data_buffer_;


// audio_track_output_android.cc
class AudioTrackOutputStream::StreamBuffer {
  // ....
};

http://codereview.chromium.org/8718014/diff/11001/media/audio/android/audio_m...
File media/audio/android/audio_manager_android.h (right):

http://codereview.chromium.org/8718014/diff/11001/media/audio/android/audio_m...
media/audio/android/audio_manager_android.h:10: // Android implemention of
AudioManager
nit: comments end w/ periods

http://codereview.chromium.org/8718014/diff/11001/media/audio/android/audio_t...
File media/audio/android/audio_track_output_android.cc (right):

http://codereview.chromium.org/8718014/diff/11001/media/audio/android/audio_t...
media/audio/android/audio_track_output_android.cc:210: if (!manager &&
params.IsValid())
it doesn't even look like AudioTrackOutputStream uses manager -- it assigns it
in the ctor then that's it

do we even need to be passing the manager instance around?

http://codereview.chromium.org/8718014/diff/11001/media/audio/android/audio_t...
media/audio/android/audio_track_output_android.cc:212: else
nit: else not needed

http://codereview.chromium.org/8718014/diff/11001/media/base/media_android.cc
File media/base/media_android.cc (right):

http://codereview.chromium.org/8718014/diff/11001/media/base/media_android.cc...
media/base/media_android.cc:15: void InitializeMediaLibraryForTesting() {
nit: collapse empty methods

[michaelbai, 2011-12-01 18:58:06]

PTAL

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
File media/audio/android/audio_track_output_android.h (right):

http://codereview.chromium.org/8718014/diff/1/media/audio/android/audio_track...
media/audio/android/audio_track_output_android.h:29: class StreamBuffer {
On 2011/12/01 17:24:13, scherkus wrote:
> On 2011/11/30 17:20:34, michaelbai wrote:
> > On 2011/11/29 20:00:28, scherkus wrote:
> > > nit: forward declare this class before where you use it w/ data_buffer_
and
> > > implement the class inside the .cc
> > 
> > As it is used with scoped_ptr<>, forward declare would not work.
> 
> scoped_ptr<> only needs the full definition of the class if it invokes the
> ctor/dtor inside a complication unit
> 
> I don't see anything inside this + other files where that's the case, in which
> case you can do the following:
> 
> // audio_track_output_android.h
> class StreamBuffer;
> scoped_ptr<StreamBuffer> data_buffer_;
> 
> 
> // audio_track_output_android.cc
> class AudioTrackOutputStream::StreamBuffer {
>   // ....
> };

Great! It worked, don't know what happen last time I tried.

http://codereview.chromium.org/8718014/diff/11001/media/audio/android/audio_m...
File media/audio/android/audio_manager_android.h (right):

http://codereview.chromium.org/8718014/diff/11001/media/audio/android/audio_m...
media/audio/android/audio_manager_android.h:10: // Android implemention of
AudioManager
On 2011/12/01 17:24:13, scherkus wrote:
> nit: comments end w/ periods

Done.

http://codereview.chromium.org/8718014/diff/11001/media/audio/android/audio_t...
File media/audio/android/audio_track_output_android.cc (right):

http://codereview.chromium.org/8718014/diff/11001/media/audio/android/audio_t...
media/audio/android/audio_track_output_android.cc:210: if (!manager &&
params.IsValid())
On 2011/12/01 17:24:13, scherkus wrote:
> it doesn't even look like AudioTrackOutputStream uses manager -- it assigns it
> in the ctor then that's it
> 
> do we even need to be passing the manager instance around?

Done.

http://codereview.chromium.org/8718014/diff/11001/media/audio/android/audio_t...
media/audio/android/audio_track_output_android.cc:212: else
On 2011/12/01 17:24:13, scherkus wrote:
> nit: else not needed

Done.

http://codereview.chromium.org/8718014/diff/11001/media/base/media_android.cc
File media/base/media_android.cc (right):

http://codereview.chromium.org/8718014/diff/11001/media/base/media_android.cc...
media/base/media_android.cc:15: void InitializeMediaLibraryForTesting() {
On 2011/12/01 17:24:13, scherkus wrote:
> nit: collapse empty methods

Done.

[scherkus (not reviewing), 2011-12-01 19:01:13]

few more nits then we'll be good to go

http://codereview.chromium.org/8718014/diff/20001/media/audio/android/audio_t...
File media/audio/android/audio_track_output_android.cc (right):

http://codereview.chromium.org/8718014/diff/20001/media/audio/android/audio_t...
media/audio/android/audio_track_output_android.cc:35: };
DISALLOW_COPY_AND_ASSIGN()

http://codereview.chromium.org/8718014/diff/20001/media/audio/android/audio_t...
media/audio/android/audio_track_output_android.cc:228: if (!manager &&
params.IsValid())
nit: do we even need to pass in manager to this method?

http://codereview.chromium.org/8718014/diff/20001/media/audio/android/audio_t...
File media/audio/android/audio_track_output_android.h (right):

http://codereview.chromium.org/8718014/diff/20001/media/audio/android/audio_t...
media/audio/android/audio_track_output_android.h:29:
AudioTrackOutputStream(const AudioParameters& params);
explicit

[michaelbai, 2011-12-01 20:48:18]

PTAL

http://codereview.chromium.org/8718014/diff/20001/media/audio/android/audio_t...
File media/audio/android/audio_track_output_android.cc (right):

http://codereview.chromium.org/8718014/diff/20001/media/audio/android/audio_t...
media/audio/android/audio_track_output_android.cc:35: };
On 2011/12/01 19:01:14, scherkus wrote:
> DISALLOW_COPY_AND_ASSIGN()

Done.

http://codereview.chromium.org/8718014/diff/20001/media/audio/android/audio_t...
media/audio/android/audio_track_output_android.cc:228: if (!manager &&
params.IsValid())
On 2011/12/01 19:01:14, scherkus wrote:
> nit: do we even need to pass in manager to this method?

Done.

http://codereview.chromium.org/8718014/diff/20001/media/audio/android/audio_t...
File media/audio/android/audio_track_output_android.h (right):

http://codereview.chromium.org/8718014/diff/20001/media/audio/android/audio_t...
media/audio/android/audio_track_output_android.h:29:
AudioTrackOutputStream(const AudioParameters& params);
On 2011/12/01 19:01:14, scherkus wrote:
> explicit

Done.

[scherkus (not reviewing), 2011-12-01 20:49:22]

LGTM

[commit-bot: I haz the power, 2011-12-01 20:55:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/8718014/16011

[commit-bot: I haz the power, 2011-12-01 21:24:51]

Try job failure for 8718014-16011 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-02 00:36:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/8718014/18003

[commit-bot: I haz the power, 2011-12-02 03:04:41]

Try job failure for 8718014-18003 (retry) on win_rel for step "browser_tests".
It's a second try, previously, steps "browser_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-02 16:57:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/8718014/18003

[commit-bot: I haz the power, 2011-12-02 18:27:38]

Change committed as 112739