Skip to content

Rework definition of ABSN output behavior for rate-adjusted duration param #1681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rtoy merged 6 commits into from
Jul 12, 2018
Merged

Rework definition of ABSN output behavior for rate-adjusted duration param #1681

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8411937
Rework definition of ABSN output behavior to account for playbackRate…
Jun 21, 2018
77670a7
Merge branch 'gh-pages' into 1660-duration-definition
Jun 27, 2018
ff4a0fe
Remove mention of stop() in definition of duration parameter.
Jun 27, 2018
5bee8b4
Regenerate index.html
Jun 27, 2018
992eda2
Initialize bufferDuration outside of process() in ABSN algorithm.
Jun 28, 2018
a57fe08
Address review comments
Jul 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Initialize bufferDuration outside of process() in ABSN algorithm.
  • Loading branch information
joeberkovitz committed Jun 28, 2018
commit 992eda2fc24df6425c327eac46c5d07d900e4593
2 changes: 1 addition & 1 deletion index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4822,6 +4822,7 @@ let stop = Infinity; // Set by stop()

// Variables for tracking node's playback state
let bufferTime = 0, started = false, enteredLoop = false;
let bufferDuration = 0;
Copy link
Member

@rtoy rtoy Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename this? Maybe playbackDuration or something? I find it confusing because there's duration, buffer.duration, and now bufferDuration.

Copy link
Author

@joeberkovitz joeberkovitz Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I am now remembering that I called it bufferDuration because it is duration in terms of the buffer frame rate, not the audible playback duration.

Copy link
Member

@rtoy rtoy Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, what should it be? I think that it's now really the audible playback duration, right?

Copy link
Contributor

@karlt karlt Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is changing duration from a measurement in the audible timeline to a measurement across the buffer. i.e. the same units as offset.

Copy link
Member

@rtoy rtoy Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm thoroughly confused. I thought if start(when, offset,1) were called and the playback rate is 1/10, the audio would be heard for 10 sec and not 1 sec. (Assuming there's at least 1 sec of audio in the buffer, or we're looping.)

Copy link
Contributor

@karlt karlt Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for hearing the 10s. playback would end when bufferDuration is 1.

Perhaps bufferTimeElapsed?

Copy link
Author

@joeberkovitz joeberkovitz Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rtoy we agree on the meaning. I'll let you modify the branch as heading out of touch for 2 weeks.

Copy link
Member

@rtoy rtoy Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joeberkovitz I'll take over this PR. Thanks for your work on this and see in a few weeks.

I think there's just a couple of minor things that need to be fixed:

  • Rename bufferDuration
  • Set bufferTime probably to just offset.

Copy link
Member

@rtoy rtoy Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed bufferDuration and initialized bufferTime to offset.

let dt = 1 / context.sampleRate;

// Handle invocation of start method call
Expand Down Expand Up @@ -4868,7 +4869,6 @@ function playbackSignal(position) {
// of |numberOfFrames| sample frames to be output.
function process(numberOfFrames) {
let currentTime = context.currentTime; // context time of next rendered frame
let bufferDuration = 0; // cumulative duration in "buffer time" (not adjusted for playback rate)
let output = []; // accumulates rendered sample frames

// Combine the two k-rate parameters affecting playback rate
Expand Down
Loading