[Scummvm-devel] Issues with kyrandia engine

Max Horn max at quendi.de
Mon Jun 7 20:07:59 CEST 2010


[Could  you please turn off HTML mails? It is very annoying for me to quote your email (I basically had to go to raw mode and copy the content from there), because of the HTML quoting :-(. ]


Am 07.06.2010 um 18:27 schrieb yotam barnoy:

> > This sounds like your throttling logic is broken. It is only correct if one
> > assumes that one either only ever gets rare screen updates (always more than
> > 1/30th apart), or always gets very frequent updates (in your case, that
> > would mean that screen updates should happen at least 30 times per sec, but
> > to be on the safe side and actually always get at least 30FPS, the engine
> > would have to make at least 60 updates per sec, cf Nyquist Frequency).
> >
> I'm not sampling the updates.

Mathematically what you are doing is equivalent to (non-uniform) sampling :). You "take a sample" when you actually update the screen. You then ignore all data for 1/30th of a second, before "taking the next sample" -- thus, you sample at a non-uniform rate which is bounded above by 30 Hz.

> I don't need to get at least 30 fps -- I just
> want to limit it so I don't get more than 30 FPS. If I update at 60fps I'll
> get close to 60fps from SCI, which is way more than I want. Plus I use the
> code that's posted on the wiki (which Johannes referenced). Also, there's no
> guarantee that 60fps will pick up the update I need.

I am not asking you to update at 60FPS, more to the contrary: I am saying that right now, engines must request screen updates with at least 60FPS in order for your code to reliably update the screen at 30 FP :).

Point is: Your current rate throttling logic, which throws away any update requests that come within 1/30th of a second after the last update, is incorrect, given the current OSystem specification. It *cannot* work correctly in general. Indeed, it will only work correctly if the engine requests updates at less than 30 Hz, or at least 60 Hz. Hence it works for e.g. SCI, but not for Kyra.

The major two ways I see to change it are to either follow my proposal and implement a correct logic; or to modify the OSystem specification by changing the semantics of updateScreen.


> > A more appropriate approach would be this: Whenever you get a screen update
> > request, set a "screen is dirty variable". Then, update the screen from a
> > thread resp. triggered by a timer at regular intervals.
> 
> 
> Not a bad idea, but it involves 60 extra context switches a second even when
> there's nothing to draw, plus protection of all screen operations using a
> mutex plus additional complications I'm only starting to think about. I
> think that's the reason video processing is usually in the main thread in
> games.

I don't think that's quite true anymore -- After all, what is the main thread? It is just one arbitrary thread that you designate so. Let's say the thread doing the graphics is called "main thread". Yet many games have the game logic in a separate thread.... :)

But this is really totally besides the point: If you say that on your system, the PSP, you cannot handle this via threads or timers due to efficiency reasons, then so it is! 

However, I wonder if it is really that slow? Is context switching and mutex locking / semaphore handling really that bad ? As for the code complexity, I am not sure what additional complications you forsee, and am curious to hear them. From my point of view, it looks rather straight forward -- but maybe I am missing something here, in particular since I am not familiar with the PSP, so there may be restrictions that render what I say moot... 

Barring this, here are some suggestions on how the code might roughly look like. This is based on a file "pspthreadman.h", revision 2433, from the PSP SDK; if this is wrong file to look at, I'll be happy to give alternate descriptions based on other API references :).

First, the update thread would simply look like this:
  while (true) {
    perform actual screen update;
    sceKernelSleepThread();
  }
This assumes that sceKernelSleepThread sleeps the thread forever (I am not sure what it really does as the docs are not very clear). Alternatively, maybe it can use sceKernelSuspendThread to suspend itself. If that also doesn't work, a mutex or semaphore can be used.

Now, updateScreen would do roughly this:
 void OSystemPSP::updateScreen() {
    if (screen update timer is running)
      return;
    if (time_since_last_screen_update > 1/30th sec) {
      perform actual screen update
      return;
    }
    schedule a new timer to be fired in (1/30th sec - time_since_last_screen_update)
 }
My hope is that you can use sceKernelCreateVTimer() here . If not, then you could simply use the OSystem Timer API. The timer, once fired, does just a single thing: It wakes / resumes the update thread.



> 
> > If you don't have threads/timers or if those are not available, then a good
> > workaround is to hook into various OSystem methods (like pollEvent,
> > delayMillis, getMillis, ...) and add code to them which checks if a screen
> > update is necessary, and if so, performs one. Various backends (e.g. the NDS
> > one) use this approach
> >
> 
> That feels like a hack to me -- the API is supposed to do one thing and we
> really do something else because the proper function isn't being called.

It is a hack, yes -- but not one needed because the proper function isn't being called. It *is* being called, your throttling logic just incorrectly ignores the call :-). So the hack is needed to work around this incorrect logic.

If you remove the incorrect throttling code, things work fine.

That said, it may be difficult to correctly implement the OSystem API with all its semantics on the PSP while retaining speed. Same for other platforms. So, it's perfectly valid to ask whether we need to revise our OSystem API with regards to OSystem yet again. You essentially ask for that below, I'll address it there.

> Nevertheless, I think I'll go with this option and just add the processing
> to pollEvent.

That would be a pragmatic thing to do, I guess.


> My opinion is that since the engines are in control of the main loop, they
> should call updateScreen regularly, at least so long as there's anything on
> the screen. They already telegraph their intentions using the copyRect
> calls, so that the backends know when there's a need to update the screen.
> This is also the way that all the other engines behave as far as I can tell.

Actually I don't think it's always the case for all of them, otherwise various ports wouldn't use the hack of calling updateScreen in pollEvents  etc. :-).

> I'm not complaining though -- I understand this behavior was made
> specifically for the PSP when the backend wasn't able to handle many
> updateScreen calls.

So, you are asking for the updateScreen API to be changed to work like this: "updateScreen() gives the backend the chance to perform a screen update. A backend may not do *anything* in response to an updateScreen() call. On the other hand, Engines may, and *must*, invoke updateScreen() as often as possible, ideally at least 60 times per seconds, to ensure smooth screen updates on all target platforms"

This is quite a change in semantics. Personally, I consider this a serious *hack*, too. Just a hack of the API, instead of a code hack. Instead of fixing a shortcoming of the backend, it covers it up by asking the frontend code to push out update requests as often as they can. This is something I am used to from programming 8bit micro controllers, but I really dislike the idea of using this on hardware which is capable of running smooth 3D animations in high colors... Yuck :/.

In short, I'd really prefer if don't need to do this.



> 
> I'll try the slight modification to the engine just to see that this is
> indeed the issue, Johannes.
> 


Please do!

Bye,
Max



More information about the Scummvm-devel mailing list