Remove AudioNodeEngine::NodeMutex() usage

RESOLVED FIXED in Firefox 42

Status

()

P3
normal
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

Trunk
mozilla42
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
Most clients of the NodeMutex() and AudioNode* Node() methods on AudioNodeEngine are calling from the main thread and so should be using NodeMainThread().

ProduceAudioBlock() in AnalyserNodeEngine and ScriptProcessorNodeEngine use NodeMutex() and Node() to check whether the node is alive before dispatching the event.  This check is not necessary, because the event needs to handle the case of the Node getting destroyed before the event runs anyway.
Priority: -- → P3
(Assignee)

Comment 1

3 years ago
Created attachment 8633757 [details] [diff] [review]
use NodeMainThread() to avoid NodeMutex() use on main thread
Attachment #8633757 - Flags: review?(padenot)
(Assignee)

Updated

3 years ago
Assignee: nobody → karlt
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8633758 [details] [diff] [review]
move mSharedBuffers ownership to engine

so that it is always available.
Attachment #8633758 - Flags: review?(padenot)
(Assignee)

Comment 3

3 years ago
Created attachment 8633759 [details] [diff] [review]
don't check engine's Node existence in ProcessBlock

If DestroyMediaStream() has cleared the node on the engine, then it will call
MediaStream::Destroy().  I doubt the additional code and cost of repeatedly
checking this is worth the savings in no-op events dispatched to main thread
between MediaStream::Destroy() on the main thread and destruction on the graph
thread.

Also ScriptProcessorNode should not be potentially ignoring its output buffer
immediately when the ScriptProcessor loses its last reference.  At least the
block currently being processed should output the output buffer.
(Deterministically playing the remainder of the output buffer would be best.)
Attachment #8633759 - Flags: review?(padenot)

Updated

3 years ago
Attachment #8633757 - Flags: review?(padenot) → review+

Updated

3 years ago
Attachment #8633758 - Flags: review?(padenot) → review+

Updated

3 years ago
Attachment #8633759 - Flags: review?(padenot) → review+
https://hg.mozilla.org/mozilla-central/rev/ad3370352602
https://hg.mozilla.org/mozilla-central/rev/d3818f780309
https://hg.mozilla.org/mozilla-central/rev/c7ddd289198c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Assignee)

Updated

3 years ago
Depends on: 1189168
You need to log in before you can comment on or make changes to this bug.