Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add preservesPitch tests #24405

Merged
merged 2 commits into from Jul 15, 2020
Merged

Add preservesPitch tests #24405

merged 2 commits into from Jul 15, 2020

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

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

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}
Copy link
Collaborator

wpt-pr-bot left a comment

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

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2259375 branch 3 times, most recently from b20bbcd to a69f2a4 Jul 1, 2020
@wpt-pr-bot wpt-pr-bot added the html label Jul 7, 2020
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2259375 branch 2 times, most recently from 634b909 to 0ade20b Jul 7, 2020
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
Copy link
Member

foolip commented Jul 14, 2020

@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
Copy link
Member

foolip commented Jul 14, 2020

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
Copy link
Member

foolip commented Jul 15, 2020

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

@foolip
Copy link
Member

foolip commented Jul 15, 2020

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 foolip merged commit 09fbe27 into master Jul 15, 2020
20 checks passed
20 checks passed
Azure Pipelines Build #20200715.21 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
download-firefox-nightly Community-TC (pull_request)
Details
lint Community-TC (pull_request)
Details
sink-task Community-TC (pull_request)
Details
update-built Community-TC (pull_request)
Details
wpt-chrome-dev-results Community-TC (pull_request)
Details
wpt-chrome-dev-results-without-changes Community-TC (pull_request)
Details
wpt-chrome-dev-stability Community-TC (pull_request)
Details
wpt-decision-task Community-TC (pull_request)
Details
wpt-firefox-nightly-results Community-TC (pull_request)
Details
wpt-firefox-nightly-results-without-changes Community-TC (pull_request)
Details
wpt-firefox-nightly-stability Community-TC (pull_request)
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@foolip foolip deleted the chromium-export-cl-2259375 branch Jul 15, 2020
@foolip
Copy link
Member

foolip commented Jul 15, 2020

I've sent fixes in #24599.

@domenic domenic mentioned this pull request Jul 29, 2020
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.