Skip to content
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.

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

Add outLength to full API, tells caller how many samples were written #144

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

tortis
Copy link
Contributor

@tortis tortis commented Aug 16, 2024

Includes both number of frames and number of samples for convenience and to avoid confusion.

Real-world example usage:

const outLength = {frames: 0, samples: 0};
resampler.full(res.samples, sampleBuffer, outLength);
res.samples = sampleBuffer.slice(0, outLength.samples);

Ref #143

@aolsenjazz
Copy link
Owner

aolsenjazz commented Aug 16, 2024

I'm glad that you brought up the idea of frames. It seems truer to implementation of SRC internally to use frames rather than samples. Would you revise to just use frames? Again, thanks for suggesting that.

Edit: Unless you think there's a compelling use case for including samples in the object. I just don't see it atm.

@tortis
Copy link
Contributor Author

tortis commented Aug 16, 2024

Sounds good, I amended the commit, so it just has frames now.

@aolsenjazz
Copy link
Owner

Thanks - 1 more step. I had to update node version in CI for tests to pass - would you merge with main and update to 2.1.2? I'll publish on NPM and worry about test coverage when I have some time.

Thanks for the extra time.

tortis added 2 commits August 16, 2024 13:20
Includes both number of frames and number of samples for convenience and
to avoid confusion.

Real-world example usage:

```typescript
const outLength = {frames: 0};
resampler.full(res.samples, sampleBuffer, outLength);
res.samples = sampleBuffer.slice(0, outLength.samples);
```
@tortis
Copy link
Contributor Author

tortis commented Aug 16, 2024

I rebased the upstream, and ran npm version 2.1.2.
I can't push the tag to the upstream, if you wanna keep version tags, so I'll leave that to you.

Thanks for helping get this change out!

@aolsenjazz aolsenjazz merged commit bcb1764 into aolsenjazz:main Aug 16, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants