Add preservesPitch tests #24405

chromium-wpt-export-bot · 2020-06-30T23:51:39Z

This CL adds Web Platform Tests that check the behavior of the
preservesPitch flag.

The tests rely on WebAudio to analyze the frequency of an Audio element,
as the playback rate changes.

Firefox's mozPreservePitch does not affect MediaElementAudioSourceNode,
which means that these tests won't work on Firefox. See:
https://bugzilla.mozilla.org/show_bug.cgi?id=1648277

Change-Id: Ic25d474f96e32eb8d8584188d879a018daa6ae73
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2259375
Commit-Queue: Thomas Guilbert <tguilbert@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#787947}

Edited by @foolip to also include a commit with stability fixes:

Deflake preservesPitch tests

This CL changes preservesPitch tests to wait until the audioElement's
currentTime is at least 0.5 seconds before trying to detect the pitch.
Without this check, the test can try to detect the pitch before the
audio has played enough, which can return a dominant frequency of 0Hz
and fail the test.

The CL also makes a few stylistic changes, and fixes an off-by-one error
in the number of increments used to calculate frequencies.

Bug: 1096238
Change-Id: I6e98e172862a47bea1c4026737138293914f7906
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298281
Auto-Submit: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788535}

wpt-pr-bot

The review process for this patch is being conducted in the Chromium project.

This CL adds Web Platform Tests that check the behavior of the preservesPitch flag. The tests rely on WebAudio to analyze the frequency of an Audio element, as the playback rate changes. Firefox's mozPreservePitch does not affect MediaElementAudioSourceNode, which means that these tests won't work on Firefox. See: https://bugzilla.mozilla.org/show_bug.cgi?id=1648277 Change-Id: Ic25d474f96e32eb8d8584188d879a018daa6ae73 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2259375 Commit-Queue: Thomas Guilbert <tguilbert@chromium.org> Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/heads/master@{#787947}

foolip · 2020-07-14T09:42:06Z

@tguilbert-google this export PR has gotten stuck and not merged automatically because the test appears to be flaky on Firefox. Here are the results copied from Taskcluster logs:

Subtest	Results	Messages
	OK
`Test that preservesPitch is present and unprefixed.`	FAIL	`assert_true: expected true got false`
`Test that presevesPitch is on by default`	PASS
`The default playbackRate should not affect pitch`	FAIL: 9/10, PASS: 1/10	`assert_approx_equals: The actual pitch should be close to the expected pitch. expected 440 +/- 21.533203125 but got 387.59765625;assert_approx_equals: The actual pitch should be close to the expected pitch. expected 440 +/- 21.533203125 but got 0`
`The default playbackRate should not affect pitch, even with preservesPitch=false`	FAIL: 9/10, PASS: 1/10	`assert_approx_equals: The actual pitch should be close to the expected pitch. expected 440 +/- 21.533203125 but got 387.59765625;assert_approx_equals: The actual pitch should be close to the expected pitch. expected 440 +/- 21.533203125 but got 0`
`Speed-ups should not change the pitch when preservesPitch=true`	FAIL: 9/10, PASS: 1/10	`assert_approx_equals: The actual pitch should be close to the expected pitch. expected 440 +/- 21.533203125 but got 387.59765625;assert_approx_equals: The actual pitch should be close to the expected pitch. expected 440 +/- 21.533203125 but got 0`
`Slow-downs should not change the pitch when preservesPitch=true`	PASS
`Speed-ups should change the pitch when preservesPitch=false`	FAIL	`assert_approx_equals: The actual pitch should be close to the expected pitch. expected 880 +/- 21.533203125 but got 430.6640625;assert_approx_equals: The actual pitch should be close to the expected pitch. expected 880 +/- 21.533203125 but got 387.59765625;assert_approx_equals: The actual pitch should be close to the expected pitch. expected 880 +/- 21.533203125 but got 0`
`Slow-downs should change the pitch when preservesPitch=false`	FAIL	`assert_approx_equals: The actual pitch should be close to the expected pitch. expected 220 +/- 21.533203125 but got 430.6640625`

I think that Firefox actually doesn't preserve pitch when connected to Web Audio and should fail some of these tests, but why are the tests flaky? And what's going on with the "but got 0" failures? That seems more like what you'd get if the analyser got silence.

foolip · 2020-07-14T10:19:14Z

Having tinkered a bit with the test locally, I think I see the problem. The test simply waits for a timeupdate event, but that timeupdate event could be fired before there's enough data to analyse. Waiting until audio.currentTime has passed 1 second seems to help. I used this helper:

function waitUntil(time) {
    return new Promise((resolve) => {
        audio.ontimeupdate = () => {
            if (audio.currentTime >= time) {
                resolve();
            }
        };
    });
}

This CL changes preservesPitch tests to wait until the audioElement's currentTime is at least 0.5 seconds before trying to detect the pitch. Without this check, the test can try to detect the pitch before the audio has played enough, which can return a dominant frequency of 0Hz and fail the test. The CL also makes a few stylistic changes, and fixes an off-by-one error in the number of increments used to calculate frequencies. Bug: 1096238 Change-Id: I6e98e172862a47bea1c4026737138293914f7906 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298281 Auto-Submit: Thomas Guilbert <tguilbert@chromium.org> Commit-Queue: Philip Jägenstedt <foolip@chromium.org> Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/heads/master@{#788535}

foolip · 2020-07-15T08:28:58Z

A fix has landed in https://chromium-review.googlesource.com/c/chromium/src/+/2298281, I'll include that in this PR too.

foolip · 2020-07-15T09:17:24Z

https://wpt.fyi/results/html/semantics/embedded-content/media-elements/preserves-pitch.html?label=pr_head&max-count=1&pr=24405 reveals that the tests don't work on Safari. There are some typos, but also WebAudio/web-audio-api#2218 is causing problems.

I will land this PR now and send some fixes.

foolip · 2020-07-15T10:29:14Z

I've sent fixes in #24599.

chromium-wpt-export-bot added chromium-export do not merge yet labels Jun 30, 2020

wpt-pr-bot added assets preserves-pitch labels Jun 30, 2020

wpt-pr-bot approved these changes Jun 30, 2020

View reviewed changes

chromium-wpt-export-bot force-pushed the chromium-export-cl-2259375 branch 3 times, most recently from b20bbcd to a69f2a4 Compare July 7, 2020 00:22

wpt-pr-bot added the html label Jul 7, 2020

chromium-wpt-export-bot force-pushed the chromium-export-cl-2259375 branch 2 times, most recently from 634b909 to 0ade20b Compare July 13, 2020 22:37

chromium-wpt-export-bot removed the do not merge yet label Jul 13, 2020

chromium-wpt-export-bot force-pushed the chromium-export-cl-2259375 branch from 0ade20b to 46005c7 Compare July 13, 2020 23:36

foolip mentioned this pull request Jul 15, 2020

Implementations disagree about the initial state when creating an AudioContext with user interaction WebAudio/web-audio-api#2218

Closed

foolip merged commit 09fbe27 into master Jul 15, 2020

foolip deleted the chromium-export-cl-2259375 branch July 15, 2020 09:17

domenic mentioned this pull request Jul 29, 2020

Add preservesPitch to HTMLMediaElement whatwg/html#5769

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preservesPitch tests #24405

Add preservesPitch tests #24405

chromium-wpt-export-bot commented Jun 30, 2020 •

edited by foolip

wpt-pr-bot left a comment

foolip commented Jul 14, 2020

foolip commented Jul 14, 2020 •

edited

foolip commented Jul 15, 2020

foolip commented Jul 15, 2020

foolip commented Jul 15, 2020

Add preservesPitch tests #24405

Add preservesPitch tests #24405

Conversation

chromium-wpt-export-bot commented Jun 30, 2020 • edited by foolip

wpt-pr-bot left a comment

Choose a reason for hiding this comment

foolip commented Jul 14, 2020

foolip commented Jul 14, 2020 • edited

foolip commented Jul 15, 2020

foolip commented Jul 15, 2020

foolip commented Jul 15, 2020

chromium-wpt-export-bot commented Jun 30, 2020 •

edited by foolip

foolip commented Jul 14, 2020 •

edited