Add channel seek and playback position support#15
Open
applebutter180 wants to merge 3 commits into
Open
Conversation
Add public Channel APIs for seeking to an AmTime playback position and querying the current playback position. Wire the calls through channel state, real channels, and Amplimix cursor access using millisecond-based public units and frame-based mixer cursors internally. Keep v1 behavior scoped to single-layer channels and blended instancing, while warning and rejecting separate instancing. Reset converter and pipeline state on the mixer thread after cursor changes to avoid racing audio teardown, and preserve audio converter channel conversion state across resets. Add tests for normal seek, paused seek, playback-position advancement, blended instancing, separate-instancing rejection, and converter reset configuration preservation. Co-Authored-By: Codex GPT-5.5 High <[email protected]>
na2axl
requested changes
May 10, 2026
Member
na2axl
left a comment
na2axl
left a comment
There was a problem hiding this comment.
@applebutter180 thank you for your contribution. Here is a small initial review before further tests:
I don't recommend to set the cursor right away. It's better to make use of the command system in Amplimix to push a SetCursor+ResetState command to be executed right after the current mix loop iteration. That will ensure no possible reads of sound data during update. GetCursor() is safe since it only reads data.
You can do that directly in RealChannel::Seek(), with something like:
_mixer->PushCommand([this, layerId = data.mixerLayerId, cursor]() {
_mixer->SetCursor(_channelId, layerId, cursor);
_mixer->ResetLayerState(_channelId, layerId); // You may need to create this method
return true;
});Amplimix::ResetLayerState() will:
- Get the pointer to the layer
- Reset the data converter
- Reset the pipeline (but using
AmplimixLayerImpl::ResetPipeline()insted of raw reset)
Rename the public channel seek API to SetPlaybackPosition() while keeping the internal RealChannel::Seek() path, and update the public Doxygen style to match the SDK headers. Defer playback cursor changes through the Amplimix command queue and add ResetLayerState() so cursor updates reset the converter and pipeline after the current mix iteration. This follows maintainer feedback and avoids mutating mixer state directly during playback. Stabilize the playback-position and basic playback tests for CI, including the macOS timing-sensitive sound test, Ubuntu playback-position advance test, and Windows collection cleanup assertion. Add coverage tests for negative seek clamping, invalid/stale channel state rejection, and invalid mixer cursor operations. Co-Authored-By: Codex GPT-5.5 High <[email protected]>
na2axl
requested changes
May 10, 2026
Member
na2axl
left a comment
na2axl
left a comment
There was a problem hiding this comment.
LGTM! The remaining review comments are just esthetics. I will run some manual smoke tests locally before the final merge.
Comment on lines
+218
to
+219
| * If the audio on this channel | ||
| * doesn't support positional data, this method will return an invalid location. |
Member
There was a problem hiding this comment.
Revert accidental line break here
| AMPLIMIX_STORE(&lay->baseSampleRateRatio, baseRatio); | ||
| // store the initial value for sample rate ratio | ||
| AMPLIMIX_STORE(&lay->sampleRateRatio, baseRatio * pitch * speed); | ||
|
|
Comment on lines
+95
to
+96
| * @param position The playback | ||
| * position in milliseconds. |
Comment on lines
+105
to
+106
| * @return The current playback position in | ||
| * milliseconds. |
| AM_EXPECT(channel.GetInstancingMode() == eChannelInstanceMode_Blended); | ||
|
|
||
| constexpr AmTime seekPosition = 1000.0; | ||
| AM_EXPECT(channel.SetPlaybackPosition(seekPosition)); |
Member
There was a problem hiding this comment.
We can add an expectation that checks if the current playaback position is < than seekPosition before to set
| AM_EXPECT(channel.Playing()); | ||
|
|
||
| constexpr AmTime seekPosition = 1000.0; | ||
| AM_EXPECT(channel.SetPlaybackPosition(seekPosition)); |
Member
There was a problem hiding this comment.
We can add an expectation that checks if the current playback position is < than seekPosition before to set
| AM_EXPECT_EQ(channel.GetPlaybackState(), eChannelPlaybackState_Paused); | ||
|
|
||
| constexpr AmTime seekPosition = 25.0; | ||
| AM_EXPECT(channel.SetPlaybackPosition(seekPosition)); |
Member
There was a problem hiding this comment.
We can add an expectation that checks if the current playback position is < than seekPosition before to set
Undo accidental formatting changes in nearby documentation and mixer setup code. Add explicit pre-seek playback-position expectations to the seek tests so they verify the cursor actually moves forward from an earlier position. Co-Authored-By: Codex GPT-5.5 High <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add public Channel APIs for setting and querying playback position:
SetPlaybackPosition(AmTime) and GetPlaybackPosition(). The public API uses
AmTime milliseconds, while the mixer continues to use frame-based cursors
internally.
Keep v1 behavior scoped to single-layer channels and blended instancing.
Separate instancing remains unsupported for channel-level playback-position
control and is rejected with a warning. The internal RealChannel::Seek()
path is retained as the implementation detail behind the public API.
Defer playback cursor changes through the Amplimix command queue so cursor
updates run after the current mix iteration. Add AmplimixImpl::ResetLayerState()
to reset the layer data converter and pipeline through ResetPipeline() after
a cursor change, avoiding direct mixer-state mutation during audio rendering.
Add and update tests for normal playback-position changes, paused changes,
playback-position advancement, blended instancing, separate-instancing
rejection, negative position clamping, invalid/stale channel state rejection,
invalid mixer cursor operations, and CI-stable playback cleanup.
This should address #14