From 6de742b01b5373eb1796e4ed88e23eda1be2e4b9 Mon Sep 17 00:00:00 2001 From: nattthebear Date: Sat, 29 Aug 2020 12:42:14 -0400 Subject: [PATCH] Revert a series of questionable zwinder changes These all seem to have been made from the point of view of testing every possible value the UI allowed, and then fixing them all with as little effort as possible, with the fixes going deep in core logic instead of in the validation layer, and the fixes not making anything really "work"; just making it stop complaining. ZWinderBuffers now require TargetFrameLength >= 1 again: A value of zero doesn't make much sense here, and didn't actually behave differently than 1. ZWinderBuffers now require Size > 0 again: A size 0 buffer will never capture anything and has no value. If you don't want a buffer, don't make one at all. I believe that omitting some buffers might make sense for the state manager, maybe; those more familiar with all of its uses will have to chime in. If that is the case, then the state manager should not instantiate buffers it does not plan to use. ZWinderBuffers now throw exceptions again when a single state is bigger than the entire buffer: If you're in this situation, things are phenomenally broken and your buffer is never going to capture anything successfully. Users need to fix their settings in this case; they don't need to have a completely non functional system silently claim to work while not doing anything, leaving them befuddled as to why seeking is taking forever. --- .../movie/tasproj/ZwinderStateManager.cs | 15 +++--- .../rewind/ZwinderBuffer.cs | 51 ++++++------------- .../Movie/ZwinderStateManagerTests.cs | 4 +- 3 files changed, 24 insertions(+), 46 deletions(-) diff --git a/src/BizHawk.Client.Common/movie/tasproj/ZwinderStateManager.cs b/src/BizHawk.Client.Common/movie/tasproj/ZwinderStateManager.cs index 39aaeec908..ce750aee89 100644 --- a/src/BizHawk.Client.Common/movie/tasproj/ZwinderStateManager.cs +++ b/src/BizHawk.Client.Common/movie/tasproj/ZwinderStateManager.cs @@ -281,10 +281,11 @@ namespace BizHawk.Client.Common return; } - bool currentCaptured =_current.Capture(frame, + _current.Capture(frame, s => { source.SaveStateBinary(new BinaryWriter(s)); + StateCache.Add(frame); }, index => { @@ -298,10 +299,11 @@ namespace BizHawk.Client.Common return; } - bool recentCaptured = _recent.Capture(state.Frame, + _recent.Capture(state.Frame, s => { state.GetReadStream().CopyTo(s); + StateCache.Add(state.Frame); }, index2 => { @@ -316,12 +318,8 @@ namespace BizHawk.Client.Common AddToReserved(state2); } }); - if (recentCaptured) - StateCache.Add(state.Frame); }, force); - if (currentCaptured) - StateCache.Add(frame); } // Returns whether or not a frame has a reserved state within the frame interval on either side of it @@ -358,14 +356,13 @@ namespace BizHawk.Client.Common private void CaptureGap(int frame, IStatable source) { - bool captured = _gapFiller.Capture( + _gapFiller.Capture( frame, s => { + StateCache.Add(frame); source.SaveStateBinary(new BinaryWriter(s)); }, index => StateCache.Remove(index)); - if (captured) - StateCache.Add(frame); } public void Clear() diff --git a/src/BizHawk.Client.Common/rewind/ZwinderBuffer.cs b/src/BizHawk.Client.Common/rewind/ZwinderBuffer.cs index b1ba257f9b..0c8629a5a8 100644 --- a/src/BizHawk.Client.Common/rewind/ZwinderBuffer.cs +++ b/src/BizHawk.Client.Common/rewind/ZwinderBuffer.cs @@ -20,22 +20,23 @@ namespace BizHawk.Client.Common public ZwinderBuffer(IRewindSettings settings) { long targetSize = settings.BufferSize * 1024 * 1024; + if (settings.TargetFrameLength < 1) + { + throw new ArgumentOutOfRangeException(nameof(settings.TargetFrameLength)); + } Size = 1L << (int)Math.Floor(Math.Log(targetSize, 2)); _sizeMask = Size - 1; + _buffer = new MemoryBlock((ulong)Size); + _buffer.Protect(_buffer.Start, _buffer.Size, MemoryBlock.Protection.RW); _targetFrameLength = settings.TargetFrameLength; + _states = new StateInfo[STATEMASK + 1]; _useCompression = settings.UseCompression; - if (Size > 1) - { - _buffer = new MemoryBlock((ulong)Size); - _buffer.Protect(_buffer.Start, _buffer.Size, MemoryBlock.Protection.RW); - _states = new StateInfo[STATEMASK + 1]; - } } public void Dispose() { - _buffer?.Dispose(); + _buffer.Dispose(); } @@ -134,16 +135,15 @@ namespace BizHawk.Client.Common /// Maybe captures a state, if the conditions are favorable /// /// frame number to capture - /// will be called with the stream if capture is to be attempted + /// will be called with the stream if capture is to be performed /// - /// If provided, will be called with the index of states that are about to be removed. This will happen during + /// If provided, will be called with the index of states that are about to be removed. This will happen during /// calls to Write() inside `callback`, and any reuse of the old state will have to happen immediately /// - /// Returns true if the state was sucesfully captured; otherwise false. - public bool Capture(int frame, Action callback, Action indexInvalidated = null, bool force = false) + public void Capture(int frame, Action callback, Action indexInvalidated = null, bool force = false) { - if ((!force && !ShouldCapture(frame)) || _buffer == null) - return false; + if (!force && !ShouldCapture(frame)) + return; if (Count == STATEMASK) { @@ -158,7 +158,7 @@ namespace BizHawk.Client.Common Func notifySizeReached = () => { if (Count == 0) - return 0; + throw new IOException("A single state must not be larger than the buffer"); indexInvalidated?.Invoke(0); _firstStateIndex = (_firstStateIndex + 1) & STATEMASK; return Count > 0 @@ -177,19 +177,12 @@ namespace BizHawk.Client.Common callback(stream); } - if (stream.Length > Size) - { - Util.DebugWriteLine("Failed to capture; state size exceeds buffer size."); - return false; - } - _states[_nextStateIndex].Frame = frame; _states[_nextStateIndex].Start = start; _states[_nextStateIndex].Size = (int)stream.Length; _nextStateIndex = (_nextStateIndex + 1) & STATEMASK; //Util.DebugWriteLine($"Size: {Size >> 20}MiB, Used: {Used >> 20}MiB, States: {Count}"); - return true; } private Stream MakeLoadStream(int index) @@ -293,11 +286,8 @@ namespace BizHawk.Client.Common nextByte += _states[i].Size; } // TODO: Use spans to avoid this extra copy in .net core - if (nextByte > 0) - { - var dest = _buffer.GetStream(_buffer.Start, (ulong)nextByte, true); - WaterboxUtils.CopySome(reader.BaseStream, dest, nextByte); - } + var dest = _buffer.GetStream(_buffer.Start, (ulong)nextByte, true); + WaterboxUtils.CopySome(reader.BaseStream, dest, nextByte); } public static ZwinderBuffer Create(BinaryReader reader) @@ -376,16 +366,7 @@ namespace BizHawk.Client.Common { long requestedSize = _position + buffer.Length; while (requestedSize > _notifySize) - { _notifySize = _notifySizeReached(); - // 0 means abort, because the buffer is too small - if (_notifySize == 0) - { - // we set _position so that we can check later if the attempted write length was larger than the available buffer size - _position += buffer.Length; - return; - } - } long n = buffer.Length; if (n > 0) { diff --git a/src/BizHawk.Tests/Client.Common/Movie/ZwinderStateManagerTests.cs b/src/BizHawk.Tests/Client.Common/Movie/ZwinderStateManagerTests.cs index 68b2145f8c..8683d2c028 100644 --- a/src/BizHawk.Tests/Client.Common/Movie/ZwinderStateManagerTests.cs +++ b/src/BizHawk.Tests/Client.Common/Movie/ZwinderStateManagerTests.cs @@ -71,8 +71,8 @@ namespace BizHawk.Tests.Client.Common.Movie var stateCount = 0; for (int i = 0; i < 1000000; i++) { - if (zb.Capture(i, s => ss.SaveStateBinary(new BinaryWriter(s)), j => stateCount--, true)) - stateCount++; + zb.Capture(i, s => ss.SaveStateBinary(new BinaryWriter(s)), j => stateCount--, true); + stateCount++; } Assert.AreEqual(zb.Count, stateCount); }