search

bvibber / audio-feeder

Small JS library to abstract Web Audio raw PCM output
Other
53 stars 7 forks source link

Add ability to change playback rate (tempo) #32

Closed velochy closed 5 years ago

velochy commented 5 years ago

Moving over the discussion from https://github.com/brion/ogv.js/issues/352 as it mainly relates to this project.

velochy commented 5 years ago

@brion would like your thoughts on one question.

As you correctly noted, ogv.js would need to have playbackPosition changed to inputPlaybackPosition pretty much everywhere for things to work.

Assuming this would be the case for other practical applications of audioFeeder too, would it maybe make more sense, to instead change naming so that the current playbackPosition becomes outputPlaybackPosition and playbackPosition = inputPlaybackPosition instead?

velochy commented 5 years ago

@brion I believe I have the first working version.

The changes needed to this project: https://github.com/velochy/audio-feeder/commit/6a4fadbc091df8d965a577ba26695b3a262f9cbf

And the minimal changes needed to ogv.js: https://github.com/velochy/ogv.js/commit/385fed804243a215c71db30c75d6a87c670fbcb7

Compiling my version of ogv.js and changing o.playbackRate from the console in the demo seems to work, both when slowing down or speeding up.

I'm really impressed about the engineering of ogv.js considering just how minimal the changes have to be for it to work - especially considering video speed throttling seems to be "free" as in requiring no extra code and just working. Compliments @brion!

I would ask you (and anyone else happening to read this) to give these changes a quick review and see if anything seems out of place or should be done differently. I will make a PR to both projects once the first batch of comments has been handled.

The main issue that remains to be fixed is the fact that quite a lot of unnecessary work is currently done when no tempo change is in effect. This should be rather easy to fix inside phase-vocoder.js without affecting the bindings, so it should not affect your code review.

bvibber commented 5 years ago

Awesome! Will take a peek later today.

bvibber commented 5 years ago

Yeah, I'd probably recommend switching inputPlaybackPosition to playbackPosition and have outputPlaybackPosition for the output units.

bvibber commented 5 years ago

Added a few comments. :) Probably want to return input units for both playbackPosition and getDurationBuffered(), so it's easy to tell where in the input data we are and how close to the end of the data stream we are to append more.

Note for video-only files without audio I'll need to adjust the non-audio timeline in ogv.js to handle variable rate as well, but I can do that later as the most significant case is for files with audio to make speech faster. :)

velochy commented 5 years ago

@brion this is obviously your call to make but Id like to make a case for keeping output units for durationBuffered:

To my understanding, durationBuffered is used mainly to decide when to provide more examples. To that effect, output units work better as they are directly connected to how long it has before running out. Switching to input units runs the risk of running out quickly if input is sped up by a lot.

Then again: there is a case to be made for keeping it in the same coordinate system as playbackDuration so I can understand you going that way too.

Either way, just let me know and I will do it the way you decide.

bvibber commented 5 years ago

@velochy nod sensible. Let's keep durationBuffered in output units, and if I find I need the input unit time it's easy to convert. :)

bvibber commented 5 years ago

Implemented by @velochy's patch. :) Thanks!