Opened 10 years ago

Closed 10 years ago

#4628 closed bug (fixed)

ScreenSaver SetTickSize(): apparent bug? and patch

Reported by: rogueeve Owned by: korli
Priority: normal Milestone: R1
Component: Add-Ons/Screen Savers Version: R1/alpha1
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

I believe I may have found a bug in the ScreenSaverRunner (/src/kits/screensaver/ScreenSaverRunner.cpp:202).

The Be Book states in regards to the SetTickSize() function:

<< The default tick size is 50000 (50 milliseconds). Setting the tick size to 0 causes Draw()/DirectDraw() to be called with almost no delay; in reality, there will be some variable delay depending on scheduling latency >>

-- which implies that there is no specific minimum tick rate. However, the replacement ScreenSaverRunner used by Haiku is unable to honor tick sizes smaller than 50ms, and will never tick at a rate faster than 50ms. If you check the code:

const uint32 kTickBase = 50000;

<--snip-->

break the idle time up into ticks so that we can evaluate

the quit condition with greater responsiveness

otherwise a screen saver that sets, say, a 30 second tick

will result in the screen saver not responding to deactivation

for that length of time

snooze(kTickBase);

if (system_time() - lastTickTime < tick)

continue;

else {

re-evaluate the tick time after each successful wakeup -

screensavers can adjust it on the fly and we must be prepared to accomodate that

tick = fSaver ? fSaver->TickSize() : kTickBase;

lastTickTime = system_time();

}

If the screen saver sets the tick size smaller than kTickBase (50000us), then the runner will see it but stills snoozes for 50000 before checking if the time is up. This prevents the screensaver from running at it's requested rate.

The addition of something such as

if (tick < kTickBase) kTickBase = tick;

else if (kTickBase < 50000 && tick >= 50000) kTickBase = 50000;

after "tick = fSaver ? fSaver->TickSize() : kTickBase", should solve the issue. Of course a #define to replace 50000 might be nice as well. I assume the "k" in "kTickBase" is hungarian notation for "constant", so that might would want to be changed also.

Attachments (1)

ScreenSaverRunnerFix_4628.diff (1.7 KB) - added by rogueeve 10 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 10 years ago by leavengood

I agree this is a bug as far as being compatible with BeOS, but it sure seems unwise for a screensaver to be animating at such a high rate. I know I would be annoyed with and would not use a screensaver that consumed more CPU on my idle computer than is used when I'm using it.

But it isn't up to Haiku to decide how fast a screensaver should animate, so I'm OK with this getting fixed.

I'd take ownership but I don't have time at the moment.

comment:2 in reply to:  1 Changed 10 years ago by rogueeve

Obviously setting the rate to 0 as mentioned in the book is probably a sign of a really-not-very-nice screensaver, but I don't think reasonable rates other than that are necessarily cpu hogs-- you also have to take into account how much work is being done per tick. In my case I was porting a simple screensaver from Linux which does an animation every 10 milliseconds. Running flat out; I got >2000fps out of this thing, so that gives you an idea how little work is being done per frame at the usual frame rate.

I applied the patch I suggested to my own build and it does work.

comment:3 Changed 10 years ago by leavengood

OK, you make a good point. Could you create a patch from your changes and attach it to this ticket? That way it will be extra easy to fix. Usually we try to create patches from the root of trunk, so:

svn diff src/kits/screensaver > ScreenSaverRunnerFix_4628.diff

Changed 10 years ago by rogueeve

comment:4 in reply to:  3 Changed 10 years ago by rogueeve

Sure; give that a try.

comment:5 Changed 10 years ago by leavengood

Resolution: fixed
Status: newclosed

Fixed in hrev33431.

Note: See TracTickets for help on using tickets.