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: