Closed Bug 1391482 Opened 7 years ago Closed 7 years ago

delay conversion of output of decodeAudioData() from int16_t to float until necessary

Categories

(Core :: Web Audio, enhancement, P1)

Product:
Core
Component:
Web Audio
Platform:
ARM
Android
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

Details

Attachments

(15 files)

bug 1391482 use AudioChunk to generalize AudioBuffer shared channel storage
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 add a mechanism to pass an AudioChunk from node to engine
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 generalize shared channel data from AudioBuffer as AudioChunk
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 add a fallible SharedBuffer::Create()
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 keep custom oscillator data buffer on graph thread only long enough to initialize the PeriodicWave
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 use AudioChunk to store and pass PeriodicWave data to engine
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 remove now-unused ThreadSharedFloatArrayBufferList SetBuffer() variant
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 accept int16_t sample buffers in AudioBufferSourceNode
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 accept int16_t sample buffers in ConvolverNode
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 move AudioBuffer parameter checking to constructor
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 add a method to create an AudioBuffer from AudioChunk data
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 accept int16_t-sample initialization of AudioBuffer
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 permit AudioChunk channel data initialization when mBuffer is not shared
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 generalize WebAudioDecodeJob buffer as AudioChunk
59 bytes, text/x-review-board-request
padenot
: review+
Details
bug 1391482 delay conversion of output of decodeAudioData() from int16_t to float until necessary
59 bytes, text/x-review-board-request
padenot
: review+
Details
Some web audio game client authors have observed significantly increased memory requirements of AudioBuffers storing Float32 data compared with native games typically using signed 16-bit. This issue is particularly problematic on mobile devices, where typically less memory is available. Initially raised at https://lists.w3.org/Archives/Public/public-audio/2013OctDec/0294.html Gecko defines MOZ_SAMPLE_TYPE_S16 on mobile platforms, in which case decode and resampling for decodeAudioData() is already performed in 16-bit arithmetic. Instead of converting to float for storage, this conversion can be performed on demand. In AudioBufferSourceNode usage (the typical case), only one block need be converted at a time. Any buffer generated by decodeAudioData() will be stored in 16-bit format until and unless JS requests it in Float32 ArrayBuffer format via getChannelData(). Other platforms decode and resample in float format. I do not intend to convert back to signed 16-bit for those platforms.
Most of the changes here are about passing a more generic type than ThreadSharedFloatArrayBufferList, one that can contain either float or 16-bit data. AudioChunk already exists for that and has the nice advantage that it keeps the length together in the same struct.
This approach was suggested in https://github.com/WebAudio/web-audio-api/issues/283#issuecomment-108012641 and previously.
Rank: 15
Priority: -- → P1
Comment on attachment 8898567 [details] bug 1391482 use AudioChunk to generalize AudioBuffer shared channel storage https://reviewboard.mozilla.org/r/169932/#review178034
Attachment #8898567 - Flags: review?(padenot) → review+
Comment on attachment 8898568 [details] bug 1391482 add a mechanism to pass an AudioChunk from node to engine https://reviewboard.mozilla.org/r/169934/#review178036
Attachment #8898568 - Flags: review?(padenot) → review+
Comment on attachment 8898569 [details] bug 1391482 generalize shared channel data from AudioBuffer as AudioChunk https://reviewboard.mozilla.org/r/169936/#review178038
Attachment #8898569 - Flags: review?(padenot) → review+
Comment on attachment 8898570 [details] bug 1391482 add a fallible SharedBuffer::Create() https://reviewboard.mozilla.org/r/169938/#review178010 ::: dom/media/SharedBuffer.h:67 (Diff revision 1) > + return InternalCreate(&malloc, aSize); > + } > + > static already_AddRefed<SharedBuffer> Create(size_t aSize) > { > + // Use moz_x_malloc() to include its diagnostic message indicating the s/moz_x_malloc/moz_xmalloc/ ::: dom/media/SharedBuffer.h:86 (Diff revision 1) > CheckedInt<size_t> size = sizeof(SharedBuffer); > size += aSize; > if (!size.isValid()) { > MOZ_CRASH(); > } > - void* m = moz_xmalloc(size.value()); > + void* m = (*aMalloc)(size.value()); I thought we had a style guide entry stating that `new` should be preferred in new code. I can't find it though. Here it does not matter too much because we're allocating bytes and not elements of a bigger size, so we can't forget the multiplication.
Attachment #8898570 - Flags: review?(padenot) → review+
Comment on attachment 8898571 [details] bug 1391482 keep custom oscillator data buffer on graph thread only long enough to initialize the PeriodicWave https://reviewboard.mozilla.org/r/169940/#review178040
Attachment #8898571 - Flags: review?(padenot) → review+
Comment on attachment 8898572 [details] bug 1391482 use AudioChunk to store and pass PeriodicWave data to engine https://reviewboard.mozilla.org/r/169942/#review178022 ::: commit-message-9d9c4:1 (Diff revision 1) > +bug 1391482 use AudioChunk to store and pass PeriodicWave data to engine r?padenot A bit weird to have an `AudioChunk` containing Fourier coefficients, but it makes the code cleaner indeed.
Attachment #8898572 - Flags: review?(padenot) → review+
Comment on attachment 8898573 [details] bug 1391482 remove now-unused ThreadSharedFloatArrayBufferList SetBuffer() variant https://reviewboard.mozilla.org/r/169944/#review178042
Attachment #8898573 - Flags: review?(padenot) → review+
Comment on attachment 8898574 [details] bug 1391482 accept int16_t sample buffers in AudioBufferSourceNode https://reviewboard.mozilla.org/r/169946/#review178030 ::: dom/media/webaudio/AudioBufferSourceNode.cpp:308 (Diff revision 1) > aOutput->ChannelFloatsForWrite(i) + *aOffsetWithinBlock; > > + if (mBuffer.mBufferFormat == AUDIO_FORMAT_FLOAT32) { > + const float* inputData = > + mBuffer.ChannelData<float>()[i] + mBufferPosition; > - WebAudioUtils::SpeexResamplerProcess(resampler, i, > + WebAudioUtils::SpeexResamplerProcess(resampler, i, This will allocate if the resampling rate is less than 0.25 and the platform uses int16 for audio output, and the buffer is originally in float32. One one hand, this whole patch set would be amazing on desktop (and we don't have to change the spec to implement it), and this would mean we allocate a bit more in some case. On the other hand, solving the memory problem can also be done with explicit in-memory compression, and would be better. This is likely to require a spec change. The allocation can also be removed by compiling two copies of the resampler, one for int, one for float.
Attachment #8898574 - Flags: review?(padenot) → review+
Comment on attachment 8898575 [details] bug 1391482 accept int16_t sample buffers in ConvolverNode https://reviewboard.mozilla.org/r/169948/#review178044
Attachment #8898575 - Flags: review?(padenot) → review+
Comment on attachment 8898576 [details] bug 1391482 move AudioBuffer parameter checking to constructor https://reviewboard.mozilla.org/r/169950/#review178046
Attachment #8898576 - Flags: review?(padenot) → review+
Comment on attachment 8898577 [details] bug 1391482 add a method to create an AudioBuffer from AudioChunk data https://reviewboard.mozilla.org/r/169952/#review178048
Attachment #8898577 - Flags: review?(padenot) → review+
Comment on attachment 8898578 [details] bug 1391482 accept int16_t-sample initialization of AudioBuffer https://reviewboard.mozilla.org/r/169954/#review178052
Attachment #8898578 - Flags: review?(padenot) → review+
Comment on attachment 8898579 [details] bug 1391482 permit AudioChunk channel data initialization when mBuffer is not shared https://reviewboard.mozilla.org/r/169956/#review178054
Attachment #8898579 - Flags: review?(padenot) → review+
Comment on attachment 8898580 [details] bug 1391482 generalize WebAudioDecodeJob buffer as AudioChunk https://reviewboard.mozilla.org/r/169958/#review178056
Attachment #8898580 - Flags: review?(padenot) → review+
Comment on attachment 8898581 [details] bug 1391482 delay conversion of output of decodeAudioData() from int16_t to float until necessary https://reviewboard.mozilla.org/r/169960/#review178058
Attachment #8898581 - Flags: review?(padenot) → review+
Comment on attachment 8898570 [details] bug 1391482 add a fallible SharedBuffer::Create() https://reviewboard.mozilla.org/r/169938/#review178010 > I thought we had a style guide entry stating that `new` should be preferred in new code. I can't find it though. > > Here it does not matter too much because we're allocating bytes and not elements of a bigger size, so we can't forget the multiplication. I filed bug 1394341 on the existing mismatch between moz_xmalloc and delete. Using vector new char[] here would still mismatch with scalar delete SharedBuffer*. I'd prefer ::operator new() and ::operator delete() over new char[] and delete[] char*, because it is bytes not chars being allocated and I'd like to avoids reinterpret_cast from char*. However, I think it is easier to share code with malloc/moz_x_malloc than ::operator new(size_t) and ::operator new(size_t, const mozilla::fallible_t). malloc and free are also better understood by everybody. I'll drop this issue and follow up in bug 1394341.
Comment on attachment 8898572 [details] bug 1391482 use AudioChunk to store and pass PeriodicWave data to engine https://reviewboard.mozilla.org/r/169942/#review178022 > A bit weird to have an `AudioChunk` containing Fourier coefficients, but it makes the code cleaner indeed. Yes, I guess there is the possibility of wanting to pass non-audio data buffers through this mechanism, which would be a bit odd. I don't feel so bad about Fourier coefficients because they are frequency-domain audio with the same units and types as time domain audio. (AudioChunk has neither a fundamental frequency, nor a sample rate member.) I'll drop this issue because I don't think you expecting this to be addressed, and I don't have an better ideas.
Comment on attachment 8898574 [details] bug 1391482 accept int16_t sample buffers in AudioBufferSourceNode https://reviewboard.mozilla.org/r/169946/#review178030 > This will allocate if the resampling rate is less than 0.25 and the platform uses int16 for audio output, and the buffer is originally in float32. > > One one hand, this whole patch set would be amazing on desktop (and we don't have to change the spec to implement it), and this would mean we allocate a bit more in some case. > > On the other hand, solving the memory problem can also be done with explicit in-memory compression, and would be better. This is likely to require a spec change. > > The allocation can also be removed by compiling two copies of the resampler, one for int, one for float. Perhaps I'm missing your point here. Yes, there can be an allocation when the resampler works in int16. On such platforms, having an int16 source buffer removes that allocation. When the buffer is in float on int16 platforms, this allocation could be avoided by clamping inputLimit to WEBAUDIO_BLOCK_SIZE*4. I'll leave that to a separate bug, as that is no worse with these changes. If something similar were to be done on desktop, then that allocation could also happen on desktop, and so that should first be addressed. On desktop, int16 buffers should probably only be generated from encodings that are intended for int16 output, such as int16 wav. I'll drop this issue, because I don't think you are requesting anything in this bug, but please speak up if I misunderstand. Thanks for the reviews!
Yes, to be clear, everything here is r+ and can land, I was merely thinking out loud.
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ca07d3a43b11 use AudioChunk to generalize AudioBuffer shared channel storage r=padenot https://hg.mozilla.org/integration/autoland/rev/fea3e5cd3590 add a mechanism to pass an AudioChunk from node to engine r=padenot https://hg.mozilla.org/integration/autoland/rev/a9c7abf92455 generalize shared channel data from AudioBuffer as AudioChunk r=padenot https://hg.mozilla.org/integration/autoland/rev/527ea972cdf3 add a fallible SharedBuffer::Create() r=padenot https://hg.mozilla.org/integration/autoland/rev/fc35c12c815f keep custom oscillator data buffer on graph thread only long enough to initialize the PeriodicWave r=padenot https://hg.mozilla.org/integration/autoland/rev/7ffc044e742a use AudioChunk to store and pass PeriodicWave data to engine r=padenot https://hg.mozilla.org/integration/autoland/rev/0e12a1bdb2fa remove now-unused ThreadSharedFloatArrayBufferList SetBuffer() variant r=padenot https://hg.mozilla.org/integration/autoland/rev/ee13302be6c8 accept int16_t sample buffers in AudioBufferSourceNode r=padenot https://hg.mozilla.org/integration/autoland/rev/6feba222e86b accept int16_t sample buffers in ConvolverNode r=padenot https://hg.mozilla.org/integration/autoland/rev/b9a522cc88c9 move AudioBuffer parameter checking to constructor r=padenot https://hg.mozilla.org/integration/autoland/rev/cb6ac4267562 add a method to create an AudioBuffer from AudioChunk data r=padenot https://hg.mozilla.org/integration/autoland/rev/497e04031fc3 accept int16_t-sample initialization of AudioBuffer r=padenot https://hg.mozilla.org/integration/autoland/rev/7f096b0d1d0e permit AudioChunk channel data initialization when mBuffer is not shared r=padenot https://hg.mozilla.org/integration/autoland/rev/c02da061fc56 generalize WebAudioDecodeJob buffer as AudioChunk r=padenot https://hg.mozilla.org/integration/autoland/rev/4d4ed9b64bdb delay conversion of output of decodeAudioData() from int16_t to float until necessary r=padenot
Flags: in-testsuite-
The rooting analysis does not analyze the values of function pointers, and so flags a hazard because it doesn't know that it is not a hazard. I'll rearrange SharedBuffer::Create() to work around this: Function '_ZN7mozilla3dom12nsSpeechTask9SendAudioEN2JS6HandleINS2_5ValueEEES5_P9JSContext$uint32 mozilla::dom::nsSpeechTask::SendAudio(class JS::Handle<JS::Value>, class JS::Handle<JS::Value>, JSContext*)' has unrooted 'nogc' of type 'JS::AutoCheckCannotGC' live across GC call '_ZN7mozilla3domL11makeSamplesEPsj$nsSpeechTask.cpp:RefPtr<mozilla::SharedBuffer> mozilla::dom::makeSamples(int16_t*, uint32_t)' at dom/media/webspeech/synth/nsSpeechTask.cpp:303 nsSpeechTask.cpp:296: Call(59,60, nogc.AutoCheckCannotGC(0)) nsSpeechTask.cpp:298: Call(60,61, __temp_24 := tsrc.operator 111()) nsSpeechTask.cpp:298: Call(61,62, data := JS_GetInt16ArrayData(__temp_24**,isShared,nogc)) nsSpeechTask.cpp:299: Assume(62,69, isShared*, false) nsSpeechTask.cpp:303: Call(69,70, __temp_25 := makeSamples(data*,dataLen*)) [[GC call]] nsSpeechTask.cpp:303: Call(70,71, samples.operator=(__temp_25)) nsSpeechTask.cpp:303: Call(71,72, __temp_25.~SharedBuffer>()) nsSpeechTask.cpp:303: Call(72,73, nogc.~AutoCheckCannotGC()) GC Function: _ZN7mozilla3domL11makeSamplesEPsj$nsSpeechTask.cpp:RefPtr<mozilla::SharedBuffer> mozilla::dom::makeSamples(int16_t*, uint32_t) static already_AddRefed<mozilla::SharedBuffer> mozilla::SharedBuffer::Create(size_t) static already_AddRefed<mozilla::SharedBuffer> mozilla::SharedBuffer::InternalCreate(void* (*)(size_t), size_t) IndirectCall: aMalloc
Comment on attachment 8898570 [details] bug 1391482 add a fallible SharedBuffer::Create() https://reviewboard.mozilla.org/r/169938/#review178010 > I filed bug 1394341 on the existing mismatch between moz_xmalloc and delete. > > Using vector new char[] here would still mismatch with scalar delete > SharedBuffer*. > > I'd prefer ::operator new() and ::operator delete() over new char[] and > delete[] char*, because it is bytes not chars being allocated and I'd like to > avoids reinterpret_cast from char*. > > However, I think it is easier to share code with malloc/moz_x_malloc than > ::operator new(size_t) and ::operator new(size_t, const mozilla::fallible_t). > > malloc and free are also better understood by everybody. > > I'll drop this issue and follow up in bug 1394341. The changes required to avoid the function pointer also make it easy to use different variants of operator new. Using the non-array operator new balances with the non-array operator delete called when the ref count drops to zero, making a class operator delete unnecessary. This is different enough that I'll request review again.
Comment on attachment 8898570 [details] bug 1391482 add a fallible SharedBuffer::Create() Sorry about the churn. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c137902a1fea43cd3aace67f4a2f6b8a9469b97
Attachment #8898570 - Flags: review+ → review?(padenot)
Blocks: 1394341
Comment on attachment 8898570 [details] bug 1391482 add a fallible SharedBuffer::Create() https://reviewboard.mozilla.org/r/169938/#review178954 Thanks!
Attachment #8898570 - Flags: review?(padenot) → review+
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f8fc4ff76ed9 use AudioChunk to generalize AudioBuffer shared channel storage r=padenot https://hg.mozilla.org/integration/autoland/rev/043d871b9b81 add a mechanism to pass an AudioChunk from node to engine r=padenot https://hg.mozilla.org/integration/autoland/rev/4192bfee266c generalize shared channel data from AudioBuffer as AudioChunk r=padenot https://hg.mozilla.org/integration/autoland/rev/d5cc91a56399 add a fallible SharedBuffer::Create() r=padenot https://hg.mozilla.org/integration/autoland/rev/529abb028f5a keep custom oscillator data buffer on graph thread only long enough to initialize the PeriodicWave r=padenot https://hg.mozilla.org/integration/autoland/rev/1967d3942792 use AudioChunk to store and pass PeriodicWave data to engine r=padenot https://hg.mozilla.org/integration/autoland/rev/927b04969910 remove now-unused ThreadSharedFloatArrayBufferList SetBuffer() variant r=padenot https://hg.mozilla.org/integration/autoland/rev/bea3d472cab9 accept int16_t sample buffers in AudioBufferSourceNode r=padenot https://hg.mozilla.org/integration/autoland/rev/ffef0925fba1 accept int16_t sample buffers in ConvolverNode r=padenot https://hg.mozilla.org/integration/autoland/rev/8654ca00ee9f move AudioBuffer parameter checking to constructor r=padenot https://hg.mozilla.org/integration/autoland/rev/b643a1847ffd add a method to create an AudioBuffer from AudioChunk data r=padenot https://hg.mozilla.org/integration/autoland/rev/c949d19c37d0 accept int16_t-sample initialization of AudioBuffer r=padenot https://hg.mozilla.org/integration/autoland/rev/480d761a45ae permit AudioChunk channel data initialization when mBuffer is not shared r=padenot https://hg.mozilla.org/integration/autoland/rev/76419265414e generalize WebAudioDecodeJob buffer as AudioChunk r=padenot https://hg.mozilla.org/integration/autoland/rev/a3aa8b09341f delay conversion of output of decodeAudioData() from int16_t to float until necessary r=padenot
Depends on: 1404220
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: