Repository navigation
Read a resumed start off the ready item held paused (#502) - #513
Merged
Merged
Conversation
…ow enough to miss it SuperPlayerPlaybackTests read every start at the first poll that found the player playing and held it to a one-second window, which is a duration: a loaded host saw testOneFewerThanTheBoundIsStillRemembered 1.04 s late (#502). The start is now read once the item is ready and before play(), held to an AAC frame either side, and the player is then seen to play on from there. ADR-0021 rule 1 gains an addendum admitting the reading beside the old one, and the sibling suites that keep the old reading argue their window in place of the reference to the constant this removes. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01MJAhQmFgdRRAKUaMHibQLe
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
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.
Closes #502
Cause
SuperPlayerPlaybackTestsread every start withfirstPositionPlaying:play(), wait for the first 50 ms poll that findstimeControlStatus == .playing, then readcurrentTime(), held toexpected - 0.05 … expected + 1.0. That upper window is a duration: whatever the host spends between the player starting and the read is added to the position. A loadedios/checkrun saw the resume 1.04 s late. Five of the six tests in the class share the helper and therefore the race. The bare-URL test shares it too, through the same helper.Reproduction (before the fix)
I added
try await Task.sleep(for: .milliseconds(1_100))between the.playingwait and thecurrentTime()read infirstPositionPlaying. This makes the read lose the race on purpose, without adding host load. All five sharing tests then failed every time, for example:first seen playing at 3.622 s, expected 2.530 s(testOneFewerThanTheBoundIsStillRemembered)first seen playing at 1.082 s, expected 0.0 sfirst seen playing at 5.083 s, expected 4.0 s(testAnAtPositionStartsThere)Fix
The new
startedPosition(_:)waits for the item to reach.readyToPlay(the readiness reading ADR-0021's #401 addendum admits). It asserts the player is still.paused, then readscurrentTime(). Nothing moves the position at that point, however slow the host is. Only then does itplay(), wait for.playing, and assert that the player went on from there (playing >= start - frameTolerance). There is no later bound.assertStartedis nowXCTAssertEqual(position, expected, accuracy: frameTolerance), so the window is ±0.05 s where it used to be −0.05/+1.0.startToleranceis gone from this class.The same 1.1 s sleeps were then put before both reads in the fixed helper, and the whole class still passed.
Decisions argued
timeControlStatusandcurrentTime()are already admitted. It also records that the snapshot, pool and session suites keep the first-poll reading with their own window, and that each is a candidate for the held reading should it flake. Converting them is outside Flake (iOS): testOneFewerThanTheBoundIsStillRemembered misses its 1.0 s window under load #502's scope;PlaybackSessionPlaybackTestsis Flake (iOS): PlaybackSessionPlaybackTests reads a stale Now Playing elapsed time (12.0 vs 4.0) #504's.testABareURLItemHasNoIdentityAndResumesNothingused to replace a playing item. It still does, which keeps the only replace-while-playing case in the file. It now pauses on the same main-actor turn as thesetMediaRequest, before the new item can have loaded. Without that pause the new item inherits the rate and has no held start to read.SnapshotPlaybackTests,PlayerPoolPlaybackTestsandPlaybackSessionPlaybackTestsreferred toSuperPlayerPlaybackTests.startTolerancefor their own window's reason. Each now carries the reason itself. Only comments changed in those files, with no behaviour change; thePlaybackSessionPlaybackTestsedit is limited to the one comment, since Flake (iOS): PlaybackSessionPlaybackTests reads a stale Now Playing elapsed time (12.0 vs 4.0) #504 owns that file.docs/testing.md's iOS-harness paragraph says which suite reads which way.Review summary
// REPROsleeps were still present at review time (mid-experiment); they are not in the commit.startToleranceinPlayerPoolPlaybackTests: fixed.docs/testing.mdoverclaimed: they read as if every suite now used the held reading. Both are narrowed.firstPositionPlayingandassertStartedcopies across suites. They predate this change, and folding them together would touch Flake (iOS): PlaybackSessionPlaybackTests reads a stale Now Playing elapsed time (12.0 vs 4.0) #504's file..pausedcheck was added.play()is no longer bounded. This is noted in the helper's doc: it would be AVFoundation's doing, since SuperPlayer's seek is zero-tolerance and lands before the item is ready.Verification
xcodebuild test -test-iterations 20 -only-testing:SuperPlayerTestkitTests/SuperPlayerPlaybackTests/testOneFewerThanTheBoundIsStillRememberedon thesp-502simulator: 20 of 20 passed.SuperPlayerPlaybackTestsclass passed, both plainly and with the 1.1 s sleeps before both reads.SP_SIMULATOR=sp-502 ios/checkpassed.🤖 Generated with Claude Code
https://claude.ai/code/session_01MJAhQmFgdRRAKUaMHibQLe