bbc / peaks.js

JavaScript UI component for interacting with audio waveforms
https://waveform.prototyping.bbc.co.uk
GNU Lesser General Public License v3.0
3.2k stars 279 forks source link

Improvement made to player.playSegment time check #363

Closed ffxsam closed 3 years ago

ffxsam commented 3 years ago

Switched from using setInterval() to requestAnimationFrame() so it can execute in a loop as fast as the browser/computer can handle without causing any kind of blocking. Should get more precision out of this method.

ffxsam commented 3 years ago

Hmm, I'm not sure why the tests failed exactly.

Feel free to just close this PR if you want - it's not a huge deal. But typically it's better to use requestAnimationFrame() over setInterval() with short intervals to loop at high rates.

chrisn commented 3 years ago

Not sure why the tests fail. I'm happy to merge this so long as tests are passing.

chrisn commented 3 years ago

I don't why the test reports Cannot read property 'pause' of null. I tested this using one of the demo pages and got a different error. After calling playSegment(), I found that timeChecker() is being called before the media element starts playing and so the self.isPlaying() call returns false and so the browser logs an error that the media element play() is cancelled by calling pause(), and the segment doesn't play.

I think the solution is to wait for a player.play event before starting the rAF loop.

ffxsam commented 3 years ago

My strong hunch is that we need to be waiting for the Promise to resolve (for the self.play() call).

chrisn commented 3 years ago

That would be ideal, but some legacy browsers don't return a promise.

ffxsam commented 3 years ago

Depends on how far back you wanna support, I suppose. Of the big three (Chrome, Firefox, Safari), Safari was the straggler as far as play() not returning a Promise. That was resolved back in.. version 9 I think?

chrisn commented 3 years ago

I found one small issue with this PR - if you call playSegment() multiple times without first stopping playback or waiting for the segment to end, you'll end up with multiple timeChecker callbacks being run in each rAF cycle.

ffxsam commented 3 years ago

@chrisn Looks like you brought some of the changes from this PR into the main branch. Shall I close this?

chrisn commented 3 years ago

Yes, thank you. I may come back to this, though, to use the promise that play() returns.