Skip to content

@remotion/player: Fix audio hiccups on rapid play/pause toggling#9041

Closed
iliya-vidrush wants to merge 7 commits into
remotion-dev:mainfrom
iliya-vidrush:audio-hiccups-on-rapid-play/pause-toggle
Closed

@remotion/player: Fix audio hiccups on rapid play/pause toggling#9041
iliya-vidrush wants to merge 7 commits into
remotion-dev:mainfrom
iliya-vidrush:audio-hiccups-on-rapid-play/pause-toggle

Conversation

@iliya-vidrush

@iliya-vidrush iliya-vidrush commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #8984

Problem

Rapidly toggling play/pause in the Player caused audible audio hiccups: the sound stopped and restarted stepwise instead of settling to the final state, and every restart faded in.

Two independent mechanisms caused this:

  1. resume() in shared-audio-tags.tsx ramped the master gain 0 to 1 over 30 ms on every resume, so every play toggle produced an audible dip - even when the context merely unfroze nodes that suspend() had frozen in place sample-exact.
  2. The statechange listener in use-playback.ts force-moved the audio sync anchor on every suspend. It never notified the media players, so the already-queued (frozen) nodes stayed scheduled against the old anchor and were misaligned by the shift after resume.

Changes

  • remotion: resume() only applies the fade-in when there are nodes in nodesToResume that need to be started. Those begin mid-waveform and can click, which is what the fade is for. A plain unfreeze is sample-exact and no longer fades.
  • @remotion/player: the suspend re-anchor now goes through the normal drift guard (ALLOWED_GLOBAL_TIME_ANCHOR_SHIFT + latency) instead of force: true. On a plain pause the drift is below the guard, so the anchor stays put and pause/play becomes a pure freeze/unfreeze of the same scheduled nodes - no teardown, no re-prime. If the drift does exceed the guard, the anchor is corrected and 'changed' is dispatched so the media players rebuild their queue against the new anchor (previously the anchor moved silently, leaving the frozen queue misaligned).
  • @remotion/player: removed the now-unused force option from setGlobalTimeAnchor.
  • Type cleanups: extracted the inline option types in shared-audio-tags.tsx, set-global-time-anchor.ts and use-playback.ts into named types. No behavior change.

Tradeoffs

  • Small drift (below the guard) is no longer zeroed on every pause. It can accumulate across many toggles until the guard trips, which then triggers one clean re-anchor + rebuild. The maximum tolerated A/V drift is unchanged - it is the same guard the seek path already applies during playback. The old per-pause "correction" was itself defective, since it de-synced the frozen queue by the shift.
  • A pure resume no longer fades in, so sound starts instantly at a non-zero-crossing - the same kind of step as the existing abrupt stop on pause (there was never a fade-out). Browsers apply their own short ramp on context suspend/resume, so I expect this to be inaudible, but it is the one thing that needs a listen test.

Testing

Only apply the 30ms 0-to-1 gain ramp in resume() when there are nodes
in nodesToResume that need to be started. Those begin mid-waveform and
can click, which is what the fade is meant to mask.

If there is nothing to start, resume() merely unfreezes the nodes that
suspend() froze in place, which continues sample-exact. Fading in that
case caused an audible dip on every play/pause toggle.

Ref remotion-dev#8984
Route the statechange re-anchor through the normal drift guard instead
of forcing it. A plain pause does not move the playhead, so the anchor
stays put and the queued audio nodes remain valid - suspend/resume
becomes a freeze/unfreeze of the same nodes without a teardown.

If the drift does exceed the guard, re-anchor and dispatch 'changed' so
the media players rebuild their queue against the new anchor.
Previously the anchor was moved silently on every suspend, leaving the
frozen queue misaligned by the shift.

Ref remotion-dev#8984
All call sites now go through the drift guard, so drop the force
parameter together with the force-only zero-shift check and the
'forcibly' log wording.

Ref remotion-dev#8984
Move the inline parameter type of setGlobalTimeAnchor into a named
type for readability. No behavior change.
Name the repeated inline object types: RegisterAudioOptions (shared by
the context value, registerAudio and useSharedAudio), UpdateAudioOptions
(RegisterAudioOptions plus id) and the props types of the two
providers. No behavior change.
@vercel

