Closed Bug 1161025 Opened 9 years ago Closed 4 years ago

Prevent AudioBufferSourceNode.buffer to be set more than once

Categories

(Core :: Web Audio, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla74
Tracking Status
firefox74 --- fixed

People

(Reporter: padenot, Assigned: u538564)

Details

(Keywords: dev-doc-needed, Whiteboard: [spec])

Attachments

(3 files, 4 obsolete files)

Hi Paul, I would like to work on this bug
Attached patch patch for 1161025 (obsolete) — Splinter Review
Attachment #8609250 - Flags: review?(padenot)
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.
Attachment #8609250 - Flags: review?(padenot)
Attachment #8609329 - Flags: review?(padenot)
Comment on attachment 8609329 [details] [diff] [review]
patch for 1161025 with corrections

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

Cool ! Thanks !
Attachment #8609329 - Flags: review?(padenot) → review+
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.
Attachment #8610044 - Flags: review?(ehsan)
Assignee: nobody → padenot
Status: NEW → ASSIGNED
Assignee: padenot → tesc.bugzilla
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.
Attachment #8610044 - Flags: review?(ehsan) → review-
(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.
Flags: needinfo?(ehsan)
(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?
Flags: needinfo?(ehsan)
(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.
(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.  :-)
(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.
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(tesc.bugzilla)
Attached patch patch for 1161025 (obsolete) — Splinter Review
Attachment #8609250 - Attachment is obsolete: true
Attachment #8609329 - Attachment is obsolete: true
Flags: needinfo?(tesc.bugzilla)
Attachment #8612743 - Flags: review?(padenot)
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
Attachment #8612743 - Flags: review?(padenot) → review-
Attached patch Patch for 1161025 (obsolete) — Splinter Review
Sorry for all the trailing spaces!
Attachment #8612743 - Attachment is obsolete: true
Attachment #8612847 - Flags: review?(padenot)
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.
Attachment #8612847 - Flags: review?(padenot)
Attachment #8612847 - Flags: review?(ehsan)
Attachment #8612847 - Flags: review+
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|
Attachment #8612847 - Flags: review?(ehsan) → review+
Attached patch 1161025.diffSplinter Review
Attachment #8612847 - Attachment is obsolete: true
Attachment #8613521 - Flags: review?(padenot)
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.
NI to myself so I think to verify this WebIDL thing.
Flags: needinfo?(padenot)
(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?  :(
yeah that's what I thought, and why I expressed reserve regarding this one. I'm going to go complain.
Flags: needinfo?(padenot)
Comment on attachment 8613521 [details] [diff] [review]
1161025.diff

(clearing the review, we're waiting on the WG)
Attachment #8613521 - Flags: review?(padenot)
Paul - WG status?  Still waiting?  Thanks
Flags: needinfo?(padenot)
Whiteboard: [spec]
Rank: 25
We're chatting about this at TPAC in two weeks.

https://github.com/WebAudio/web-audio-api/issues/436
Flags: needinfo?(padenot)
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3

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.

Flags: needinfo?(padenot)

(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.

Flags: needinfo?(padenot)
Attachment #9118786 - Attachment description: Bug 1161025 - Align AudioBufferSourceNode.buffer setter witht the spec. r?karlt → Bug 1161025 - reject attempts to set non-null AudioBufferSourceNode.buffer more than once r?karlt
Attachment #9118786 - Attachment description: Bug 1161025 - reject attempts to set non-null AudioBufferSourceNode.buffer more than once r?karlt → Bug 1161025 - Align AudioBufferSourceNode.buffer setter witht the spec. r?karlt
Attachment #9118786 - Attachment description: Bug 1161025 - Align AudioBufferSourceNode.buffer setter witht the spec. r?karlt → Bug 1161025 - reject attempts to set non-null AudioBufferSourceNode.buffer more than once r?karlt
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
Attachment #9118786 - Attachment description: Bug 1161025 - reject attempts to set non-null AudioBufferSourceNode.buffer more than once r?karlt → Bug 1161025 - Reject attempts to set non-null AudioBufferSourceNode.buffer more than once r?karlt
Attachment #9118786 - Attachment description: Bug 1161025 - Reject attempts to set non-null AudioBufferSourceNode.buffer more than once r?karlt → Bug 1161025 - Reject attempts to set non-null AudioBufferSourceNode.buffer more than once r?karlt,baku
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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
Flags: needinfo?(padenot)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: