Bugzilla

Assignee

Comment 2

•

6 years ago

This is just an OOM, but worth fixing because it's trivial to hit. I've toyed with a couple ideas:
1 - Putting a limit on the impulse response length (not amazing and not too reliable)
2 - Allocate a new buffer on the main thread, copy the data and normalize, so that we can signal the OOM early, and then delete the buffer on the rendering thread when we're done with all the FFTs (works, not super elegant, causes transient memory pressure)
3 - Doing the normalization, doing the FFTs and then undoing it (multiplying by the inverse): we can't because of our buffer sharing mechanism (maybe _in practice_ it would be safe, but well...).
4 - Doing the normalization on the input of the convolution process: this is trading a one time cost into a cost per rendering quantum, not great
5 - Use fallible alloc on the rendering thread, and try to signal back the error somehow, without throwing (using the console maybe ?) and turn the ConvolveNode into a noop.
6 - Plumb the normalization factor down the various objects created here, counting on the fact that we're using multi-stage convolution, and do our temporary allocation chunk per chunk, hoping that they are small enough (worst case, 32768 / 2 floats I think, which is around 13kB).

I'm leaning towards option 2 or 6 here. Karl, what do you think? Do you have another idea?

Flags: needinfo?(padenot) → needinfo?(karlt)

Comment 3

•

6 years ago

•

Edited

I agree this is worth fixing.  I wonder how large a buffer is feasible
with realtime processing, but large buffers are doable with offline
processing.

I agree it is sensible to focus on the allocations that are the full length
of the buffer and leave the smaller FFT allocations infallible.

FFTConvolver and FFTBlock have float arrays of length fftSize, which for
maxFFTSize = 32768 is 128kB for each of those arrays.  Failure in one of those
allocations would qualify for OOM | small on crash-stats.

You've covered the options well.  I don't have other ideas.

Note, however, that ReverbConvolver::m_accumulationBuffer() is also sized to
the length of the buffer.  This could be made fallible, with failure leading
to zero output.  Reporting an error early would require moving Reverb
construction to the main thread, so I think I'd only do 2 if moving the entire
construction.

Option 6 initially looked appealing.  Maybe its worth doing to save one
allocation, but it is not a complete solution because it doesn't address
m_accumulationBuffer.

I don't have any other ideas for signalling an error to the main thread for
option 5, but I think zero output on allocation failure is acceptable.
i.e. option 5 with a console warning is a good option.

I'd either do 5 or 2 with all of Reverb construction.

My thoughts on performance:

The normalization that is performed is not very useful, but it is the default
and so I guess we should be a bit concerned about performance.

Because FFTs are limited to maxFFTSize, the cost of a single process() call
eventually becomes proportional to the size of the impulse buffer.

At least for moderate sized FFTs less than maxFFTSize, the cost of a
normalization (proportional to impulse buffer) may be large or at least
significant compared to process() cost.

That leads me to suspect that moving calculation of the normalization factor
to the main thread would eventually be beneficial for the rendering thread.
(i.e. part of option 2, at least)

Improvements from moving calculation of the normalization factor are not going
to be immediately noticed because all of the FFTs are currently set up on
construction of the Reverb on the rendering thread.  i.e. cost is proportional
to size.  ReverbAccumulationBuffer zero initialization is proportional too.
Moving Reverb construction to the main thread would be better.  Even
better I suspect would be to delay each FFTBlock initialization until
required, to stagger them.  FFTConvolver::process() does not use the fftKernel
until m_readWriteIndex == halfSize.  That would require to some extent
reverting
https://hg.mozilla.org/mozilla-central/rev/cd8681109e9606502d08341f5c9a58e4b74118ab
This is all a separate project, but where we want to be heading may influence
choices here.

I'm not so concerned about efficiency in the offline processing, super large
buffer situation.  For feasible realtime sizes, I suspect the transient memory
pressure would not be a big concern.

Flags: needinfo?(karlt)

Comment hidden (obsolete)

