Skip to content

Execute pending resets before loading Memory PCM#962

Merged
dpwe merged 1 commit into
shorepine:mainfrom
mateusz28011:fix/memory-pcm-reset-order
Jul 24, 2026
Merged

Execute pending resets before loading Memory PCM#962
dpwe merged 1 commit into
shorepine:mainfrom
mateusz28011:fix/memory-pcm-reset-order

Conversation

@mateusz28011

Copy link
Copy Markdown
Contributor

Summary

  • execute due AMY events before allocating a Memory PCM preset
  • extend the existing TestLoadSample golden test to cover amy.reset()
    immediately followed by amy.load_sample()

Problem

amy.reset() queues RESET_ALL_OSCS, while Memory PCM allocation performed
by load_sample() happens immediately when the transfer header is parsed.

This allows the following ordering:

  1. RESET_ALL_OSCS is queued
  2. the new Memory PCM preset is allocated and loaded
  3. the queued reset executes
  4. amy_reset_oscs() calls pcm_unload_all_presets()
  5. the newly loaded preset is removed

As a result, the same PCM note can produce different audio depending on when
the pending reset executes.

Patch loading already prevents the equivalent ordering race by calling
amy_execute_deltas() immediately before patches_load_patch() (#372).
This change applies the same ordering boundary immediately before pcm_load().

Only due events are executed; future scheduled events remain queued.

Regression test

The existing TestLoadSample golden test now resets AMY immediately before
loading its sample.

Before the fix:

TestLoadSample: signal=-27.1 dB err=-26.2 dB

After the fix:

TestLoadSample: signal=-33.7 dB err=-100.0 dB

The existing reference WAV does not change.

Test plan

  • python3 -m amy.test quiet — 119 tests pass
  • make check-c-api
  • make web — not run locally because emcc is unavailable; covered by CI

@dpwe
dpwe merged commit ef0f885 into shorepine:main Jul 24, 2026
@dpwe

dpwe commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Thanks!

@bwhitman

Copy link
Copy Markdown
Collaborator

⛓️ tulipcc integration PR opened

This merge was pinned into tulipcc for full-system CI: shorepine/tulipcc#1228

Test it there and merge that PR to move tulipcc onto this AMY.

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.

3 participants