Closed Bug 1184801 Opened 5 years ago Closed 5 years ago

AnalyserNode corrupts input stream for other consumers

Categories

(Core :: Web Audio, defect)

39 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

AnalyserNodeEngine::ProcessBlock() uses const_cast to scale its input chunk
data.  However input chunk data was intended as read-only so that it could be
passed without copying.  The in-place scale corrupts the data for other
consumers.

https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/dom/media/webaudio/AnalyserNode.cpp#l79
Using testharness.js so this can be a web platform test, but using mochitest
for now so that the test is run by debug builds.
AllocateAudioBlock() expects const_cast to be used to write data for its new
chunk.  I'll look into whether there's a better way to do that.  It could
return a non-const pointer to the data to be written.
This moves the application of volume and averaging of channels to the main
thread, performed when required.  It avoids a potential allocation on the
graph thread.  If doing a full analysis for frequency data, the extra work on
the main thread should be negligible.  I assume that repeatedly fetching the
same time domain data with getFloatFrequencyData() is not something we need to
optimize for.  If, in rare circumstances, the extra main thread work turns out
to be significant, then the main thread work in getters is self-regulating,
but too much load on the graph thread leads to catastrophic failure in the
audio.

This also fixes some bugs:

Input and output streams for other consumers are not corrupted by in-place
scaling of data intended to be read-only.

When there are additional channels and fftSize < WEBAUDIO_BLOCK_SIZE, the
channels are not written past the current length of a buffer, so there is no
longer a dependency on nsTArray's behavior of never not reducing allocation
size on length changes.

The most recent fftSize samples are now used even when fftSize <
WEBAUDIO_BLOCK_SIZE, instead of the first fftSize samples in the most recent
block.

Enough time domain data is recorded so that getters will work immediately
following a change in fftSize.
Attachment #8637714 - Flags: review?(padenot)
This testcase generates cpu usage from 100 AnalyserNodes.
Before connected, the AnalyserNode input is null.
After connect, the input is non-null.

The patch noticeably improves cpu usage only when input is non-null.

I assume most of the saving is from removing the allocation when null.  If
this bug were to be fixed through also allocating for non-null on the graph
threads, then I expect we'd have that additional cost all the time.

CPU usage of threads at 44.1kHz:

                before         after  
input       null non-null   null non-null
firefox      70    65        56    65
threaded-ml  23    20        12    20

Oddly the usage was quite different if, in another page in another tab, an
empty context is created, the sampleRate is fetched, and the page is closed.
Main thread CPU reduces considerably, but graph thread usage increases.

The pattern of change from the patch is similar.

                before           after
            null non-null   null non-null
firefox      30    25        18    25
threaded-ml  31    25        15    25
I wrote this to test the small fftSize behavior with multiple channels.
Even on current m-c it doesn't fail because nsTArray doesn't shrink buffers on
SetLength() and we don't have bounds checking of the array access, but we
don't seem to test this path elsewhere, so it seems worth checking in the
test anyway.
Blocks: 1186779
Visual C++ warns with unary minus on unsigned types.
Attachment #8637722 - Flags: review?(padenot)
Attachment #8637714 - Attachment is obsolete: true
Attachment #8637714 - Flags: review?(padenot)
Comment on attachment 8637722 [details] [diff] [review]
process AnalyserNode input only when required

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

Looks good, but it will need rebasing on top of the const-cast changes
Attachment #8637722 - Flags: review?(padenot) → review+
Flags: in-testsuite+
Version: Trunk → 39 Branch
Depends on: 1190291
This extends the test to check the AnalyserNode output, which was also
corrupted, but depends on changes in bug 1190291.
You need to log in before you can comment on or make changes to this bug.