[Webaudio] Add negative playbackRate WPTs and TestExpectations (1/3) [chromium/src : main]
Michael Wilson (Gerrit)
Michael Wilson added 3 comments![]( "Open in Gerrit")
crbug.com/40719652 external/wpt/webaudio/the-audio-api/the-audiobuffersourcenode-interface/audiobuffersource-playbackrate-negative.html [ Failure Timeout ]Do we know what section of the test is timing out? If it's just failing it would be better if we could have an -expected.txt file so we know what the specific output is.
const expected = [4.0, 3.0, 2.0, 1.0];These expected values do not seem to be interpolated. Should they be approximately the following instead?
```suggestion
const expected = [3.5, 2.5, 1.5, 0.5];
```
The spec says the interpolation method is UA-dependent, but since the test waveform is a ramp it's probably fine to use the above.
source.playbackRate.value = 10;This isn't a negative rate; does this belong in the same test suite or would it be better in a separate test file?
Related details
- Hongchan Choi
- Mahesh Kannan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi added 11 comments![]( "Open in Gerrit")
crbug.com/40719652 external/wpt/webaudio/the-audio-api/the-audiobuffersourcenode-interface/audiobuffersource-playbackrate-negative.html [ Failure Timeout ]Do we know what section of the test is timing out? If it's just failing it would be better if we could have an -expected.txt file so we know what the specific output is.
This test should not be timed out because everything is based on OAC. It should deterministically fail if something is not right.
const buffer = context.createBuffer(1, bufferFrames, sampleRate);```suggestion
const buffer = new AudioBuffer(1, bufferFrames, sampleRate);
```
if (bufferData) {
channelData.set(bufferData);
} else {
for (let i = 0; i < bufferFrames; i++) {
channelData[i] = i + 1.0;
}
}Add a comment on what's this is for.
const source = context.createBufferSource();
source.buffer = buffer;```suggestion
const source = new AudioBufferSourceNode(context, {buffer});
```
assert_equals(renderedData[0], 4.0, "Frame 0 should be 4.0");Use single quote for string. (Google JS/TS style)
assert_true(true, "Did not hang!");What does this mean?
Is there any actual edge case that might hang?
const buffer = context.createBuffer(1, 4, 44100);
const source = context.createBufferSource();Use the constructor (instead of the factory method)
source.buffer = buffer;
source.loop = true;
source.loopStart = 1 / 44100;
source.loopEnd = 2 / 44100;
source.playbackRate.value = 10;All these can be a part of the constructor option.
await context.startRendering();
await endedPromise;Is this order specific? Or can we use Promise.all()?
Related details
- Mahesh Kannan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mahesh Kannan (Gerrit)
Mahesh Kannan added 12 comments![]( "Open in Gerrit")
const buffer = context.createBuffer(1, bufferFrames, sampleRate);```suggestion
const buffer = new AudioBuffer(1, bufferFrames, sampleRate);
```
Done
if (bufferData) {
channelData.set(bufferData);
} else {
for (let i = 0; i < bufferFrames; i++) {
channelData[i] = i + 1.0;
}
}Add a comment on what's this is for.
Done
const source = context.createBufferSource();
source.buffer = buffer;```suggestion
const source = new AudioBufferSourceNode(context, {buffer});
```
Done
assert_equals(renderedData[0], 4.0, "Frame 0 should be 4.0");Use single quote for string. (Google JS/TS style)
Done
These expected values do not seem to be interpolated. Should they be approximately the following instead?
```suggestion
const expected = [3.5, 2.5, 1.5, 0.5];
```
The spec says the interpolation method is UA-dependent, but since the test waveform is a ramp it's probably fine to use the above.
Acknowledged
assert_true(true, "Did not hang!");What does this mean?
Is there any actual edge case that might hang?
This is a defensive test case that checks the scenario where `loopStart` and `loopEnd` are identical, and the `playbackRate` is zero. In this scenario, we expect that the system doesn't hang or get stuck. This was added since I asked AI to add defensive test cases but if we feel this test can be moved elsewhere since its 0 playback rate, that would work as well.
const buffer = context.createBuffer(1, 4, 44100);
const source = context.createBufferSource();Use the constructor (instead of the factory method)
Done
source.buffer = buffer;
source.loop = true;
source.loopStart = 1 / 44100;
source.loopEnd = 2 / 44100;
source.playbackRate.value = 10;All these can be a part of the constructor option.
Done
This isn't a negative rate; does this belong in the same test suite or would it be better in a separate test file?
Removed this test since it not negative playback rate related.
source.start(0);Why zero?
Removed this test
await context.startRendering();
await endedPromise;Is this order specific? Or can we use Promise.all()?
Removed this test
You need a new line at the EOF
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mahesh Kannan (Gerrit)
Mahesh Kannan added 2 comments![]( "Open in Gerrit")
source.start(0);Mahesh KannanWhy zero?
Removed this test
Done
await context.startRendering();
await endedPromise;Mahesh KannanIs this order specific? Or can we use Promise.all()?
Removed this test
Done
Mahesh Kannan (Gerrit)
Mahesh Kannan added 1 comment![]( "Open in Gerrit")
crbug.com/40719652 external/wpt/webaudio/the-audio-api/the-audiobuffersourcenode-interface/audiobuffersource-playbackrate-negative.html [ Failure Timeout ]Hongchan ChoiDo we know what section of the test is timing out? If it's just failing it would be better if we could have an -expected.txt file so we know what the specific output is.
This test should not be timed out because everything is based on OAC. It should deterministically fail if something is not right.
Removed this entry and added an `-expected.txt` file instead.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mahesh Kannan (Gerrit)
Mahesh Kannan added 1 comment![]( "Open in Gerrit")
assert_true(true, "Did not hang!");Mahesh KannanWhat does this mean?
Is there any actual edge case that might hang?
This is a defensive test case that checks the scenario where `loopStart` and `loopEnd` are identical, and the `playbackRate` is zero. In this scenario, we expect that the system doesn't hang or get stuck. This was added since I asked AI to add defensive test cases but if we feel this test can be moved elsewhere since its 0 playback rate, that would work as well.
- Hongchan Choi
- Michael Wilson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wilson (Gerrit)
Michael Wilson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Wilson
| Code-Review | +1 |
1 comment
LGTM, but please wait for Hongchan's +1 before submitting.
Related details
- Hongchan Choi
- Mahesh Kannan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mahesh Kannan (Gerrit)
Mahesh Kannan added 1 comment![]( "Open in Gerrit")
LGTM, but please wait for Hongchan's +1 before submitting.
Acknowledged
Related details
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/59613.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi voted and added 7 comments![]( "Open in Gerrit")
Votes added by Hongchan Choi
| Code-Review | +1 |
7 comments
LGTM with nits
* @param {number} renderFrames - Total frames to render in OfflineAudioContext.nit: Wrap at column 80
renderFrames, bufferFrames, setupSource, verifyResult, bufferData = null) {
const sampleRate = 44100;
const context = new OfflineAudioContext(1, renderFrames, sampleRate);
const buffer = new AudioBuffer({ numberOfChannels: 1, length: bufferFrames, sampleRate: sampleRate });nit: wrap at column 80
}, 'AudioBufferSourceNode loops backwards with negative rate (offset in loop)');Ditto.
}, 'AudioBufferSourceNode handles offset exactly at duration with neg rate');Please expand this. I prefer fully spelling things out when it's possible.
Also wrap at column 80.
// Test completes if it does not hang.
assert_true(true, 'Did not hang!');Perhaps I am missing something here, but in which case this test case can hang?
}, 'AudioBufferSourceNode does not hang with zero delta and playbackRate 0');Ditto.
Related details
- Mahesh Kannan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mahesh Kannan (Gerrit)
Mahesh Kannan added 6 comments![]( "Open in Gerrit")
* @param {number} renderFrames - Total frames to render in OfflineAudioContext.nit: Wrap at column 80
Acknowledged
renderFrames, bufferFrames, setupSource, verifyResult, bufferData = null) {
const sampleRate = 44100;
const context = new OfflineAudioContext(1, renderFrames, sampleRate);
const buffer = new AudioBuffer({ numberOfChannels: 1, length: bufferFrames, sampleRate: sampleRate });nit: wrap at column 80
Acknowledged
}, 'AudioBufferSourceNode loops backwards with negative rate (offset in loop)');Mahesh KannanDitto.
Acknowledged
}, 'AudioBufferSourceNode handles offset exactly at duration with neg rate');Please expand this. I prefer fully spelling things out when it's possible.
Also wrap at column 80.
Acknowledged
// Test completes if it does not hang.
assert_true(true, 'Did not hang!');Perhaps I am missing something here, but in which case this test case can hang?
This is just a defensive test case that ensures that we do not enter an invalid state when loopStart and loopEnd are the same values with 0 playback rate
}, 'AudioBufferSourceNode does not hang with zero delta and playbackRate 0');Mahesh KannanDitto.
Marked as resolved.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mahesh Kannan (Gerrit)
Mahesh Kannan added 1 comment![]( "Open in Gerrit")
PTAL! I have kept open the thread on the `hang` testcase for discussion!
Related details
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hongchan Choi
| Code-Review | +1 |
1 comment
// Test completes if it does not hang.
assert_true(true, 'Did not hang!');Mahesh KannanPerhaps I am missing something here, but in which case this test case can hang?
This is just a defensive test case that ensures that we do not enter an invalid state when loopStart and loopEnd are the same values with 0 playback rate
I'd revise this to:
```suggestion
assert_true(true, 'Test ran successfully!');
```
Because the "hang" usually means that timeout or page unresponsive. I don't think this test case can cause that sort of things.
Related details
- Mahesh Kannan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mahesh Kannan (Gerrit)
Mahesh Kannan added 1 comment![]( "Open in Gerrit")
// Test completes if it does not hang.
assert_true(true, 'Did not hang!');Mahesh KannanPerhaps I am missing something here, but in which case this test case can hang?
Hongchan ChoiThis is just a defensive test case that ensures that we do not enter an invalid state when loopStart and loopEnd are the same values with 0 playback rate
I'd revise this to:
```suggestion
assert_true(true, 'Test ran successfully!');
```Because the "hang" usually means that timeout or page unresponsive. I don't think this test case can cause that sort of things.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mahesh Kannan (Gerrit)
Mahesh Kannan voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
11 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/web_tests/external/wpt/webaudio/the-audio-api/the-audiobuffersourcenode-interface/audiobuffersource-playbackrate-negative.html
Insertions: 3, Deletions: 4.
@@ -266,14 +266,13 @@
source.start(0, 2 / sampleRate);
},
(renderedData) => {
- // Test completes if it does not hang.
- assert_true(true, 'Did not hang!');
+ assert_true(true, 'Test ran successfully!');
}
);
- }, 'AudioBufferSourceNode does not hang with zero delta and ' +
+ }, 'AudioBufferSourceNode ran successfully with zero delta and ' +
'playbackRate 0');
</script>
</body>
-</html>
+</html>
```
Change information
[Webaudio] Add negative playbackRate WPTs and TestExpectations (1/3)
This CL adds comprehensive Web Platform Tests for
AudioBufferSourceNode's negative playback rate behavior. Because feature
is not yet implemented, the test is marked as expected to fail or
timeout in TestExpectations.
interface/audiobuffersource-playbackrate-negative.html
- A third_party/blink/web_tests/external/wpt/webaudio/the-audio-api/the-audiobuffersourcenode-interface/audiobuffersource-playbackrate-negative-expected.txt
- A third_party/blink/web_tests/external/wpt/webaudio/the-audio-api/the-audiobuffersourcenode-interface/audiobuffersource-playbackrate-negative.html
Code-Review: +1 by Michael Wilson, +1 by Hongchan Choi
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/59613
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |