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

Klang, Klank: Fix initialization #5817

Merged
merged 3 commits into from Aug 14, 2022

Conversation

mtmccrea
Copy link
Member

@mtmccrea mtmccrea commented Jul 17, 2022

Purpose and Motivation

Part fo the initialization sample fix project, specifically in the OscUGens subset.

Types of changes

  • Bug fix

Changes to Klang:

  • correct the initial phase (it was off by 1 sample).

Change to Klank:

  • Don’t assume init sample is zero—reorganize coeff calc func and ctor to get correct init sample.
  • The memory allocation for m_coefs was moved to the Ctor, otherwise, the return statement escapes the Klank_SetCoefs, but not the Ctor, so m_coefs might still be accessed after allocation fails.
  • Calculate the correct init sample by calling Klank_next(unit, 1), temporarily setting mFilterLoops and mFilterRemain for the single cycle of initialization.
    • Usually the pattern of calling _next(unit, 1) works, but Klank_next doesn’t actually use the inNumSamples argument, because it uses the looping scheme designed to optimize second-order filter calculations. As a consequence, a full block of calculations are run over which memory assigned for input data is accessed before it’s been written, so it’s running unnecessary calculations with garbage memory.
    • Here’s a small dump from that internal loop:
[Klank_next 133] 0.052083
[Klank_next 144] 0.103113
[Klank_next 155] 5818479576238205845020707127296.000000
[Klank_next 133] 11667158119450385126825574531072.000000
[Klank_next 144] 17284522609726230684285924278272.000000
...
  • The last commit removes commented Print statements. I’m not sure what the etiquette is for touching old code for trivial changes, so I can revert this commit if needed.

To-do list

  • Code is tested
  • All tests are passing (TestUGen_RTAlloc)
  • [-] Updated documentation
  • This PR is ready for review

Klang: Previously the init sample was correct, but y1 was set to the same value as y0, and y2 was set to the value of y1.

This update makes y0 independent, properly sets y1 and y2 so both the init sample and the first sample will be equal and in their proper place in time (essentially one sample later in time).
Don’t assume init sample is zero—reorganize coeff calc func and ctor to get correct init sample.

The memory allocation for `m_coefs` was moved to the Ctor, otherwise, the `return` statement escapes the `Klank_SetCoefs`, but not the Ctor, so `m_coefs` might still be accessed after allocation fails.

Calculate the correct init sample by calling `Klank_next(unit, 1)`, temporarily setting `mFilterLoops` and  `mFilterRemain` for the single cycle of initialization.

Usually the pattern of calling `_next(unit, 1)` works, but `Klank_next` doesn’t actually use the `inNumSamples` argument, because it uses the looping scheme designed to optimize second-order filter calculations. As a consequence, a full block of calculations are run over which memory assigned for input data is accessed before it’s been written, so it’s running unnecessary calculations with garbage memory.
@mtmccrea mtmccrea force-pushed the topic/klang-klank-init-sample branch from 0a7bb00 to 470b463 Compare July 17, 2022 13:44
@telephon
Copy link
Member

Don’t assume init sample is zero—reorganize coeff calc func and ctor to get correct init sample.

just to make sure: if you write

{ Klank.ar(`[[1000], [1], [1]], Impulse.ar(0)) }.play

The Klank does interpret the input as a trigger, right?

@jamshark70
Copy link
Contributor

jamshark70 commented Aug 11, 2022

@telephon --

if you write { Klank.ar(Ref([[1000], [1], [1]]), Impulse.ar(0)) }.play, the Klank does interpret the input as a trigger, right?

klank-impulse

@mtmccrea
Copy link
Member Author

Yes, just to show the triggered behavior a bit more clearly, here is
{ Klank.ar(`[[1000], [1], [0.05]], Impulse.ar(0)) }.plot(0.1)
Screen Shot 2022-08-12 at 10 03 01

@mtmccrea mtmccrea mentioned this pull request Aug 14, 2022
4 tasks
@joshpar joshpar added this to the 3.13.0 milestone Aug 14, 2022
@joshpar joshpar merged commit ee3d5df into supercollider:develop Aug 14, 2022
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.

None yet

4 participants