vercel Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
bugs Ready Ready Preview, Comment Jul 13, 2026 6:39am
remotion Ready Ready Preview, Comment Jul 13, 2026 6:39am

Request Review

Name the usePlayback parameter object (UsePlaybackOptions) and the
raf/timeout union used for the queued frame call (QueuedFrameCall).
No behavior change.
@iliya-vidrush iliya-vidrush changed the title Audio hiccups on rapid play/pause toggle @remotion/player: Fix audio hiccups on rapid play/pause toggling Jul 13, 2026
@iliya-vidrush
iliya-vidrush marked this pull request as ready for review July 13, 2026 04:20

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed changes — fix rapid play/pause audio hiccups in the Player by conditionally skipping the resume fade-in and removing the forced audio-context-suspend re-anchor.

  • packages/core/src/audio/shared-audio-tags.tsx — only applies the 30 ms master-gain fade-in when nodesToResume has entries; pure resume/unfreeze paths no longer dip the gain, which removes the per-play fade artifact.
  • packages/player/src/set-global-time-anchor.ts — removes the force option and the force-specific logging, making every anchor update go through the same drift guard.
  • packages/player/src/use-playback.ts — on statechange to suspended/running-to-suspended, setGlobalTimeAnchor is now called without force; if the drift exceeds the guard it dispatches 'changed', otherwise the queued nodes stay in place.

⚠️ Suspend statechange handler reads live frame, which can mis-represent a pure pause as drift

On a plain pause, suspend() is requested immediately, but the statechange event fires asynchronously once the context actually suspends. By then the Player may have rendered additional frames (or already be resuming). The handler calls getCurrentFrame() at event time to compute the anchor, but the frozen audio nodes were scheduled against the frame that was current when suspend() was requested, not against whatever frame is current when the event eventually fires.

If those two frames differ, setGlobalTimeAnchor can report a shift that is larger than the guard even though the audio queue itself is still aligned with the original anchor. Because this path now dispatches 'changed' on large shifts, it can tear down and rebuild the audio iterator in the middle of a rapid pause/play burst — reintroducing the kind of hiccup the PR is meant to remove.

This is most likely to happen when the user toggles play/pause faster than the audio context's suspend latency.

Technical details
# Suspend statechange handler reads live frame

## Affected sites
- `packages/player/src/use-playback.ts:144``absoluteTimeInSeconds: getCurrentFrame() / config.fps` uses the current frame at event time, not the frame that was current when the suspend was requested.
- `packages/player/src/use-playback.ts:153` — if `setGlobalTimeAnchor` returns `true`, the handler dispatches `'changed'`, which in `@remotion/media` triggers `audioSyncAnchorChanged()` -> `destroyIterator()` -> re-prime.

## Required outcome
A pure pause/play cycle should not trigger an anchor change or audio queue rebuild.
The anchor computation on `statechange` should reflect the playhead position that the currently queued audio nodes were scheduled against.

## Suggested approach
Capture the frame at the moment `suspend()` is requested (or at least at the moment playback transitions from playing to paused) in a ref, and use that ref value in the `statechange` handler instead of `getCurrentFrame()`.

For example, the effect that handles `playing` changes could store the current frame in a ref when it calls `sharedAudioContext.suspend()`, and the `statechange` handler could read that ref.

## Open questions for the human
- Is the current behavior already known to be safe on all target browsers, or has it been observed to re-hiccup in practice during very rapid toggling?
- Does the seek effect (keyed on `frame`) already provide enough correction when a real seek happens during the suspended window?

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

Comment thread packages/player/src/use-playback.ts
Comment thread packages/player/src/set-global-time-anchor.ts
Comment thread packages/player/src/use-playback.ts
…ive frame

The statechange handler computed the audio time anchor from
getCurrentFrame() at the moment the 'statechange' event fired. That
event is async relative to the suspend() request, so during a rapid
play/pause burst it could read a frame from an already-resuming state,
measure spurious drift, and dispatch 'changed' - tearing down and
rebuilding the queued audio nodes even though no seek occurred.

