ServerInfo

[econoraptor]

Added ServerInfo, implemented TickInterval, added molotov_projectile to MapEquipment

[main--]

Thanks for the pull request!

Things I noticed:

• While indentation throughout this project is already severely screwed up, please at least try to keep things consistent within your contribution. There's no reason to mix tabs and spaces in a file you're adding (as opposed to changing).

• The molotov change is unrelated and could be a separate pull request. This is not a big deal, but we could merge this simple and straightforward fix immediately while the bigger changes require some discussion.

• I assume the UgcMapId field is working at the moment because the upper 32 bits are always zero. Because if that, I'll fix the varint code to support 64 bits when merging this (this is mostly a note for myself but as I'm short on free time these days, this will probably be faster if you decide to take a stab at this).

• Please try to avoid static variables. Using them correctly is incredibly hard and in fact, your code introduces a rather critical bug: Consider a multithreaded application that uses demoinfo. The static fields would always contain the data of the "current" demo but this breaks when multiple demos are parsed concurrently!

  Instead, please try to propagate the data back to the DemoParser object:

  // In DemoParser.cs:
  public float TickTime { get; internal set; }
  // obviously derive TickRate from that
  
  // In ServerInfo.cs:
  public void Parse(IBitStream bitstream, DemoParser parser)
  {
      // [parsing code]
  
      parser.TickTime = TickInterval;
  }

[econoraptor]

Ok later tonight I'll make the changes and submit a new request and a separate one for the molotov change. The tabbing is different in the upload than it is in Visual Studio for some reason.

[moritzuehling]

Espeically the static needs to be fixed, because else it is an outright bug at the moment.

[econoraptor]

I made the changes but I'm holding off on the pull request for now because after trying some more demos I've found that it's not always the case that TickInterval = Header.PlaybackTime / Header.PlaybackFrames

For instance in the demo for this match http://www.hltv.org/?pageid=188&matchid=28372:

TickInterval = 1/128, but PlaybackTime/PlaybackFrames = 1/64

[main--]

The tabbing is different in the upload than it is in Visual Studio for some reason.

That's because most IDEs (including Visual Studio) display tabs as 4 spaces - even though they're defined as 8 spaces which is also the way GitHub renders them.

submit a new request

You can just force-push onto this one - GitHub handles that just fine.

[econoraptor]

For some reason I didn't realize this before, but tick_interval is a server metric while PlaybackTime and PlaybackFrames are metrics for the demo itself. I've been trying to find something that I could possibly compare tick_interval to in order to get the demo tickrate, but I haven't come up with anything. Does anyone have any suggestions?

P.S. I noticed that after loading the demo there was some console output that read as follows:
"Attempting to heal incomplete demo file: assuming 507904 ticks, 507707 frames, 3968.00 seconds @128hz"

This leads me to believe that valve is imputing the values rather than reading them directly from the stream.

[ghost]

I feel like stating the obvious but server tick rate and demo snapshot rate are stored in the demo header. You just have to compute them from the (frames,ticks,time) triplet.

[econoraptor]

Some headers get corrupted, so we're trying to find a workaround. See #91

[ghost]

In that case the CSVCMsg_ServerInfo::tick_interval gives the server tick rate. The snapshot rate can easily be deduce by CNETMsg_Tick::tick message step/increment.

[ghost]

In other words the number of ticks and frames should be simply the values of the tick and seq-in/seq-out field of the last packet of the demo. It should be consistant CNETMsg_Tick messages which should give you a way to verify everything is in order.

[econoraptor]

I'm not sure what you mean by the seq-in/seq-out field, but it sounds like what you're suggesting is to compare tick_interval to the difference between sequential ticks. So, for instance, a 64 tick demo recorded on a 128 tick server should have a gap of two between the ticks.

[ghost]

Yes, there is a gap in the ticks. That's not very important if you only intend to fix the demo. All you need is to count the ticks (last tick number minus first ``real tick'' number) and the number of frames (should be more or less the number of CNETMsg_Tick messages). Then compute the playtime with the server tick rate in the CSVCMsg_ServerInfo message. Update the demo header then (optionally but it's cleaner) close the demo with a stop demo message (type 7) which should be missing in those broken demos.

What I call seq-in and seq-out are the 2 32bit integers part of the sequence info stored in every demo packet messages (demo message type 2). Basically you don't have to decode the demo packet data (protobuf) to get those, you just need to parse the demo messages. However AFAIK the only way to get the server tick rate (beside the header info) is to decode the CSVCMsg_ServerInfo message. Because the CNETMsg_Tick::tick does not start with 0 it's easier to locate the first ``real'' packet using the demo messages tick and sequence info (tick == 0 and seq-in/out == 1).

