Demos: Fix keyframe start point

[Mushman]

When writing demo files, after writing the header, DSDA will then write the keyframe buffer length to the next 4 bytes, before writing the keyframe itself. However, when loading demos with keyframes, it wasn't shifting the keyframe buffer pointer past these 4 bytes for the buffer length, causing a premature crash when loading due to using misaligned values.

Testing

Use these two demo files for Doom 1 E1M1:

1. Pistol start on Nightmare, without any keyframes
2. Loaded game before the first door, with a keyframe
   • Made possible via an as yet uncommited improvement on my Always Record branch
   • Successfully tested playing demos initiated from the in-game console using demo.start too (I haven't uploaded a file for this)

[Pedro-Beirao]

In resume-00002.lmp, look at the ground: the flat is wrong.
On first look the demo looked fine, but I could tell that something was off 😅. Its hard to notice in this case, but I tried with this Doom 2 Map01 demo, and its much more obvious wrong.lmp.zip

I'll assume that when this was first implemented, it worked fine. So why is it crashing now, and why is only now that this padding is needed?
Are the texture errors caused by this padding?

Screenshots

[Mushman]

Thanks for the feedback. I hadn't checked more extensively after my simple testing, sorry.

The reason I felt this was the problem was when I was analysing the memory as DSDA was loading a demo, it was reading the basic game flags for skill, map, and player flags early, which didn't immediately break things, but after would attempt to load the music state, which should read FF as per the demo, but was often reading 00, or a negative 32-bit integer due to uninitalised memory, and crashing while trying to handle the music queue. After going back and forth many times, I realised that the demo loading wasn't accounting for the bytes for storing the tic count. Shifting the pointer forward to the correct demo buffer start point avoided this crash, while also aligning the bytes properly, in particular the skill and map that I was using as for reference.

However, I can't argue with the obvious brokenness you found. It seems there is a memory corruption elsewhere, or I broke something without realising. It may be progress to solving the issue insomuch as it's no longer crashing, but clearly this PR not ready yet. Apologies for the mistake!

[Mushman]

Update

After some further testing with @Pedro-Beirao, it appears that the issue they are seeing is a pre-existing issue with demo playback on arm64 platforms, such as MacOS. They tested recording, and playing back a demo on master under arm64, and found the flat corruption issue was still present there.

In my retesting with these 2 new demos*, I was not able to reproduce the issue on Windows x64, either with a local build, or the MSVC build from GitHub CI. We will probably need other users to help us test this PR's changes, please!

*I recorded these demos under the following conditions:

1. flat.lmp
   • Using the -record CLI argument
   • Does not contain a keyframe
2. flat-00002.lmp
   • From the in-game console's demo.start command
   • Has a keyframe

Thanks for being so patient! We might be able to use this additional reserach to plan out a fix for this other notable bug we found along the way...

[andrey-budko]

I didn't try to understand why, but this patch fixes the floor/ceiling issue for me (win10 x64, msys2/mingw)

diff --git a/prboom2/src/p_saveg.c b/prboom2/src/p_saveg.c
index f928b410d..447741896 100644
--- a/prboom2/src/p_saveg.c
+++ b/prboom2/src/p_saveg.c
@@ -264,7 +264,9 @@ void P_UnArchiveWorld (void)
     P_LOAD_X(sec->floorheight);
     P_LOAD_X(sec->ceilingheight);
     P_LOAD_X(sec->floorpic);
+    sec->floorpic++;
     P_LOAD_X(sec->ceilingpic);
+    sec->ceilingpic++;
     P_LOAD_X(sec->lightlevel);
     P_LOAD_X(sec->special);
     P_LOAD_X(sec->tag);