0000970: [Demos] Playback requirements identification improvements

(0004382)
Dusk (developer)
2012-08-17 13:46
edited on: 2012-08-17 14:32

I had planned on getting demo WADs stored in the demo rather than relying on the filename for them.. guess it's about time to get that done.

As for >"CLIENTDEMO_ProcessDemoHeader: Expected CLD_DEMOSTART.", I don't know what to do about this. The format obviously changed but I don't know the details of the older demo formats.

EDIT: OK that's done now. I left the changelog entry out for now - if Torr wants this in 1.0 I'll add it and commit this to a new head.

(0004392)
Torr Samaho (administrator)
2012-08-18 19:21

If we add this kind of wad check, we should also consider checking everything. For instance the order in which the wads are loaded may be very important. Your check also allows the player to load more wads that may break the demo. Do we want the check to allow this kind of things? We could also just let the check print visible warnings instead of erroring out.

BTW: I think the memset + strncpy construction is not necessary. memset clears much more than necessary. Anyway, the whole thing would be much easier if you use TArray<FString> to store szWADNames.

(0004396)
Dusk (developer)
2012-08-18 23:04

Quote
Your check also allows the player to load more wads that may break the demo.

I was doing that to allow the player to auto-load wads without breaking the demo but that can actually be checked for..

Quote
For instance the order in which the wads are loaded may be very important.

Hmm. Maybe use the authentication checksum for the actual check and print wad list if the check fails?

Quote
BTW: I think the memset + strncpy construction is not necessary. memset clears much more than necessary. Anyway, the whole thing would be much easier if you use TArray<FString> to store szWADNames.

Hmm. Probably. :P

(0004679)
Dusk (developer)
2012-09-16 01:55

Alright, how's this?

(0004681)
Torr Samaho (administrator)
2012-09-16 07:11
edited on: 2012-09-16 09:38

Much better, but

    ULONG ulWADCount = NETWORK_ReadShort( &g_ByteStream );

    // Read the names of WADs and store them in the array
    FString WadNames[ulWADCount];

doesn't work. It doesn't even compile under VC++ as the size of C arrays has to be known at compile time. As I said earlier, use a TArray to store the strings, i.e. replace FString WadNames[ulWADCount] by TArray<FString> and use "Push" to populate the array.

EDIT: BTW: I think putting dates in the comment is pointless. They are bound not to be updated when something is changed, make things less readable and it's usually easier to use "hg annotate" to find out when a change was made.

(0004685)
Dusk (developer)
2012-09-16 10:08

Hm. worked fine in GCC...

(0004688)
Torr Samaho (administrator)
2012-09-16 10:19

I'm pretty sure that the C standard only requires arrays whose size is known at compile time to work. VC++ 2005 definitely doesn't compile this.

(0004690)
Dusk (developer)
2012-09-16 10:48
edited on: 2012-09-16 10:49

Changed into a TArray

Also lifted dates from comments.

(0004693)
Torr Samaho (administrator)
2012-09-16 11:23

The code looks good now, but I just noticed a problem when testing your latest patch: The authentication doesn't check the maps at all. So you can still play a demo that was recorded on the completely wrong map since g_lumpsAuthenticationChecksum only checks protected non-map lumps.

(0004699)
Dusk (developer)
2012-09-16 12:02
edited on: 2012-09-16 12:03

That's certainly a bit problematic. Should there be some map checksum kinda thing that would be the checksum of every map loaded, and have that checksum be stored after the network checksum in the demo?

(0004702)
Torr Samaho (administrator)
2012-09-16 12:44

Doing a per map checksum would be the way that's most consistent with how clients and server do the authentication. The client could save the hashes it sends to the server in the demo and then check the maps while playing the demo to the stored hashes.

(0004704)
Dusk (developer)
2012-09-16 13:14
edited on: 2012-09-16 13:15

But then you could run into authentication errors when the demo has already started up. Could get a little frustrating when the demo suddenly bombs out.

(0004706)
Torr Samaho (administrator)
2012-09-16 13:26

