Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(474)

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
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.

Description

Android 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 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+724 lines, -11 lines) Patch
M chrome/app/generated_resources.grd View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +9 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_message_filter.h View 1 2 3 4 5 6 7 8 2 chunks +9 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_message_filter.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +20 lines, -0 lines 0 comments Download
M content/common/view_messages.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +5 lines, -0 lines 0 comments Download
M content/common/webkitplatformsupport_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +4 lines, -0 lines 0 comments Download
M content/common/webkitplatformsupport_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +24 lines, -0 lines 0 comments Download
M media/audio/android/audio_manager_android.cc View 1 2 3 1 chunk +3 lines, -3 lines 0 comments Download
A media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java View 1 2 3 4 5 1 chunk +166 lines, -0 lines 0 comments Download
M media/base/android/media_jni_registrar.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +3 lines, -0 lines 0 comments Download
A media/base/android/webaudio_media_codec_bridge.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +69 lines, -0 lines 0 comments Download
A media/base/android/webaudio_media_codec_bridge.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +128 lines, -0 lines 0 comments Download
A media/base/media_android.cc View 1 chunk +31 lines, -0 lines 0 comments Download
M media/media.gyp View 1 2 3 4 5 6 7 8 9 10 5 chunks +19 lines, -5 lines 0 comments Download
M webkit/glue/webkitplatformsupport_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +5 lines, -0 lines 0 comments Download
M webkit/glue/webkitplatformsupport_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +23 lines, -0 lines 0 comments Download
M webkit/media/android/audio_decoder_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +183 lines, -3 lines 0 comments Download
M webkit/media/audio_decoder.h View 1 2 3 4 5 6 7 8 1 chunk +17 lines, -0 lines 0 comments Download

Messages

