Created attachment 148255 [details] Patch

Attachment 148255 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/At..." exit_code: 1 Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 148258 [details] Patch

@Peter: Do you know if we want Web Audio enabled for chromium-android?

(In reply to comment #4) > @Peter: Do you know if we want Web Audio enabled for chromium-android? strongly suggest to enable it for chromium-android. As we know, IOS6 safari already supports Web Audio API.

Enabling compilation of the Web Audio API will not per se make the feature work, as I suspect that we'll need to do work in the Chromium layer/Java as well. For one, this change would make the Chromium-Android build rely on FFMPEG, which we don't include in the build right now either. Furthermore, this will significantly influence binary size, which is another consideration which we'll have to make. I can investigate the impact of this patch later this week, but please hold off for now.

(In reply to comment #6) > Enabling compilation of the Web Audio API will not per se make the feature work, as I suspect that we'll need to do work in the Chromium layer/Java as well. For one, this change would make the Chromium-Android build rely on FFMPEG, which we don't include in the build right now either. Furthermore, this will significantly influence binary size, which is another consideration which we'll have to make. > > I can investigate the impact of this patch later this week, but please hold off for now. thanks for the clarification and make me understand the consideration. I am glad to contribute if at last it is decided to enable it. thanks.

we should not use ffmpeg, and we should hook web audio either with openmax SL or android mediaplayer

(In reply to comment #8) > we should not use ffmpeg, and we should hook web audio either with openmax SL or android mediaplayer here web audio doesn't use ffmpeg decode/encode and playback related code, it only use the FFT routine from ffmpeg. be specific, it used av_rdft_calc/av_rdft_init/av_rdft_end/av_rdft_end to process the audio data. it has nothing to do with audio playback.is it ok?

(In reply to comment #9) > (In reply to comment #8) > > we should not use ffmpeg, and we should hook web audio either with openmax SL or android mediaplayer > > here web audio doesn't use ffmpeg decode/encode and playback related code, it only use the FFT routine from ffmpeg. be specific, it used av_rdft_calc/av_rdft_init/av_rdft_end/av_rdft_end to process the audio data. > > it has nothing to do with audio playback.is it ok? Then we shouldn't need to include ffmpeg, just include some fft transformation files

(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > we should not use ffmpeg, and we should hook web audio either with openmax SL or android mediaplayer > > > > here web audio doesn't use ffmpeg decode/encode and playback related code, it only use the FFT routine from ffmpeg. be specific, it used av_rdft_calc/av_rdft_init/av_rdft_end/av_rdft_end to process the audio data. > > > > it has nothing to do with audio playback.is it ok? > > Then we shouldn't need to include ffmpeg, just include some fft transformation files qinmin, after some investigation, I found that the fft function will need some base functions in ffmpeg like av_malloc etc, so it will be hard to seperate fft function from the ffmpeg project. third_party/ffmpeg has two targets, one is ffmpegsumo, which will produce a .so library(most decode/encode function here), and another one is ffmpeg, which will produce a static library(stub and basic functions). As we only depends on the ffmpeg target and it is a static library. I believe it is harmless to use it. Is there any consideration that we cannot use it?

By how much does the target size increase in release build with this feature enabled and linked with the ffmpeg static lib? Knowing that would help answer whether it is suitable or not.

(In reply to comment #12) > By how much does the target size increase in release build with this feature enabled and linked with the ffmpeg static lib? Knowing that would help answer whether it is suitable or not. thanks for the comment, I will build chromium for android w/o web audio and w/ it with ffmpeg and post the data. thanks

(In reply to comment #12) > By how much does the target size increase in release build with this feature enabled and linked with the ffmpeg static lib? Knowing that would help answer whether it is suitable or not. As currently we cannot build a chromium browser for android, so I just checked the size of DumpRenderTree native library. for x86 platform, adding webaudio with ffmpeg only increase the binary size by 259596 bytes. src/out/Release/DumpRenderTree_apk/libs/x86$ ll 35009324 Jul 4 17:12 libDumpRenderTree_no_webaudio.so 35268920 Jul 4 17:42 libDumpRenderTree.so and the size of libffmpeg.a is 2.7K. src/out/Release$ ls -al ./obj.target/third_party/ffmpeg/libffmpeg.a -h 2.7K Jul 4 17:18 ./obj.target/third_party/ffmpeg/libffmpeg.a

for arm platform, the data is : /src/out/Release/DumpRenderTree_apk/libs/armeabi-v7a$ ll 22038100 Jul 4 20:18 libDumpRenderTree-no-webaudio.so 22157828 Jul 4 20:47 libDumpRenderTree.so

Comment on attachment 148258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148258&action=review I think it's fine to start this work upstream, we can always disable it for releases if the size increase proves to be too significant. Sorry for the delay, and thank you for the work! > Source/WTF/wtf/Atomics.h:109 > +#define WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED 1 This should be defined in Platform.h. Certain versions of the Android NDK had issues with thread-safe reference counting, which is why this wasn't enabled yet, but I *think* it's fine to be enabled now. I'll verify this downstream. > Source/WebCore/WebCore.gyp/WebCore.gyp:1423 > + ['"WTF_USE_WEBAUDIO_FFMPEG=1" in feature_defines', { As said, none of FFMPEG is available in Android builds right now. We will need to find a way around this..

Created attachment 153169 [details] Patch

Comment on attachment 148258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148258&action=review >> Source/WTF/wtf/Atomics.h:109 >> +#define WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED 1 > > This should be defined in Platform.h. Certain versions of the Android NDK had issues with thread-safe reference counting, which is why this wasn't enabled yet, but I *think* it's fine to be enabled now. I'll verify this downstream. for other platforms this MACRO is also defined in this file. Should I keep it here for consistency or move all the definition to Platform.h? thanks >> Source/WebCore/WebCore.gyp/WebCore.gyp:1423 >> + ['"WTF_USE_WEBAUDIO_FFMPEG=1" in feature_defines', { > > As said, none of FFMPEG is available in Android builds right now. We will need to find a way around this.. I used the FFTFrame Stub to bring up web audio. and will implement another FFTFrame for android like FFTFrameAndroid or FFTFrameOpenMax later. is this ok? thanks To get rid of ffmpeg on android, I change the features.gypi to undefine WTF_USE_WEBAUDIO_FFMPEG.

Comment on attachment 153169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153169&action=review The patch compiles fine upstream, and in our branch as well after adding the ffmpeg library to third_party/. There are a bunch of failing ASSERTs and more work is necessary before the feature can be enabled by default, but I agree that fixing the compile is a good first step. Raymond has expressed interest in helping out as well :-). Using the following small script: -- var context = new webkitAudioContext(), oscillator = context.createOscillator; // Oscillator defaults to sine wave document.write(context.sampleRate); oscillator.connect(context.destination); oscillator.noteOn(0); -- The output is 16000 (the sample rate), but it crashes fairly soon after with a segfault when it tried to actually produce audio. Relevant logcat output is: W/WebKit ( 687): SHOULD NEVER BE REACHED W/WebKit ( 687): third_party/WebKit/Source/WebCore/platform/audio/FFTFrameStub.cpp(43) : WebCore::FFTFrame::FFTFrame(unsigned int) F/libc ( 687): Fatal signal 11 (SIGSEGV) at 0xbbadbeef (code=1), thread 703 (SandboxedProces) F/chromium( 590): [FATAL:render_process_host_impl.cc(1004)] Check failed: false. Invalid message with type = 2228317 F/libc ( 590): Fatal signal 11 (SIGSEGV) at 0xdeadbaad (code=1), thread 590 (oid.apps.chrome) E/chromium( 687): [ERROR:audio_decoder_android.cc(13)] Not implemented reached in bool webkit_media::DecodeAudioFileData(WebKit::WebAudioBus*, char const*, size_t, double) W/WebKit ( 687): ASSERTION FAILED: impulseResponse.get() W/WebKit ( 687): third_party/WebKit/Source/WebCore/platform/audio/HRTFElevation.cpp(180) : static bool WebCore::HRTFElevation::calculateKernelsForAzimuthElevation(int, int, float, const WTF::String&, WTF::RefPtr<WebCore::HRTFKernel>&, WTF::RefPtr<WebCore::HRTFKernel>&) There is more dependent code which has yet to be upstreamed, so it's still unable to work correctly :(. To conclude: with the following two changes applied, I'm fine with this landing in WebKit from Android's side. You'll still need a reviewer's look for the Web Audio side, however. > Source/WTF/wtf/Atomics.h:109 > +#define WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED 1 Could you please split this up to a new bug? I'd like David Turner (digit@google.com) to look at this one. > Source/WebKit/chromium/features.gypi:161 > + 'ENABLE_WEB_AUDIO=1', Since this fixes the compile but doesn't make it work yet, please keep it disabled by default for now.

Comment on attachment 153169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153169&action=review peter, thanks for reviewing and evaluating it. this patch uses an empty FFTFrame Stub instead of ffmpeg, so don t need ffmpeg, you should be able to compile it without ffmpeg in the third_party. So should I use ffmpeg in it to make it work firstly? if so, I can submit another patch to make it be able to compile and then we can work raymond and other people to fix all the issues on android. >> Source/WTF/wtf/Atomics.h:109 >> +#define WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED 1 > > Could you please split this up to a new bug? I'd like David Turner (digit@google.com) to look at this one. yes, I will do it. thanks >> Source/WebKit/chromium/features.gypi:161 >> + 'ENABLE_WEB_AUDIO=1', > > Since this fixes the compile but doesn't make it work yet, please keep it disabled by default for now. set ENABLE_WEB_AUDIO=0 will exclude all source code in building. So in order to compile it, we have to set it to 1.

peter, if this feature also cannot work downstream, I am also very interested to make it work upstream firstly. chris, could you have a look at this patch? thanks

(In reply to comment #20) > (From update of attachment 153169 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153169&action=review > > peter, thanks for reviewing and evaluating it. > > this patch uses an empty FFTFrame Stub instead of ffmpeg, so don > t need ffmpeg, you should be able to compile it without ffmpeg in the third_party. > > So should I use ffmpeg in it to make it work firstly? if so, I can submit another patch to make it be able to compile and then we can work raymond and other people to fix all the issues on android. I tried both.. Should have clarified, sorry. The patch as-is is fine, with the two comments applied. > > >> Source/WTF/wtf/Atomics.h:109 > >> +#define WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED 1 > > > > Could you please split this up to a new bug? I'd like David Turner (digit@google.com) to look at this one. > > yes, I will do it. thanks > > >> Source/WebKit/chromium/features.gypi:161 > >> + 'ENABLE_WEB_AUDIO=1', > > > > Since this fixes the compile but doesn't make it work yet, please keep it disabled by default for now. > > set ENABLE_WEB_AUDIO=0 will exclude all source code in building. So in order to compile it, we have to set it to 1. Yes, I intentionally asked you to keep it at 0 for now. When set to 1, your patch will have the side-effect of exposing the API to web developers, which breaks feature detection. Since it's not yet functional, we shouldn't do this. As an alternative, we could enable the compile but disable the feature at run-time. Since it's enabled by default for Chromium, this would require a Chromium patch changing the default for Android, too.

Created attachment 153249 [details] Patch

(In reply to comment #22) > I tried both.. Should have clarified, sorry. The patch as-is is fine, with the two comments applied. > > > > > Yes, I intentionally asked you to keep it at 0 for now. When set to 1, your patch will have the side-effect of exposing the API to web developers, which breaks feature detection. Since it's not yet functional, we shouldn't do this. > > As an alternative, we could enable the compile but disable the feature at run-time. Since it's enabled by default for Chromium, this would require a Chromium patch changing the default for Android, too. https://bugs.webkit.org/show_bug.cgi?id=91740 filed WTF_USE_LOCKFREE_THREADSAFEREFCOUNTED should be defined in Platform.h instead of wtf/Atomics.h and disabe web audio by default. thanks

Created attachment 153259 [details] Patch

+kbr

Created attachment 153269 [details] Patch

(In reply to comment #19) > (From update of attachment 153169 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153169&action=review > > The patch compiles fine upstream, and in our branch as well after adding the ffmpeg library to third_party/. There are a bunch of failing ASSERTs and more work is necessary before the feature can be enabled by default, but I agree that fixing the compile is a good first step. Raymond has expressed interest in helping out as well :-). Yes I am! > > > Using the following small script: > -- > var context = new webkitAudioContext(), > oscillator = context.createOscillator; // Oscillator defaults to sine wave > > document.write(context.sampleRate); > > oscillator.connect(context.destination); > oscillator.noteOn(0); The oscillator needs a working FFT. > W/WebKit ( 687): third_party/WebKit/Source/WebCore/platform/audio/HRTFElevation.cpp(180) : static bool WebCore::HRTFElevation::calculateKernelsForAzimuthElevation(int, int, float, const WTF::String&, WTF::RefPtr<WebCore::HRTFKernel>&, WTF::RefPtr<WebCore::HRTFKernel>&) The HRTF panner has some ASSERTs and dependencies that the sample rate is near 44.1kHz. It might be useful to disable the loading of the HRTF stuff for now.

(In reply to comment #20) > (From update of attachment 153169 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153169&action=review > > peter, thanks for reviewing and evaluating it. > > this patch uses an empty FFTFrame Stub instead of ffmpeg, so don > t need ffmpeg, you should be able to compile it without ffmpeg in the third_party. > > So should I use ffmpeg in it to make it work firstly? if so, I can submit another patch to make it be able to compile and then we can work raymond and other people to fix all the issues on android. I would prefer to use ffmpeg for now. I believe Chris suggested that to start with. But perhaps as a first cut, we can just use your empty FFTFrame stub, but that also means lots of functionality and demos no longer work because many use the convolver or dynamics compressor or other things that need the FFT.

Comment on attachment 153269 [details] Patch Patch looks fine as long as this will not suddenly cause the sources to be compiled in by default. r=me

Comment on attachment 153269 [details] Patch Clearing flags on attachment: 153269 Committed r123175: <http://trac.webkit.org/changeset/123175>

All reviewed patches have been landed. Closing bug.

Comment on attachment 148258 [details] Patch Cleared review? from obsolete attachment 148258 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

(In reply to comment #33) > (From update of attachment 148258 [details]) > Cleared review? from obsolete attachment 148258 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again). this patch should be obsoleted and no need for review. don't know why it is not obsoleted. the latest patch already landed webkit. thanks