Reverb::Reverb mutates data that needs to be read-only

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reverb::Reverb temporarily modifies the buffers of a ThreadSharedFloatArrayBufferList by casting away const (and then restores them).

This is very bad, since those buffers might be concurrently read from the main thread, and quite unnecessary.
Comment on attachment 771854 [details] [diff] [review]
fix

Review of attachment 771854 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/blink/Reverb.cpp
@@ +83,5 @@
> +    nsAutoTArray<const float*,4> irChannels;
> +    for (size_t i = 0; i < impulseResponse->GetChannels(); ++i) {
> +        irChannels.AppendElement(impulseResponse->GetData(i));
> +    }
> +    nsAutoTArray<float,1024> tempBuf;

So, we seem to be using a stack-allocated array here (or deleted at the end of the scope if we reallocate), and I would worry about the lifetime (since we assign the pointer to some other stuff that are going to outlive the duration of this frame), but in fact, 4 calls below the ctor, in ReverbConvolverStage::ReverbConvolverStage, we copy it again. A little comment could be nice.

@@ +89,5 @@
>      if (normalize) {
>          scale = calculateNormalizationScale(impulseResponse, impulseResponseBufferLength, sampleRate);
>  
>          if (scale) {
> +            tempBuf.SetLength(irChannels.Length()*impulseResponseBufferLength);

This does a malloc, here, on the examples I tried. 1024 elements is around 10ms of audio when doing stereo convolution (1024 / 2 / 48000.), and I'd expect most real-world applications to use longer impulse response.

This is no big deal in terms of perfs since this is a one-time thing, but specifying 1024 in the array is kind of useless. For example, the average length of the 885 impulses I have on this computer is 105539 samples.
Attachment #771854 - Flags: review?(paul) → review+
Comment on attachment 771854 [details] [diff] [review]
fix

Review of attachment 771854 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/blink/Reverb.cpp
@@ +89,5 @@
>      if (normalize) {
>          scale = calculateNormalizationScale(impulseResponse, impulseResponseBufferLength, sampleRate);
>  
>          if (scale) {
> +            tempBuf.SetLength(irChannels.Length()*impulseResponseBufferLength);

One thing which we can do here instead of this nsAutoTArray is use alloca and fall back to dynamic allocation if that fails.  This is the sort of thing that I would usually say we should not prematurely optimize, except that memory allocation can be expensive given all of the other profiles that I look at these days. :(
This should be alright perf wise, it's only when doing the init, and should not be hot. Maybe if an author keeps setting an impulse, but well, they should not do that anyways.
(In reply to Paul Adenot (:padenot) from comment #2)
> So, we seem to be using a stack-allocated array here (or deleted at the end
> of the scope if we reallocate), and I would worry about the lifetime (since
> we assign the pointer to some other stuff that are going to outlive the
> duration of this frame), but in fact, 4 calls below the ctor, in
> ReverbConvolverStage::ReverbConvolverStage, we copy it again. A little
> comment could be nice.

Sure.

> This is no big deal in terms of perfs since this is a one-time thing, but
> specifying 1024 in the array is kind of useless. For example, the average
> length of the 885 impulses I have on this computer is 105539 samples.

What do you suggest? Make it a plain nsTArray? I can do that.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> One thing which we can do here instead of this nsAutoTArray is use alloca
> and fall back to dynamic allocation if that fails.  This is the sort of
> thing that I would usually say we should not prematurely optimize, except
> that memory allocation can be expensive given all of the other profiles that
> I look at these days. :(

Interesting idea. Do we/should we have a helper class to handle this?
Being able to allocate arrays on the stack with a heap allocation fallback if we don't have enough space is definitely something we could use in WebAudio (and in the media code in general).

If we get that, then we can use it for the aforementioned allocation of 1024 floats, otherwise, a plain nsTArray will do.
Doing that in ns*TArray is hard, because stack allocation is not C++ friendly (for example, allocating something on the stack in an ns*TArray method is of no use, since the stack memory will be invalid when the member function returns.

To the best of my knowledge, we currently don't have helper classes/macros for this kind of thing.  But if you want, you can file a follow-up bug, since Paul's point about this probably not being very hot is right on.
https://hg.mozilla.org/mozilla-central/rev/6b4ff981027f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.