Add HTMLMediaElement.preservesPitch

[haywirez]

Would appreciate some comments on how to improve. Ideally the standard should also point out that changes to the playbackRate and preservesPitch need to be handled continuously and not interrupt playback (e.g. firefox has the correct behavior, safari not...). Thank you 🙏

---

/acknowledgements.html ( diff )
/media.html ( diff )

[foolip]

I will push a commit adding the skeleton for the missing pieces of this PR.

[foolip]

Note that this is the definition for web developers, the green box that you see here:
https://html.spec.whatwg.org/multipage/media.html#playing-the-media-resource

Here, I suggest following the structure of the ended/playbackRate attributes, with one "Returns true if ... The default value is true." statement and a separate "Can be set, to change ...".

[foolip]

@haywirez, thanks for sending the PR! I've left some comments, hopefully that is enough for you to continue with this.

When the spec text is done, a corresponding PR in web-platform-tests will also be needed. Unfortunately, since this concerns the audio output, that isn't directly testable. However, we will have to decide whether the pitch adjustment should affect https://webaudio.github.io/web-audio-api/#mediaelementaudiosourcenode or not, and that can be tested. Checking how that behaves in Firefox and Safari would be a good start.

[haywirez]

Thanks, appreciate the help!

[haywirez]

Finally got around pushing some changes 🎉

[annevk]

The problem with Participation here is that you're not a public member of the haywirez-org GitHub organization (and someone will need to verify your organization still I think).

[haywirez]

Thanks - should I squash the commits to make the log simpler as well?

[annevk]

There's no need for that, we typically squash when landing on master. (If you want some control over the eventual commit message it helps to leave it in a comment so that whoever merges can take it into account.)

[haywirez]

@foolip continuing, I think the pitch adjustment should affect MediaElementAudioSourceNode, at least I don't see any reason why it shouldn't - same as any of the other attributes.

[haywirez]

Force pushed to reauthor commit with correct e-mail address

[foolip]

@foolip continuing, I think the pitch adjustment should should affect MediaElementAudioSourceNode, at least I don't see any reason why it shouldn't - same as any of the other attributes.

That sounds reasonable. Have you tested what Firefox and Safari do?

[haywirez]

I discovered that changing the .playbackRate with or without .moz/webkitPreservesPitch on MediaElementSourceNode’s source doesn’t seem have any effect in Firefox - but it does in Chrome. It should though, as otherwise we don't have a good mechanism to change the playbackRate and the whole thing kind of becomes pointless.

I'm trying to put together a codepen so I can demonstrate easier, but meanwhile you can try this or a similar code in the browser console while on the correct domain (CORS issues otherwise...)

var AudioContext = window.AudioContext || window.webkitAudioContext
var ctx = new AudioContext()
var audio = new Audio()
audio.src = 'https://songsling.io/loop-130.mp3'
audio.loop = true
audio.playbackRate = 0.5
var meNode = ctx.createMediaElementSource(audio)
meNode.connect(ctx.destination)
audio.addEventListener('play', () => setTimeout(x => audio.pause(), 2*1.846*1000))
audio.play()

[foolip]

That's interesting, and I guess it would make sense for either both of playbackRate and preservesPitch to take effect for Web Audio, or for neither of them to work. If playbackRate already works in Chrome, maybe having them both work is the best bet.

Have you tested Edge and Safari?

[haywirez]

Slapped together a quick ridiculous test page:
https://haywirez.com/mesn-playbackrate/index.html

As far as I can tell:

Safari (12.0 macOS): only plays a small fragment, then mutes playback altogether (no matter the settings).

Edge (41.16299.371.0 Win10): only plays back when playbackRate is 1, if it's anything else, sound immediately gets muted

Firefox (60.0.2/62.0.2 Win10, 62.0.2 macOS): changing playbackRate has no effect whatsoever

Chrome (69.0.3497.100 macOS): playbackRate works properly, but there's no preservesPitch control...

Weird.

[haywirez]

Relevant, the Web Audio API spec's editor's draft clearly states:
https://webaudio.github.io/web-audio-api/#mediaelementaudiosourcenode

The HTMLMediaElement MUST behave in an identical fashion after the MediaElementAudioSourceNode has been created, except that the rendered audio will no longer be heard directly, but instead will be heard as a consequence of the MediaElementAudioSourceNode being connected through the routing graph. Thus pausing, seeking, volume, src attribute changes, and other aspects of the HTMLMediaElement MUST behave as they normally would if not used with a MediaElementAudioSourceNode.

[haywirez]

Not sure what to do next here

[foolip]

@haywirez, thanks for that testing and finding the relevant part of the Web Audio API spec. Given the mix of results and that there's no consistency in behavior, I think aiming for what the Web Audio spec asks for is the most reasonable path.

That should mean that we can add tests that verify that the pitch changes depending on the use of preservesPitch, but lacking a browser to verify those tests again, it's very easy to accidentally write broken tests. Would you like to give it a try anyway, or WDYT?

If that does not seem like a good use of time, what I'd suggest is that we file an issue on web-platform-tests to test that aspect, and link to it in each browser bug created for this PR, asking the vendors to create tests when they implement this.

[foolip]

The spec changes LGTM. Would @annevk or @domenic care to take an editorial pass as well?

[domenic]

LGTM modulo wrapping, which I can fix up.

[foolip]

Thanks @domenic! @haywirez, we will also want to merge some tests together with this spec change. Are you willing to work on a web-platform-tests PR for that? I'm happy to review that also.

[haywirez]

Thanks! Definitely up for writing the tests. I'll probably need some guidance, bit unclear how to test these. I'll look into it tomorrow

[foolip]

Here are some resources to get started:
https://web-platform-tests.org/writing-tests/testharness.html
https://github.com/web-platform-tests/wpt/blob/master/html/semantics/embedded-content/media-elements/playing-the-media-resource/playbackRate.html (similar existing test)

[foolip]

@tguilbert-google is adding tests for this in https://chromium-review.googlesource.com/c/chromium/src/+/2259375.

@tguilbert-google would you also be willing to revive this spec change (as a new PR) so that we can land spec and tests together?

[foolip]

Sorry, strike that. @haywirez started this PR, no need to create a new one if we can iterate on this one.

[haywirez]

Funny, I started a similar test in my fork but my sine wave + zero-crossing rate detector approach didn't work accurately enough and I ran out of time. Before that I was also trying one based on the FFT analyzer — looks like the chromium test case is similar, happy that the team got it done ✌️

[domenic]

It looks like the tests are ready. I'm going to rebase this and merge!

[domenic]

Hmm, @haywirez, can you check "allow edits from maintainers" on this PR?

[haywirez]

@domenic I don't see that option here strangely, perhaps someone from the organization has control over it? @foolip?
Maybe I have to convert it back to a draft?

[domenic]

Weird... maybe it doesn't exist for very old PRs. I guess I'll close this one and re-create it (with you as the committer still, of course!)

[haywirez]

Thanks for all the extra effort! 😄
Hope I’ll be of more use in the future!

[annevk]

See isaacs/github#1681 and https://git.luolix.topmunity/t/how-can-we-enable-allow-edits-from-maintainers-by-default/2847/8. It's a missing feature for forks from organizations.