That's also true. From the top of my head some alternatives are
- only check the first map (bombing out is replaced by an unwatchable demo on problematic maps which is not really less frustrating)
- build a single MD5 hash of all loaded wads (taking into account the order in which the maps are loaded, similar to how g_lumpsAuthenticationChecksum is built) and check this hash
- check the MD5 hash of all loaded wads and their loading order
With the latter two you should ignore the auto loaded wads (g_lumpsAuthenticationChecksum takes care of this)

(0004707)
Dusk (developer)
2012-09-16 14:06
edited on: 2012-09-16 14:31

Or maybe this?
'http://pastebin.com/VMryWfKf [^]'

Based it off listmaps and auto-compatibility code.

EDIT: Using NETWORK_GenerateMapLumpMD5Hash (mdata, mdata->lumpnum, sum); seemed to get me d41d8cd98f00b204e9800998ecf8427e for every map entry. Maybe I'm using it wrong?
EDIT 2: ... I certainly am... but I dunno is it worth getting it to work properly when the bit from compatibility.cpp works fine in substitution as well.

EDIT 3:
OK it worx (aside from the style I forgot to fix up..)
Now I just think, maybe this could be merged into the network authentication checksum, or is there a reason for network authentication only testing the current map?

(0004708)
Torr Samaho (administrator)
2012-09-16 16:56
edited on: 2012-09-16 16:57

Quote from Dusk
OK it worx (aside from the style I forgot to fix up..)

Seems to work nicely. What would still be nice is if this checksum is only calculated when a demo is recorded or played. I don't know the performance penalty the checksum calculation has on weak machines on large map sets, so I think it's safer to only calculate it when you really need it.

Quote from Dusk
Now I just think, maybe this could be merged into the network authentication checksum, or is there a reason for network authentication only testing the current map?

This would kill Freedoom/Doom network compatibility completely (now it just kills it for demos, but at least it can be worked around with demo_pure 0 which should be sufficient). Also it's not necessary to prevent somebody from joining when a map differs between client and server that will not be played anyway. The current system works fine and we don't really gain anything by replacing it (except that the startup will be a slower to a still unknown degree and a slight code simplification).

(0004712)
Dusk (developer)
2012-09-16 17:57

Quote
This would kill Freedoom/Doom network compatibility completely

Hm, that's right.

Quote
What would still be nice is if this checksum is only calculated when a demo is recorded or played. I don't know the performance penalty the checksum calculation has on weak machines on large map sets, so I think it's safer to only calculate it when you really need it.

That is done.
If this is good now, I'll collect this into a new single commit that can be pulled up.

(0004713)
Torr Samaho (administrator)
2012-09-16 18:03

Looks good :).

(0004718)
Dusk (developer)
2012-09-20 10:53

This was pulled during tracker downtime.

(0004755)
Watermelon (developer)
2012-09-23 17:06

How can I test this? Do I need to create a demo with the latest version and then check it against different wads or another version?

(0004766)
Torr Samaho (administrator)
2012-09-23 18:34

You can use this binary. And you guessed right, you need to record demos with the new binary and then try to play them with the correct and with wrong wads.

(0004776)
unknownna (updater)
2012-09-23 20:00

Will the console tell you which version of Zandronum the demo was recorded with if you try to play an old demo with a newer version of Zandronum?

It seems that you're still able to play demos even if you don't load all files, e.g., music files.

(0004781)
Dusk (developer)
2012-09-23 20:24
edited on: 2012-09-23 21:55

The thing uses special lump and map collection authentication for the check. Loading protected lumps should cause it to fail authenticating the demo. However, music files are not protected, thus leaving music out or loading extra music should not cause authentication failures.

(0006247)
Arco (updater)
2013-04-06 14:11

Old demos from Skulltag cannot be played, as there's an authentication error that's associated with the iwad. The new demos that are recorded with v1.1 can only play if Zandronum has access to the correct wads.

(0006252)
Dusk (developer)
2013-04-06 16:44

