Merge #5773

5773: Various fixes for server demo recording and playback r=def- a=Robyt3

Closes #5362.

## Checklist

- [X] Tested the change ingame
- [ ] Provided screenshots if it is a visual change
- [X] Tested in combination with possibly related configuration options
- [X] Written a unit test (especially base/) or added coverage to integration test
- [X] Considered possible null pointers and out of bounds array indexing
- [X] Changed no physics that affect existing maps
- [ ] Tested the change with [ASan+UBSan or valgrind's memcheck](https://github.com/ddnet/ddnet/#using-addresssanitizer--undefinedbehavioursanitizer-or-valgrinds-memcheck) (optional)


Co-authored-by: Robert Müller <robytemueller@gmail.com>

scripts/integration_test.sh

@@ -159,6 +159,7 @@ function wait_for_fifo() {
 	done
 }
 
+echo "[*] launch server"
 $tool ../DDNet-Server \
 	"sv_input_fifo server.fifo;
 	sv_rcon_password rcon;
@@ -168,6 +169,7 @@ $tool ../DDNet-Server \
 	sv_register 0;
 	sv_port $port" > stdout_server.txt 2> stderr_server.txt || fail server "$?" &
 
+echo "[*] launch client 1"
 $tool ../DDNet \
 	"cl_input_fifo client1.fifo;
 	player_name client1;
@@ -179,12 +181,18 @@ $tool ../DDNet \
 
 if [ "$arg_valgrind_memcheck" == "1" ]; then
 	wait_for_fifo client1.fifo 120
-	sleep 1
+	sleep 20
 else
 	wait_for_fifo client1.fifo 50
 	sleep 1
 fi
 
+echo "[*] start demo recording"
+echo "record server" > server.fifo
+echo "record client1" > client1.fifo
+sleep 1
+
+echo "[*] launch client 2"
 $tool ../DDNet \
 	"cl_input_fifo client2.fifo;
 	player_name client2;
@@ -225,6 +233,7 @@ say "/mc
 ;saytime"
 EOF
 sleep 1
+
 echo "[*] test rcon commands"
 tr -d '\n' > client1.fifo << EOF
 rcon say hello from admin;
@@ -235,10 +244,33 @@ muteid 1 900 spam;
 unban_all;
 EOF
 sleep 1
-echo "[*] test map change"
-echo "rcon sv_map Tutorial" > client1.fifo
+
+echo "[*] stop demo recording"
+echo "stoprecord" > server.fifo
+echo "stoprecord" > client1.fifo
 sleep 1
 
+echo "[*] test map change"
+echo "rcon sv_map Tutorial" > client1.fifo
+if [ "$arg_valgrind_memcheck" == "1" ]; then
+	sleep 30
+else
+	sleep 15
+fi
+
+echo "[*] play demos"
+echo "play demos/server.demo" > client1.fifo
+echo "play demos/client1.demo" > client2.fifo
+if [ "$arg_valgrind_memcheck" == "1" ]; then
+	sleep 20
+else
+	sleep 5
+fi
+
+# Kill all processes first so all outputs are fully written
+kill_all
+wait
+sleep 1
 
 # TODO: remove the first grep after https://github.com/ddnet/ddnet/pull/5036 is merged
 if ! grep -qE '^\[[0-9]{4}-[0-9]{2}-[0-9]{2} ([0-9]{2}:){2}[0-9]{2}\]\[chat\]: 0:-2:client1: hello world$' server.log && \
@@ -247,6 +279,7 @@ then
 	touch fail_chat.txt
 	echo "[-] Error: chat message not found in server log"
 fi
+
 if ! grep -q 'cmdlist' client1.log || \
 	! grep -q 'pause' client1.log || \
 	! grep -q 'rank' client1.log || \
@@ -262,10 +295,16 @@ then
 	echo "[-] Error: admin message not found in server log"
 fi
 
-kill_all
-wait
-
-sleep 1
+if ! grep -q "\[demo_player\]: Stopped playback" client1.log
+then
+	touch fail_demo_server.txt
+	echo "[-] Error: demo playback of server demo in client 1 was not started/finished"
+fi
+if ! grep -q "\[demo_player\]: Stopped playback" client2.log
+then
+	touch fail_demo_client.txt
+	echo "[-] Error: demo playback of client demo in client 2 was not started/finished"
+fi
 
 ranks="$(sqlite3 ddnet-server.sqlite < <(echo "select * from record_race;"))"
 num_ranks="$(echo "$ranks" | wc -l | xargs)"

src/engine/client/client.cpp

@@ -1299,7 +1299,6 @@ static void FormatMapDownloadFilename(const char *pName, const SHA256_DIGEST *pS
 
 const char *CClient::LoadMapSearch(const char *pMapName, SHA256_DIGEST *pWantedSha256, int WantedCrc)
 {
-	const char *pError = 0;
 	char aBuf[512];
 	char aWanted[SHA256_MAXSTRSIZE + 16];
 	aWanted[0] = 0;
@@ -1316,15 +1315,15 @@ const char *CClient::LoadMapSearch(const char *pMapName, SHA256_DIGEST *pWantedS
 
 	// try the normal maps folder
 	str_format(aBuf, sizeof(aBuf), "maps/%s.map", pMapName);
-	pError = LoadMap(pMapName, aBuf, pWantedSha256, WantedCrc);
+	const char *pError = LoadMap(pMapName, aBuf, pWantedSha256, WantedCrc);
 	if(!pError)
-		return pError;
+		return nullptr;
 
 	// try the downloaded maps
 	FormatMapDownloadFilename(pMapName, pWantedSha256, WantedCrc, false, aBuf, sizeof(aBuf));
 	pError = LoadMap(pMapName, aBuf, pWantedSha256, WantedCrc);
 	if(!pError)
-		return pError;
+		return nullptr;
 
 	// backward compatibility with old names
 	if(pWantedSha256)
@@ -1332,16 +1331,22 @@ const char *CClient::LoadMapSearch(const char *pMapName, SHA256_DIGEST *pWantedS
 		FormatMapDownloadFilename(pMapName, 0, WantedCrc, false, aBuf, sizeof(aBuf));
 		pError = LoadMap(pMapName, aBuf, pWantedSha256, WantedCrc);
 		if(!pError)
-			return pError;
+			return nullptr;
 	}
 
 	// search for the map within subfolders
 	char aFilename[IO_MAX_PATH_LENGTH];
 	str_format(aFilename, sizeof(aFilename), "%s.map", pMapName);
 	if(Storage()->FindFile(aFilename, "maps", IStorage::TYPE_ALL, aBuf, sizeof(aBuf)))
+	{
 		pError = LoadMap(pMapName, aBuf, pWantedSha256, WantedCrc);
+		if(!pError)
+			return nullptr;
+	}
 
-	return pError;
+	static char s_aErrorMsg[256];
+	str_format(s_aErrorMsg, sizeof(s_aErrorMsg), "Could not find map '%s'", pMapName);
+	return s_aErrorMsg;
 }
 
 void CClient::ProcessConnlessPacket(CNetChunk *pPacket)
@@ -3903,7 +3908,10 @@ const char *CClient::DemoPlayer_Play(const char *pFilename, int StorageType)
 	if(pError)
 	{
 		if(!m_DemoPlayer.ExtractMap(Storage()))
+		{
+			DisconnectWithReason(pError);
 			return pError;
+		}
 
 		Sha = m_DemoPlayer.GetMapInfo()->m_Sha256;
 		pError = LoadMapSearch(pMapInfo->m_aName, &Sha, Crc);

src/engine/server.h

@@ -229,7 +229,6 @@ public:
 	virtual void ChangeMap(const char *pMap) = 0;
 
 	virtual void DemoRecorder_HandleAutoStart() = 0;
-	virtual bool DemoRecorder_IsRecording() = 0;
 
 	// DDRace
 

src/engine/server/server.cpp

@@ -799,9 +799,13 @@ int CServer::SendMsg(CMsgPacker *pMsg, int Flags, int ClientID)
 		if(RepackMsg(pMsg, Pack7, true))
 			return -1;
 
-		// write message to demo recorder
+		// write message to demo recorders
 		if(!(Flags & MSGFLAG_NORECORD))
-			m_aDemoRecorder[MAX_CLIENTS].RecordMessage(Pack6.Data(), Pack6.Size());
+		{
+			for(auto &Recorder : m_aDemoRecorder)
+				if(Recorder.IsRecording())
+					Recorder.RecordMessage(Pack6.Data(), Pack6.Size());
+		}
 
 		if(!(Flags & MSGFLAG_NOSEND))
 		{
@@ -837,9 +841,12 @@ int CServer::SendMsg(CMsgPacker *pMsg, int Flags, int ClientID)
 			return 0;
 		}
 
+		// write message to demo recorders
 		if(!(Flags & MSGFLAG_NORECORD))
 		{
+			if(m_aDemoRecorder[ClientID].IsRecording())
 				m_aDemoRecorder[ClientID].RecordMessage(Pack.Data(), Pack.Size());
+			if(m_aDemoRecorder[MAX_CLIENTS].IsRecording())
 				m_aDemoRecorder[MAX_CLIENTS].RecordMessage(Pack.Data(), Pack.Size());
 		}
 
@@ -3252,11 +3259,6 @@ void CServer::DemoRecorder_HandleAutoStart()
 	}
 }
 
-bool CServer::DemoRecorder_IsRecording()
-{
-	return m_aDemoRecorder[MAX_CLIENTS].IsRecording();
-}
-
 void CServer::SaveDemo(int ClientID, float Time)
 {
 	if(IsRecording(ClientID))
@@ -3267,7 +3269,7 @@ void CServer::SaveDemo(int ClientID, float Time)
 		char aOldFilename[IO_MAX_PATH_LENGTH];
 		char aNewFilename[IO_MAX_PATH_LENGTH];
 		str_format(aOldFilename, sizeof(aOldFilename), "demos/%s_%d_%d_tmp.demo", m_aCurrentMap, m_NetServer.Address().port, ClientID);
-		str_format(aNewFilename, sizeof(aNewFilename), "demos/%s_%s_%5.2f.demo", m_aCurrentMap, m_aClients[ClientID].m_aName, Time);
+		str_format(aNewFilename, sizeof(aNewFilename), "demos/%s_%s_%05.2f.demo", m_aCurrentMap, m_aClients[ClientID].m_aName, Time);
 		Storage()->RenameFile(aOldFilename, aNewFilename, IStorage::TYPE_SAVE);
 	}
 }

src/engine/server/server.h

@@ -287,7 +287,6 @@ public:
 	void Ban(int ClientID, int Seconds, const char *pReason) override;
 
 	void DemoRecorder_HandleAutoStart() override;
-	bool DemoRecorder_IsRecording() override;
 
 	//int Tick()
 	int64_t TickStartTime(int Tick);

src/engine/shared/demo.cpp

@@ -124,7 +124,9 @@ int CDemoRecorder::Start(class IStorage *pStorage, class IConsole *pConsole, con
 		CloseMapFile = true;
 	}
 
-	if(MapFile)
+	if(m_NoMapData)
+		MapSize = 0;
+	else if(MapFile)
 		MapSize = io_length(MapFile);
 
 	// write header

src/game/server/gamecontext.cpp

@@ -3734,7 +3734,7 @@ void CGameContext::OnSnap(int ClientID)
 {
 	// add tuning to demo
 	CTuningParams StandardTuning;
-	if(ClientID == -1 && Server()->DemoRecorder_IsRecording() && mem_comp(&StandardTuning, &m_Tuning, sizeof(CTuningParams)) != 0)
+	if(Server()->IsRecording(ClientID > -1 ? ClientID : MAX_CLIENTS) && mem_comp(&StandardTuning, &m_Tuning, sizeof(CTuningParams)) != 0)
 	{
 		CMsgPacker Msg(NETMSGTYPE_SV_TUNEPARAMS);
 		int *pParams = (int *)&m_Tuning;