Michael Wilson (Gerrit)

unread,

Jun 17, 2025, 7:21:44 PMJun 17

to Punith Nayak, Alvin Ji, Adam Raine, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Alvin Ji and Punith Nayak

Michael Wilson added 8 comments

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Michael Wilson . resolved

Looking good overall, please see other comments.

File third_party/blink/web_tests/webaudio/AudioParam/audioparam-setTargetAtTime-limit.html

Line 18, Patchset 3 (Latest): function assert_arrays_close(actual, expected, opts, desc) {

Michael Wilson . unresolved

Can we adapt `assert_array_approx_equals` from testharness.js?

https://crsrc.org/c/third_party/blink/web_tests/resources/testharness.js;l=1712

Line 20, Patchset 3 (Parent):


        // A really short time constant so that setTargetAtTime approaches the
        // limiting value well before the end of the test.

Michael Wilson . unresolved

Let's keep this comment too.

Line 24, Patchset 3 (Parent):


        // Find the time where setTargetAtTime is close enough to the limit.
        // Since we're approaching 1, use a value of eps smaller than
        // kSetTargetThreshold (1.5e-6) in AudioParamTimeline.cpp.  This is to
        // account for round-off in the actual implementation (which uses a
        // filter and not the formula.)

Michael Wilson . unresolved

Let's keep some form of this comment, maybe the below:
```
        // Find the time where setTargetAtTime is close enough to the limit.
        // Since we're approaching 1, use a value of eps smaller than
        // the target 1 threshold (1.5e-6).  This is to
        // account for round-off in the actual implementation (which uses a
        // filter and not the formula.)
```

Line 45, Patchset 3 (Parent):

        // Use the equation for setTargetAtTime to figure out when we are close
        // to 0:
        //
        //   v(t) = exp(-t/tau)
        //
        // So find t such that exp(-t/tau) <= eps.  Thus t >= - tau * log(eps).
        //
        // For eps, use exp(-10).

Michael Wilson . unresolved

Let's keep this comment.

Line 56, Patchset 3 (Parent):

        // A really short time constant so that setTargetAtTime approaches the
        // limiting value well before the end of the test.

Michael Wilson . unresolved

Let's keep this comment.

Line 60, Patchset 3 (Parent):

        // Find the time where setTargetAtTime is close enough to the limit.
        // Since we're approaching 0, use a value of eps smaller than
        // kSetTargetZeroThreshold (1e-20) in AudioParamTimeline.cpp.  This is
        // to account for round-off in the actual implementation (which uses a
        // filter and not the formula.)

Michael Wilson . unresolved

I'd like to keep some form of this comment, but the values are outdated.  Maybe the below:
```
        // Find the time where setTargetAtTime is close enough to the limit.
        // Since we're approaching 0, use a value of eps smaller than
        // the target zero threshold (1e-20).  This is
        // to account for round-off in the actual implementation (which uses a
        // filter and not the formula.)
```

Line 73, Patchset 3 (Parent): // Experimentally determined

Michael Wilson . unresolved

Let's keep this comment.

Open in Gerrit

Related details

Attention is currently required from:

Alvin Ji
Punith Nayak

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Punith Nayak (Gerrit)

unread,

Jun 20, 2025, 5:31:52 PMJun 20

to Alvin Ji, Adam Raine, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Alvin Ji and Michael Wilson

Punith Nayak added 7 comments

File third_party/blink/web_tests/webaudio/AudioParam/audioparam-setTargetAtTime-limit.html

Line 18, Patchset 3: function assert_arrays_close(actual, expected, opts, desc) {

Michael Wilson . resolved

Can we adapt `assert_array_approx_equals` from testharness.js?
https://crsrc.org/c/third_party/blink/web_tests/resources/testharness.js;l=1712

Punith Nayak

Done

Line 20, Patchset 3 (Parent):


        // A really short time constant so that setTargetAtTime approaches the
        // limiting value well before the end of the test.

Michael Wilson . resolved

Let's keep this comment too.

Punith Nayak

Done

Line 24, Patchset 3 (Parent):


        // Find the time where setTargetAtTime is close enough to the limit.
        // Since we're approaching 1, use a value of eps smaller than
        // kSetTargetThreshold (1.5e-6) in AudioParamTimeline.cpp.  This is to
        // account for round-off in the actual implementation (which uses a
        // filter and not the formula.)

Michael Wilson . resolved

Let's keep some form of this comment, maybe the below:
```
        // Find the time where setTargetAtTime is close enough to the limit.
        // Since we're approaching 1, use a value of eps smaller than
        // the target 1 threshold (1.5e-6).  This is to
        // account for round-off in the actual implementation (which uses a
        // filter and not the formula.)
```

Punith Nayak

Done

Line 45, Patchset 3 (Parent):

        // Use the equation for setTargetAtTime to figure out when we are close
        // to 0:
        //
        //   v(t) = exp(-t/tau)
        //
        // So find t such that exp(-t/tau) <= eps.  Thus t >= - tau * log(eps).
        //
        // For eps, use exp(-10).

Michael Wilson . resolved

Let's keep this comment.

Punith Nayak

Done

Line 56, Patchset 3 (Parent):

        // A really short time constant so that setTargetAtTime approaches the
        // limiting value well before the end of the test.

Michael Wilson . resolved

Let's keep this comment.

Punith Nayak

Done

Line 60, Patchset 3 (Parent):

        // Find the time where setTargetAtTime is close enough to the limit.
        // Since we're approaching 0, use a value of eps smaller than
        // kSetTargetZeroThreshold (1e-20) in AudioParamTimeline.cpp.  This is
        // to account for round-off in the actual implementation (which uses a
        // filter and not the formula.)

Michael Wilson . resolved

I'd like to keep some form of this comment, but the values are outdated.  Maybe the below:
```
        // Find the time where setTargetAtTime is close enough to the limit.
        // Since we're approaching 0, use a value of eps smaller than
        // the target zero threshold (1e-20).  This is
        // to account for round-off in the actual implementation (which uses a
        // filter and not the formula.)
```

Punith Nayak

Done

Line 73, Patchset 3 (Parent): // Experimentally determined

Michael Wilson . resolved

Let's keep this comment.

Punith Nayak

Done

Open in Gerrit

Related details

Attention is currently required from:

Alvin Ji
Michael Wilson

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Michael Wilson (Gerrit)

unread,

Jun 20, 2025, 5:47:27 PMJun 20

to Punith Nayak, Alvin Ji, Adam Raine, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Alvin Ji and Punith Nayak

Michael Wilson voted and added 5 comments

Votes added by Michael Wilson

Code-Review

+1

5 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Michael Wilson . resolved

LGTM with nits.

File third_party/blink/web_tests/webaudio/AudioParam/audioparam-setTargetAtTime-limit.html

Line 103, Patchset 5 (Latest): // Use the equation for setTargetAtTime to figure out when we are close


      // to 0:
      //
      //   v(t) = exp(-t/tau)
      //
      // So find t such that exp(-t/tau) <= eps.  Thus t >= - tau * log(eps).
      //
      // For eps, use exp(-10).

Michael Wilson . unresolved

Nit: since this comment is talking about approaching zero, it might be in better context if it was moved before line 145.

Line 112, Patchset 5 (Latest): let sampleRate = 48000;

Michael Wilson . unresolved

Nit: since there is another variable with the same name in the last promise test, and since this value is only used twice, I think it might be more clear to put the value directly in the tests. But it's up to you.

Line 129, Patchset 5 (Latest): sampleRate: sampleRate,

Michael Wilson . unresolved

See other comment
```suggestion
          sampleRate: 48000,
```

Line 148, Patchset 5 (Latest): sampleRate: sampleRate,

Michael Wilson . unresolved

See other comment
```suggestion
          sampleRate: 48000,
```

Open in Gerrit

Related details

Attention is currently required from:

Alvin Ji
Punith Nayak

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Punith Nayak (Gerrit)

unread,

Jun 20, 2025, 6:29:14 PMJun 20

to Alvin Ji, Adam Raine, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Alvin Ji and Michael Wilson

Punith Nayak added 4 comments

File third_party/blink/web_tests/webaudio/AudioParam/audioparam-setTargetAtTime-limit.html

Line 103, Patchset 5: // Use the equation for setTargetAtTime to figure out when we are close


      // to 0:
      //
      //   v(t) = exp(-t/tau)
      //
      // So find t such that exp(-t/tau) <= eps.  Thus t >= - tau * log(eps).
      //
      // For eps, use exp(-10).

Michael Wilson . resolved

Nit: since this comment is talking about approaching zero, it might be in better context if it was moved before line 145.

Punith Nayak

Done

Line 112, Patchset 5: let sampleRate = 48000;

Michael Wilson . resolved

Nit: since there is another variable with the same name in the last promise test, and since this value is only used twice, I think it might be more clear to put the value directly in the tests. But it's up to you.

Punith Nayak

Done

Line 129, Patchset 5: sampleRate: sampleRate,

Michael Wilson . resolved

See other comment
```suggestion
          sampleRate: 48000,
```

Punith Nayak

Done

Line 148, Patchset 5: sampleRate: sampleRate,

Michael Wilson . resolved

See other comment
```suggestion
          sampleRate: 48000,
```

Punith Nayak

Done

Open in Gerrit

Related details

Attention is currently required from:

Alvin Ji
Michael Wilson

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Michael Wilson (Gerrit)

unread,

Jun 20, 2025, 6:29:46 PMJun 20

to Punith Nayak, Alvin Ji, Adam Raine, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Alvin Ji and Punith Nayak

Michael Wilson voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Alvin Ji
Punith Nayak

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Alvin Ji (Gerrit)

unread,

Jun 20, 2025, 7:55:57 PMJun 20

to Punith Nayak, Adam Raine, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Punith Nayak

Alvin Ji voted and added 3 comments

Votes added by Alvin Ji

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Alvin Ji . resolved

LGTM with nits

File third_party/blink/web_tests/webaudio/AudioParam/audioparam-setTargetAtTime-limit.html

Line 79, Patchset 6 (Latest): const msg = 'setTargetAtTime(' + options.v1 +

Alvin Ji . unresolved

nit: let's avoid using abbreviation, use `message` instead.

Line 168, Patchset 6 (Latest): const src = new ConstantSourceNode(context);

Alvin Ji . unresolved

ditto: using `source` for source node.

Open in Gerrit

Related details

Attention is currently required from:

Punith Nayak

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Punith Nayak (Gerrit)

unread,

Jun 24, 2025, 7:42:29 PMJun 24

to Alvin Ji, Adam Raine, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Alvin Ji and Michael Wilson

Punith Nayak added 2 comments

File third_party/blink/web_tests/webaudio/AudioParam/audioparam-setTargetAtTime-limit.html

Line 79, Patchset 6: const msg = 'setTargetAtTime(' + options.v1 +

Alvin Ji . resolved

nit: let's avoid using abbreviation, use `message` instead.

Punith Nayak

Done

Line 168, Patchset 6: const src = new ConstantSourceNode(context);

Alvin Ji . resolved

ditto: using `source` for source node.

Punith Nayak

Done

Open in Gerrit

Related details

Attention is currently required from:

Alvin Ji
Michael Wilson

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Alvin Ji (Gerrit)

unread,

Jun 25, 2025, 6:35:06 AMJun 25

to Punith Nayak, Adam Raine, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Adam Raine, Michael Wilson and Punith Nayak

Alvin Ji voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Adam Raine
Michael Wilson
Punith Nayak

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Adam Raine (Gerrit)

unread,

Jun 25, 2025, 3:20:32 PMJun 25

to Punith Nayak, Alvin Ji, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Michael Wilson and Punith Nayak

Adam Raine voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Michael Wilson
Punith Nayak

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Punith Nayak (Gerrit)

unread,

Jun 25, 2025, 3:27:33 PMJun 25

to Adam Raine, Alvin Ji, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Michael Wilson

Punith Nayak voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention is currently required from:

Michael Wilson

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jun 25, 2025, 3:55:21 PMJun 25

to Punith Nayak, Adam Raine, Alvin Ji, AyeAye, chromium...@chromium.org, blink-...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[webaudio] Migrate setTargetAtTime limit test to testharness.js

Convert webaudio/audioparam/set-target-at-time-approach-limit.html
from the legacy audit.js runner to pure testharness.js

Bug: 396477777

Change-Id: I733f4d0c43250f5697e5b5357c11eb3b7c68799e

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6635588

Reviewed-by: Alvin Ji <alv...@chromium.org>

Reviewed-by: Adam Raine <asr...@chromium.org>

Commit-Queue: Punith Nayak <punith...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1478730}

Files:

M third_party/blink/web_tests/webaudio/AudioParam/audioparam-setTargetAtTime-limit.html

Change size: L

Delta: 1 file changed, 133 insertions(+), 131 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Adam Raine, +1 by Alvin Ji

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[webaudio] Migrate setTargetAtTime limit test to testharness.js [chromium/src : main]

Michael Wilson (Gerrit)

Michael Wilson added 8 comments

Related details

Punith Nayak (Gerrit)

Punith Nayak added 7 comments

Related details

Michael Wilson (Gerrit)

Michael Wilson voted and added 5 comments

Votes added by Michael Wilson

5 comments

Related details

Punith Nayak (Gerrit)

Punith Nayak added 4 comments

Related details

Michael Wilson (Gerrit)

Michael Wilson voted Code-Review+1

Related details

Alvin Ji (Gerrit)

Alvin Ji voted and added 3 comments

Votes added by Alvin Ji

3 comments

Related details

Punith Nayak (Gerrit)

Punith Nayak added 2 comments

Related details

Alvin Ji (Gerrit)

Alvin Ji voted Code-Review+1

Related details

Adam Raine (Gerrit)

Adam Raine voted Code-Review+1

Related details

Punith Nayak (Gerrit)

Punith Nayak voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information