Prevent AudioBufferSourceNode.buffer to be set more than once

ASSIGNED
Assigned to

Status

()

P3
normal
Rank:
25
ASSIGNED
4 years ago
a year ago

People

(Reporter: padenot, Assigned: u538564)

Tracking

({dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [spec])

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Comment 1

4 years ago
Hi Paul, I would like to work on this bug
(Assignee)

Comment 2

4 years ago
Created attachment 8609250 [details] [diff] [review]
patch for 1161025
Attachment #8609250 - Flags: review?(padenot)
(Reporter)

Comment 3

4 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.
Attachment #8609250 - Flags: review?(padenot)
(Assignee)

Comment 4

4 years ago
Created attachment 8609329 [details] [diff] [review]
patch for 1161025 with corrections
Attachment #8609329 - Flags: review?(padenot)
(Reporter)

Comment 5

4 years ago
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+
(Reporter)

Comment 6

4 years ago
Created attachment 8610044 [details] [diff] [review]
Prevent AudioBufferSourceNode.buffer to be set more than once

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)
(Reporter)

Updated

4 years ago
Assignee: nobody → padenot
Status: NEW → ASSIGNED
(Reporter)

Updated

4 years ago
Assignee: padenot → tesc.bugzilla

Comment 7

4 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.
Attachment #8610044 - Flags: review?(ehsan) → review-
(Reporter)

Comment 8

4 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.
Flags: needinfo?(ehsan)

Comment 9

4 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?
Flags: needinfo?(ehsan)
(Reporter)

Comment 10

4 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

4 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

4 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.
Flags: needinfo?(ehsan)

Comment 13

4 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.
Flags: needinfo?(ehsan)
(Reporter)

Comment 14

4 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.
Flags: needinfo?(tesc.bugzilla)
(Assignee)

Comment 15

4 years ago
Created attachment 8612743 [details] [diff] [review]
patch for 1161025
Attachment #8609250 - Attachment is obsolete: true
Attachment #8609329 - Attachment is obsolete: true
Flags: needinfo?(tesc.bugzilla)
Attachment #8612743 - Flags: review?(padenot)
(Reporter)

Comment 16

4 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
Attachment #8612743 - Flags: review?(padenot) → review-
(Assignee)

Comment 17

4 years ago
Created attachment 8612847 [details] [diff] [review]
Patch for 1161025

Sorry for all the trailing spaces!
Attachment #8612743 - Attachment is obsolete: true
Attachment #8612847 - Flags: review?(padenot)
(Reporter)

Comment 18

4 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.
Attachment #8612847 - Flags: review?(padenot)
Attachment #8612847 - Flags: review?(ehsan)
Attachment #8612847 - Flags: review+

Comment 19

4 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|
Attachment #8612847 - Flags: review?(ehsan) → review+
(Assignee)

Comment 20

4 years ago
Created attachment 8613521 [details] [diff] [review]
1161025.diff
Attachment #8612847 - Attachment is obsolete: true
Attachment #8613521 - Flags: review?(padenot)
(Reporter)

Comment 21

4 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

4 years ago
NI to myself so I think to verify this WebIDL thing.
Flags: needinfo?(padenot)

Comment 23

4 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

4 years ago
yeah that's what I thought, and why I expressed reserve regarding this one. I'm going to go complain.
Flags: needinfo?(padenot)
(Reporter)

Comment 25

4 years ago
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]
Keywords: dev-doc-needed
(Reporter)

Comment 27

3 years ago
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
You need to log in before you can comment on or make changes to this bug.