Bug 78758 – DelayNode has a fixed one second max delay time

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Rogers]

See DelayNode specification:
https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#DelayNode-section

One possible solution (but needs more thought) is to make the AudioContext method createDelayNode() take an optional max delay time parameter.

[Raymond Toy]

(In reply to comment #0)
> See DelayNode specification:
> https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#DelayNode-section
> 
> One possible solution (but needs more thought) is to make the AudioContext method createDelayNode() take an optional max delay time parameter.

Seems like a reasonable thing to do.  I'll look into it.

[Raymond Toy]

Created attachment 127465 [details]
Patch

[Chris Rogers]

Comment on attachment 127465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127465&action=review

Looks pretty good overall, but I have a few comments:

> Source/WebCore/webaudio/AudioContext.h:116
> +    PassRefPtr<DelayNode> createDelayNode(double maxDelayTime = 1);

It's unfortunate to have a magic constant 1 here, where we used to have a constant (now removed in this patch) DefaultMaxDelayTime

Just out of curiousness, what happens if you remove the default value here, and then don't pass a value in the JS code?
    var delay = context.createDelayNode();

> LayoutTests/webaudio/delaynode-scheduling.html:33
> +    var delay = context.createDelayNode(2);

Can we please leave this unchanged, since the normal case for testing is to pass no arguments

> LayoutTests/webaudio/delaynode.html:33
> +    var delay = context.createDelayNode(2);

I think it's best to leave this test unchanged, since the default use is to *not* pass a maxDelayTime as an argument, and we have to test that case.

Instead, I recommend cloning this test, and call it "delay-testing-maxdelay.html" with this change, and also explicitly setting delayTimeSeconds = 1.5

Does that make sense?

> LayoutTests/webaudio/resources/delay-testing.js:4
> +var delayTimeSeconds = 1.5;

Can we leave this value at 0.5 here, then reset the value to 1.5 *only* in a new test called "delay-testing-maxdelay.html"
which will essentially be the same as the old "delay-testing.html" except it will pass a max value in createDelayNode() and reset delayTimeSeconds to the higher value?

[Raymond Toy]

Created attachment 129284 [details]
Patch

[Raymond Toy]

Comment on attachment 127465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127465&action=review

>> Source/WebCore/webaudio/AudioContext.h:116
>> +    PassRefPtr<DelayNode> createDelayNode(double maxDelayTime = 1);
> 
> It's unfortunate to have a magic constant 1 here, where we used to have a constant (now removed in this patch) DefaultMaxDelayTime
> 
> Just out of curiousness, what happens if you remove the default value here, and then don't pass a value in the JS code?
>     var delay = context.createDelayNode();

Doesn't compile because the generated javascript creates a different function signature.

How about if I move the old DefaultMaxDelayTime from DelayDSPProcessor to AudioContext?

>> LayoutTests/webaudio/delaynode-scheduling.html:33
>> +    var delay = context.createDelayNode(2);
> 
> Can we please leave this unchanged, since the normal case for testing is to pass no arguments

Reverted.

>> LayoutTests/webaudio/delaynode.html:33
>> +    var delay = context.createDelayNode(2);
> 
> I think it's best to leave this test unchanged, since the default use is to *not* pass a maxDelayTime as an argument, and we have to test that case.
> 
> Instead, I recommend cloning this test, and call it "delay-testing-maxdelay.html" with this change, and also explicitly setting delayTimeSeconds = 1.5
> 
> Does that make sense?

Change reverted and added new test file to test the longer delay.

>> LayoutTests/webaudio/resources/delay-testing.js:4
>> +var delayTimeSeconds = 1.5;
> 
> Can we leave this value at 0.5 here, then reset the value to 1.5 *only* in a new test called "delay-testing-maxdelay.html"
> which will essentially be the same as the old "delay-testing.html" except it will pass a max value in createDelayNode() and reset delayTimeSeconds to the higher value?

Done.

[Chris Rogers]

Comment on attachment 129284 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129284&action=review

> Source/WebCore/webaudio/AudioContext.h:46
> +const double defaultMaxDelayTime = 1;

We can't define constants in the header file like this.  They must be defined in a .cpp file for proper namespace/linkage

> Source/WebCore/webaudio/AudioContext.h:118
> +    PassRefPtr<DelayNode> createDelayNode(double maxDelayTime = defaultMaxDelayTime);

How about this idea:

Create two versions of createDelayNode(), one which takes no argument and one which takes the maxDelayTime argument.
Then, in the .cpp file you can have one call the other with a method-local constant "defaultMaxDelayTime"

> LayoutTests/webaudio/delaynode-maxdelay-expected.txt:1
> +Tests basic functionality of DelayNode.

This message needs to change, saying something about max-delay.

> LayoutTests/webaudio/delaynode-maxdelay.html:15
> +description("Tests basic functionality of DelayNode.");

This message needs to change, saying something about max-delay.

> LayoutTests/webaudio/delaynode-maxdelay.html:32
> +

Please add comment here saying that we "explicitly" create the node with the given max-delay time.

> LayoutTests/webaudio/delaynode-maxdelay.html:33
> +    var delay = context.createDelayNode(2);

Please add comment here saying that we "explicitly" choose a delay time greater than the default delay time of 1 second to test that our max-delay time is respected.

[Raymond Toy]

(In reply to comment #6)
> (From update of attachment 129284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129284&action=review
> 
> > Source/WebCore/webaudio/AudioContext.h:46
> > +const double defaultMaxDelayTime = 1;
> 
> We can't define constants in the header file like this.  They must be defined in a .cpp file for proper namespace/linkage

It compiled fine, but maybe it's not in the namespace you want.  Is that what you mean?
> 
> > Source/WebCore/webaudio/AudioContext.h:118
> > +    PassRefPtr<DelayNode> createDelayNode(double maxDelayTime = defaultMaxDelayTime);
> 
> How about this idea:
> 
> Create two versions of createDelayNode(), one which takes no argument and one which takes the maxDelayTime argument.
> Then, in the .cpp file you can have one call the other with a method-local constant "defaultMaxDelayTime"

This works.

[Raymond Toy]

Created attachment 129468 [details]
Patch

[Raymond Toy]

Comment on attachment 129284 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129284&action=review

>>> Source/WebCore/webaudio/AudioContext.h:118
>>> +    PassRefPtr<DelayNode> createDelayNode(double maxDelayTime = defaultMaxDelayTime);
>> 
>> How about this idea:
>> 
>> Create two versions of createDelayNode(), one which takes no argument and one which takes the maxDelayTime argument.
>> Then, in the .cpp file you can have one call the other with a method-local constant "defaultMaxDelayTime"
> 
> This works.

Done.

>> LayoutTests/webaudio/delaynode-maxdelay-expected.txt:1
>> +Tests basic functionality of DelayNode.
> 
> This message needs to change, saying something about max-delay.

Done.

>> LayoutTests/webaudio/delaynode-maxdelay.html:15
>> +description("Tests basic functionality of DelayNode.");
> 
> This message needs to change, saying something about max-delay.

Done.

>> LayoutTests/webaudio/delaynode-maxdelay.html:32
>> +
> 
> Please add comment here saying that we "explicitly" create the node with the given max-delay time.

Done.

>> LayoutTests/webaudio/delaynode-maxdelay.html:33
>> +    var delay = context.createDelayNode(2);
> 
> Please add comment here saying that we "explicitly" choose a delay time greater than the default delay time of 1 second to test that our max-delay time is respected.

Done.

[Chris Rogers]

Comment on attachment 129468 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129468&action=review

Looks good overall with one comment:

> Source/WebCore/webaudio/AudioContext.cpp:449
> +    

I'm not sure why you don't simply call:

return createDelayNode(defaultMaxDelayTime) instead of lines 450:452

[Raymond Toy]

Created attachment 129548 [details]
Patch

[Raymond Toy]

Comment on attachment 129468 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129468&action=review

>> Source/WebCore/webaudio/AudioContext.cpp:449
>> +    
> 
> I'm not sure why you don't simply call:
> 
> return createDelayNode(defaultMaxDelayTime) instead of lines 450:452

Yeah, that's much better.  Done.

[Chris Rogers]

Comment on attachment 129548 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 129548 [details]
Patch

Rejecting attachment 129548 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Chris Rogers]

Comment on attachment 129548 [details]
Patch

Re-trying commit queue - committers.py seems fine...

[WebKit Review Bot]

Comment on attachment 129548 [details]
Patch

Rejecting attachment 129548 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t line 164.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 9
Updating OpenSource
From git://git.webkit.org/WebKit
   60dd0f4..2366bf5  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 109402 = b1cd2e5d03d1f0c3fa07b74ca5f82e76fc87a604
last_rev is higher!: 109402 >= 109304 at /usr/lib/git-core/git-svn line 1528

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/11779601

[Raymond Toy]

(In reply to comment #15)
> (From update of attachment 129548 [details])
> Re-trying commit queue - committers.py seems fine...

Still didn't work.  Can you try this again when you get a chance?

[Chris Rogers]

(In reply to comment #17)
> (In reply to comment #15)
> > (From update of attachment 129548 [details] [details])
> > Re-trying commit queue - committers.py seems fine...
> 
> Still didn't work.  Can you try this again when you get a chance?

It looks like the patch might not have applied cleanly (purple bot).  Could you re-upload the patch and I can try again?

[Raymond Toy]

Created attachment 129943 [details]
Patch

[WebKit Review Bot]

Comment on attachment 129943 [details]
Patch

Rejecting attachment 129943 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
_by_email
    return self._reviewer_only(self.account_by_email(email))
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/config/committers.py", line 632, in account_by_email
    return self._email_to_account_map().get(email.lower()) if email else None
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/config/committers.py", line 525, in _email_to_account_map
    assert(email not in self._accounts_by_email)  # We should never have duplicate emails.
AssertionError

Full output: http://queues.webkit.org/results/11807027

[Eric Seidel (no email)]

Someone borked the committers.py file methinks.

[WebKit Review Bot]

Comment on attachment 129943 [details]
Patch

Clearing flags on attachment: 129943

Committed r109666: <http://trac.webkit.org/changeset/109666>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.