Prevent AudioBufferSourceNode.buffer to be set more than once
Categories
(Core :: Web Audio, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox74 | --- | fixed |
People
(Reporter: padenot, Assigned: u538564)
Details
(Keywords: dev-doc-needed, Whiteboard: [spec])
Attachments
(3 files, 4 obsolete files)
6.03 KB,
patch
|
ehsan.akhgari
:
review-
|
Details | Diff | Splinter Review |
4.08 KB,
patch
|
Details | Diff | Splinter Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
Spec resolution: https://github.com/WebAudio/web-audio-api/issues/288
Reporter | ||
Comment 3•9 years ago
|
||
Comment on attachment 8609250 [details] [diff] [review] patch for 1161025 Review of attachment 8609250 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, can you address the comments. We'll be good to land after that. ::: dom/media/webaudio/AudioBufferSourceNode.cpp @@ +145,4 @@ > } > } > virtual void SetBuffer(already_AddRefed<ThreadSharedFloatArrayBufferList> aBuffer) override > + { nit: trailing spaces ::: dom/webidl/AudioBufferSourceNode.webidl @@ +12,4 @@ > > interface AudioBufferSourceNode : AudioNode { > > + [SetterThrows] attribute AudioBuffer? buffer; We usually put the [SetterThrows] thing on it's own line above the attribute.
Reporter | ||
Comment 5•9 years ago
|
||
Comment on attachment 8609329 [details] [diff] [review] patch for 1161025 with corrections Review of attachment 8609329 [details] [diff] [review]: ----------------------------------------------------------------- Cool ! Thanks !
Reporter | ||
Comment 6•9 years ago
|
||
Ehsan, I forgot once again about the push hook for .webidl changes. Would you mind having a look? There has virtually no compatibility risk because it is what other implementations always did, and there is only a marginal fraction content that only work with Gecko.
Reporter | ||
Updated•9 years ago
|
Reporter | ||
Updated•9 years ago
|
Comment 7•9 years ago
|
||
Comment on attachment 8610044 [details] [diff] [review] Prevent AudioBufferSourceNode.buffer to be set more than once Review of attachment 8610044 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/webaudio/AudioBufferSourceNode.cpp @@ +60,5 @@ > AudioDestinationNode* aDestination) : > AudioNodeEngine(aNode), > mStart(0.0), mBeginProcessing(0), > mStop(STREAM_TIME_MAX), > + mBuffer(nullptr), Nit: This is not needed, as the default constructor does it for you. ::: dom/media/webaudio/test/test_bug1161025.html @@ +15,5 @@ > + var ctx = new AudioContext(); > + var source = ctx.createBufferSource(); > + var b0 = ctx.createBuffer(32,798,22050); > + var b1 = ctx.createBuffer(32,28,22050); > + source.buffer = null; What is the desired behavior for setting the buffer to null? As things stand right now, the only reason why this test passes is that the set to null occurs first, before the set to b0. However, this patch makes it impossible to null out the buffer once it's set, which sounds like the wrong thing to do. I think the right thing to do would be to prevent buffer to be set twice to a non-null value. r- because of this. Note that the test needs to ensure the nullability as well. @@ +18,5 @@ > + var b1 = ctx.createBuffer(32,28,22050); > + source.buffer = null; > + source.buffer = b0; > + source.start(0); > + source.buffer = b1; Everything except this last statement in the test should be outside of the try block, otherwise the test is wrong.
Reporter | ||
Comment 8•9 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7) > ::: dom/media/webaudio/test/test_bug1161025.html > @@ +15,5 @@ > > + var ctx = new AudioContext(); > > + var source = ctx.createBufferSource(); > > + var b0 = ctx.createBuffer(32,798,22050); > > + var b1 = ctx.createBuffer(32,28,22050); > > + source.buffer = null; > > What is the desired behavior for setting the buffer to null? As things > stand right now, the only reason why this test passes is that the set to > null occurs first, before the set to b0. However, this patch makes it > impossible to null out the buffer once it's set, which sounds like the wrong > thing to do. > > I think the right thing to do would be to prevent buffer to be set twice to > a non-null value. > > r- because of this. What would be the behaviour or use-case for setting the buffer to an AudioBuffer, and then nulling it out? Ignoring setting it to null? Outputting silence? The spec resolution intent was that once a buffer is set, you don't touch the buffer property anymore. If you want silence, you can stop(), but that's it.
Comment 9•9 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #8) > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > comment #7) > > ::: dom/media/webaudio/test/test_bug1161025.html > > @@ +15,5 @@ > > > + var ctx = new AudioContext(); > > > + var source = ctx.createBufferSource(); > > > + var b0 = ctx.createBuffer(32,798,22050); > > > + var b1 = ctx.createBuffer(32,28,22050); > > > + source.buffer = null; > > > > What is the desired behavior for setting the buffer to null? As things > > stand right now, the only reason why this test passes is that the set to > > null occurs first, before the set to b0. However, this patch makes it > > impossible to null out the buffer once it's set, which sounds like the wrong > > thing to do. > > > > I think the right thing to do would be to prevent buffer to be set twice to > > a non-null value. > > > > r- because of this. > > What would be the behaviour or use-case for setting the buffer to an > AudioBuffer, and then nulling it out? Ignoring setting it to null? > Outputting silence? Dropping references to it, so that it can be freed. AudioBuffers are potentially huge objects. > The spec resolution intent was that once a buffer is set, you don't touch > the buffer property anymore. If you want silence, you can stop(), but that's > it. Yes, but I think being able to drop all references to the buffer is important. Why can't we make null a special case?
Reporter | ||
Comment 10•9 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9) > (In reply to Paul Adenot (:padenot) from comment #8) > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > > comment #7) > > > ::: dom/media/webaudio/test/test_bug1161025.html > > > @@ +15,5 @@ > > > > + var ctx = new AudioContext(); > > > > + var source = ctx.createBufferSource(); > > > > + var b0 = ctx.createBuffer(32,798,22050); > > > > + var b1 = ctx.createBuffer(32,28,22050); > > > > + source.buffer = null; > > > > > > What is the desired behavior for setting the buffer to null? As things > > > stand right now, the only reason why this test passes is that the set to > > > null occurs first, before the set to b0. However, this patch makes it > > > impossible to null out the buffer once it's set, which sounds like the wrong > > > thing to do. > > > > > > I think the right thing to do would be to prevent buffer to be set twice to > > > a non-null value. > > > > > > r- because of this. > > > > What would be the behaviour or use-case for setting the buffer to an > > AudioBuffer, and then nulling it out? Ignoring setting it to null? > > Outputting silence? > > Dropping references to it, so that it can be freed. AudioBuffers are > potentially huge objects. Can make it so that when stop() occurs, the buffer is dropped by the node? It should be functionally equivalent and not observable. > > The spec resolution intent was that once a buffer is set, you don't touch > > the buffer property anymore. If you want silence, you can stop(), but that's > > it. > > Yes, but I think being able to drop all references to the buffer is > important. Why can't we make null a special case? I'm not opposed to it, but it's incompatible (although not in a compatibility-breaking manner) with all the content out there. For reference, blink refuse to set `.buffer` to something that is not an `AudioBuffer`, and currently warns when you're setting an `AudioBuffer` for the second time.
Comment 11•9 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #10) > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > comment #9) > > (In reply to Paul Adenot (:padenot) from comment #8) > > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > > > comment #7) > > > > ::: dom/media/webaudio/test/test_bug1161025.html > > > > @@ +15,5 @@ > > > > > + var ctx = new AudioContext(); > > > > > + var source = ctx.createBufferSource(); > > > > > + var b0 = ctx.createBuffer(32,798,22050); > > > > > + var b1 = ctx.createBuffer(32,28,22050); > > > > > + source.buffer = null; > > > > > > > > What is the desired behavior for setting the buffer to null? As things > > > > stand right now, the only reason why this test passes is that the set to > > > > null occurs first, before the set to b0. However, this patch makes it > > > > impossible to null out the buffer once it's set, which sounds like the wrong > > > > thing to do. > > > > > > > > I think the right thing to do would be to prevent buffer to be set twice to > > > > a non-null value. > > > > > > > > r- because of this. > > > > > > What would be the behaviour or use-case for setting the buffer to an > > > AudioBuffer, and then nulling it out? Ignoring setting it to null? > > > Outputting silence? > > > > Dropping references to it, so that it can be freed. AudioBuffers are > > potentially huge objects. > > Can make it so that when stop() occurs, the buffer is dropped by the node? > It should be functionally equivalent and not observable. We can't do that, since start() may be called again, right? > > > The spec resolution intent was that once a buffer is set, you don't touch > > > the buffer property anymore. If you want silence, you can stop(), but that's > > > it. > > > > Yes, but I think being able to drop all references to the buffer is > > important. Why can't we make null a special case? > > I'm not opposed to it, but it's incompatible (although not in a > compatibility-breaking manner) with all the content out there. For > reference, blink refuse to set `.buffer` to something that is not an > `AudioBuffer`, and currently warns when you're setting an `AudioBuffer` for > the second time. Not setting .buffer to an AudioBuffer a second time makes sense. I'm objecting to not being able to set it to null, especially since it is marked as nullable in the spec: <http://webaudio.github.io/web-audio-api/#widl-AudioBufferSourceNode-buffer> We can't have it both ways, this either needs to not be nullable, or we should be able to assign it to null. I don't know of any other nullable property that can't be set to null in some case. Not sure how the part about Blink not being able to set .buffer to something that is not an AudioBuffer is related. I'm not suggesting that we should do that. :-)
Reporter | ||
Comment 12•9 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #11) > (In reply to Paul Adenot (:padenot) from comment #10) > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > > comment #9) > > > (In reply to Paul Adenot (:padenot) from comment #8) > > > > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > > > > comment #7) > > > > > ::: dom/media/webaudio/test/test_bug1161025.html > > > > > @@ +15,5 @@ > > > > > > + var ctx = new AudioContext(); > > > > > > + var source = ctx.createBufferSource(); > > > > > > + var b0 = ctx.createBuffer(32,798,22050); > > > > > > + var b1 = ctx.createBuffer(32,28,22050); > > > > > > + source.buffer = null; > > > > > > > > > > What is the desired behavior for setting the buffer to null? As things > > > > > stand right now, the only reason why this test passes is that the set to > > > > > null occurs first, before the set to b0. However, this patch makes it > > > > > impossible to null out the buffer once it's set, which sounds like the wrong > > > > > thing to do. > > > > > > > > > > I think the right thing to do would be to prevent buffer to be set twice to > > > > > a non-null value. > > > > > > > > > > r- because of this. > > > > > > > > What would be the behaviour or use-case for setting the buffer to an > > > > AudioBuffer, and then nulling it out? Ignoring setting it to null? > > > > Outputting silence? > > > > > > Dropping references to it, so that it can be freed. AudioBuffers are > > > potentially huge objects. > > > > Can make it so that when stop() occurs, the buffer is dropped by the node? > > It should be functionally equivalent and not observable. > > We can't do that, since start() may be called again, right? No, this is one-shot. One start, one stop, you're done, you need to create a new node with the same buffer to play the buffer again. If you call start() again, `InvalidStateError` is thrown. > > > > > The spec resolution intent was that once a buffer is set, you don't touch > > > > the buffer property anymore. If you want silence, you can stop(), but that's > > > > it. > > > > > > Yes, but I think being able to drop all references to the buffer is > > > important. Why can't we make null a special case? > > > > I'm not opposed to it, but it's incompatible (although not in a > > compatibility-breaking manner) with all the content out there. For > > reference, blink refuse to set `.buffer` to something that is not an > > `AudioBuffer`, and currently warns when you're setting an `AudioBuffer` for > > the second time. > > Not setting .buffer to an AudioBuffer a second time makes sense. I'm > objecting to not being able to set it to null, especially since it is marked > as nullable in the spec: > <http://webaudio.github.io/web-audio-api/#widl-AudioBufferSourceNode-buffer> That can change if it does not make sense, I know a guy :-) > We can't have it both ways, this either needs to not be nullable, or we > should be able to assign it to null. I don't know of any other nullable > property that can't be set to null in some case. > > Not sure how the part about Blink not being able to set .buffer to something > that is not an AudioBuffer is related. I'm not suggesting that we should do > that. :-) Agreed that something is going to change. This is all pretty safe as far as backward compatibility is concerned. Considering allowing setting `buffer` to null while the AudioBufferSourceNode is playing adds more code and more things to spec (since we need to spec what happens when you null the buffer attribute while the node is playing), I think I'd rather make `buffer` non nullable (in light of the fact that you can't re-play an AudioBufferSourceNode). I don't feel strongly about a particular solution, though.
Comment 13•9 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #12) > > > Can make it so that when stop() occurs, the buffer is dropped by the node? > > > It should be functionally equivalent and not observable. > > > > We can't do that, since start() may be called again, right? > > No, this is one-shot. One start, one stop, you're done, you need to create a > new node with the same buffer to play the buffer again. If you call start() > again, `InvalidStateError` is thrown. Oh, right, you're completely right. My bad. > > Not setting .buffer to an AudioBuffer a second time makes sense. I'm > > objecting to not being able to set it to null, especially since it is marked > > as nullable in the spec: > > <http://webaudio.github.io/web-audio-api/#widl-AudioBufferSourceNode-buffer> > > That can change if it does not make sense, I know a guy :-) Heh, good :D > > We can't have it both ways, this either needs to not be nullable, or we > > should be able to assign it to null. I don't know of any other nullable > > property that can't be set to null in some case. > > > > Not sure how the part about Blink not being able to set .buffer to something > > that is not an AudioBuffer is related. I'm not suggesting that we should do > > that. :-) > > Agreed that something is going to change. This is all pretty safe as far as > backward compatibility is concerned. > > Considering allowing setting `buffer` to null while the > AudioBufferSourceNode is playing adds more code and more things to spec > (since we need to spec what happens when you null the buffer attribute while > the node is playing), I think I'd rather make `buffer` non nullable (in > light of the fact that you can't re-play an AudioBufferSourceNode). I don't > feel strongly about a particular solution, though. I think making it non-nullable is hard, because we need to be able to return something from the property when you read it before setting it for the first time, right? What would you like to return? Also, making it non-nullable is backwards incompatible, although we can hope that content doesn't rely on it yet. But the first problem remains.
Reporter | ||
Comment 14•9 years ago
|
||
Yeah I think you're right. Thomas, would you mind updating the patch to reflect Ehsan's last message? It should be possible le to set it to null after a buffer has been set.
Assignee | ||
Comment 15•9 years ago
|
||
Reporter | ||
Comment 16•9 years ago
|
||
Comment on attachment 8612743 [details] [diff] [review] patch for 1161025 Review of attachment 8612743 [details] [diff] [review]: ----------------------------------------------------------------- Ehsan, that should do it, care to have a look ? ::: dom/media/webaudio/AudioBufferSourceNode.h @@ +54,5 @@ > + if (aBuffer != nullptr && mBuffer != nullptr) { > + aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); > + return; > + } > + nit: trailing space ::: dom/media/webaudio/test/test_bug1161025.html @@ +34,5 @@ > + } > + > + try { > + source.buffer = b1; > + ok(true, 'No exception thrown'); This should not work. You're supposed to not be able to set another buffer after nulling it, hence the title of the bug :-) @@ +38,5 @@ > + ok(true, 'No exception thrown'); > + } catch(e) { > + ok(false, 'Should not get exception'); > + } > + nit: lots of trailing spaces
Assignee | ||
Comment 17•9 years ago
|
||
Sorry for all the trailing spaces!
Reporter | ||
Comment 18•9 years ago
|
||
Comment on attachment 8612847 [details] [diff] [review] Patch for 1161025 Review of attachment 8612847 [details] [diff] [review]: ----------------------------------------------------------------- Ehsan, that should implement what we talked about.
Comment 19•9 years ago
|
||
Comment on attachment 8612847 [details] [diff] [review] Patch for 1161025 Review of attachment 8612847 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this is exactly what I had in mind! ::: dom/media/webaudio/AudioBufferSourceNode.h @@ +53,2 @@ > { > + if (aBuffer != nullptr && mBufferSet) { Nit: Just say: |aBuffer && mBufferSet|
Assignee | ||
Comment 20•9 years ago
|
||
Reporter | ||
Comment 21•9 years ago
|
||
So, the WG just decided it does not make sense to re-set it to null and that it should throw, but I said to them that if it's nullable it should be possible to null it. I'm going to look at the WebIDL spec to check if it's legal to interpose some custom behaviour here to thrown when it's set again to null.
Reporter | ||
Comment 22•9 years ago
|
||
NI to myself so I think to verify this WebIDL thing.
Comment 23•9 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #21) > So, the WG just decided it does not make sense to re-set it to null and that > it should throw, but I said to them that if it's nullable it should be > possible to null it. I'm going to look at the WebIDL spec to check if it's > legal to interpose some custom behaviour here to thrown when it's set again > to null. Oh, that sounds awful. Does the working group pay attention to the fact that making these APIs work differently than the same stuff in every other API in the Web platform is bad form? :(
Reporter | ||
Comment 24•9 years ago
|
||
yeah that's what I thought, and why I expressed reserve regarding this one. I'm going to go complain.
Reporter | ||
Comment 25•9 years ago
|
||
Comment on attachment 8613521 [details] [diff] [review] 1161025.diff (clearing the review, we're waiting on the WG)
Comment 26•9 years ago
|
||
Paul - WG status? Still waiting? Thanks
Updated•9 years ago
|
Updated•9 years ago
|
Reporter | ||
Comment 27•9 years ago
|
||
We're chatting about this at TPAC in two weeks. https://github.com/WebAudio/web-audio-api/issues/436
Comment 28•7 years ago
|
||
Mass change P2->P3 to align with new Mozilla triage process.
Reporter | ||
Comment 29•4 years ago
|
||
Ok so we've decided a while back what to do: it's allowed to set those attributes it to null
. I'm going to rebase https://bugzilla.mozilla.org/attachment.cgi?id=8613521 and land, since I didn't write it and it implements the correct behaviour.
Reporter | ||
Comment 30•4 years ago
|
||
Reporter | ||
Comment 31•4 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #29)
Ok so we've decided a while back what to do: it's allowed to set those attributes it to
null
. I'm going to rebase https://bugzilla.mozilla.org/attachment.cgi?id=8613521 and land, since I didn't write it and it implements the correct behaviour.
This was in fact missing an edge case.
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 32•4 years ago
|
||
Pushed by padenot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e3b626560b0d reject attempts to set non-null AudioBufferSourceNode.buffer more than once r=karlt,baku
Comment 33•4 years ago
|
||
Backed out changeset e3b626560b0d (bug 1161025) for causing crashtest failures on 966636.html.
Backout revision https://hg.mozilla.org/integration/autoland/rev/33d4b89801dabda575589825f3aa9b4bb6c07818
Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=287294351&repo=autoland
Paul can you please take a look?
Updated•4 years ago
|
Updated•4 years ago
|
Comment 34•4 years ago
|
||
Pushed by padenot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/70bdc769f150 Reject attempts to set non-null AudioBufferSourceNode.buffer more than once r=karlt,baku
Comment 35•4 years ago
|
||
bugherder |
Reporter | ||
Updated•4 years ago
|
Description
•