diff --git a/Packages/ShowRecorderKit/Sources/Recording/SampleRing.swift b/Packages/ShowRecorderKit/Sources/Recording/SampleRing.swift index 03b3c2e9..a0185dd2 100644 --- a/Packages/ShowRecorderKit/Sources/Recording/SampleRing.swift +++ b/Packages/ShowRecorderKit/Sources/Recording/SampleRing.swift @@ -133,11 +133,20 @@ final class SampleRing: @unchecked Sendable { } /// Consumer: hands up to `maxFrames` of each channel to `body` (in up to two contiguous pieces) - /// and then releases them. Returns the number of frames consumed. + /// and then releases them. Returns the number of frames consumed. Never reads past the next drop, even when audio + /// has been stored after the drop since the caller last looked at it: its silence belongs where it fell (#734). @discardableResult func consume(maxFrames: Int, _ body: (_ channel: Int, _ samples: UnsafeBufferPointer) throws -> Void) rethrows -> Int { let start = read.load(ordering: .relaxed) - let frames = min(maxFrames, written.load(ordering: .acquiring) - start) + // `written` first: a drop at or before it was logged before the audio that follows it was stored, so it is seen. + var end = written.load(ordering: .acquiring) + let nextDropIndex = dropsRead.load(ordering: .relaxed) + if nextDropIndex < dropsLogged.load(ordering: .acquiring) { + let fell = dropStorage[(nextDropIndex % dropCapacity) * 2] + // `max` is only defensive: a drop before `start` has audio after it, so the writer has already taken it. + if fell < end { end = max(fell, start) } + } + let frames = min(maxFrames, end - start) guard frames > 0 else { return 0 } let offset = start % capacity let first = min(frames, capacity - offset) diff --git a/Packages/ShowRecorderKit/Tests/RecordingTests/SampleRingDropTests.swift b/Packages/ShowRecorderKit/Tests/RecordingTests/SampleRingDropTests.swift index 6c5fb43b..6e6514df 100644 --- a/Packages/ShowRecorderKit/Tests/RecordingTests/SampleRingDropTests.swift +++ b/Packages/ShowRecorderKit/Tests/RecordingTests/SampleRingDropTests.swift @@ -45,6 +45,43 @@ struct SampleRingDropTests { #expect(ring.nextDrop() != nil) } + @Test("Reading never goes past a drop, even when audio is stored after it between the writer's look and its read (#734)") + func readStopsAtTheDrop() { + let ring = SampleRing(channelCount: 1, capacity: 1_000) + write(800, to: ring) + write(300, to: ring) // dropped, after 800 frames + drain(700, from: ring) + #expect(ring.nextDrop() == nil, "the writer looks here: nothing stored after the drop yet") + write(100, to: ring) // arrives before the writer reads + + let taken = ring.consume(maxFrames: 4_096) { _, _ in } + #expect(taken == 100, "only up to the drop at 800, not the 100 stored after it") + #expect(ring.nextDrop() == SampleRing.Drop(storedFrames: 800, length: 300)) + } + + @Test("A drop with nothing stored after it yet doesn't hold back the audio before it") + func pendingDropDoesNotRestrict() { + let ring = SampleRing(channelCount: 1, capacity: 1_000) + write(800, to: ring) + write(300, to: ring) // dropped, nothing stored since + + #expect(ring.consume(maxFrames: 4_096) { _, _ in } == 800) + } + + @Test("Reading stops at each drop in turn, past the log's wraparound") + func manyDropsInTurn() { + let ring = SampleRing(channelCount: 1, capacity: 1_000, dropLogCapacity: 2) + for _ in 0..<5 { + write(100, to: ring) + write(950, to: ring) // dropped + write(100, to: ring) // stored after it + #expect(ring.consume(maxFrames: 4_096) { _, _ in } == 100, "up to the drop") + #expect(ring.nextDrop() != nil) + ring.consumeDrop() + #expect(ring.consume(maxFrames: 4_096) { _, _ in } == 100, "then the audio after it") + } + } + @Test("Blocks dropped one after another are one drop; the last one is available once the producer has stopped") func backToBackMerge() { let ring = SampleRing(channelCount: 1, capacity: 1_000) @@ -81,22 +118,21 @@ struct SampleRingDropTests { @Test("When the log is full the lost frames still add up exactly; the extra detail is only a total") func fullLog() { let ring = SampleRing(channelCount: 1, capacity: 1_000, dropLogCapacity: 2) - write(900, to: ring) - // Three separate drops, with a little audio stored between each (the reader keeps up with the audio only). + // Three separate drops, with a little audio stored between each, before the reader gets to any of them (it + // can't read past a drop, so they have to pile up in the ring). for _ in 0..<3 { - write(200, to: ring) // dropped: 100 free - drain(900, from: ring) - write(900, to: ring) + write(100, to: ring) + write(950, to: ring) // dropped: not enough room } - #expect(ring.overflowedFrames.load(ordering: .relaxed) == 600) + #expect(ring.overflowedFrames.load(ordering: .relaxed) == 2_850) var logged = 0 while let drop = ring.nextDrop() { logged += drop.length ring.consumeDrop() } - #expect(logged == 400, "two drops fit the log") - #expect(ring.takeUnloggedDroppedFrames() == 200, "the third is kept as a total") + #expect(logged == 1_900, "two drops fit the log") + #expect(ring.takeUnloggedDroppedFrames() == 950, "the third is kept as a total") #expect(ring.takeUnloggedDroppedFrames() == 0, "and handed over once") }