[econoraptor]

Ok that makes sense. I'll get my feet wet with the bitstream internals and see if I can figure out how to access the demo messages.

[main--]

Note that messing with bitstream internals shouldn't be necessary for this particular part - the sequence numbers are right here.

[econoraptor]

Based on some comments on gitter it looks like this is still a problem so I took a stab at it tonight. I now have what appears to be a working version that results in the same imputed values as what valve has when the demo is viewed in game. I still need to check a few things and clean up the code.

The bad news is that, at least with my test case of the one corrupted demo I'm working with, the parser still tries to read past the end of the file which results in a System.IO.EndOfStreamException. When viewed in game the demo ends abruptly probably somewhere within a minute or so early (what I assume is the last round, 5v3).

How do you want the exception handled? Should I leave it as is and let end users decide what to do? Or do you want it processed in some way?

[main--]

Correctly parsing corrupt demos is pretty much impossible and not a goal of this project. Throwing an EndOfStreamException for a demo that ends prematurely is actually perfect.

[moritzuehling]

Correctly parsing corrupt demos is pretty much impossible and not a goal of this project

The reason for this boils down to two points:

• Work vs. Reward: Most demos aren't broken, and if they are broken it's in many different ways
• Fixing bugs of corrupt demos might even introduce bugs for 'normal' demos, what I would hate to do.

Also, as @main-- said: EndOfStreamException is perfect here.

[econoraptor]

If anyone has additional corrupted demos they can link that would be helpful.

I figured it would be best to leave the exception as it is, but I just thought I would check.

The branch is ready to be tested. The main thing I'm worried about is making sure that no variables are being modified or events raised on the first pass through the demo and that nothing is being duplicated when the second bitstream is instantiated and set to the end of the header. As far as I can tell that's the case, but I'm not certain.

[main--]

tl;dr I'd like to avoid the parser restart at all costs

---

The main thing I'm worried about is making sure that no variables are being modified or events raised on the first pass through the demo and that nothing is being duplicated when the second bitstream is instantiated and set to the end of the header. As far as I can tell that's the case, but I'm not certain.

By the way, this exactly why we objected to your use of static in earlier versions of this pull request. What you're doing here is already quite decent (modulo other comments). To be 100% sure you could create a new DemoParser object (or just carefully look at every single field inside of it) but since we (AFAIK) successfully avoided static entirely so far, things are actually guaranteed to be fine then.

[main--]

Is there any reason why you can't just add paramters/flags to the existing methods?

[econoraptor]

I started with that, but it got kind of messy. Each call to a parser in ParseTick and ParseDemoPacket needs to be conditional on whether we're skipping the parsing or doing a normal run.

It also seemed cleaner to isolate the logic because 99% of the time when someone's looking at the code they'll be looking at those functions for normal parsing purposes and the additional logic will just be in the way.

If you'd prefer it the other way though, I have no problem implementing it like that.

[main--]

You're not actually reading data here, just throwing it away. It's much faster to skip it in the bitstream (instead of decoding) by using the BeginChunk()+EndChunk() trick:

BitStream.BeginChunk(1337); // how many bits you want to skip
BitStream.EndChunk();

[econoraptor]

I wasn't sure if it was really as simple as just adding up the number of bits in the Parseheader code or if there was more going on underneath the surface. It sounds like you're saying there isn't so I'll go ahead and chunk it.

[main--]

The only nontrivial part is ReadCString() but look at the code and you'll see that it essentially just reads a fixed amount of bytes.

[main--]