(In reply to Karl Tomlinson (:karlt) from comment #3)
> Note, however, that ReverbConvolver::m_accumulationBuffer() is also sized to
> the length of the buffer.  This could be made

*fallible*

Assignee

Comment 5

•

6 years ago

Attached file Bug 1493779 - Add a test for ConvolverNode with very large buffer. r?karlt — Details

Assignee

Comment 6

•

6 years ago

Attached file Bug 1493779 - Gracefuly handle OOM when setting a buffer on ConvolverNode. r?karlt — Details

When OOMing when allocating the temporary buffer, we return an error from the
ctor via an output parameter, and make the ConvolverNode output silence.
Additionaly, a warning is issued each time we fail to set a buffer to the buffer
property of a ConvolverNode.

This is much more complicated than anticipated, because we can't, from the
ConvolverNodeEngine::SetBuffer method, find an object we can AddRef, to then
move the reference into a runnable, to then dispatch this runnable to the main
thread, to then find the right owner and then report the error.

Instead, we propagate the error back to the AudioNodeStream, grip the stream,
and go from there. This means AudioNodeStream::SetBuffer has to be modified,
along with the other consumers.

Comment 7

•

6 years ago

Comment on attachment 9012253 [details]
Bug 1493779 - Add a test for ConvolverNode with very large buffer. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

Attachment #9012253 - Flags: review+

Assignee

Updated

•

6 years ago

Assignee: nobody → padenot

Assignee

Comment 8

•

6 years ago

Attached file Bug 1493779 - Gracefuly handle OOM when setting a buffer on ConvolverNode. r?karlt (obsolete) — Details

When OOMing when allocating the temporary buffer, we return an error from the
ctor via an output parameter, and make the ConvolverNode output silence.
Additionaly, a warning is issued each time we fail to set a buffer to the buffer
property of a ConvolverNode.

This is much more complicated than anticipated, because we can't, from the
ConvolverNodeEngine::SetBuffer method, find an object we can AddRef, to then
move the reference into a runnable, to then dispatch this runnable to the main
thread, to then find the right owner and then report the error.

Instead, we propagate the error back to the AudioNodeStream, grip the stream,
and go from there. This means AudioNodeStream::SetBuffer has to be modified,
along with the other consumers.

Phabricator Automation

Updated

•

6 years ago

Attachment #9013598 - Attachment is obsolete: true

Cristian Brindusan [:cbrindusan]

Comment 9

•

6 years ago

Pushed by padenot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e380ab03ba8d
Add a test for ConvolverNode with very large buffer. r=karlt
https://hg.mozilla.org/integration/autoland/rev/cefa70540aac
Gracefuly handle OOM when setting a buffer on ConvolverNode.  r=karlt

Comment 10

•

6 years ago

Backed out 2 changesets (Bug 1493779) for mda failures in test_convolverNodeOOM.html.

Backout: https://hg.mozilla.org/integration/autoland/rev/ff0aee3fd2ae32e5fb88ebd4636988b717c5d9c4

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending,running,success,testfailed,busted,exception&revision=cefa70540aacffeffaec04a4221c7ebfc0acceca&selectedJob=203340119

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=203340119&repo=autoland&lineNumber=11605

Flags: needinfo?(padenot)

Assignee

Comment 11

•

6 years ago

hrm, sometimes the `createBuffer` itself is ooming. I'm now catching this and finishing the test early.

Flags: needinfo?(padenot)

Comment 12

•

6 years ago

Pushed by padenot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/53950ea4b105
Add a test for ConvolverNode with very large buffer. r=karlt
https://hg.mozilla.org/integration/autoland/rev/e78d9996b1a4
Gracefuly handle OOM when setting a buffer on ConvolverNode.  r=karlt

Bogdan Tara[:bogdan_tara | bogdant]

Comment 13

•

6 years ago

Backout by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d9f492389275
Backed out 2 changesets for test_convolverNodeOOM.html failures CLOSED TREE

Comment 14

•

6 years ago

Backed out 2 changesets (Bug 1493779) for test_convolverNodeOOM.html failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e78d9996b1a44045dcf8a85722189a3db77033d3

Backout link: https://hg.mozilla.org/integration/autoland/rev/d9f4923892754c462907bdc524efcf37d75306a7

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=203446073&repo=autoland&lineNumber=1590

[task 2018-10-04T17:06:12.537Z] 17:06:12     INFO -  145 INFO TEST-START | dom/media/webaudio/test/test_convolverNodeOOM.html
[task 2018-10-04T17:06:23.160Z] 17:06:23     INFO -  Buffered messages logged at 17:06:11
[task 2018-10-04T17:06:23.162Z] 17:06:23     INFO -  146 INFO TEST-PASS | dom/media/webaudio/test/test_convolverNodeOOM.html | Correct number of channels for expected buffer 0
[task 2018-10-04T17:06:23.163Z] 17:06:23     INFO -  Buffered messages finished
[task 2018-10-04T17:06:23.163Z] 17:06:23     INFO -  147 INFO TEST-UNEXPECTED-FAIL | dom/media/webaudio/test/test_convolverNodeOOM.html | uncaught exception - NS_ERROR_OUT_OF_MEMORY:  at createGraph@http://mochi.test:8888/tests/dom/media/webaudio/test/test_convolverNodeOOM.html:28:19
[task 2018-10-04T17:06:23.163Z] 17:06:23     INFO -  runTestOnContext@http://mochi.test:8888/tests/dom/media/webaudio/test/webaudio.js:202:20
[task 2018-10-04T17:06:23.163Z] 17:06:23     INFO -  testOnNormalContext@http://mochi.test:8888/tests/dom/media/webaudio/test/webaudio.js:222:7
[task 2018-10-04T17:06:23.163Z] 17:06:23     INFO -  runTestFunction@http://mochi.test:8888/tests/dom/media/webaudio/test/webaudio.js:253:5
[task 2018-10-04T17:06:23.164Z] 17:06:23     INFO -  rval@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:146:17
[task 2018-10-04T17:06:23.164Z] 17:06:23     INFO -      simpletestOnerror@SimpleTest/SimpleTest.js:1635:11
[task 2018-10-04T17:06:23.164Z] 17:06:23     INFO -  148 INFO TEST-OK | dom/media/webaudio/test/test_convolverNodeOOM.html | took 4114ms

Flags: needinfo?(padenot)

Firefox Bug Husbandry Bot

Assignee

Comment 15

•

6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f52f556deda92ad5b139b2ef7e8537e568957c8 green if we put the getChannelData in the try {} block.

Flags: needinfo?(padenot)

QA Contact: drno

Updated

•

6 years ago

QA Contact: drno

Comment 16

•

6 years ago

Pushed by padenot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2430e0fe8d6d
Add a test for ConvolverNode with very large buffer. r=karlt
https://hg.mozilla.org/integration/autoland/rev/4dbc5f131040
Gracefuly handle OOM when setting a buffer on ConvolverNode.  r=karlt

Andreea Pavel [:apavel]

Comment 17

•

6 years ago

Backed out for failing android mochitest media on a CLOSED TREE

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=203978797&revision=4dbc5f131040f3df6ac99dd42574fa4c8d7a00bd

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=203978797&repo=autoland&lineNumber=1981

Backout: https://hg.mozilla.org/integration/autoland/rev/594b22ac459fc11ee7948cd7be54b88a86bd54c1

Flags: needinfo?(padenot)

Assignee

Comment 18

•

6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23d98cd3996734a96b3059268816da775c8727a2, with the test disabled on Android.

Flags: needinfo?(padenot)

Ryan VanderMeulen [:RyanVM]

Comment 19

•

6 years ago

Pushed by padenot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76ae4cbbb7e4
Add a test for ConvolverNode with very large buffer. r=karlt
https://hg.mozilla.org/integration/autoland/rev/0db994314ad3
Gracefuly handle OOM when setting a buffer on ConvolverNode.  r=karlt

Raul Gurzau (:RaulG)

Comment 21

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/76ae4cbbb7e4
https://hg.mozilla.org/mozilla-central/rev/0db994314ad3

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox64: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla64

Updated

•

6 years ago

status-firefox62: --- → wontfix

status-firefox63: --- → wontfix

status-firefox-esr60: --- → wontfix

Flags: in-testsuite? → in-testsuite+

Updated

•

5 years ago

Updated

•

4 years ago

Blocks: asan-maintenance