That sounds like correct behavior, then. Demos recorded with older versions never worked in Skulltag or Zandronum so that's not a bug as much as a missing feature.

This issue is already marked as resolved. If you feel that is not the case, please reopen it and explain why.
Supporters:	No one explicitly supports this issue yet.
Opponents:	No one explicitly opposes this issue yet.

Issue History
Date Modified	Username	Field	Change
2012-08-13 16:29	Bloax	New Issue
2012-08-17 13:45	Dusk	Assigned To	=> Dusk
2012-08-17 13:45	Dusk	Status	new => assigned
2012-08-17 13:46	Dusk	Note Added: 0004382
2012-08-17 14:32	Dusk	Note Edited: 0004382	View Revisions
2012-08-17 14:36	Dusk	Status	assigned => needs review
2012-08-18 19:21	Torr Samaho	Note Added: 0004392
2012-08-18 23:04	Dusk	Note Added: 0004396
2012-08-18 23:04	Dusk	Status	needs review => assigned
2012-09-16 01:55	Dusk	Note Added: 0004679
2012-09-16 01:55	Dusk	Status	assigned => needs review
2012-09-16 07:11	Torr Samaho	Note Added: 0004681
2012-09-16 07:31	Torr Samaho	Status	needs review => feedback
2012-09-16 09:38	Torr Samaho	Note Edited: 0004681	View Revisions
2012-09-16 10:07	Dusk	Status	feedback => assigned
2012-09-16 10:08	Dusk	Note Added: 0004685
2012-09-16 10:19	Torr Samaho	Note Added: 0004688
2012-09-16 10:48	Dusk	Note Added: 0004690
2012-09-16 10:48	Dusk	Status	assigned => needs review
2012-09-16 10:49	Dusk	Note Edited: 0004690	View Revisions
2012-09-16 11:23	Torr Samaho	Note Added: 0004693
2012-09-16 11:23	Torr Samaho	Status	needs review => feedback
2012-09-16 12:02	Dusk	Note Added: 0004699
2012-09-16 12:03	Dusk	Note Edited: 0004699	View Revisions
2012-09-16 12:44	Torr Samaho	Note Added: 0004702
2012-09-16 13:14	Dusk	Note Added: 0004704
2012-09-16 13:15	Dusk	Note Edited: 0004704	View Revisions
2012-09-16 13:26	Torr Samaho	Note Added: 0004706
2012-09-16 14:06	Dusk	Note Added: 0004707
2012-09-16 14:08	Dusk	Note Edited: 0004707	View Revisions
2012-09-16 14:09	Dusk	Note Edited: 0004707	View Revisions
2012-09-16 14:10	Dusk	Note Edited: 0004707	View Revisions
2012-09-16 14:31	Dusk	Note Edited: 0004707	View Revisions
2012-09-16 16:56	Torr Samaho	Note Added: 0004708
2012-09-16 16:57	Torr Samaho	Note Edited: 0004708	View Revisions
2012-09-16 16:57	Torr Samaho	Note Revision Dropped: 4708: 0002551
2012-09-16 17:57	Dusk	Note Added: 0004712
2012-09-16 18:03	Torr Samaho	Note Added: 0004713
2012-09-20 10:53	Dusk	Note Added: 0004718
2012-09-20 10:53	Dusk	Status	feedback => needs testing
2012-09-23 17:06	Watermelon	Note Added: 0004755
2012-09-23 18:34	Torr Samaho	Note Added: 0004766
2012-09-23 20:00	unknownna	Note Added: 0004776
2012-09-23 20:01	unknownna	Relationship added	related to 0000147
2012-09-23 20:24	Dusk	Note Added: 0004781
2012-09-23 21:55	Dusk	Note Edited: 0004781	View Revisions
2012-12-18 20:44	Dusk	Target Version	=> 1.1
2013-04-06 14:11	Arco	Note Added: 0006247
2013-04-06 16:44	Dusk	Note Added: 0006252
2013-04-06 16:44	Dusk	Status	needs testing => resolved
2013-04-06 16:44	Dusk	Fixed in Version	=> 1.1
2013-04-06 16:44	Dusk	Resolution	open => fixed
2018-09-30 20:22	Blzut3	Status	resolved => closed