Capture the frame at each suspend() request in a ref and re-anchor
against that instead. The suspended nodes were scheduled against that
frame, so it is the correct reference and a plain pause/play cycle no
longer trips the drift guard.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

test

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

Comment thread packages/player/src/use-playback.ts
@pullfrog

pullfrog Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Important

The latest commit (c0e9a30) correctly captures the frame at the moment of the explicit suspend() request, addressing the prior review's concern about using the live frame at statechange time. One remaining issue: suspendRequestFrame is never reset after an explicit pause/resume cycle, so a later browser-initiated AudioContext suspend could re-anchor against a stale frame.

Reviewed changes (since prior Pullfrog review) — Added suspendRequestFrame so the statechange handler re-anchors against the frame that was current when suspend() was requested, and dispatches 'changed' only when the anchor actually moves past the drift guard.

  • packages/player/src/use-playback.tssuspendRequestFrame ref is set before each Remotion-initiated suspend() and consumed in the statechange handler.
  • packages/player/src/use-playback.tssetGlobalTimeAnchor is no longer called with force: true; the drift guard plus a 'changed' dispatch replace the forced re-anchor.
  • packages/core/src/audio/shared-audio-tags.tsx and packages/player/src/set-global-time-anchor.ts — type cleanups.

⚠️ Stale suspendRequestFrame on browser-initiated suspend

suspendRequestFrame.current is only overwritten right before Remotion-initiated suspend() calls (lines 185, 216, 276). After the user resumes playback, the ref keeps the frame from the last explicit pause. If the browser later auto-suspends the AudioContext (e.g., tab backgrounding or iOS audio policy), the statechange handler still runs and re-anchors against that old frame instead of falling back to getCurrentFrame() via the ?? operator — because the ref is no longer null.

That can move the anchor to the wrong time and, because the handler now dispatches 'changed' on a large shift, trigger an unnecessary audio queue rebuild.

Technical details
# Stale `suspendRequestFrame` on browser-initiated suspend

## Affected sites
- `packages/player/src/use-playback.ts:81` — ref declaration
- `packages/player/src/use-playback.ts:154` — consumes the ref in the statechange handler
- `packages/player/src/use-playback.ts:185` — sets ref when `!playing`
- `packages/player/src/use-playback.ts:216` — sets ref when `!isPlaying()`
- `packages/player/src/use-playback.ts:276` — sets ref during buffering

## Required outcome
- A browser-initiated `AudioContext` suspend must not re-anchor against a stale frame from an earlier Remotion-initiated pause.
- The explicit suspend path must continue to use the frame captured at the moment `suspend()` is requested.

## Suggested approach
Reset `suspendRequestFrame.current = null` when playback resumes, so the `?? getCurrentFrame()` fallback is used for auto-suspends. Add the reset in two places:
- At the top of the `playing` branch in `useEffect` (after the `if (!playing) { ... return; }` block).
- In the effect's cleanup function.
This mirrors the fact that the ref is only meaningful while transitioning into a Remotion-requested suspend.

ℹ️ Nitpicks

  • packages/player/src/use-playback.ts:276 — The buffering suspend site only sets suspendRequestFrame.current inside the !muted branch, while the other two suspend sites set it unconditionally. Consider moving the assignment outside the muted check for consistency.

Pullfrog  | View workflow run | via Pullfrog | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@samohovets samohovets self-assigned this Jul 15, 2026
@samohovets

Copy link
Copy Markdown
Member

Thanks for taking on this! I can still reproduce the issue though (0:08 in the video)

The issue is reproducing both when the external audio interface is selected and the embedded one (MacBook speakers)

audio-choppiness.mp4

@JonnyBurger

Copy link
Copy Markdown
Member

We are more aggressively closing AI-generated PRs if they are not obvious and in this case, we found an issue.
Please make a better repro / better demonstrate that this solves an issue / make a smaller PR with more obvious intention, it will be easier to merge for us. Thanks nonetheless!

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.

@remotion/media: Audio hiccups on rapid play/pause toggling in the Player

3 participants