From 0961dc4169818ffc4cc9f875dcef3cde4dd88208 Mon Sep 17 00:00:00 2001 From: Roger McFarlane Date: Wed, 22 Jul 2020 20:00:14 +0000 Subject: [PATCH] Bug 1654511 [wpt PR 24606] - [Sheriff] Revert "Deflake preservesPitch tests", a=testonly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Automatic update from web-platform-tests [Sheriff] Revert "Deflake preservesPitch tests" This reverts commit 7b6c4007896853f91f32bd69b2c770f0210c66e5. Reason for revert: Post this change, preservesPitch seems to consistently fail. https://ci.chromium.org/p/chromium/builders/ci/Mac10.14%20Tests Original change's description: > 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 > Commit-Queue: Philip Jägenstedt > Reviewed-by: Philip Jägenstedt > Cr-Commit-Position: refs/heads/master@{#788535} TBR=tguilbert@chromium.org,foolip@chromium.org Change-Id: I0042f73ca9c5de7d82be1200794c45b46a003f9b No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 1096238 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2299183 Reviewed-by: Roger McFarlane Commit-Queue: Roger McFarlane Cr-Commit-Position: refs/heads/master@{#788626} -- Reland preservesPitch test deflake, fix timeouts This CL relands "Deflake preservesPitch tests", which was reverted by: commit f19f3404371bba3cac38bf1bcf672ef5f3e96558. It also attempts to incorporate some of the WPT changes made upstream, but not yet landed, in: https://github.com/web-platform-tests/wpt/pull/24599 Specifically, it fixes typos in the Safari prefixes, and explicitly starts the audio context. However, it is different from the PR, since it reuses the same Audio element, MediaElementAudioSourceNode and AudioContext. This is an attempt to cut down on overhead costs and fix test timeouts. We instead reset the audio.currentTime to 0, and only create a new analyser node for each test. Bug: 1105877, 1096238 Change-Id: Ie528ec0b7c38d9df59fcb04696c810e6d1c232f6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2300929 Commit-Queue: Thomas Guilbert Auto-Submit: Thomas Guilbert Reviewed-by: Philip Jägenstedt Cr-Commit-Position: refs/heads/master@{#789262} -- wpt-commits: 9abf97af00b4a3f55ecba875bf1cdbecb4a1a1ee, 9a67b791ec7d9c6538411430677ac25eb6ef60b0 wpt-pr: 24606 --- .../media-elements/pitch-detector.js | 22 +++++-- .../media-elements/preserves-pitch.html | 66 +++++++++++++------ 2 files changed, 61 insertions(+), 27 deletions(-) diff --git a/testing/web-platform/tests/html/semantics/embedded-content/media-elements/pitch-detector.js b/testing/web-platform/tests/html/semantics/embedded-content/media-elements/pitch-detector.js index 595bde3120c7..78f22ccd8576 100644 --- a/testing/web-platform/tests/html/semantics/embedded-content/media-elements/pitch-detector.js +++ b/testing/web-platform/tests/html/semantics/embedded-content/media-elements/pitch-detector.js @@ -4,11 +4,14 @@ window.AudioContext = window.AudioContext || window.webkitAudioContext; var FFT_SIZE = 2048; -function getPitchDetector(media, t) { - var audioContext = new AudioContext(); - t.add_cleanup(() => audioContext.close()); +var audioContext; +var sourceNode; - var sourceNode = audioContext.createMediaElementSource(media); +function getPitchDetector(media) { + if(!audioContext) { + audioContext = new AudioContext(); + sourceNode = audioContext.createMediaElementSource(media); + } var analyser = audioContext.createAnalyser(); analyser.fftSize = FFT_SIZE; @@ -16,13 +19,20 @@ function getPitchDetector(media, t) { sourceNode.connect(analyser); analyser.connect(audioContext.destination); - return () => getPitch(analyser); + return { + ensureStart() { return audioContext.resume(); }, + detect() { return getPitch(analyser); }, + cleanup() { + sourceNode.disconnect(); + analyser.disconnect(); + }, + }; } function getPitch(analyser) { // Returns the frequency value for the nth FFT bin. var binConverter = (bin) => - (analyser.context.sampleRate/2)*((bin)/(analyser.frequencyBinCount-1)); + (audioContext.sampleRate/2)*((bin)/(analyser.frequencyBinCount-1)); var buf = new Uint8Array(analyser.frequencyBinCount); analyser.getByteFrequencyData(buf); diff --git a/testing/web-platform/tests/html/semantics/embedded-content/media-elements/preserves-pitch.html b/testing/web-platform/tests/html/semantics/embedded-content/media-elements/preserves-pitch.html index 9a86cbc210b2..1cf6c7639036 100644 --- a/testing/web-platform/tests/html/semantics/embedded-content/media-elements/preserves-pitch.html +++ b/testing/web-platform/tests/html/semantics/embedded-content/media-elements/preserves-pitch.html @@ -15,8 +15,8 @@ function getPreservesPitch(audio) { if ("mozPreservesPitch" in HTMLAudioElement.prototype) { return audio.mozPreservesPitch; } - if ("wekbitPreservesPitch" in HTMLAudioElement.prototype) { - return audio.wekbitPreservesPitch; + if ("webkitPreservesPitch" in HTMLAudioElement.prototype) { + return audio.webkitPreservesPitch; } return undefined; } @@ -27,8 +27,8 @@ function setPreservesPitch(audio, value) { audio.preservesPitch = value; } else if ("mozPreservesPitch" in HTMLAudioElement.prototype) { audio.mozPreservesPitch = value; - } else if ("wekbitPreservesPitch" in HTMLAudioElement.prototype) { - audio.wekbitPreservesPitch = value; + } else if ("webkitPreservesPitch" in HTMLAudioElement.prototype) { + audio.webkitPreservesPitch = value; } } @@ -37,21 +37,28 @@ test(function(t) { }, "Test that preservesPitch is present and unprefixed."); test(function(t) { - let audio = document.createElement('audio'); - assert_true(getPreservesPitch(audio)); + let defaultAudio = document.createElement('audio'); + assert_true(getPreservesPitch(defaultAudio)); - setPreservesPitch(audio, false); - assert_false(getPreservesPitch(audio)); -}, "Test that presevesPitch is on by default"); + setPreservesPitch(defaultAudio, false); + assert_false(getPreservesPitch(defaultAudio)); +}, "Test that preservesPitch is on by default"); + + +var audio; + +function addTestCleanups(t, detector) { + t.add_cleanup(() => { + audio.pause(); + audio.currentTime = 0; + }); + t.add_cleanup(() => detector.cleanup()); +} function testPreservesPitch(preservesPitch, playbackRate, expectedPitch, description) { promise_test(async t => { - let audio = document.createElement('audio'); - - var detector = getPitchDetector(audio, t); - - // This file contains 5 seconds of a 440hz sine wave. - audio.src = "/media/sine440.mp3"; + let detector = getPitchDetector(audio); + addTestCleanups(t, detector); audio.playbackRate = playbackRate; setPreservesPitch(audio, preservesPitch); @@ -66,13 +73,12 @@ function testPreservesPitch(preservesPitch, playbackRate, expectedPitch, descrip }); } - await test_driver.bless("Play audio element", () => audio.play() ) + // Wait until we have played some audio. Otherwise, the detector + // might return a pitch of 0Hz. + audio.play(); + await waitUntil(0.25); - // Wait until we have at least played some audio. Otherwise, the - // detector might return a pitch of 0Hz. - await waitUntil(0.5); - - var pitch = detector(); + var pitch = detector.detect(); // 25Hz is larger than the margin we get from 48kHz and 44.1kHz // audio being analyzed by a FFT of size 2048. If we get something @@ -89,6 +95,24 @@ function testPreservesPitch(preservesPitch, playbackRate, expectedPitch, descrip var REFERENCE_PITCH = 440; +promise_test(async t => { + // Create the audio element only once, in order to lower the chances of + // tests timing out. + audio = document.createElement('audio'); + + // This file contains 5 seconds of a 440hz sine wave. + audio.src = "/media/sine440.mp3"; + + let detector = getPitchDetector(audio); + addTestCleanups(t, detector); + + // The first time we run the test, we need to interact with the + // AudioContext and Audio element via user gestures. + await test_driver.bless("Play audio element", () => { + return Promise.all([audio.play(), detector.ensureStart()]); + }); +}, "Setup Audio element and AudioContext") + testPreservesPitch(true, 1.0, REFERENCE_PITCH, "The default playbackRate should not affect pitch")