Notes
(0004382) Dusk (developer) 2012-08-17 13:46 edited on: 2012-08-17 14:32	I had planned on getting demo WADs stored in the demo rather than relying on the filename for them.. guess it's about time to get that done. As for >"CLIENTDEMO_ProcessDemoHeader: Expected CLD_DEMOSTART.", I don't know what to do about this. The format obviously changed but I don't know the details of the older demo formats. EDIT: OK that's done now. I left the changelog entry out for now - if Torr wants this in 1.0 I'll add it and commit this to a new head.

(0004392) Torr Samaho (administrator) 2012-08-18 19:21	If we add this kind of wad check, we should also consider checking everything. For instance the order in which the wads are loaded may be very important. Your check also allows the player to load more wads that may break the demo. Do we want the check to allow this kind of things? We could also just let the check print visible warnings instead of erroring out. BTW: I think the memset + strncpy construction is not necessary. memset clears much more than necessary. Anyway, the whole thing would be much easier if you use TArray<FString> to store szWADNames.

(0004396) Dusk (developer) 2012-08-18 23:04	Quote Your check also allows the player to load more wads that may break the demo. I was doing that to allow the player to auto-load wads without breaking the demo but that can actually be checked for.. Quote For instance the order in which the wads are loaded may be very important. Hmm. Maybe use the authentication checksum for the actual check and print wad list if the check fails? Quote BTW: I think the memset + strncpy construction is not necessary. memset clears much more than necessary. Anyway, the whole thing would be much easier if you use TArray<FString> to store szWADNames. Hmm. Probably. :P

(0004679) Dusk (developer) 2012-09-16 01:55	Alright, how's this?

(0004681) Torr Samaho (administrator) 2012-09-16 07:11 edited on: 2012-09-16 09:38	Much better, but ULONG ulWADCount = NETWORK_ReadShort( &g_ByteStream ); // Read the names of WADs and store them in the array FString WadNames[ulWADCount]; doesn't work. It doesn't even compile under VC++ as the size of C arrays has to be known at compile time. As I said earlier, use a TArray to store the strings, i.e. replace FString WadNames[ulWADCount] by TArray<FString> and use "Push" to populate the array. EDIT: BTW: I think putting dates in the comment is pointless. They are bound not to be updated when something is changed, make things less readable and it's usually easier to use "hg annotate" to find out when a change was made.

(0004685) Dusk (developer) 2012-09-16 10:08	Hm. worked fine in GCC...

(0004688) Torr Samaho (administrator) 2012-09-16 10:19	I'm pretty sure that the C standard only requires arrays whose size is known at compile time to work. VC++ 2005 definitely doesn't compile this.

(0004690) Dusk (developer) 2012-09-16 10:48 edited on: 2012-09-16 10:49	Changed into a TArray Also lifted dates from comments.

(0004693) Torr Samaho (administrator) 2012-09-16 11:23	The code looks good now, but I just noticed a problem when testing your latest patch: The authentication doesn't check the maps at all. So you can still play a demo that was recorded on the completely wrong map since g_lumpsAuthenticationChecksum only checks protected non-map lumps.

(0004699) Dusk (developer) 2012-09-16 12:02 edited on: 2012-09-16 12:03	That's certainly a bit problematic. Should there be some map checksum kinda thing that would be the checksum of every map loaded, and have that checksum be stored after the network checksum in the demo?

(0004702) Torr Samaho (administrator) 2012-09-16 12:44	Doing a per map checksum would be the way that's most consistent with how clients and server do the authentication. The client could save the hashes it sends to the server in the demo and then check the maps while playing the demo to the stored hashes.

(0004704) Dusk (developer) 2012-09-16 13:14 edited on: 2012-09-16 13:15	But then you could run into authentication errors when the demo has already started up. Could get a little frustrating when the demo suddenly bombs out.

