chrisguttandin / timingsrc

A library to synchronize a MediaElement with a TimingObject.
MIT License
32 stars 4 forks source link

Audio element throws error when playback rate outside of valid range #13

Closed AlexanderArvidsson closed 2 years ago

AlexanderArvidsson commented 2 years ago

Hi! πŸ‘‹

Firstly, thanks for your work on this project! πŸ™‚

Today I used patch-package to patch timingsrc@1.2.3 for the project I'm working on.

Chromium based browsers throws an error when trying to modify the playback rate of an audio element if it's below 0.0625. Upon further inspection of the source code of chromium, they seem to throw errors if the range is outside the range [0.0625, 16]. Check it here: https://source.chromium.org/chromium/chromium/src/+/f23e01e01c7072467e0f5eb1f299fdd0032fdc46:third_party/blink/renderer/core/html/media/html_media_element.h;l=108-110

I can create a PR with the changes applied to the source code if you wish. I was unsure if we should pass nextValue or assignedValue to the rate assignments setter. I do not speak RxJS, so am not sure exactly what it does. I did not want to mess up any kind of stepwise calculations and such.

Also, is the Firefox bug still present? Is there a source for this?

Here is the diff that solved my problem:

diff --git a/node_modules/timingsrc/build/es2019/factories/set-playback-rate.js b/node_modules/timingsrc/build/es2019/factories/set-playback-rate.js
index 752572c..a8d2c00 100644
--- a/node_modules/timingsrc/build/es2019/factories/set-playback-rate.js
+++ b/node_modules/timingsrc/build/es2019/factories/set-playback-rate.js
@@ -1,11 +1,17 @@
+const MIN_PLAYBACK_RATE = 0.0625
+const MAX_PLAYBACK_RATE = 16.0
+
 export const createSetPlaybackRate = (playbackRateAssignments) => {
     return (mediaElement, previousValue, nextValue) => {
         const playbackRateAssignment = playbackRateAssignments.get(mediaElement);
         if (playbackRateAssignment === undefined ||
             playbackRateAssignment[0] !== previousValue ||
             playbackRateAssignment[1] !== nextValue) {
+            // Playback rate cannot go below 0.0625 or above 16.0 on chromium (maybe other browsers too?)
+            // https://source.chromium.org/chromium/chromium/src/+/f23e01e01c7072467e0f5eb1f299fdd0032fdc46:third_party/blink/renderer/core/html/media/html_media_element.h;l=108-110
+            const assignedValue = nextValue === 0 ? 0 : Math.max(MIN_PLAYBACK_RATE, Math.min(MAX_PLAYBACK_RATE, nextValue));
             // There is currently a bug in Firefox which causes problems when switching back to a playbackRate of exactly 1.
-            mediaElement.playbackRate = nextValue === 1 ? (previousValue > 1 ? 1.00001 : 0.99999) : nextValue;
+            mediaElement.playbackRate = assignedValue === 1 ? (previousValue > 1 ? 1.00001 : 0.99999) : assignedValue;            
             playbackRateAssignments.set(mediaElement, [mediaElement.playbackRate, nextValue]);
         }
     };

This issue body was partially generated by patch-package.

chrisguttandin commented 2 years ago

Hi @AlexanderArvidsson,

thanks for your detailed bug report.

Unfortunately the bug in Firefox got no attention yet from the Firefox team. It's documented here: https://bugzilla.mozilla.org/show_bug.cgi?id=1683437. Feel free to subscribe to it to make it at least a little bit more popular.

Normally I try to add an expectation test for each of the browser bugs that get "fixed" or circumvented in the code. That's my way to keep track of those bugs. Once the test fails I know it got fixed. I did for example add a lot of those tests over here. https://github.com/chrisguttandin/standardized-audio-context/tree/master/test/expectation

Sadly that bug in Firefox is hard to test automatically. I would need to wire up something which records the system audio and analyze that to determine if there are clicks or not. I chose to subscribe to the bug on Bugzilla instead. :-)

But the playbackRate limitations that you discovered can be tested very well. My approach to fix this would be to add an expectation test first that checks that Chrome supports anything between 0.0625 and 16 but throws an error for values outside of that range.

At runtime I would also do a quick check with a dummy media element to check if this is limitation needs to be applied. Firefox does for example seem to support values from 0 to +Infinity. Therefore I would like to let Firefox go beyond Chrome's limitations.

If you want I could start by adding the expectation test first and you continue by adding your fix afterwards. Does that sound like a good plan to you, too?

AlexanderArvidsson commented 2 years ago

@chrisguttandin That sounds great, good to see such thorough testing for the different browsers (I believe I recall using your standardized-audio-context package in the past hehe). If you start by adding the expectation test results here, we can see which browsers really struggle and implement the fix for those. I also suspect Electron, Edge and other chromium based browsers (Opera?) will suffer the same problem. We have a desktop application running in Electron at our company that I could see if the same issue appears without my fix applied.

There is also the case with the range [-16, -0.0625] that should be tested to see if it also applies there.

Also to note, setting playbackRate in video.js to a value outside the range specified does not throw an error, I suspect video.js handles it internally though. I have not yet tried doing it on a pure video element, but I do know for sure it happens for audio elements. I'm a bit short on time to do more thorough testing though, and I'm guessing your expectation tests will cover these cases regardless!

Thanks for taking your time with this!

On another note, I am experiencing some problems with audio synchronization on Safari. I'll open another issue eventually though with more details since it's unrelated to this.

chrisguttandin commented 2 years ago

Hi @AlexanderArvidsson I created a PR over [here](). Could you please take a look. For some reason I can't add you as an official reviewer.

I think this is how Video.js handles the error.

https://github.com/videojs/video.js/blob/0211d736dee702614d58cb773f6059617424fa76/src/js/tech/html5.js#L1128-L1143