Your whitespace is still inconsistent in several places (this is just one example). Please configure your editor to use tabs (most are capable of setting this on a per-project level so you hopefully won't have to temporarily mess up your config just for this) and then run the code formatter on your changes.

[main--]

Bitstreams are IDisposable, so you have to call Dispose() on the old bitstream first to avoid memory leaks.

[main--]

This is potentially problematic. Consider:

• The input stream is not necessarily seekable (it could be e.g. a HTTP download). In this case, the NotSupportedException would only be thrown if the code coincidentally happens to hit one of these broken demos. Very hard to debug.
• The demo doesn't necessarily start at the beginning of the input stream. There may be some other data that the user read before handing the stream to demoinfo.

Both cases are very unlikely but still effectively break our API. My favorite solution would be to avoid the restart entirely (Can we? What are the consequences?) but if that's totally impossible, you need to verify in the constructor that the assumptions you make here about the stream actually hold. Because there is a real usecase for parsing from non-seekable streams (the heatmap website for instance), there also needs to be a flag to bypass this check and revert to today's behavior in this case.

[econoraptor]

I can try copying the stream and reading the copy for the ticks, then continuing with the initial stream for a normal demo parse.

If that doesn't work then your second concern can probably(?) be solved by simply storing the current position of the stream and seeking back to it rather than reading from the beginning.

[main--]

Perhaps I'm missing something obvious but it's still not clear to me why we actually need to restart here. It's been a while since I've last touched the internals of this but as far as I remember, the parser itself doesn't actually rely on the correct header data being there, does it?

Sure, we hand out the bad data from the demo header but I'd say most applications can handle getting this info only later during parsing (when the ServerInfo message is parsed). Those who can't still have the option of restarting the parser from the outside and it's actually even easier for them as they don't have to worry about internal object state.

[econoraptor]

Are you saying you'd prefer to just commit ServerInfo and then put the formulas/code snippet in the readme or something so people can handle it on a per application basis? That seems reasonable.

[econoraptor]

A very quick and dirty implementation where it's left to the user.

using System;
using System.IO;
using DemoInfo;
using System.Diagnostics;
using System.Collections.Generic;

namespace DevNullPlayer
{
	class MainClass
	{
		public static void Main(string[] args)
		{

			using (var input = File.OpenRead(args[0])) {
				var parser = new DemoParser(input);
				
				if (parser.Header.PlaybackTime == 0)
				{
					using (var headerinput = File.OpenRead(args[0]))
					{
						var headerparser = new DemoParser(headerinput);
						headerparser.ParseHeader();
						try
						{
							headerparser.ParseToEnd();
						}
						catch (EndOfStreamException)
						{
							;
						}
						
						parser.Header.PlaybackTicks = headerparser.IngameTick;
						parser.Header.PlaybackFrames = headerparser.CurrentTick - (headerparser.TickAtStart + 1);
						parser.Header.PlaybackTime = headerparser.TickInterval * headerparser.IngameTick;
					}
				}
				parser.ParseToEnd();
			}
		}
	}
}

[main--]

Again, please correct me if I'm wrong here but AFAICS the only reason why you would need this is if your code needs these header fields from the start for some reason, right? Where is the ServerInfo message usually located in those demos? Because if it tends to be somewhere near the beginning, I still believe that most applications should be able to deal with this without restarting the parser.

---

I'm not sure if overwriting the DemoHeader fields is the best approach though. There should definitely be a new parser event that notifies you when we read the ServerInfo and I'd suggest to simply hand out the relevant data with this event.

It's a bit unfortunate that this shifts a part the burden to user code but as we saw, the only alternative is changing/breaking our API in a subtle way and I'd like to avoid that at all costs.

[econoraptor]

As far as I know, we can get the Tickrate from TickInterval on-the-fly, but we can't get PlaybackFrames or PlaybackTime. Neither of these are critical to parsing the demo.

[main--]

Alright then. As I said, there just needs to be an event that fires when we parse the ServerInfo and then applications can handle it. Perhaps it might make sense to include a wrapper class in demoinfo that handles the restart (instead of just documenting what to do somewhere) but I don't really care about that - feel free create one if you think there's widespread need for it.

[econoraptor]

It looks like there have been some more comments on this problem. Sorry it has taken me so long to get back to it. I have some time to work on this again, and I have a much better solution.

Here's how it would work:
We want: Tickrate = Demo Frames/Second
From ServerInfo we have: TickInterval = Seconds/Server Tick
We can look at the difference in IngameTicks between consecutive TickDone events to get: TickGap = Server Ticks/Demo Frame

So the result is: TickInterval * TickGap = 1/Tickrate

There is one small problem with this. TickGap isn't consistent for the first few ticks. I've looked at about 15 demos and the highest I've seen is an IngameTick of 12 before it became consistent. How would you like this handled? I figure the best way would be to leave the timings as they currently are for the first 50 ticks or so and throw a warning so that users know their timing data for those ticks will be wrong.

[econoraptor]

I forced over the old junk, so everything in these recent commits should stem from your master.

I'm not really sure what the standard is for giving this kind of warning. #warning didn't seem entirely appropriate, so I just used Console.Writeline.