(0004706) Torr Samaho (administrator) 2012-09-16 13:26	That's also true. From the top of my head some alternatives are - only check the first map (bombing out is replaced by an unwatchable demo on problematic maps which is not really less frustrating) - build a single MD5 hash of all loaded wads (taking into account the order in which the maps are loaded, similar to how g_lumpsAuthenticationChecksum is built) and check this hash - check the MD5 hash of all loaded wads and their loading order With the latter two you should ignore the auto loaded wads (g_lumpsAuthenticationChecksum takes care of this)

(0004707) Dusk (developer) 2012-09-16 14:06 edited on: 2012-09-16 14:31	Or maybe this? 'http://pastebin.com/VMryWfKf [^]' Based it off listmaps and auto-compatibility code. EDIT: Using NETWORK_GenerateMapLumpMD5Hash (mdata, mdata->lumpnum, sum); seemed to get me d41d8cd98f00b204e9800998ecf8427e for every map entry. Maybe I'm using it wrong? EDIT 2: ... I certainly am... but I dunno is it worth getting it to work properly when the bit from compatibility.cpp works fine in substitution as well. EDIT 3: OK it worx (aside from the style I forgot to fix up..) Now I just think, maybe this could be merged into the network authentication checksum, or is there a reason for network authentication only testing the current map?

(0004708) Torr Samaho (administrator) 2012-09-16 16:56 edited on: 2012-09-16 16:57	Quote from Dusk OK it worx (aside from the style I forgot to fix up..) Seems to work nicely. What would still be nice is if this checksum is only calculated when a demo is recorded or played. I don't know the performance penalty the checksum calculation has on weak machines on large map sets, so I think it's safer to only calculate it when you really need it. Quote from Dusk Now I just think, maybe this could be merged into the network authentication checksum, or is there a reason for network authentication only testing the current map? This would kill Freedoom/Doom network compatibility completely (now it just kills it for demos, but at least it can be worked around with demo_pure 0 which should be sufficient). Also it's not necessary to prevent somebody from joining when a map differs between client and server that will not be played anyway. The current system works fine and we don't really gain anything by replacing it (except that the startup will be a slower to a still unknown degree and a slight code simplification).

(0004712) Dusk (developer) 2012-09-16 17:57	Quote This would kill Freedoom/Doom network compatibility completely Hm, that's right. Quote What would still be nice is if this checksum is only calculated when a demo is recorded or played. I don't know the performance penalty the checksum calculation has on weak machines on large map sets, so I think it's safer to only calculate it when you really need it. That is done. If this is good now, I'll collect this into a new single commit that can be pulled up.

(0004713) Torr Samaho (administrator) 2012-09-16 18:03	Looks good :).

(0004718) Dusk (developer) 2012-09-20 10:53	This was pulled during tracker downtime.

(0004755) Watermelon (developer) 2012-09-23 17:06	How can I test this? Do I need to create a demo with the latest version and then check it against different wads or another version?

(0004766) Torr Samaho (administrator) 2012-09-23 18:34	You can use this binary. And you guessed right, you need to record demos with the new binary and then try to play them with the correct and with wrong wads.

(0004776) unknownna (updater) 2012-09-23 20:00	Will the console tell you which version of Zandronum the demo was recorded with if you try to play an old demo with a newer version of Zandronum? It seems that you're still able to play demos even if you don't load all files, e.g., music files.

(0004781) Dusk (developer) 2012-09-23 20:24 edited on: 2012-09-23 21:55	The thing uses special lump and map collection authentication for the check. Loading protected lumps should cause it to fail authenticating the demo. However, music files are not protected, thus leaving music out or loading extra music should not cause authentication failures.

(0006247) Arco (updater) 2013-04-06 14:11	Old demos from Skulltag cannot be played, as there's an authentication error that's associated with the iwad. The new demos that are recorded with v1.1 can only play if Zandronum has access to the correct wads.

(0006252) Dusk (developer) 2013-04-06 16:44	That sounds like correct behavior, then. Demos recorded with older versions never worked in Skulltag or Zandronum so that's not a bug as much as a missing feature.