Total messages: 52 (0 generated)
Raymond Toy (Google)
Please review.
7 years, 9 months ago (2013-03-27 23:24:16 UTC) #1
bulach
thanks raymond! I'm not all familiar here, so just some drive-by comments and suggestions. I'll ...
7 years, 9 months ago (2013-03-28 13:39:25 UTC) #2
Raymond Toy (Google)
On 2013/03/28 13:39:25, bulach wrote: > thanks raymond! I'm not all familiar here, so just ...
7 years, 9 months ago (2013-03-28 16:09:15 UTC) #3
Raymond Toy (Google)
Just a few comments. Your other comments will be addressed in an upcoming patch. https://codereview.chromium.org/12457043/diff/23001/media/base/android/webaudio_media_codec_bridge.cc ...
7 years, 9 months ago (2013-03-28 17:38:31 UTC) #4
bulach
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 webkit/media/android/audio_decoder_android.cc:29: int getWriteFd() { return pipefd_[1]; }; On 2013/03/28 17:38:31, ...
7 years, 9 months ago (2013-03-28 17:42:31 UTC) #5
Raymond Toy (Google)
On 2013/03/28 17:42:31, bulach wrote: > 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 > ...
7 years, 9 months ago (2013-03-28 17:47:26 UTC) #6
bulach
great, thanks!! :) On Thu, Mar 28, 2013 at 5:47 PM, <rtoy@google.com> wrote: > On ...
7 years, 9 months ago (2013-03-28 17:48:47 UTC) #7
felipeg
LGTM only minor comments (blank lines, etc..) https://codereview.chromium.org/12457043/diff/34001/media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java File media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java (right): https://codereview.chromium.org/12457043/diff/34001/media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java#newcode33 media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:33: You can ...
7 years, 8 months ago (2013-03-29 17:18:29 UTC) #8
Raymond Toy (Google)
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 File media/base/android/webaudio_media_codec_bridge.cc ...
7 years, 8 months ago (2013-03-29 19:47:23 UTC) #9
felipeg_google
Cool, thanks Ray On Fri, Mar 29, 2013 at 8:47 PM, <rtoy@google.com> wrote: > Thanks ...
7 years, 8 months ago (2013-03-29 19:48:19 UTC) #10
Raymond Toy (Google)
Adding OWNERS to review list. Please review at your convenience.
7 years, 8 months ago (2013-03-29 22:26:13 UTC) #11
jam
On 2013/03/29 22:26:13, rtoy wrote: > Adding OWNERS to review list. > > Please review ...
7 years, 8 months ago (2013-03-29 23:25:46 UTC) #12
bulach
lgtm, thanks! just nits for android.. feel free to go ahead once the respective owners ...
7 years, 8 months ago (2013-04-02 11:05:11 UTC) #13
no longer working on chromium
Hi Raymond, I defer my part to Dale and remove myself from the reviewer list. ...
7 years, 8 months ago (2013-04-02 12:02:58 UTC) #14
cpu_(ooo_6.6-7.5)
note that grd files afaik don't have owners.
7 years, 8 months ago (2013-04-02 17:28:08 UTC) #15
palmer
https://codereview.chromium.org/12457043/diff/45001/content/browser/renderer_host/render_message_filter.cc File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/12457043/diff/45001/content/browser/renderer_host/render_message_filter.cc#newcode1155 content/browser/renderer_host/render_message_filter.cc:1155: base::SharedMemoryHandle input_fd, These variable names seem swapped, based on ...
7 years, 8 months ago (2013-04-02 18:01:07 UTC) #16
Raymond Toy (Google)
On 2013/03/29 23:25:46, jam wrote: > On 2013/03/29 22:26:13, rtoy wrote: > > Adding OWNERS ...
7 years, 8 months ago (2013-04-02 21:50:31 UTC) #17
Raymond Toy (Google)
https://codereview.chromium.org/12457043/diff/45001/media/audio/android/audio_manager_android.cc File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/12457043/diff/45001/media/audio/android/audio_manager_android.cc#newcode86 media/audio/android/audio_manager_android.cc:86: // TODO(xians): figure out the right output sample rate ...
7 years, 8 months ago (2013-04-02 21:52:21 UTC) #18
Raymond Toy (Google)
https://codereview.chromium.org/12457043/diff/45001/media/base/android/media_jni_registrar.cc File media/base/android/media_jni_registrar.cc (left): https://codereview.chromium.org/12457043/diff/45001/media/base/android/media_jni_registrar.cc#oldcode26 media/base/android/media_jni_registrar.cc:26: VideoCaptureDeviceAndroid::RegisterVideoCaptureDevice }, On 2013/04/02 21:52:21, rtoy wrote: > On ...
7 years, 8 months ago (2013-04-02 22:07:39 UTC) #19
Raymond Toy (Google)
Please review patch set 5, which fixes an issue that prevented DumpRenderTree from compiling. Only ...
7 years, 8 months ago (2013-04-03 21:40:44 UTC) #20
digit1
https://codereview.chromium.org/12457043/diff/64001/media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java File media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java (right): https://codereview.chromium.org/12457043/diff/64001/media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java#newcode37 media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java:37: e.printStackTrace(); I assume you would want to call encodedFD.detachFd() ...
7 years, 8 months ago (2013-04-04 15:56:46 UTC) #21
Raymond Toy (Google)
https://codereview.chromium.org/12457043/diff/64001/media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java File media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java (right): https://codereview.chromium.org/12457043/diff/64001/media/base/android/java/src/org/chromium/media/WebAudioMediaCodecBridge.java#newcode57 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: ...
7 years, 8 months ago (2013-04-04 17:47:35 UTC) #22
acolwell GONE FROM CHROMIUM
https://codereview.chromium.org/12457043/diff/72001/media/audio/android/audio_manager_android.cc File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/12457043/diff/72001/media/audio/android/audio_manager_android.cc#newcode88 media/audio/android/audio_manager_android.cc:88: static const int kDefaultSampleRate = 44100; Is the TODO ...
7 years, 8 months ago (2013-04-04 20:53:52 UTC) #23
Raymond Toy (Google)
https://codereview.chromium.org/12457043/diff/72001/media/audio/android/audio_manager_android.cc File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/12457043/diff/72001/media/audio/android/audio_manager_android.cc#newcode88 media/audio/android/audio_manager_android.cc:88: static const int kDefaultSampleRate = 44100; On 2013/04/04 20:53:52, ...
7 years, 8 months ago (2013-04-04 22:38:05 UTC) #24
brettw
lgtm https://codereview.chromium.org/12457043/diff/76002/webkit/glue/webkitplatformsupport_impl.cc File webkit/glue/webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/12457043/diff/76002/webkit/glue/webkitplatformsupport_impl.cc#newcode983 webkit/glue/webkitplatformsupport_impl.cc:983: } Nit: leave blank line before namespace end.
7 years, 8 months ago (2013-04-08 04:14:35 UTC) #25
Raymond Toy (Google)
I think Patch 8 has addressed all comments thus far. +tommi@: Can you review the ...
7 years, 8 months ago (2013-04-09 23:55:32 UTC) #26
tommi (sloooow) - chröme
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_decoder.h#newcode9 webkit/media/audio_decoder.h:9: #include "base/callback_forward.h" should we have #if ...
7 years, 8 months ago (2013-04-10 00:01:04 UTC) #27
palmer
https://codereview.chromium.org/12457043/diff/76002/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/12457043/diff/76002/chrome/app/generated_resources.grd#newcode6630 chrome/app/generated_resources.grd:6630: + Enabling this option allows web applications to access ...
7 years, 8 months ago (2013-04-10 00:02:56 UTC) #28
palmer
https://codereview.chromium.org/12457043/diff/86001/content/browser/renderer_host/render_message_filter.h File content/browser/renderer_host/render_message_filter.h (right): https://codereview.chromium.org/12457043/diff/86001/content/browser/renderer_host/render_message_filter.h#newcode264 content/browser/renderer_host/render_message_filter.h:264: void OnWebAudioMediaCodec(base::SharedMemoryHandle memory, Looks like this is the last ...
7 years, 8 months ago (2013-04-10 00:57:53 UTC) #29
Raymond Toy (Google)
https://codereview.chromium.org/12457043/diff/86001/media/base/android/webaudio_media_codec_bridge.cc File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/86001/media/base/android/webaudio_media_codec_bridge.cc#newcode91 media/base/android/webaudio_media_codec_bridge.cc:91: << " microsec vorbis= " On 2013/04/10 00:57:53, Chris ...
7 years, 8 months ago (2013-04-10 19:23:36 UTC) #30
Raymond Toy (Google)
Please review patch 9.
7 years, 8 months ago (2013-04-10 19:25:17 UTC) #31
palmer
https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audio_decoder_android.cc File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/86001/webkit/media/android/audio_decoder_android.cc#newcode45 webkit/media/android/audio_decoder_android.cc:45: > Yes, checking |data| is a good idea. I ...
7 years, 8 months ago (2013-04-10 21:32:26 UTC) #32
Raymond Toy (Google)
+aelias: Please review the changes in content/browser/renderer_host
7 years, 8 months ago (2013-04-11 00:26:31 UTC) #33
aelias_OOO_until_Jul13
lgtm for content/browser/renderer_host
7 years, 8 months ago (2013-04-11 00:28:41 UTC) #34
Raymond Toy (Google)
Please review Patch 13. I believe this addresses all of the comments, and makes sure ...
7 years, 8 months ago (2013-04-11 18:31:36 UTC) #35
palmer
https://codereview.chromium.org/12457043/diff/117001/content/common/view_messages.h File content/common/view_messages.h (right): https://codereview.chromium.org/12457043/diff/117001/content/common/view_messages.h#newcode2358 content/common/view_messages.h:2358: // This message runs the MediaCodec for decoding audio ...
7 years, 8 months ago (2013-04-11 20:29:50 UTC) #36
Raymond Toy (Google)
I'll address the rest of the comments with a new patch, applying the suggested changes. ...
7 years, 8 months ago (2013-04-11 22:06:43 UTC) #37
bulach
lgtm but please give qinmin a chance to look, he's the expert for media/android, (sorry ...
7 years, 8 months ago (2013-04-12 07:48:15 UTC) #38
qinmin
https://codereview.chromium.org/12457043/diff/105002/content/common/webkitplatformsupport_impl.h File content/common/webkitplatformsupport_impl.h (right): https://codereview.chromium.org/12457043/diff/105002/content/common/webkitplatformsupport_impl.h#newcode40 content/common/webkitplatformsupport_impl.h:40: static void RunWebAudioMediaCodec( same here, you don't need a ...
7 years, 8 months ago (2013-04-12 14:43:08 UTC) #39
Raymond Toy (Google)
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. ...
7 years, 8 months ago (2013-04-12 15:59:04 UTC) #40
Raymond Toy (Google)
Thanks for the review. The remaining comments are addressed as suggested in patch 15. https://codereview.chromium.org/12457043/diff/105002/content/public/renderer/render_view.h ...
7 years, 8 months ago (2013-04-12 17:06:32 UTC) #41
qinmin
LGTM with nits: Lots of extra white spaces in the comments On 2013/04/12 17:06:32, rtoy ...
7 years, 8 months ago (2013-04-12 17:15:08 UTC) #42
qinmin
https://codereview.chromium.org/12457043/diff/140001/media/base/android/webaudio_media_codec_bridge.h File media/base/android/webaudio_media_codec_bridge.h (right): https://codereview.chromium.org/12457043/diff/140001/media/base/android/webaudio_media_codec_bridge.h#newcode34 media/base/android/webaudio_media_codec_bridge.h:34: // |encoded_audio_handle|. The PCM samples are sent to nit: ...
7 years, 8 months ago (2013-04-12 17:15:22 UTC) #43
palmer
Almost there! https://codereview.chromium.org/12457043/diff/140001/media/base/android/webaudio_media_codec_bridge.cc File media/base/android/webaudio_media_codec_bridge.cc (right): https://codereview.chromium.org/12457043/diff/140001/media/base/android/webaudio_media_codec_bridge.cc#newcode107 media/base/android/webaudio_media_codec_bridge.cc:107: int bytes_to_write = (buf_size >= PIPE_BUF) ? ...
7 years, 8 months ago (2013-04-12 18:34:49 UTC) #44
palmer
Thank you for all your hard work! LGTM.
7 years, 8 months ago (2013-04-12 20:38:26 UTC) #45
DaleCurtis
https://codereview.chromium.org/12457043/diff/121002/media/audio/android/audio_manager_android.cc File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/12457043/diff/121002/media/audio/android/audio_manager_android.cc#newcode88 media/audio/android/audio_manager_android.cc:88: static const int kDefaultSampleRate = 44100; So these are ...
7 years, 8 months ago (2013-04-12 21:47:44 UTC) #46
Raymond Toy (Google)
https://codereview.chromium.org/12457043/diff/121002/media/audio/android/audio_manager_android.cc File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/12457043/diff/121002/media/audio/android/audio_manager_android.cc#newcode88 media/audio/android/audio_manager_android.cc:88: static const int kDefaultSampleRate = 44100; On 2013/04/12 21:47:45, ...
7 years, 8 months ago (2013-04-12 22:12:46 UTC) #47
DaleCurtis
lgtm % tiny nit. https://codereview.chromium.org/12457043/diff/149003/webkit/media/android/audio_decoder_android.cc File webkit/media/android/audio_decoder_android.cc (right): https://codereview.chromium.org/12457043/diff/149003/webkit/media/android/audio_decoder_android.cc#newcode87 webkit/media/android/audio_decoder_android.cc:87: encoded_shared_memory_.memory(); Err, I should have ...
7 years, 8 months ago (2013-04-12 23:12:03 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtoy@google.com/12457043/159001
7 years, 8 months ago (2013-04-12 23:25:59 UTC) #49
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) gpu_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=134606
7 years, 8 months ago (2013-04-13 02:03:56 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtoy@google.com/12457043/159001
7 years, 8 months ago (2013-04-15 05:19:50 UTC) #51
commit-bot: I haz the power
7 years, 8 months ago (2013-04-15 06:06:32 UTC) #52
Message was sent while issue was closed.
Change committed as 194141

Powered by Google App Engine
This is Rietveld 408576698