Attachments (2)
Change History (30)
comment:1 by , 20 years ago
comment:2 by , 20 years ago
* "It's not clear to me why THREAD_PAUSE or why it is needed.": I added that for possibility pause feature, but it's not needed and could be simply removed.
* It's not an option to add a thread API to OSystem...", "Rather, I recommend that a "Timer *OSystem::createTimer()" method is added to OSystem...": OK, i'll change that as you described.
* "The code will end in a deadlock if a timer tries to remove itself.": What do you mean by removing itself ? delete g_timer is done properly, but i think you are talking about something different.
* "the code does busy waiting when it wants to exit threads..." I prefer not use mutexes, they are not needed while running threads. i don't know what bad thing is by using waitForThread func compared to use mutexes. Propably i don't understand your point of view of the problem :)
comment:3 by , 20 years ago
one more: "To avoid code duplication, we could still keep one (or more) default implementation(s) of class Timer around..." what do you mean exactly by default implementation ? if i move timer code into backend, it will be all of code. it will be whole timer duplication for each backend. maybe you mean SDL backend as default implementation ?
comment:4 by , 20 years ago
maybe instead adding createTimer(), add: "virtual bool OSystem::installTimerProcedure(TimerProc procedure, int32 interval, void *refCon) = 0;" and "virual void OSystem::destroyTimerProcedure(TimerProc procedure) = 0;" same as in Timer class declaration. Rest stuff hided in backend implemention.
comment:5 by , 20 years ago
* "The code will end in a deadlock if a timer tries to remove itself." -> well, if a timer proc calls removeTimerProc on itself (something which is possible right now, and AFAIK, is or was done that way at some point), then with your code, the thread will dead lock.
* adding installTimerProc / removeTimerProc directly to OSystem would be a possibility, too; but it'll also increase the work load on backend authors regarding what stuff they have to implement. Ah well, shouldn't be a major issue :-)
* waitForThread, if implemented properly (yours uses SDL_WaitThread, which is fine), isn't an issue, because it lets the thread sleep until it is needed again. so that one is actually fine
* With "default implementation", I was referring to a default implementation of the "Timer" class. So by default, the current Timer class would be used; but if a backend author wanted to provide a different Timer implementation, they could subclass it. If we add installTimerProc directly to OSystem, we could still allow for that, though, by providing an additional OSystemWithDefaultTimer subclass of OSystem which then essentially would implement that old timer code; backends could then either subclass that or OSystem.
Anyway: One thing not to forget here is that some of our backends do not support threads at all; they still have to emulate timers in another fashion. So don't rely too much on precise timers...
comment:6 by , 20 years ago
* "The code will end in a deadlock if a timer tries to remove itself." -> i got the point of that now, but i'm not sure if that is really importent. that is not userland usage, and also deadlocks are very easy to create in scummvm code, if you want :-). anyway i'll try to find solution for that.
* about precising change definition of timers: - change current timers to async mode between callback funcs for imuse and smush parallel functionality - drop not needed 100hz call inside timer code (not needed cpu usage), - implement synced stoping timer callback while destroying resources.
* "With "default implementation", I was referring to a default implementation of the "Timer" class." -> I didn't have plans for keep current Timer class. It would be good for smooth or not change backend code. But it will create two different featured timers, a/synced. If I remove hack in imuse for async timer, it will create logical dealock for old Timer code.
comment:7 by , 20 years ago
* Deadlock: err yeah, deadlocks are easy to create. So are crashes. We could start to override random memory locations, too.... My point is: that argument is bogus. And I don't know what you mean with "userland usage" ? Removing and installing timer procs is very much "userland".
* Regarding "Timer default implementation": it appears that you did *not* read all that I wrote. I specifically did answer you suggestion (which doesn't use a Timer class anymore) and explained how a "default implementation" could be done there. Which would allow a smooth transition of existing backends to the new system. So please read again what I wrote.
* I still would like to remind you that you still have to keep in mind that not all backends will support async timers. Hence, our code should not depend on that feature if possible.
comment:8 by , 20 years ago
* by "userland" I mean not developers. Devs should care what they do. Anyway i didn't looked yet for solve this yet.
* Yes, but i did short cut in code. ok, if you prefer more smooth implemention i can back Timer class.
* if async mode can't be implemented in all backends then that feature can't be declared.
comment:9 by , 20 years ago
regarding to "[ 996674 ] SmushPlayer can crash while using threaded timer": while implementing optionaly new timer code in backends by porters, they should to know, that removing func is synced(waiting for ending func) as declared feature to "removeTimerProc".
comment:10 by , 20 years ago
I still don't think you understood me right, aquadran. I am *not* saying that a "Timer" class should be kept. Rather, I explained that a default implementation of the timer code should be kept. Whether that is in a Timer class, or in a special OSystem subclass, doesn't matter.
* Async can't be implemented in all backends. For sure. I am not sure what you mean with your cryptic remark "if async mode can't be implemented in all backends then that feature can't be declared." If you mean that an async timer is only of use to you if it is available on all our target systems, then we have a problem.
comment:11 by , 20 years ago
To illustrate what I mean, look at the attached patch which shows a mockup of how OSystem and the new OSystemWithDefaultTimerCode would look like!
comment:12 by , 20 years ago
new version of timer code, it keep current timer class, but usage is only by OSystemWithDefaultTimerCode class. after moving timer code into that class, it could create initialisation problems at initializsation and finishing, then why i prefer not touch current timer class code. btw. i didnt checked runtime, it could not work. it's compilable and it's some proposal draft.
comment:13 by , 20 years ago
Some remarks:
* I'd change OSystem_SDL::deleteTimer() to first set all timers state to THREAD_REMOVE; and only after that do a second loop which calls SDL_WaitThread. This will reduce the dead lock risk.
* The TimerSlot::wait() code is still wrong; i.e. this code makes no sense: if ((currentTime - _lastTime) < _interval) { uint64 elapsedTime = (currentTime - _startTime) * 1000; interval = (_interval - (elapsedTime % _interval)) / 1000; } It probably should be: if ((currentTime - _lastTime) * (uint64)1000< _interval) uint64 elapsedTime = (currentTime - _startTime) * (uint64)1000; interval = (_interval - (elapsedTime % _interval)) / 1000; }
* I still think we should use double instead of uint64
* I don't quite understand why you want to keep the Timer class?
comment:14 by , 20 years ago
* "I'd change OSystem_SDL::deleteTimer() to first set all timers state to THREAD_REMOVE; and only after that do a second loop which calls SDL_WaitThread. This will reduce the dead lock risk" i'll look
* "the TimerSlot::wait() code is still wrong; i.e. this code makes nosens", "* I still think we should use double instead of uint64" i didn't focues on that part now.
* "The TimerSlot::wait() code is still wrong; i.e. this code makes no sense": funny, but it works :)
* "I don't quite understand why you want to keep the Timer class" - it's temporary until all backends switch to new timer code, - timer class depend on some osystem funcs(timer callback, timer ticks, mutex locks), if i move into osystem, it will not be guarante that needed stuff will be ready for moved timer code.
comment:15 by , 20 years ago
* Maybe you *think* TimerSlot::wait() works, because it appears to produce the behavior you expect. Just think about it! You are comparing two values A and B, when you really should be comparing A*1000 and B. So instead of if (A*1000 < B) you do if (A < B) Now clearly, the first condition implies the second -- if A*1000 is less than B, then also A is less than B (assuming both A and B are positive numbers, which is the case here). So what happens is that you far too often enter that "if" statement. You even enter if when (A >= B) in many cases. This will normally not have any visible effect, but it *is* incorrect. In fact, this bug means that you may skip interval's -- if the timer takes longer than _interval micorseconds in one of its iterations, the code now skips the next iteration. Whether that is a good thing or not is another question; but it certainly is not what the good *seems* to do ...
* I still don't see why you want to keep the Timer class. All its functionality should be moved into the OSystemWithDefaultTimerCode class. Otherwise, people might accidentally keep using g_timer. And it's not problem that the Timer code depends on other OSystem APIs, either. Where's your point? Just add a virtual protected OSystem::init() method which OSystemWithDefaultTimerCode overrides to init the timer code.
I attached an updated patch to illustrate my points. BTW, I usually prefer using the "unified" diff format, as it's easier to read, IMO... at least it provides some context for changes :-)
comment:16 by , 20 years ago
i moved timer into timer osystem, changed thread init, and changed from uint64 to double. try build this patch, i have build problems related timers calls to system funcs which are virtuals.
comment:17 by , 20 years ago
Attached a compiling version of your patch. Some notes:
* please use 4 space wide tabs (and not an 8 space indention)
* you can't call virtual methods of a child class from a destructor. Simple reason: at the time the destructor runs, the destructor of the child class has already run. As a part of it, the vtable will have been torn down. Hence you can't call anything reference by the vtable. Even if that wasn't so, at the time we invoke that virtual method of our subclass, the subclass has already been deinitialized -- thus errors and crashes are likely to occur.
* hence I moved the timer deinit code into a new deinit() method. Right now that method is never called, and I have to wonder whether it actually should be called, ever. After all, the subclass should automatically get rid of any timers in its destructor (or rather in its quit() method) anyway
* There was some mess up with the TimerFunc/TimerProc/ TimerCallback types which I corrected
comment:19 by , 20 years ago
i'm geting that compilable problem: backends/sdl/sdl.cpp: In function `OSystem* OSystem_SDL_create()': backends/sdl/sdl.cpp:35: error: cannot allocate an object of type `OSystem_SDL' backends/sdl/sdl.cpp:35: error: because the following virtual functions are abstract: common/system.h:666: error: virtual void OSystemWithDefaultTimerCode::setTimerCallback(int (*)(int), int)
by , 20 years ago
| Attachment: | timer-v6.patch added |
|---|
comment:21 by , 20 years ago
That last version of your patch didn't apply cleanly for me at first, but now it did; I made a new patch file (using "cvs diff") against latest CVS which applies without flaws.
Now, this starts looking good. I guess next thing should be to get some testing under the belt of this...
comment:22 by , 20 years ago
First regression I noticed: Start a new COMI game. Choose a skill level. Now the next thing that happens is the "CMI" text fading into view; you hear monkeys shouting, and then the THX sound plays. Now, if you ESC at this point, it will abort the sound and show the "Part I" screen. Not so with this patch applied: the sound will keep playing for a few moments before it gets stopped, even though we are already showing the part I screen. Similar sound/music-abort-issues occurred in BASS when I just tested.
comment:23 by , 20 years ago
(note: that regression isn't too big a surprise to me -- as I said, a lot of our code relies on the fact that the times are sync. And the iMuse/INSANE/SMUSH code in general doesn't look like it's fully thread safe either. It probably would require a fully audit of that code to ensure all is right... ideally, all interaction between those modules and the rest of the SCUMM engine should be limited to as few places as possible, to make it easier to verify that it is right.
comment:26 by , 19 years ago
| Status: | new → closed |
|---|
comment:28 by , 6 years ago
| Component: | → --Other-- |
|---|
OK, here are some points: * It's not an option to add a thread API to OSystem in my eyes. We went to great length to get rid of it, as it degrades portability a lot.
* Rather, I recommend that a "Timer *OSystem::createTimer()" method is added to OSystem, which is responsible for creating the g_timer object (i.e. in main(), we'd do "g_timer = g_system->createTimer()" instead of "g_timer = new Timer()". Then, class Timer is turned into a pure virtual/ abstract class That way, backends may implement timers any way they want (SDL vs. morphos timers vs. other timers). To avoid code duplication, we could still keep one (or more) default implementation(s) of class Timer around which either use a timer callback or threads. I don't want to go into details here, though
These were design problems. Now to the implemention: * the code does busy waiting when it wants to exit threads etc. That's not really nice, would be cleaner to use a mutex. But ah well, it should work either way * It's not clear to me why THREAD_PAUSE or why it is needed. Could you explain? If it is not needed, we could get rid of beginOfThread(), too * The code will end in a deadlock if a timer tries to remove itself. I am not really sure whether that problem can be avoided with your implementation