Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12457043:
Android implementation of WebAudio audio file decoder (Closed)
|
Created:
7 years, 9 months ago by Raymond Toy (Google) Modified:
7 years, 8 months ago Reviewers:
digit, aelias_OOO_until_Jul13, tommi (sloooow) - chröme, Min Qin, DaleCurtis, bulach, felipeg, palmer, acolwell GONE FROM CHROMIUM, qinmin, no sievers, felipeg_google, brettw, cpu_(ooo_6.6-7.5), digit1 CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAndroid implementation of WebAudio audio file decoder.
This implements WebAudio's DecodeAudioFileData that decodes in-memory audio files.
This in conjunction with https://codereview.chromium.org/12843039/ and https://bugs.webkit.org/show_bug.cgi?id=109755 adds support for WebAudio on Android.
Everything is behind an about:flags flag, and is disabled by default. By default, everything is also not compiled in until use_openmax_dl_fft is set to 1.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194141
Patch Set 1 : #
Total comments: 25
Patch Set 2 : #
Total comments: 17
Patch Set 3 : #
Total comments: 47
Patch Set 4 : #Patch Set 5 : Fix compiling DumpRenderTree #
Total comments: 7
Patch Set 6 : #
Total comments: 13
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Total comments: 28
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : Sync to latest #Patch Set 12 : #
Total comments: 15
Patch Set 13 : #
Total comments: 6
Patch Set 14 : #
Total comments: 7
Patch Set 15 : #
Total comments: 5
Patch Set 16 : #
Total comments: 7
Patch Set 17 : #Patch Set 18 : #
Total comments: 1
Patch Set 19 : #Messages
Total messages: 52 (0 generated)
Please review.
thanks raymond! I'm not all familiar here, so just some drive-by comments and suggestions. I'll let digit and felipe take a deeper dive in terms of the actual usage of the android api. there's a lot of logging going on here.. are they all necessary? https://codereview.chromium.org/12457043/diff/23001/content/common/view_messa... File content/common/view_messages.h (right): https://codereview.chromium.org/12457043/diff/23001/content/common/view_messa... content/common/view_messages.h:2342: nit: extra \n https://codereview.chromium.org/12457043/diff/23001/content/common/webkitplat... File content/common/webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/12457043/diff/23001/content/common/webkitplat... content/common/webkitplatformsupport_impl.cc:61: return base::Bind(&WebKitPlatformSupportImpl::RunWebAudioMediaCodec); nit: unindent https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java (right): https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:23: private static native void nativeOnChunkDecoded( nit: move these native declarations to the bottom of this file https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:38: String LOG_TAG = "WebAudioMediaCodec"; nit: LOG_TAG is normally a private static final member https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:40: "decodeAudioFile start ======================================="); nit: remove the ==== decoration, saves some bytes. :) but more to the point, are all of these logs really needed? there's plenty of them here.. https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:43: // Todo(rtoy): What is the correct timeout value for reading nit: s/Todo/TODO/ https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:55: // TODO Auto-generated catch block nit: remove? https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:99: } this would be simpler as: int channelConfig = channelCount == 2 ? AudioFormat.CHANNEL_OUT_STEREO : AudioFormat.CHANNEL_OUT_MONO; https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:115: // You must select a track. You will read samples from the nit: a while ago there's a discussion about not using pronouns in comments. :) // A track must be selected and will be used to read samples. https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:142: 0, //offset nit: 0 /* offset */, is more common as it avoids commenting the entire rest of the line.. https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:15: #include "jni/MediaCodec_jni.h" it seems this is unused.. https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:43: env_ = AttachCurrentThread(); nit: as above, avoid caching env and context. https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:53: if (rc != 0) { nit: if (rc) VLOG(0) << "Couldn't close output fd: rc = " << rc; (avoid != 0 comparison, and remove braces for one line block..) https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:58: if (rc != 0) { as above.. https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:88: info[2] = 0.5 + (duration_us * 0.000001 * sample_rate); since this is in integer space, how about: const long kUsPerSec = 1000000; info[2] = (duration_us + kUsPerSec - 1) / kUsPerSec * sample_rate; https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:117: write(output_fd_, buffer, nwrite); question: perhaps check the return value was actually nwrite? https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:127: ret = JNI_MediaCodec::RegisterNativesImpl(env); as above, this seems unused, also there's no need to DCHECK the clazz, so probably just return RegisterNativesImpl(env); https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.h (right): https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.h:48: jobject j_context_; nit: better not cache these two... JNIEnv* should be obtained by AttachCurrentThread(), and context via base::android::GetApplicationContext() https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:29: int getWriteFd() { return pipefd_[1]; }; nit: add a \n also, please make 28-29 either get_read_fd() (as is_valid), or change them all to ChromiumStyle() https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:97: if (!audio_decoder.is_valid()) { nit: remove { https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:106: base::FileDescriptor fd(audio_decoder.getWriteFd(), true); nit: remove extra space before "fd(" https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:186: decoded_frames++; nit: ++decoded_frames;
On 2013/03/28 13:39:25, bulach wrote: > thanks raymond! I'm not all familiar here, so just some drive-by comments and > suggestions. I'll let digit and felipe take a deeper dive in terms of the actual > usage of the android api. > > there's a lot of logging going on here.. are they all necessary? No, they're not necessary, and I'll remove more. But the log information about the mime type, channel count and duration have been important to see. It did show a bug in MediaCodec decoding some Ogg files which returns a huge multi-year duration. It also looks like it doesn't handle >4 channels for wav files. So I'd like to keep these for a while. Thanks for the other comments. I'll address them in another patch. > > https://codereview.chromium.org/12457043/diff/23001/content/common/view_messa... > File content/common/view_messages.h (right): > > https://codereview.chromium.org/12457043/diff/23001/content/common/view_messa... > content/common/view_messages.h:2342: > nit: extra \n > > https://codereview.chromium.org/12457043/diff/23001/content/common/webkitplat... > File content/common/webkitplatformsupport_impl.cc (right): > > https://codereview.chromium.org/12457043/diff/23001/content/common/webkitplat... > content/common/webkitplatformsupport_impl.cc:61: return > base::Bind(&WebKitPlatformSupportImpl::RunWebAudioMediaCodec); > nit: unindent > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... > File > media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java > (right): > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... > media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:23: > private static native void nativeOnChunkDecoded( > nit: move these native declarations to the bottom of this file > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... > media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:38: > String LOG_TAG = "WebAudioMediaCodec"; > nit: LOG_TAG is normally a private static final member > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... > media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:40: > "decodeAudioFile start ======================================="); > nit: remove the ==== decoration, saves some bytes. :) but more to the point, are > all of these logs really needed? there's plenty of them here.. > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... > media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:43: > // Todo(rtoy): What is the correct timeout value for reading > nit: s/Todo/TODO/ > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... > media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:55: > // TODO Auto-generated catch block > nit: remove? > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... > media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:99: > } > this would be simpler as: > int channelConfig = channelCount == 2 ? AudioFormat.CHANNEL_OUT_STEREO : > AudioFormat.CHANNEL_OUT_MONO; > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... > media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:115: > // You must select a track. You will read samples from the > nit: a while ago there's a discussion about not using pronouns in comments. :) > > // A track must be selected and will be used to read samples. > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/java/s... > media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:142: > 0, //offset > nit: 0 /* offset */, is more common as it avoids commenting the entire rest of > the line.. > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... > File media/base/android/webaudio_media_codec_bridge.cc (right): > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... > media/base/android/webaudio_media_codec_bridge.cc:15: #include > "jni/MediaCodec_jni.h" > it seems this is unused.. > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... > media/base/android/webaudio_media_codec_bridge.cc:43: env_ = > AttachCurrentThread(); > nit: as above, avoid caching env and context. > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... > media/base/android/webaudio_media_codec_bridge.cc:53: if (rc != 0) { > nit: > if (rc) > VLOG(0) << "Couldn't close output fd: rc = " << rc; > > (avoid != 0 comparison, and remove braces for one line block..) > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... > media/base/android/webaudio_media_codec_bridge.cc:58: if (rc != 0) { > as above.. > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... > media/base/android/webaudio_media_codec_bridge.cc:88: info[2] = 0.5 + > (duration_us * 0.000001 * sample_rate); > since this is in integer space, how about: > const long kUsPerSec = 1000000; > info[2] = (duration_us + kUsPerSec - 1) / kUsPerSec * sample_rate; > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... > media/base/android/webaudio_media_codec_bridge.cc:117: write(output_fd_, buffer, > nwrite); > question: perhaps check the return value was actually nwrite? > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... > media/base/android/webaudio_media_codec_bridge.cc:127: ret = > JNI_MediaCodec::RegisterNativesImpl(env); > as above, this seems unused, also there's no need to DCHECK the clazz, so > probably just return RegisterNativesImpl(env); > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... > File media/base/android/webaudio_media_codec_bridge.h (right): > > https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... > media/base/android/webaudio_media_codec_bridge.h:48: jobject j_context_; > nit: better not cache these two... JNIEnv* should be obtained by > AttachCurrentThread(), and context via base::android::GetApplicationContext() > > https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... > File webkit/media/android/audio_decoder_android.cc (right): > > https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... > webkit/media/android/audio_decoder_android.cc:29: int getWriteFd() { return > pipefd_[1]; }; > nit: add a \n > also, please make 28-29 either get_read_fd() (as is_valid), or change them all > to ChromiumStyle() > > https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... > webkit/media/android/audio_decoder_android.cc:97: if (!audio_decoder.is_valid()) > { > nit: remove { > > https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... > webkit/media/android/audio_decoder_android.cc:106: base::FileDescriptor > fd(audio_decoder.getWriteFd(), true); > nit: remove extra space before "fd(" > > https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... > webkit/media/android/audio_decoder_android.cc:186: decoded_frames++; > nit: ++decoded_frames;
Just a few comments. Your other comments will be addressed in an upcoming patch. https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:88: info[2] = 0.5 + (duration_us * 0.000001 * sample_rate); On 2013/03/28 13:39:25, bulach wrote: > since this is in integer space, how about: > const long kUsPerSec = 1000000; > info[2] = (duration_us + kUsPerSec - 1) / kUsPerSec * sample_rate; I think it's clearer to do a floating-point conversion. And to be correct, I think it should be ((duration_us * sample_rate) + kUsPerSec - 1) / kUsPerSec https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:29: int getWriteFd() { return pipefd_[1]; }; On 2013/03/28 13:39:25, bulach wrote: > nit: add a \n > also, please make 28-29 either get_read_fd() (as is_valid), or change them all > to ChromiumStyle() I was following http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names
https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:29: int getWriteFd() { return pipefd_[1]; }; On 2013/03/28 17:38:31, rtoy wrote: > On 2013/03/28 13:39:25, bulach wrote: > > nit: add a \n > > also, please make 28-29 either get_read_fd() (as is_valid), or change them all > > to ChromiumStyle() > > I was following > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names hmm, I think that means either: GetReadFd() or get_read_fd() but no "g"etReadFd() right? so afaict, the whole set should be either: - is_valid, get_read_fd, get_write_fd or - IsValid, GetReadFd, GetWriteFd also, while at it, I think they can all be made const. :)
On 2013/03/28 17:42:31, bulach wrote: > https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... > File webkit/media/android/audio_decoder_android.cc (right): > > https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audi... > webkit/media/android/audio_decoder_android.cc:29: int getWriteFd() { return > pipefd_[1]; }; > On 2013/03/28 17:38:31, rtoy wrote: > > On 2013/03/28 13:39:25, bulach wrote: > > > nit: add a \n > > > also, please make 28-29 either get_read_fd() (as is_valid), or change them > all > > > to ChromiumStyle() > > > > I was following > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names > > hmm, I think that means either: > GetReadFd() > or > get_read_fd() > > but no "g"etReadFd() right? > so afaict, the whole set should be either: > - is_valid, get_read_fd, get_write_fd > or > - IsValid, GetReadFd, GetWriteFd > > also, while at it, I think they can all be made const. :) Oops. getReadFd is certainly wrong. :-) I will use IsValid and friends as you suggest and make them const.
great, thanks!! :) On Thu, Mar 28, 2013 at 5:47 PM, <rtoy@google.com> wrote: > On 2013/03/28 17:42:31, bulach wrote: > > https://codereview.chromium.**org/12457043/diff/23001/** > webkit/media/android/audio_**decoder_android.cc<https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audio_decoder_android.cc> > >> File webkit/media/android/audio_**decoder_android.cc (right): >> > > > https://codereview.chromium.**org/12457043/diff/23001/** > webkit/media/android/audio_**decoder_android.cc#newcode29<https://codereview.chromium.org/12457043/diff/23001/webkit/media/android/audio_decoder_android.cc#newcode29> > >> webkit/media/android/audio_**decoder_android.cc:29: int getWriteFd() { >> return >> pipefd_[1]; }; >> On 2013/03/28 17:38:31, rtoy wrote: >> > On 2013/03/28 13:39:25, bulach wrote: >> > > nit: add a \n >> > > also, please make 28-29 either get_read_fd() (as is_valid), or change >> them >> all >> > > to ChromiumStyle() >> > >> > I was following >> > >> > http://google-styleguide.**googlecode.com/svn/trunk/** > cppguide.xml#Function_Names<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names> > > hmm, I think that means either: >> GetReadFd() >> or >> get_read_fd() >> > > but no "g"etReadFd() right? >> so afaict, the whole set should be either: >> - is_valid, get_read_fd, get_write_fd >> or >> - IsValid, GetReadFd, GetWriteFd >> > > also, while at it, I think they can all be made const. :) >> > > Oops. getReadFd is certainly wrong. :-) I will use IsValid and friends as > you > suggest and make them const. > > https://codereview.chromium.**org/12457043/<https://codereview.chromium.org/1... >
LGTM only minor comments (blank lines, etc..) https://codereview.chromium.org/12457043/diff/34001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java (right): https://codereview.chromium.org/12457043/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:33: You can drop a few vertical blank lines in this file. https://codereview.chromium.org/12457043/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:75: drop one blank line https://codereview.chromium.org/12457043/diff/34001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:81: mime.equals("audio/vorbis")); It may be easier to understand if you store this in a local var with a meaningful name and / or add a comment about the vorbis duration issue. https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:19: using base::android::DetachFromVM; this is not being used https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:38: drop blank https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:45: int rc = close(output_fd_); s/rc/result or something meaningful https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:79: Drop blank line https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:83: info[2] = 0.5 + (duration_us * 0.000001 * sample_rate); add a comment regarding the math here (I already forgot) and also a comment regarding the format you are using for passing the data through the pipe. In this case would be the 4 consecutive longs as a long info[4] array https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:100: drop blank https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:108: // Write out the data to the pipe atomically, in small chunks if what you mean by atomically ? https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.h (right): https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.h:25: WebAudioMediaCodecBridge(base::SharedMemoryHandle input_fd, Add a comment saying that this class takes ownership of the file descriptors (it closes in the destructor). https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.h:49: int input_fd_; // Owned. https://codereview.chromium.org/12457043/diff/34001/webkit/media/android/audi... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/34001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:99: you can also drop some blank lines here https://codereview.chromium.org/12457043/diff/34001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:112: runner.Run(encoded_data_handle, fd); I guess you cannot call directly GetWebAudioMediaCodecRunner() here, since it will create an invalid dependency to content/common/ ? This is a good solution. https://codereview.chromium.org/12457043/diff/34001/webkit/media/audio_decoder.h File webkit/media/audio_decoder.h (right): https://codereview.chromium.org/12457043/diff/34001/webkit/media/audio_decode... webkit/media/audio_decoder.h:24: typedef base::Callback<void (base::SharedMemoryHandle, base::FileDescriptor)> Add a comment about what it is and why we need this callback.
Thanks for the review. Next patch should address all of your comments. https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:108: // Write out the data to the pipe atomically, in small chunks if On 2013/03/29 17:18:29, felipeg1 wrote: > what you mean by atomically ? Irrelevant here since we are the only writer of the pipe. I'll remove this. https://codereview.chromium.org/12457043/diff/34001/webkit/media/android/audi... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/34001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:112: runner.Run(encoded_data_handle, fd); On 2013/03/29 17:18:29, felipeg1 wrote: > I guess you cannot call directly > GetWebAudioMediaCodecRunner() here, since it will create an invalid dependency > to content/common/ ? > > This is a good solution. Yes, that creates an invalid dependency. This is the basic solution that Phillipe Liard suggested.
Cool, thanks Ray On Fri, Mar 29, 2013 at 8:47 PM, <rtoy@google.com> wrote: > Thanks for the review. Next patch should address all of your comments. > > > > https://codereview.chromium.**org/12457043/diff/34001/media/** > base/android/webaudio_media_**codec_bridge.cc<https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaudio_media_codec_bridge.cc> > File media/base/android/webaudio_**media_codec_bridge.cc (right): > > https://codereview.chromium.**org/12457043/diff/34001/media/** > base/android/webaudio_media_**codec_bridge.cc#newcode108<https://codereview.chromium.org/12457043/diff/34001/media/base/android/webaudio_media_codec_bridge.cc#newcode108> > media/base/android/webaudio_**media_codec_bridge.cc:108: // Write out the > data to the pipe atomically, in small chunks if > On 2013/03/29 17:18:29, felipeg1 wrote: > >> what you mean by atomically ? >> > > Irrelevant here since we are the only writer of the pipe. I'll remove > this. > > > https://codereview.chromium.**org/12457043/diff/34001/** > webkit/media/android/audio_**decoder_android.cc<https://codereview.chromium.org/12457043/diff/34001/webkit/media/android/audio_decoder_android.cc> > File webkit/media/android/audio_**decoder_android.cc (right): > > https://codereview.chromium.**org/12457043/diff/34001/** > webkit/media/android/audio_**decoder_android.cc#newcode112<https://codereview.chromium.org/12457043/diff/34001/webkit/media/android/audio_decoder_android.cc#newcode112> > webkit/media/android/audio_**decoder_android.cc:112: > runner.Run(encoded_data_**handle, fd); > On 2013/03/29 17:18:29, felipeg1 wrote: > >> I guess you cannot call directly >> GetWebAudioMediaCodecRunner() here, since it will create an invalid >> > dependency > >> to content/common/ ? >> > > This is a good solution. >> > > Yes, that creates an invalid dependency. This is the basic solution > that Phillipe Liard suggested. > > https://codereview.chromium.**org/12457043/<https://codereview.chromium.org/1... >
Adding OWNERS to review list. Please review at your convenience.
On 2013/03/29 22:26:13, rtoy wrote: > Adding OWNERS to review list. > > Please review at your convenience. 14 reviewers is a bit much. please trim down, and tell each reviewer what you want them to look at.
lgtm, thanks! just nits for android.. feel free to go ahead once the respective owners are happy. https://codereview.chromium.org/12457043/diff/45001/content/common/view_messa... File content/common/view_messages.h (right): https://codereview.chromium.org/12457043/diff/45001/content/common/view_messa... content/common/view_messages.h:2339: IPC_MESSAGE_CONTROL2(ViewHostMsg_WebAudioMediaCodec, nit: I think the message name should contain a verb, so this probably should be ViewHostMsg_RunWebAudioMediaCodec ? ditto for the method it calls.. https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java (right): https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:28: ByteBuffer[] codecOutputBuffers; nit: probably best to declare this way below closer to where it's actually being used. https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:32: long TIMEOUT_US = 500; nit: final https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:33: MediaExtractor extractor; nit: declare and create in one line: MediaExtractor extractor = new MediaExtractor() https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:45: Log.d(LOG_TAG, String.format("TRACKS #: %d", extractor.getTrackCount())); nit: I still think there's a lot of logging going on, which takes up memory and cpu cycles.. if this is really important, would it be possible to have a single line with all the information needed? Log.d(LOG_TAG, "WebAudio " + extractor.getTrackCount() + " " + mime + " " + sampleRate + " " + channelCount + " " + duration_us); it seems it'd still give all the same information, but in a more compact way. wdyt? https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:89: codec = MediaCodec.createDecoderByType(mime); nit: as above: MediaCodec codec = MediaCodec.createDecoderByType(mime); https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:93: codecInputBuffers = codec.getInputBuffers(); ditto, ByteBuffer[] codecInputBuffers = codec.getInputBuffers();
Hi Raymond, I defer my part to Dale and remove myself from the reviewer list. BR, SX
note that grd files afaik don't have owners.
https://codereview.chromium.org/12457043/diff/45001/content/browser/renderer_... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/12457043/diff/45001/content/browser/renderer_... content/browser/renderer_host/render_message_filter.cc:1155: base::SharedMemoryHandle input_fd, These variable names seem swapped, based on their types! Surely, the handle should be called "handle", and the file descriptor should be called "fd". :) https://codereview.chromium.org/12457043/diff/45001/content/browser/renderer_... File content/browser/renderer_host/render_message_filter.h (right): https://codereview.chromium.org/12457043/diff/45001/content/browser/renderer_... content/browser/renderer_host/render_message_filter.h:264: void OnWebAudioMediaCodec(base::SharedMemoryHandle memory, I agree, this needs a past participle of a verb. OnWebAudioMediaCodecReceived or whatever makes sense to you. https://codereview.chromium.org/12457043/diff/45001/content/common/view_messa... File content/common/view_messages.h (right): https://codereview.chromium.org/12457043/diff/45001/content/common/view_messa... content/common/view_messages.h:2340: base::SharedMemoryHandle /* encoded audio mem */, Usually, these comments use descriptive, real variable names. E.g. /* audio_memory */, /* pcm_data_sink */, or whatever. The actual IPC handler functions should use those same variable names. https://codereview.chromium.org/12457043/diff/45001/content/common/webkitplat... File content/common/webkitplatformsupport_impl.h (right): https://codereview.chromium.org/12457043/diff/45001/content/common/webkitplat... content/common/webkitplatformsupport_impl.h:40: base::SharedMemoryHandle encoded_data_handle, Right; use these names in the IPC declaration in *_messages.h. https://codereview.chromium.org/12457043/diff/45001/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/12457043/diff/45001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:86: // TODO(xians): figure out the right output sample rate and sample rate to I take it "output sample rate and sample rate" should instead say, "default sample rate and buffer size". Right? https://codereview.chromium.org/12457043/diff/45001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:88: static const int kDefaultSampleRate = 44100; Does this mean the above TODO(xians) is resolved, or are these numbers an experiment? https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java (right): https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:32: long TIMEOUT_US = 500; NIT: "U" only sort of looks like the Greek letter mu. This reads like "timeout us". Instead, consider naming it TIMEOUT_MICROSECONDS. https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:32: long TIMEOUT_US = 500; On 2013/04/02 11:05:11, bulach wrote: > nit: final Also static, and declared in class scope instead of method scope? https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:45: Log.d(LOG_TAG, String.format("TRACKS #: %d", extractor.getTrackCount())); Agreed. https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:53: String mime = format.getString(MediaFormat.KEY_MIME); At the least, this is a lot of debug logging code that obscures the meaning of this decodeAudioFile method. Maybe move the debugging into its own method? https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:73: AudioFormat.CHANNEL_OUT_STEREO : AudioFormat.CHANNEL_OUT_MONO; So we treat 5.1 (hypothetically) as mono? https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:80: // TODO(rtoy): File a bug and find out why a huge value is Just file the bug now instead of putting a TODO in the code. Then refer to the bug # in the above comment. https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:112: long presentationTimeUs = 0; Again, maybe presentationTimeMicroS ? https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:132: // Output side NIT: Swap lines 132 and 133, so that the blank line separates the above while loop with the output side code. https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:138: int outputBufIndex = res; NIT: Maybe you can get rid of outputBufIndex (and then change the name of |res| to |outputBufIndex|)? https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:162: private static native void nativeOnChunkDecoded( NIT: Blank line above this new method to separate it from previous. https://codereview.chromium.org/12457043/diff/45001/media/base/android/media_... File media/base/android/media_jni_registrar.cc (left): https://codereview.chromium.org/12457043/diff/45001/media/base/android/media_... media/base/android/media_jni_registrar.cc:26: VideoCaptureDeviceAndroid::RegisterVideoCaptureDevice }, So we no longer use VideoCaptureDevice? https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:37: DVLOG(0) << "WebAudioMediaCodecBridge start **********************"; Combine these two DVLOG statements into one. https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:45: VLOG(0) << "Couldn't close output fd " << output_fd_ In both of these VLOGs, would it help to use strerror(3) or similar to get a description of the error? More useful than result, anyway, which will just be -1 or 0. https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:77: long info[4]; NIT: I think you can combine the declaration with the definition. Also, why long if the values are (j)ints? You might be doubling the size of the message that you write, for no reason. (Although underneath, long might be the same as jint, so it might not matter in practice.) https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:81: // this file is an ogg/vorbis file. Information is sent as a set of Use the same MIME type string as elsewhere; I think it was "audio/vorbis"? https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:85: // The number of frames is the length of the file (in us) times the NIT: "in microseconds" https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:87: info[2] = 0.5 + (duration_us * 0.000001 * sample_rate); NIT: Only one space after *. https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:90: DVLOG(0) << "InitializeDestination:"; Combine these into one statement. https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:104: signed short* decoded_buffer = Do you need the signed keyword? Also, might it be more clear and specific to say int16_t? https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:108: int bytes_left = buf_size; Not sure you need this; just use buf_size. https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:109: signed short* buffer = decoded_buffer; You don't really need to have both |buffer| and |decoded_buffer|, it seems. https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:114: int bytes_written = write(output_fd_, buffer, bytes_to_write); write(2) returns ssize_t, not int. https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:115: if (bytes_written != bytes_to_write) You detect short writes, but not errors (return of -1). On error, you might as well quit the loop. https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:119: buffer += bytes_written / sizeof(decoded_buffer[0]); You could just use sizeof(buffer[0]) or sizeof(int16_t) here. Don't need both |buffer| and |decoded_buffer|. https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.h (right): https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.h:16: // This class serves as a bridge for native code to call java functions inside NIT: Capitalization and article: "...to call Java functions inside the Android MediaCodec class." Then, simply: "See http://developer..." https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.h:23: // and decoded pcm samples are written to |output_fd|. We also take NIT: Capitalize initialisms: PCM. https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.h:25: WebAudioMediaCodecBridge(base::SharedMemoryHandle input_fd, Use the same variable names for these objects here as you do elsewhere. In particular, it's a bit confusing to call a SharedMemoryHandle an fd (even if it is one underneath). https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.h:41: jint number_of_channels, NIT: |channel_count| is more concise.
On 2013/03/29 23:25:46, jam wrote: > On 2013/03/29 22:26:13, rtoy wrote: > > Adding OWNERS to review list. > > > > Please review at your convenience. > > 14 reviewers is a bit much. please trim down, and tell each reviewer what you > want them to look at. Sorry. I've trimmed the list of reviewers to something more manageable.
https://codereview.chromium.org/12457043/diff/45001/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/12457043/diff/45001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:86: // TODO(xians): figure out the right output sample rate and sample rate to On 2013/04/02 18:01:08, Chris P. wrote: > I take it "output sample rate and sample rate" should instead say, "default > sample rate and buffer size". Right? Yes. I will fix it. https://codereview.chromium.org/12457043/diff/45001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:88: static const int kDefaultSampleRate = 44100; On 2013/04/02 18:01:08, Chris P. wrote: > Does this mean the above TODO(xians) is resolved, or are these numbers an > experiment? Still not resolved yet. We need to ask the OS for the native sampling rate. https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java (right): https://codereview.chromium.org/12457043/diff/45001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:73: AudioFormat.CHANNEL_OUT_STEREO : AudioFormat.CHANNEL_OUT_MONO; On 2013/04/02 18:01:08, Chris P. wrote: > So we treat 5.1 (hypothetically) as mono? This isn't used anywhere, so I'm deleting this. https://codereview.chromium.org/12457043/diff/45001/media/base/android/media_... File media/base/android/media_jni_registrar.cc (left): https://codereview.chromium.org/12457043/diff/45001/media/base/android/media_... media/base/android/media_jni_registrar.cc:26: VideoCaptureDeviceAndroid::RegisterVideoCaptureDevice }, On 2013/04/02 18:01:08, Chris P. wrote: > So we no longer use VideoCaptureDevice? Oops. This should not have been removed and I fixed it earlier. Don't know why it's showing up here now. https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/45001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:77: long info[4]; On 2013/04/02 18:01:08, Chris P. wrote: > NIT: I think you can combine the declaration with the definition. Also, why long > if the values are (j)ints? You might be doubling the size of the message that > you write, for no reason. (Although underneath, long might be the same as jint, > so it might not matter in practice.) It's long so we can handle very long audio files, but that seems unlikely on Android devices. And for typical audio files 4 longs is insignificant compared to the number of PCM samples.
https://codereview.chromium.org/12457043/diff/45001/media/base/android/media_... File media/base/android/media_jni_registrar.cc (left): https://codereview.chromium.org/12457043/diff/45001/media/base/android/media_... media/base/android/media_jni_registrar.cc:26: VideoCaptureDeviceAndroid::RegisterVideoCaptureDevice }, On 2013/04/02 21:52:21, rtoy wrote: > On 2013/04/02 18:01:08, Chris P. wrote: > > So we no longer use VideoCaptureDevice? > > Oops. This should not have been removed and I fixed it earlier. Don't know why > it's showing up here now. Something is wrong here. My local sources have VideoCaptureDevice, but the diff shows that it has been removed.
Please review patch set 5, which fixes an issue that prevented DumpRenderTree from compiling. Only glue/webkitplatformsupport_impl.* changed.
https://codereview.chromium.org/12457043/diff/64001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java (right): https://codereview.chromium.org/12457043/diff/64001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:37: e.printStackTrace(); I assume you would want to call encodedFD.detachFd() here too. https://codereview.chromium.org/12457043/diff/64001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:57: Log.d(LOG_TAG, "Cannot get duration"); should you exit here, or is it ok to keep duration_microseconds at 0? https://codereview.chromium.org/12457043/diff/64001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:61: Log.d(LOG_TAG, "Tracks: " + extractor.getTrackCount() This Log.d() call is really slow: it will create a StringBuilder and perform multiple appends on it before calling a function that does absolutely nothing unless you run on a non-production device. For this typical use case, it is recommended to guard this with an if (DEBUG) { Log.i() }, where DEBUG is a static final boolean that is defined to false by default. E.g.: private static final DEBUG = false; ... if (DEBUG) { Log.i(LOG_TAG, "Tracks:" + ....); } It will get optimized out on production builds, but you can easily toogle this during debugging. https://codereview.chromium.org/12457043/diff/64001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:128: if (outputBufIndex >= 0) { Shouldn't you also handle INFO_OUTPUT_BUFFERS_CHANGED / INFO_OUTPUT_FORMAT_CHANGED ? I admit the documentation isn't very clear about this.
https://codereview.chromium.org/12457043/diff/64001/media/base/android/java/s... File media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java (right): https://codereview.chromium.org/12457043/diff/64001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:57: Log.d(LOG_TAG, "Cannot get duration"); On 2013/04/04 15:56:47, digit1 wrote: > should you exit here, or is it ok to keep duration_microseconds at 0? I believe it's ok to keep going. MediaCodec will decode the correct number of samples, even if MediaFormat could not determine the duration. I don't know under what conditions this happens, but I have seen this on some mp3 files on some demos. https://codereview.chromium.org/12457043/diff/64001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:61: Log.d(LOG_TAG, "Tracks: " + extractor.getTrackCount() On 2013/04/04 15:56:47, digit1 wrote: > This Log.d() call is really slow: it will create a StringBuilder and perform > multiple appends on it before calling a function that does absolutely nothing > unless you run on a non-production device. > > For this typical use case, it is recommended to guard this with an if (DEBUG) { > Log.i() }, where DEBUG is a static final boolean that is defined to false by > default. E.g.: > > private static final DEBUG = false; > > ... > > if (DEBUG) { > Log.i(LOG_TAG, "Tracks:" + ....); > } > > It will get optimized out on production builds, but you can easily toogle this > during debugging. Ok. I'll add this, but I do hope to remove this particular log after letting webaudio bake for a bit on Android. I'd like to make the DEBUG default to true for now. https://codereview.chromium.org/12457043/diff/64001/media/base/android/java/s... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:128: if (outputBufIndex >= 0) { On 2013/04/04 15:56:47, digit1 wrote: > Shouldn't you also handle INFO_OUTPUT_BUFFERS_CHANGED / > INFO_OUTPUT_FORMAT_CHANGED ? I admit the documentation isn't very clear about > this. Oops. The case for INFO_OUTPUT_BUFFERS_CHANGED was deleted in an earlier patch. As far as I know, INFO_OUTPUT_FORMAT_CHANGED doesn't need any special handling, and the previous code just printed out the new format.
https://codereview.chromium.org/12457043/diff/72001/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/12457043/diff/72001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:88: static const int kDefaultSampleRate = 44100; Is the TODO still relevant now since you are changing these values? https://codereview.chromium.org/12457043/diff/72001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/72001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:28: bool result = bridge.DecodeInMemoryAudioFile(); nit: Does DecodeInMemoryAudioFile() really need to return a bool? You are already printing out the result of Java_WebAudioMediaCodecBridge_decodeAudioFile() inside that method and you aren't using the return value for anything else here. https://codereview.chromium.org/12457043/diff/72001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:45: VLOG(0) << "Couldn't close output fd " << pcm_output_; nit: DVLOG instead here and below? https://codereview.chromium.org/12457043/diff/72001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:114: if (bytes_written != bytes_to_write) nit: add { since this is a multi-line conditional body. https://codereview.chromium.org/12457043/diff/72001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:114: if (bytes_written != bytes_to_write) nit: Add {} since this is a multi-line conditional body. https://codereview.chromium.org/12457043/diff/72001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:124: return ret; nit: Change to "return RegisterNativesImpl(env);" since ret isn't used anywhere other than the return statement. https://codereview.chromium.org/12457043/diff/72001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.h (right): https://codereview.chromium.org/12457043/diff/72001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.h:29: bool DecodeInMemoryAudioFile(); nit: Add comment indicating what this method does and what the return value means. https://codereview.chromium.org/12457043/diff/72001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.h:29: bool DecodeInMemoryAudioFile(); nit: Can this be made private since it looks like it is only called by RunWebAudioMediaCodec() https://codereview.chromium.org/12457043/diff/72001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.h:30: static bool RegisterWebAudioMediaCodecBridge(JNIEnv* env); nit: Add comment indicating what this method does and what the return value means. https://codereview.chromium.org/12457043/diff/72001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.h:55: }; DISALLOW_COPY_AND_ASSIGN(WebAudioMediaCodecBridge) https://codereview.chromium.org/12457043/diff/72001/webkit/media/android/audi... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/72001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:28: int GetReadFd() const { return pipefd_[0]; } nit: How about just replacing pipefd_ with read_fd_ and write_fd_ so that the code is a little easier to read?
https://codereview.chromium.org/12457043/diff/72001/media/audio/android/audio... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/12457043/diff/72001/media/audio/android/audio... media/audio/android/audio_manager_android.cc:88: static const int kDefaultSampleRate = 44100; On 2013/04/04 20:53:52, acolwell wrote: > Is the TODO still relevant now since you are changing these values? Yes, it's still relevant. Android API 17 now supplies a way to get the native sample rate, and we will probably use that soon. The buffer size should be reduced to reduce latency, but that will require much experimentation and optimization to get a good choice. https://codereview.chromium.org/12457043/diff/72001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/72001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:28: bool result = bridge.DecodeInMemoryAudioFile(); On 2013/04/04 20:53:52, acolwell wrote: > nit: Does DecodeInMemoryAudioFile() really need to return a bool? You are > already printing out the result of > Java_WebAudioMediaCodecBridge_decodeAudioFile() inside that method and you > aren't using the return value for anything else here. It should probably be returned from RunWebAudioMediaCodec to indicate success or failure of the decoding. Not all of the error checking is in place yet. May I fix this in a follow up CL?
lgtm https://codereview.chromium.org/12457043/diff/76002/webkit/glue/webkitplatfor... File webkit/glue/webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/12457043/diff/76002/webkit/glue/webkitplatfor... webkit/glue/webkitplatformsupport_impl.cc:983: } Nit: leave blank line before namespace end.
I think Patch 8 has addressed all comments thus far. +tommi@: Can you review the changes in media/audio?
lgtm for media/audio/android/audio_manager_android.cc. https://codereview.chromium.org/12457043/diff/86001/webkit/media/audio_decoder.h File webkit/media/audio_decoder.h (right): https://codereview.chromium.org/12457043/diff/86001/webkit/media/audio_decode... webkit/media/audio_decoder.h:9: #include "base/callback_forward.h" should we have #if guards for these includes as well?
https://codereview.chromium.org/12457043/diff/76002/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/12457043/diff/76002/chrome/app/generated_reso... chrome/app/generated_resources.grd:6630: + Enabling this option allows web applications to access the WebAudio API. Maybe "web sites" instead of "web applications". I think? that normal people don't use the term "web application".
https://codereview.chromium.org/12457043/diff/86001/content/browser/renderer_... File content/browser/renderer_host/render_message_filter.h (right): https://codereview.chromium.org/12457043/diff/86001/content/browser/renderer_... content/browser/renderer_host/render_message_filter.h:264: void OnWebAudioMediaCodec(base::SharedMemoryHandle memory, Looks like this is the last place where you haven't unified the names (|encoded_data_handle| and |pcm_output|). https://codereview.chromium.org/12457043/diff/86001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/86001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:45: DVLOG(0) << "Couldn't close output fd " << pcm_output_; In both of these DVLOG messages, maybe it makes sense to use strerror(errno) to describe the error? Up to you. https://codereview.chromium.org/12457043/diff/86001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:53: // Process the encoded data that is in shared memory given by the NIT: You could be more concise here. "Process the data from |encoded_data_handle_|." We already know it's shared memory due to its type. https://codereview.chromium.org/12457043/diff/86001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:91: << " microsec vorbis= " Did you mean just "vorbis" here? https://codereview.chromium.org/12457043/diff/86001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:106: int16_t* buffer = decoded_buffer; I still don't think you need both |buffer| and |decoded_buffer|, but it's up to you. https://codereview.chromium.org/12457043/diff/86001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:114: if (bytes_written != bytes_to_write) { This isn't really an exceptional condition worthy of VLOG; it's part of the standard API for write(2). But, up to you. https://codereview.chromium.org/12457043/diff/86001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:119: buffer += bytes_written / sizeof(decoded_buffer[0]); It's a bit confusing that we write in terms of bytes, but increment the buffer in terms of int16_ts. What if someday the samples are 24-bit? Could that ever happen? Can we use int8_t* instead of int16_t* for decoded_buffer? Would make the code arguably easier to verify correct, and would enable us to support 24-bit samples without having to do any extra work. Or, maybe I am wrong. :) https://codereview.chromium.org/12457043/diff/86001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.h (right): https://codereview.chromium.org/12457043/diff/86001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.h:20: public: NIT: public and private get indented by 1 space in Chromium style. https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:23: public: STYLE NIT: public and private get indented by 1 space. https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:45: Might make sense to sanity-check |data| and |data_size| here. https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:67: if (read_fd_ >= 0) { Note a discrepancy here: |IsValid| checks that read_fd_ is > 0, but we will close it even if it is == 0. Seems odd. https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:70: DVLOG(0) << "Failed to close read end!"; As before, maybe it'd be useful to use strerror. https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:80: // NIT: No leading //. https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:83: // decoding in the browser. To do this, we create a shared memory It's not so much that we *do* the decoding in the browser, it's that we initiate the request to MediaCodec in the browser. MediaCodec does the decoding. (Right?) https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:132: int number_of_channels = info[0]; Sanity check this --- What would 100 mean, or -1? https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:133: unsigned long expected_number_of_samples = info[2] * number_of_channels; I would use a single term, either "samples" or "frames", but not both. https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:139: number_of_channels > media::limits::kMaxChannels || -1 would (incorrectly) pass this test. https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:145: short pipe_data[PIPE_BUF / sizeof(short)]; int16_t https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:146: std::vector<short> decoded_samples; Elsewhere, you use the more exact type, int16_t. That's a good idea; We expect 16-bit samples, so declare that. short is ambiguous. https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:150: int nsamples = nread / sizeof(short); My instinct is to use size_t for this and for |k|. https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:167: int number_of_samples = decoded_samples.size(); size_t here and on next line https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:170: destination_bus->initialize(number_of_channels, Since these are now size_ts, you may need to change the interface of |initialize|. If that is not possible, you should use base::checked_numeric_cast. https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:175: for (int m = 0; m < number_of_samples; m += number_of_channels) { size_t throughout
https://codereview.chromium.org/12457043/diff/86001/media/base/android/webaud... File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/86001/media/base/android/webaud... media/base/android/webaudio_media_codec_bridge.cc:91: << " microsec vorbis= " On 2013/04/10 00:57:53, Chris P. wrote: > Did you mean just "vorbis" here? I wanted the duration to include the units. I'll split this to make it clearer. https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:45: On 2013/04/10 00:57:53, Chris P. wrote: > Might make sense to sanity-check |data| and |data_size| here. Yes, checking |data| is a good idea. I don't have a good feel for what |data_size| should be other than it should probably be strictly positive. https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:67: if (read_fd_ >= 0) { On 2013/04/10 00:57:53, Chris P. wrote: > Note a discrepancy here: |IsValid| checks that read_fd_ is > 0, but we will > close it even if it is == 0. Seems odd. A bug. Thanks. read_fd_ could be 0.
Please review patch 9.
https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audi... webkit/media/android/audio_decoder_android.cc:45: > Yes, checking |data| is a good idea. I don't have a good feel for what > |data_size| should be other than it should probably be strictly positive. Yeah, me neither. :) There is an underlying limit for these mappings at 2 GiB, which is probably unnecessarily large for our purposes. So, |data| not NULL, |data_size| strictly positive and < 2 GiB.
+aelias: Please review the changes in content/browser/renderer_host
lgtm for content/browser/renderer_host
Please review Patch 13. I believe this addresses all of the comments, and makes sure the bots are green. (I don't think the linux-chromeos and win_rel failures are related to this CL.)
https://codereview.chromium.org/12457043/diff/117001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/12457043/diff/117001/content/common/view_mess... content/common/view_messages.h:2358: // This message runs the MediaCodec for decoding audio for webaudio. Guard this in an #ifdef for OS_ANDROID. https://codereview.chromium.org/12457043/diff/117001/media/base/android/webau... File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/117001/media/base/android/webau... media/base/android/webaudio_media_codec_bridge.cc:26: DVLOG(0) << "RunWebAudioMediaCodec"; NIT: I still think you've got some log spam in the CL. I don't see that you need to commit both of these DVLOG calls, for example. https://codereview.chromium.org/12457043/diff/117001/media/base/android/webau... media/base/android/webaudio_media_codec_bridge.cc:45: if (result) { NIT: These could be made smaller: if (close(pcm_output_)) DVLOG(0) << "Couldn't close output fd " << pcm_output_ << ": " << strerror(errno); https://codereview.chromium.org/12457043/diff/117001/media/base/android/webau... media/base/android/webaudio_media_codec_bridge.cc:68: DVLOG(0) << "decoded = " << static_cast<bool>(decoded); I think that if you need the static_cast on this line, you also need it on the next line. (I suspect you don't need it on either line.) https://codereview.chromium.org/12457043/diff/117001/media/base/android/webau... media/base/android/webaudio_media_codec_bridge.cc:111: ssize_t bytes_written = write(pcm_output_, buffer, bytes_to_write); It might also make sense to use HANDLE_EINTR here. https://codereview.chromium.org/12457043/diff/117001/webkit/media/android/aud... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/117001/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:28: bool IsValid() const { return read_fd_ >= 0 && write_fd_ >= 0; } Might also make sense to validate encoded_shared_memory_.memory(), too? https://codereview.chromium.org/12457043/diff/117001/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:54: if (encoded_shared_memory_.memory()) { NIT: You could save a level of indentation by doing: if (!encoded_shared_memory_.memory()) return; https://codereview.chromium.org/12457043/diff/117001/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:61: // Pipe was created successfully NIT: I think this comment is superfluous. NIT: You could save another level of indentation by doing: if (pipe(pipefd)) return; https://codereview.chromium.org/12457043/diff/117001/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:71: if (read_fd_ >= 0) { NIT: You could make this smaller. Since it is not fatal to get an error on close — all you do is log it harmlessly — you can replace this function body with: if (close(read_fd_)) DVLOG(0) << strerror(errno); Keep the comment though, of course. https://codereview.chromium.org/12457043/diff/117001/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:90: // shared memory and write the pcm samples back to us over a pipe. NIT: Personally, I would capitalize PCM throughout, but then I am known for my ocd. Err, OCD. https://codereview.chromium.org/12457043/diff/117001/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:132: if (nread != sizeof(info)) It "probably" would never happen, but |nread| might != sizeof(info) in a non-fatal, recoverable situation (EINTR, EAGAIN, EWOULDBLOCK). base/posix/eintr_wrapper.h has a macro you can use to handle EINTR; might be nice. https://codereview.chromium.org/12457043/diff/117001/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:150: while ((nread = read(input_fd, pipe_data, sizeof(pipe_data))) > 0) { HANDLE_EINTR might be called for here, too. https://codereview.chromium.org/12457043/diff/117001/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:171: destination_bus->channelData(k)[decoded_frames] = At this point in the code, I wonder if you might have performance problems. We've copied the sample data several times now: * getting them from the kernel by calling read * copying them from pipe_data into decoded_samples * copying them from decoded_samples into destination_bus->channelData This is not a security concern, and as long as it performs well enough there's no problem. But it might be possible to save a copy? Also, does destination_bus->channelData(k)[decoded_frames] inline perfectly, or would it help to hoist the .channelData accessor out of the loop? If I'm off-base, feel free to ignore me. :)
I'll address the rest of the comments with a new patch, applying the suggested changes. https://codereview.chromium.org/12457043/diff/117001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/12457043/diff/117001/content/common/view_mess... content/common/view_messages.h:2358: // This message runs the MediaCodec for decoding audio for webaudio. On 2013/04/11 20:29:51, Chris P. wrote: > Guard this in an #ifdef for OS_ANDROID. This is protected by line 2315. https://codereview.chromium.org/12457043/diff/117001/webkit/media/android/aud... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/117001/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:171: destination_bus->channelData(k)[decoded_frames] = On 2013/04/11 20:29:51, Chris P. wrote: > At this point in the code, I wonder if you might have performance problems. > We've copied the sample data several times now: > > * getting them from the kernel by calling read > * copying them from pipe_data into decoded_samples > * copying them from decoded_samples into destination_bus->channelData > > This is not a security concern, and as long as it performs well enough there's > no problem. But it might be possible to save a copy? > > Also, does > > destination_bus->channelData(k)[decoded_frames] > > inline perfectly, or would it help to hoist the .channelData accessor out of the > loop? > > If I'm off-base, feel free to ignore me. :) Performance is not critical at this point, but there is one scenario where this is slow loading up a bunch of very short audio samples for the HRTF panner. I have a bug filed on that to make it faster.
lgtm but please give qinmin a chance to look, he's the expert for media/android, (sorry I didn't notice you weren't there in the first place..) I have one question re: neon support below: https://codereview.chromium.org/12457043/diff/105002/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/12457043/diff/105002/media/media.gyp#newcode584 media/media.gyp:584: # WebAudio only if Neon is detected at runtime. hmm.. so for devices without Neon there will be no WebAudio at all?
https://codereview.chromium.org/12457043/diff/105002/content/common/webkitpla... File content/common/webkitplatformsupport_impl.h (right): https://codereview.chromium.org/12457043/diff/105002/content/common/webkitpla... content/common/webkitplatformsupport_impl.h:40: static void RunWebAudioMediaCodec( same here, you don't need a static member function https://codereview.chromium.org/12457043/diff/105002/content/public/renderer/... File content/public/renderer/render_view.h (right): https://codereview.chromium.org/12457043/diff/105002/content/public/renderer/... content/public/renderer/render_view.h:179: static void RunWebAudioMediaCodec( why renderview need to have this, and static? https://codereview.chromium.org/12457043/diff/105002/media/base/android/media... File media/base/android/media_jni_registrar.cc (right): https://codereview.chromium.org/12457043/diff/105002/media/base/android/media... media/base/android/media_jni_registrar.cc:27: VideoCaptureDeviceAndroid::RegisterVideoCaptureDevice }, fix the alignment https://codereview.chromium.org/12457043/diff/105002/webkit/glue/webkitplatfo... File webkit/glue/webkitplatformsupport_impl.h (right): https://codereview.chromium.org/12457043/diff/105002/webkit/glue/webkitplatfo... webkit/glue/webkitplatformsupport_impl.h:179: static void NullRunWebAudioMediaCodec( you don't need a static member function here. You can declare this function inside .cc file in an anonymous namespace
https://codereview.chromium.org/12457043/diff/105002/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/12457043/diff/105002/media/media.gyp#newcode584 media/media.gyp:584: # WebAudio only if Neon is detected at runtime. On 2013/04/12 07:48:15, bulach wrote: > hmm.. so for devices without Neon there will be no WebAudio at all? Yes, that's right. I'm not sure how well devices without Neon would perform with webaudio even if we had an FFT that runs without Neon. Probably not really usable.
Thanks for the review. The remaining comments are addressed as suggested in patch 15. https://codereview.chromium.org/12457043/diff/105002/content/public/renderer/... File content/public/renderer/render_view.h (right): https://codereview.chromium.org/12457043/diff/105002/content/public/renderer/... content/public/renderer/render_view.h:179: static void RunWebAudioMediaCodec( On 2013/04/12 14:43:08, qinmin wrote: > why renderview need to have this, and static? Not needed, thanks. I think it was a leftover from a previous implementation.
LGTM with nits: Lots of extra white spaces in the comments On 2013/04/12 17:06:32, rtoy wrote: > Thanks for the review. The remaining comments are addressed as suggested in > patch 15. > > https://codereview.chromium.org/12457043/diff/105002/content/public/renderer/... > File content/public/renderer/render_view.h (right): > > https://codereview.chromium.org/12457043/diff/105002/content/public/renderer/... > content/public/renderer/render_view.h:179: static void RunWebAudioMediaCodec( > On 2013/04/12 14:43:08, qinmin wrote: > > why renderview need to have this, and static? > > Not needed, thanks. I think it was a leftover from a previous implementation.
https://codereview.chromium.org/12457043/diff/140001/media/base/android/webau... File media/base/android/webaudio_media_codec_bridge.h (right): https://codereview.chromium.org/12457043/diff/140001/media/base/android/webau... media/base/android/webaudio_media_codec_bridge.h:34: // |encoded_audio_handle|. The PCM samples are sent to nit: extra white space in lots of the comments after "." https://codereview.chromium.org/12457043/diff/140001/media/base/android/webau... media/base/android/webaudio_media_codec_bridge.h:63: // this file descriptor. Owned. nit: what is Owned? extra white space before Owned. https://codereview.chromium.org/12457043/diff/140001/webkit/media/android/aud... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/140001/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:84: encoded_shared_memory_.memory() ; nit: extra white space
Almost there! https://codereview.chromium.org/12457043/diff/140001/media/base/android/webau... File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/140001/media/base/android/webau... media/base/android/webaudio_media_codec_bridge.cc:107: int bytes_to_write = (buf_size >= PIPE_BUF) ? PIPE_BUF : buf_size; I feel wary of using signed types for sizes and size-related arithmetic. I know it comes in as a jint, but I think the best thing to do is sanity-check, and then cast to the correct type: void WebAudioMediaCodecBridge::OnChunkDecoded( JNIEnv* env, jobject /*java object*/, jobject buf, jint buf_size) { if (buf_size <= 0 || !buf) return; int8_t* buffer = static_cast<int8_t*>(env->GetDirectBufferAddress(buf)); size_t count = static_cast<size_t>(buf_size); // Write out the data to the pipe in small chunks if necessary. while (buf_size > 0) { size_t bytes_to_write = std::min(count, PIPE_BUF); ssize_t bytes_written = HANDLE_EINTR(write(pcm_output_, buffer, bytes_to_write)); if (bytes_written == -1) break; count -= bytes_written; buffer += bytes_written; } } https://codereview.chromium.org/12457043/diff/140001/webkit/media/android/aud... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/140001/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:78: DVLOG(0) << strerror(errno); FYI: This won't have any context in the log; it'll just be some random error message. It will also hardly ever happen, so I am not sure how much it matters to you.
Thank you for all your hard work! LGTM.
https://codereview.chromium.org/12457043/diff/121002/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/12457043/diff/121002/media/audio/android/audi... media/audio/android/audio_manager_android.cc:88: static const int kDefaultSampleRate = 44100; So these are technically incorrect per a meeting with the Android folk yesterday. We need to be querying the device on both input and output for the recommended sample rate and buffer size. https://codereview.chromium.org/12457043/diff/121002/webkit/media/android/aud... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/121002/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:116: // TODO(rtoy): If we know the number of samples, we can create the Are you saying info[2] does not contain the number of samples? https://codereview.chromium.org/12457043/diff/121002/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:124: DVLOG(0) << "Reading audio file info from fd " << input_fd; DVLOG(0) should probably be DVLOG(1); here and elsewhere. https://codereview.chromium.org/12457043/diff/121002/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:172: decoded_samples[m + k] / 32768.0; This is not quite correct, the true range is: -32768, 32767. See AudioBus::FromInterleaved(). https://codereview.chromium.org/12457043/diff/140003/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/12457043/diff/140003/content/common/view_mess... content/common/view_messages.h:2359: IPC_MESSAGE_CONTROL2(ViewHostMsg_RunWebAudioMediaCodec, Seems sketchy that we're creating an IPC which provides shared memory to read from. I.e. we're exposing a "more privileged" binary format parser to the renderer. I'm not sure how else this would work though. I assume palmer has it under control :) https://codereview.chromium.org/12457043/diff/140003/media/base/android/java/... File media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java (right): https://codereview.chromium.org/12457043/diff/140003/media/base/android/java/... media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. Leaving review to android/ folks. https://codereview.chromium.org/12457043/diff/140003/webkit/media/android/aud... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/140003/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:44: int write_fd_; Needs DISALLOW_COPY_ASSIGN, etc. https://codereview.chromium.org/12457043/diff/140003/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:76: if (read_fd_ >= 0) { && close(read_fd_) ? https://codereview.chromium.org/12457043/diff/140003/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:86: encoded_shared_memory_.memory() ; spaces at end? https://codereview.chromium.org/12457043/diff/140003/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:158: std::vector<int16_t> decoded_samples; should probably pre allocate this to the expected size at least?
https://codereview.chromium.org/12457043/diff/121002/media/audio/android/audi... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/12457043/diff/121002/media/audio/android/audi... media/audio/android/audio_manager_android.cc:88: static const int kDefaultSampleRate = 44100; On 2013/04/12 21:47:45, DaleCurtis wrote: > So these are technically incorrect per a meeting with the Android folk > yesterday. We need to be querying the device on both input and output for the > recommended sample rate and buffer size. Yes. This is very high on my TODO list. https://codereview.chromium.org/12457043/diff/121002/webkit/media/android/aud... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/121002/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:116: // TODO(rtoy): If we know the number of samples, we can create the On 2013/04/12 21:47:45, DaleCurtis wrote: > Are you saying info[2] does not contain the number of samples? I do not know under what conditions this happens, but I have seen cases where MediaCodec will return a duration of 0 for some mp3s. And ogg/vorbis will return some huge (multi-year) duration. I have a fix for the ogg/vorbis issue and t hat CL will be coming soon. I need to isolate the mp3 issue and ask the Android people about this. https://codereview.chromium.org/12457043/diff/140003/webkit/media/android/aud... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/140003/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:158: std::vector<int16_t> decoded_samples; On 2013/04/12 21:47:45, DaleCurtis wrote: > should probably pre allocate this to the expected size at least? Can't do this right now because some audio files return a duration of 0 even though the file is not empty. MediaCodec will still continue to decode the entire file. High on my TODO list is to do the preallocation when the duration (number of samples) is not 0.
lgtm % tiny nit. https://codereview.chromium.org/12457043/diff/149003/webkit/media/android/aud... File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/149003/webkit/media/android/aud... webkit/media/android/audio_decoder_android.cc:87: encoded_shared_memory_.memory(); Err, I should have mentioned the indent is wrong too. It should be 4 spaces.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtoy@google.com/12457043/159001
Retried try job too often on win_rel for step(s) gpu_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtoy@google.com/12457043/159001
Message was sent while issue was closed.
Change committed as 194141 |
