[vlc-devel] Re: Win32 implementation of mdate breaks for large values returned by QueryPerformanceCounter (suspected due to a buggy gcc optimization), causing video stream playback to fail on Windows systems with a large uptime

[Skywing]

Sounds like just doing a rebuild for the next VLC-Win32 drop would be enough, then.

BTW, I would, on principle, recommend just doing away with the QueryPerformanceCounter ("QPC") code path in mdate entirely here.  That routine (that is, QPC) is really designed for isolated, short-term performance testing and not the kind of long-term timing or multiprocessor support like how VLC is using it.  Besides removing the code path that was breaking gcc, that also gets you out of slightly-less-than-savory things like the assumptions about the timer resolution being indicative of whether the system timer powers QPC or the processor performance counter powers QPC.

QPC does have a higher apparent resolution than GetTickCount, but in the end it doesn't really get you much, as GetTickCount's resolution is the same resolution used by the dispatcher when managing thread scheduling.  So, unless you are doing busy waits everywhere (which it doesn't look like is the case to me, anyway), things like msleep (which seems to eventually thunk down to Sleep), or anything that ends up doing a dispatcher sleep through WaitForSingleObject, WaitForMultipleObjects, or the like will not actually realize the improved precision that, on the surface, appears to be available through QPC.

You can boost the system timer resolution to as low as 1ms (which includes GetTickCount and the dispatcher/scheduler resolution) via timeBeginPeriod(), hardware-depending.  Even if it doesn't really get you all the way down to 1ms, it usually lets you get a bit lower than the default 16ms for uniprocessor hals or 10ms for multiprocessor hals.  The only hit there is theoretically wasting battery life on laptop, but playing software-decoded MPEG video on a laptop is probably going to hose your battery life anyway.

The workaround already in-place for wrapping with GetTickCount looks solid to me, anyway, so that code path shouldn't have any problems as far as I can see (barring further gcc weirdness).  On Windows Vista and later, you could even decide to use GetTickCount64 if you wanted, which makes the wrap-around problem non-existant on human timescales.  However, since the existing wraparound detection logic seems fine to me (and even in the improbable scenario of VLC being suspended for 49.7 days, VLC would keep working as it seems to just require a monotonically increasing sequence timer since process start), this is probably just unnecessary extra work.  It is kind of a bummer how the GetTickCount path grabs a critical section each time right now, but I don't think that will be a serious performance problem given the duration with which it is held, and in the unlikely case that it became problematic, the critical section could always be swapped for a fast SWMRG-style construct.

Oh, and thanks for the responses to my mails thus far.

-----Original Message-----
From: vlc-devel-bounce at videolan.org [mailto:vlc-devel-bounce at videolan.org] On Behalf Of Damien Fouilleul
Sent: Wednesday, January 10, 2007 5:44 PM
To: vlc-devel at videolan.org
Subject: [vlc-devel] Re: Win32 implementation of mdate breaks for large values returned by QueryPerformanceCounter (suspected due to a buggy gcc optimization), causing video stream playback to fail on Windows systems with a large uptime

Rémi Denis-Courmont wrote:
> Le mardi 9 janvier 2007 06:30, Skywing a écrit :
>   
>> There's sign extension & data loss bug in the Win32 implementation of
>> mdate (src\misc\mtime.c) that appears to be introduced by aggressive
>> gcc optimizations.  I don't have a cygwin build environment
>> installed, so I am just dropping a mail to explain the problem and
>> what likely needs to be done to fix it.
>>     
>
> Thanks for the thorough bug report.
>
> Unfortunately, the badly-optimized double-division was precisely 
> intended to avoid miscalculation with large values (as the comment 
> notes anyway), which happens if the division is done in a single step. 
> In other words, the previous implementation was really broken, and the 
> new one is broken by gcc.
>
> I feel I have wasted enough time on this already myself, to not take gcc 
> bugs into account in addition, particularly while I do not even use 
> Win32. Plus I do not have the necessary background to check for gcc 
> bugs.
>
>   
i've looked at the asssembly version of that code compiled through 
cygwin latest gcc, and i don't see the optimization problem.

-- 
This is the vlc-devel mailing-list, see http://www.videolan.org/vlc/
To unsubscribe, please read http://developers.videolan.org/lists.html

-- 
This is the vlc-devel mailing-list, see http://www.videolan.org/vlc/
To unsubscribe, please read http://developers.videolan.org/lists.html