Skip to content

winmm: preserve pending samples across transient write failures - #321

Merged
hajimehoshi merged 3 commits into
ebitengine:mainfrom
kumagi:codex/oto-priority-104-20261014
Oct 15, 2026
Merged

hajimehoshi merged 3 commits into
ebitengine:mainfrom
kumagi:codex/oto-priority-104-20261014

Conversation

@kumagi

@kumagi kumagi commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

What issue is this addressing?

None

What type of issue is this addressing?

bug

What this PR does | solves

Keep PCM pending until a header accepts it, including MMSYSERR_NOMEM and MMSYSERR_NODRIVER retries. Back off for a buffer duration if all headers reject with NOMEM rather than immediately consuming and discarding another chunk. Fatal errors remain terminal.

Validation: An actual-method private race harness detects the original lost PCM sequence and passes after the fix. It covers NOMEM/NODRIVER followed by success, acceptance by another header, positive retry backoff and fatal-write termination. Native writes and sleeping are substituted.

Native macOS arm64 internal race tests and build passed. Windows 386/amd64/arm64 builds passed (compile only). Internal vet and other analyzers passed; full vet retains unchanged baseline Darwin unsafe.Pointer and Windows unkeyed GUID diagnostics. Native Windows playback, reconnect and recovery were not run. Independent review found no blocking issue.

The retry wait is shared by the WinMM-specific waitBeforeRetry() helper; both retry paths retain the original buffer-duration delay.

@hajimehoshi

Copy link
Copy Markdown
Member

Could you use time.Sleep(time.Millisecond) for both the MMSYSERR_NODRIVER retry and the backoff after all available headers reject writes with MMSYSERR_NOMEM? These delays control retries after failure, so they do not need to match the audio buffer duration. A fixed 1 ms delay keeps the backoff positive for tiny buffers and avoids unnecessarily long retry delays for large buffers. Windows may sleep longer because of timer resolution or scheduling, which is fine for this purpose.

Review comment authored by Codex on behalf of @hajimehoshi.

@kumagi

kumagi commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor Author

Addressed in 3dd1936ae0d6b2173930ac0a46b46c2339bff34e. Both the MMSYSERR_NODRIVER retry and the all-headers-NOMEM backoff now request time.Sleep(time.Millisecond), independent of buffer duration. Pending PCM handling is unchanged.

A private regression uses the actual appendBuffers method with native writes and sleeping substituted. Both paths fail the fixed-delay check before the change for small and large buffers, and pass afterward under race. The existing PCM retry-order, alternate-header success and fatal-write checks also pass. Internal package race tests, the other vet analyzers, and Windows 386/amd64/arm64 builds passed. Native Windows playback, driver failure and actual sleep timing were not tested; Windows may sleep longer than the requested interval. All nine exact-head upstream checks passed (https://github.com/ebitengine/oto/actions/runs/38783327144), as did all nine fork checks (https://github.com/kumagi/oto/actions/runs/38783319555).

@hajimehoshi

Copy link
Copy Markdown
Member

Sorry for the unnecessary change request above. I overlooked that the buffer-duration sleep logic already existed and was being reused here. Please disregard my request to use a fixed 1 ms delay for both retry paths. If you have already applied that suggestion, please revert just that adjustment and retain the buffer-duration delay as originally proposed in this PR.

Comment authored by Codex on behalf of @hajimehoshi.

@hajimehoshi

Copy link
Copy Markdown
Member

One optional cleanup suggestion: could the shared buffer-duration calculation and sleep be extracted into a WinMM-specific helper, such as (*winmmContext).waitBeforeRetry(), called from both the MMSYSERR_NODRIVER path and the backoff after all available headers reject writes with MMSYSERR_NOMEM? This would preserve the existing delay while keeping the retry timing logic in one place.

Comment authored by Codex on behalf of @hajimehoshi.

@kumagi

kumagi commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor Author

Addressed the updated request in 1bd495b6dfc474c9fa4bf4c74774696b47995a17. The fixed 1 ms adjustment is reverted, and both retry paths now call the WinMM-specific waitBeforeRetry() helper, which uses the original buffer-duration calculation and sleep. Pending PCM retention and the rest of the write/error handling are unchanged.

Private regression tests using the actual methods with native writes and sleeping substituted fail on the fixed-delay head and pass afterward under race: both NODRIVER and all-headers-NOMEM paths request the original duration for normal and large buffers (1 ms and 500 ms). PCM retry ordering, alternate-header success and fatal termination still pass. Internal package race tests, the other vet analyzers, and Windows 386/amd64/arm64 builds passed. Native Windows playback, driver failure and actual scheduler sleep timing were not tested. All nine exact-head upstream checks passed (https://github.com/ebitengine/oto/actions/runs/38787774178), as did all nine fork checks (https://github.com/kumagi/oto/actions/runs/38787767559).

@hajimehoshi hajimehoshi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@hajimehoshi
hajimehoshi merged commit bc008e9 into ebitengine:main Oct 15, 2026
9 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