JavaScriptAudioNode donesn't need refNode() when created
Created attachment 136862 [details] Patch
Chris, from my understanding, as we will ref the jsNode before firing the event. } else { // Reference ourself so we don't accidentally get deleted before fireProcessEvent() gets called. ref(); // Fire the event on the main thread, not this one (which is the realtime audio thread). m_doubleBufferIndexForEvent = m_doubleBufferIndex; m_isRequestOutstanding = true; callOnMainThread(fireProcessEventDispatch, this); } and after the event dispatched, then jsAudioNode->fireProcessEvent(); // De-reference to match the ref() call in process(). jsAudioNode->deref(); so it is unnecessary to refNode by context. correct me if I am wrong. thanks
Attachment 136862 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:13: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 5 in 1 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 136864 [details] Patch
Comment on attachment 136864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136864&action=review > Source/WebCore/Modules/webaudio/AudioContext.cpp:-395 > - refNode(node.get()); // context keeps reference until we stop making javascript rendering callbacks We need to have a reference here because otherwise the JavaScriptAudioNode could potentially be garbage-collected before its process() method is even called. And even after process() is called, there are "in between" times when the reference count can reach zero (if the node has been garbage collected). Similar to AudioBufferSourceNode the context must keep a reference to this node since it may be created as a local variable in a function and subject to garbage collection, for example: function setupJSNode() { var jsnode = context.createJavaScriptNode(); ... } Note that since "jsnode" is a local variable it is subject to garbage collection after setupJSNode() is called -- it would be very bad if the C++ JavaScriptAudioNode were deleted at that time
(In reply to comment #5) > (From update of attachment 136864 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136864&action=review > > > Source/WebCore/Modules/webaudio/AudioContext.cpp:-395 > > - refNode(node.get()); // context keeps reference until we stop making javascript rendering callbacks > > We need to have a reference here because otherwise the JavaScriptAudioNode could potentially be garbage-collected before its process() method is even called. And even after process() is called, there are "in between" times when the > reference count can reach zero (if the node has been garbage collected). Similar to AudioBufferSourceNode the context must keep a reference to this node since it may be created as a local variable in a function and subject to garbage collection, for example: > > function setupJSNode() { > var jsnode = context.createJavaScriptNode(); > ... > } > > Note that since "jsnode" is a local variable it is subject to garbage collection after setupJSNode() is called -- it would be very bad if the C++ JavaScriptAudioNode were deleted at that time so how about use normal ref instead of connection ref? if using connection ref, the JavaScriptAudioNode will not disabled even source finished playing.
(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 136864 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=136864&action=review > > > > > Source/WebCore/Modules/webaudio/AudioContext.cpp:-395 > > > - refNode(node.get()); // context keeps reference until we stop making javascript rendering callbacks > > > > We need to have a reference here because otherwise the JavaScriptAudioNode could potentially be garbage-collected before its process() method is even called. And even after process() is called, there are "in between" times when the > > reference count can reach zero (if the node has been garbage collected). Similar to AudioBufferSourceNode the context must keep a reference to this node since it may be created as a local variable in a function and subject to garbage collection, for example: > > > > function setupJSNode() { > > var jsnode = context.createJavaScriptNode(); > > ... > > } > > > > Note that since "jsnode" is a local variable it is subject to garbage collection after setupJSNode() is called -- it would be very bad if the C++ JavaScriptAudioNode were deleted at that time > > so how about use normal ref instead of connection ref? > > if using connection ref, the JavaScriptAudioNode will not disabled even source finished playing. I'm not clear what you mean by "source finished playing"? In the current model, the JavaScriptAudioNode will live until the AudioContext is destroyed.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > (From update of attachment 136864 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=136864&action=review > > > > > > > Source/WebCore/Modules/webaudio/AudioContext.cpp:-395 > > > > - refNode(node.get()); // context keeps reference until we stop making javascript rendering callbacks > > > > > > We need to have a reference here because otherwise the JavaScriptAudioNode could potentially be garbage-collected before its process() method is even called. And even after process() is called, there are "in between" times when the > > > reference count can reach zero (if the node has been garbage collected). Similar to AudioBufferSourceNode the context must keep a reference to this node since it may be created as a local variable in a function and subject to garbage collection, for example: > > > > > > function setupJSNode() { > > > var jsnode = context.createJavaScriptNode(); > > > ... > > > } > > > > > > Note that since "jsnode" is a local variable it is subject to garbage collection after setupJSNode() is called -- it would be very bad if the C++ JavaScriptAudioNode were deleted at that time > > > > so how about use normal ref instead of connection ref? > > > > if using connection ref, the JavaScriptAudioNode will not disabled even source finished playing. > > I'm not clear what you mean by "source finished playing"? > > In the current model, the JavaScriptAudioNode will live until the AudioContext is destroyed. in the following graph: var source = context.createBufferSource(); var jsnode = context.createJavaScriptNode(4096, 1, 2); jsnode.onaudioprocess = audioProcessor; source.buffer = buffer; source.connect(jsnode); jsnode.connect(context.destination); source.noteOn(0); if the buffer is 1 second and when the buffer is playing, AudioProcessingEvent will be fired, it is ok. but after 1 seconds, the AudioBufferSourceNode reaches the end of the buffer and finished playing, I suppose there should be no more AudioProcessingEvent fired even the JavaScriptAudioNode exists. the jsnode should be disabled even not being garbage collected. But in current status, as the connection reference of jsnode is not 0 after source disabled, the jsnode will not be disabled and so there are still AudioProcessingEvent fired.
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > (In reply to comment #5) > > > > (From update of attachment 136864 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=136864&action=review > > > > > > > > > Source/WebCore/Modules/webaudio/AudioContext.cpp:-395 > > > > > - refNode(node.get()); // context keeps reference until we stop making javascript rendering callbacks > > > > > > > > We need to have a reference here because otherwise the JavaScriptAudioNode could potentially be garbage-collected before its process() method is even called. And even after process() is called, there are "in between" times when the > > > > reference count can reach zero (if the node has been garbage collected). Similar to AudioBufferSourceNode the context must keep a reference to this node since it may be created as a local variable in a function and subject to garbage collection, for example: > > > > > > > > function setupJSNode() { > > > > var jsnode = context.createJavaScriptNode(); > > > > ... > > > > } > > > > > > > > Note that since "jsnode" is a local variable it is subject to garbage collection after setupJSNode() is called -- it would be very bad if the C++ JavaScriptAudioNode were deleted at that time > > > > > > so how about use normal ref instead of connection ref? > > > > > > if using connection ref, the JavaScriptAudioNode will not disabled even source finished playing. > > > > I'm not clear what you mean by "source finished playing"? > > > > In the current model, the JavaScriptAudioNode will live until the AudioContext is destroyed. > > in the following graph: > var source = context.createBufferSource(); > var jsnode = context.createJavaScriptNode(4096, 1, 2); > jsnode.onaudioprocess = audioProcessor; > source.buffer = buffer; > source.connect(jsnode); > jsnode.connect(context.destination); > source.noteOn(0); > > if the buffer is 1 second and when the buffer is playing, AudioProcessingEvent will be fired, it is ok. > > but after 1 seconds, the AudioBufferSourceNode reaches the end of the buffer and finished playing, I suppose there should be no more AudioProcessingEvent fired even the JavaScriptAudioNode exists. the jsnode should be disabled even not being garbage collected. > > But in current status, as the connection reference of jsnode is not 0 after source disabled, the jsnode will not be disabled and so there are still AudioProcessingEvent fired. But there is no way to know that another AudioBufferSourceNode will not cause another sound to be played very quickly after the first one (at time 0 in this case) so the JavaScriptAudioNode cannot be subject to deletion so quickly. Consider cases such as "funklet": http://funklet.com/ Where there are many AudioBufferSourceNodes being triggered at precise times. Just because the first note is finished does not mean that more notes will not follow.
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > > in the following graph: > > var source = context.createBufferSource(); > > var jsnode = context.createJavaScriptNode(4096, 1, 2); > > jsnode.onaudioprocess = audioProcessor; > > source.buffer = buffer; > > source.connect(jsnode); > > jsnode.connect(context.destination); > > source.noteOn(0); > > > > if the buffer is 1 second and when the buffer is playing, AudioProcessingEvent will be fired, it is ok. > > > > but after 1 seconds, the AudioBufferSourceNode reaches the end of the buffer and finished playing, I suppose there should be no more AudioProcessingEvent fired even the JavaScriptAudioNode exists. the jsnode should be disabled even not being garbage collected. > > > > But in current status, as the connection reference of jsnode is not 0 after source disabled, the jsnode will not be disabled and so there are still AudioProcessingEvent fired. > > But there is no way to know that another AudioBufferSourceNode will not cause another sound to be played very quickly after the first one (at time 0 in this case) so the JavaScriptAudioNode cannot be subject to deletion so quickly. Consider cases such as "funklet": > http://funklet.com/ > > Where there are many AudioBufferSourceNodes being triggered at precise times. Just because the first note is finished does not mean that more notes will not follow. sorry I am somehow confused. Pls correct me if I am wrong. 1. I understand there will be other notes that need to connect to jsnode. but in these cases, the jsnode must have reference(otherwise, how can those notes connect to this jsnode?), so the JavaScriptAudioNode object will not be deleted and if another note connected to jsnode, it will be re-enabled. 2. for the case in the comment #5, if the jsnode is local variable, it must be connected to other nodes in the same scope(or other nodes cannot reach this jsnode), in this cases, the c++ JavaScriptAudioNode object will not be deleted because it has the connection reference. 3. in the spec about the lifetime of the nodes: https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#DynamicLifetime it is said : ''' the low-pass filter, panner, and second gain nodes are directly connected from the one-shot sound. So when it has finished playing the context will automatically release them (everything within the dotted line). If there are no longer any JavaScript references to the one-shot sound and connected nodes, then they will be immediately removed from the graph and deleted. ''' so what's the reason for that JavaScriptAudioNode to be different with other types of node? If it is because of the event callback access, I think the ref/deref mechanism in JavaScripAudioNode::process() is enough for this purpose. Before the process(), the JavaScriptAudioNode should be the same with other types of node.
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (In reply to comment #7) > > > > > > in the following graph: > > > var source = context.createBufferSource(); > > > var jsnode = context.createJavaScriptNode(4096, 1, 2); > > > jsnode.onaudioprocess = audioProcessor; > > > source.buffer = buffer; > > > source.connect(jsnode); > > > jsnode.connect(context.destination); > > > source.noteOn(0); > > > > > > if the buffer is 1 second and when the buffer is playing, AudioProcessingEvent will be fired, it is ok. > > > > > > but after 1 seconds, the AudioBufferSourceNode reaches the end of the buffer and finished playing, I suppose there should be no more AudioProcessingEvent fired even the JavaScriptAudioNode exists. the jsnode should be disabled even not being garbage collected. > > > > > > But in current status, as the connection reference of jsnode is not 0 after source disabled, the jsnode will not be disabled and so there are still AudioProcessingEvent fired. > > > > But there is no way to know that another AudioBufferSourceNode will not cause another sound to be played very quickly after the first one (at time 0 in this case) so the JavaScriptAudioNode cannot be subject to deletion so quickly. Consider cases such as "funklet": > > http://funklet.com/ > > > > Where there are many AudioBufferSourceNodes being triggered at precise times. Just because the first note is finished does not mean that more notes will not follow. > > sorry I am somehow confused. Pls correct me if I am wrong. > > 1. I understand there will be other notes that need to connect to jsnode. but in these cases, the jsnode must have reference(otherwise, how can those notes connect to this jsnode?), so the JavaScriptAudioNode object will not be deleted and if another note connected to jsnode, it will be re-enabled. > > 2. for the case in the comment #5, if the jsnode is local variable, it must be connected to other nodes in the same scope(or other nodes cannot reach this jsnode), in this cases, the c++ JavaScriptAudioNode object will not be deleted because it has the connection reference. > > 3. in the spec about the lifetime of the nodes: https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#DynamicLifetime > it is said : > ''' > the low-pass filter, panner, and second gain nodes are directly connected from the one-shot sound. So when it has finished playing the context will automatically release them (everything within the dotted line). If there are no longer any JavaScript references to the one-shot sound and connected nodes, then they will be immediately removed from the graph and deleted. > ''' > so what's the reason for that JavaScriptAudioNode to be different with other types of node? If it is because of the event callback access, I think the ref/deref mechanism in JavaScripAudioNode::process() is enough for this purpose. Before the process(), the JavaScriptAudioNode should be the same with other types of node. James, I think you are right about this part. But the current code is not thread-safe enough to simply remove the "ref" of the JavaScriptAudioNode as you suggest. We can investigate how to make this more thread safe to handle the case when there is no longer any reachable way to connect to the JavaScriptAudioNode.
(In reply to comment #11) > James, I think you are right about this part. But the current code is not thread-safe enough to simply remove the "ref" of the JavaScriptAudioNode as you suggest. We can investigate how to make this more thread safe to handle the case when there is no longer any reachable way to connect to the JavaScriptAudioNode. Yes, I understand it. So I propose to still let context to ref jsnode but use RefTypeNormal instead of RefTypeConnection. In this way, the jsnode object will not be deleted but will be disabled and so no extra AudioProcessingEvent fired after the AudioBufferSourceNode completed. is it ok?
Created attachment 137305 [details] Patch
Comment on attachment 137305 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.