sf::Clock restart() returns negative values in continuous work

[Mozzg]

Hello. I'm using SFML with a program for LED displays for railroad comunication in Russia. It displays nessesary information about trains on a screen and the hardware is converting video signal to LEDs. This program must run continuously and preferrably without restarts. I found an issue with SFML and how it calculates time (see below).

• OS: Windows 10 x64
• SFML Version: 2.5.1
• Compiler: Microsoft Visual Studio Enterprise 2017

I'll go straight to the point. While testing my program, I encountered an issue, where after continuous work for about 10-11 days, the program started returning negative time and it disrupts normal work. I found the issue is in Win32 ClockImpl implementation and similar to issue #1167 . Also I found out that it depends on hardware and return value of QueryPerformanceFrequency function.
This is current version of ClockImpl.cpp:

...
QueryPerformanceCounter(&time);
...
// Return the current time as microseconds
return sf::microseconds(1000000 * time.QuadPart / frequency.QuadPart);

The issue is in return statement. time.QuadPart type is signed __int64, which means if it goes over 9 223 372 036 854 775 807 it will go to negative numbers. In this calculation time.QuadPart needs to only be over 9 223 372 036 855, because this number is multiplied with 1000000 first. On my test computer, QueryPerformanceFrequency function returns 10000000, which means there is 10000000 tick per second. In this sircumstances time.QuadPart reaches it's overflow value in 9223372036855 / 10000000 seconds or 922337,2 seconds or 256,2 hours or just only 10,6 days.
On my home computer, QueryPerformanceFrequency returns 3609372. If we calculate time it takes to overflow with this frequency , it is 29,57 days, which is still not good. This program should run uninterrupted for several months or years.

This issue is easily fixable. we just need to exclude a calculation with big intermediate result, for example like this:

// Return the current time as microseconds
double inverse = 1000000 / frequency.QuadPart;
return sf::microseconds(time.QuadPart * inverse);

In my program I used sf::Time elapsed = clock.restart();. Then I add this to my different timers to see if timer is fired, like sf::Int64 elapsedMicro = elapsed.asMicroseconds(); foregroundElapsedTimer += elapsedMicro; Which calculates elapsed time like:

Time Clock::restart()
{
    Time now = priv::ClockImpl::getCurrentTime();
    Time elapsed = now - m_startTime;
    m_startTime = now;
    return elapsed;
}

If now is suddenly goes to negative like I showed above, then elapsed time will be negative too and I doubt this is intended behaviour.

Unfortunatley I already changed time calculation in my project, because I needed it to work. You can reproduce this behaviour by adding a static number of tick to time variable in ClockImpl.cpp and wait until the overflow happens, like time.QuadPart = time.QuadPart + 9223371934370;. This will simulate that your computer is running a long time. Of cource the constant you're adding must be calculated for your own computer (frequency and current ticks is different). Or just imitate same calculation in your own program, so that you don't have to rebuild SFML.

[Bromeon]

Nice catch, you're probably the first SFML user to run the clock for so long 🙂
And thanks for the elaborate description!

Even if we migrate to Chrono in SFML 3, we should still fix this in SFML 2.
Might also need to review the other platforms' implementation.

[ChristianIvicevic]

I am not familiar with the implementation of the SFML internals since I use my custom Timer class which I'd like to share for some inspiration:

/// Helper class exposing methods for simple time measurement.
class Timer final {
public:
  /// Gets the number of milliseconds elapsed since the start of the server.
  /// @return An integer representing the milliseconds elapsed since the start
  /// of the server.
  /// @remark Be aware that the resulting value can overflow which is handled by
  /// the internal helper methods of this class.
  static inline auto now() -> const uint32 {
    using namespace std::chrono;
    static const system_clock::time_point ApplicationStartTime =
        system_clock::now();
    return uint32(
        duration_cast<milliseconds>(system_clock::now() - ApplicationStartTime)
            .count());
  }

  /// Gets the number of milliseconds between the given times.
  /// @param PreviousTime The earlier time.
  /// @param NewTime The later time.
  /// @return Number of milliseconds between the given times.
  static inline auto differenceInMilliseconds(const uint32 PreviousTime,
                                              const uint32 NewTime)
      -> const uint32 {
    // Handle overflowing numbers.
    if (PreviousTime > NewTime) {
      return (0xffffffff - PreviousTime) + NewTime;
    }
    return NewTime - PreviousTime;
  }
};

[eXpl0it3r]

@Mozzg thanks for the detailed analysis. Would you be willing to provide a PR for the suggested fix?

[Mozzg]

@Mozzg thanks for the detailed analysis. Would you be willing to provide a PR for the suggested fix?

I tried to create a pull request (#1771). I'm not quite familiar with GitHub and I hope I've done it correctly. Also I can't test or compile my changes, because I used already compiled SFML libraries and I don't know how to compile SFML myself. Can anybody confirm that my changes are correct?
Also a program to test my changes and test the overflow is very dependant on a computer and current tick count, so creating a code for testing is difficult.

[eXpl0it3r]

Fixed with #1771