[Scummvm-devel] Issues with kyrandia engine

[Max Horn]

Am 07.06.2010 um 21:54 schrieb yotam barnoy:

> On Mon, Jun 7, 2010 at 9:07 PM, Max Horn <max at quendi.de> wrote:
>> 
>> [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 :-(. ]
> 
> I try to remember but gmail keeps changing it back :(
> 
>> 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.
> 
> OK I get it. Except that even looking at it that way, I'm sampling a
> signal of unknown frequency. There may be elements of the signal (the
> calls coming from the engine) that are 30Hz or less, but there could
> easily be elements that are higher than 60Hz too (for example the
> final call in Kyrandia may be closer to the previous call than 1/60.
> And I want even these elements. In fact, if the signal really was
> 30Hz, I wouldn't need Nyquist frequency -- waiting 1/30th of a second
> would do, since my phase would be synchronized with the update calls.
> Nyquist is only needed for reproduction of a signal ie. being able to
> know the true frequency content of the signal.

*If* the signal was *exactly* 30Hz, then yes, it would work. But if the signal was 29Hz or 30Hz, then it would fail *badly*. You almost never will get an exact 30Hz.. in fact, in reality you won't even get N Hz for some integer N, but rather something like 29.973 Hz :-). This is why Nyquist does apply after all :-).

> 
>> 
>> 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.
> 
> It fails for Kyra only because the engine goes above 30Hz for the last
> update right when the animation ends. Whatever Hz that is, if I knew
> it, I could adjust it. But it's possible that it goes over 60Hz too (I
> haven't measured). It works for any engine that maintains a consistent
> frequency (of any rate) of calls to updateScreen(), or one that waits
> 1/30 second until stopping the last update, which happens to be all
> the other engines, as well as Kyra before the change was made
> specifically for the old PSP code.

So, for those other engines, you happen to be lucky -- but that rate throttling logic is *still* incorrect. Consider this trivial example which, if I understand correctly, will fail to render the text on your backend:

drawLogo();
g_system->updateScreen();
drawText();
g_system->updateScreen();
g_system->delayMillis(2000); // Let logo & text show for 2 seconds before continuing.

The text is never shown because the second updateScreen comes too soon after the first one.
Of course this artificial example can be easily "fixed" by removing the first updateScreen call; but it's easy to come up with real world examples that can lead to the same problem

If we changed our API to require frontends to invoke updateScreen regularly, one would have to change that to:

drawLogo();
g_system->updateScreen();
drawText();
g_system->updateScreen();
g_system->delayMillis(1000/30);
g_system->updateScreen();
g_system->delayMillis(2000-1000/30); // Let logo & text show for 2 seconds before continuing.

Actually, even that might not work quite right due to rounding issues, and due to the fact that other backends might use other frame rates, so better make it

drawLogo();
g_system->updateScreen();
drawText();
for (int i = 0; i < 120; i++) {
  g_system->updateScreen();
  g_system->delayMillis(2000/120);
}


Faced with this, I'd rather stick with our current OSystem semantics, and let backends who want to coalesce screen updates hook into delayMillis and pollEvent for extra screen update opportunities... :)

[...]

> 
>> 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...
> 
> It's a complication that's not necessary.
[...]
> Thread synchronization is a pain. I may give it a try
> when I'm done doing other optimizations, but right now I don't want to
> get into that mess. I may even be exaggerating it and it'll turn out
> to be a piece of cake. From my experience on the PSP though that's
> usually not the case.

We are on the same line here: I think it's doable (at least from a theoretical point of view, as I am not familiar with the peculiarities of the PSP), but certainly is far more painful then adding a simple call to updateScreen() to your delayMillis and pollEvent methods (and maybe more).

BTW, I need to clarify something: I re-checked the NDS backend, and it actually does the opposite of what I was saying: It calls the sound callback and checks for input events in its updateScreen implementation ;).


> 
>> 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"
> 
> Taking a quick peek at the current notes for updateScreen() both in
> the code and in the wiki is pretty open to interpretation.

Hmm? I am not sure where you see something open for interpretation here, at least with this regard. The updateScreen docs state this (in the doxygen comment, which is authoritative and goes above anything the Wiki might say):

"Flush the whole screen, that is render the current content of the screen
framebuffer to the display.

This method could be called very often by engines. Backends are hence
supposed to only perform any redrawing if it is necessary, and otherwise
return immediately.
"

This clearly says that after the updateScreen call, the screen framebuffer content should be visible on the display. Strictly speaking that means that even a slight delay of a few milliseconds is not allowed. 
But I guess it's OK to delay a bit, as the PSP backend does, as long as the delay is not visible to the human eye.


> In
> particular, the wiki even gives the 'sampling' code as an example
> (albeit with a warning :)

Yeah, the warning should be made much bigger. Or maybe a link to this discussion should be added :). 

> 
> I guess I agree with you though that it's not necessarily the right
> way to go -- the frontend shouldn't worry about the backend -- but the
> fact is that that's how the API has been understood so far as far as I
> can tell. Are there many backends that use a thread to draw at regular
> intervals?

No, but then most backends do not use rate throttling. The SDL backend for example simply always performs a screen update when updateScreen is called *and* the screen content has changed. Hence it does not have any problems at all.


> 2 factors have been pushing to rely on the calls from the frontend as
> far as I can tell: 1. Our games have low frame rates and thus running
> at an independent draw rate seems like a waste 2. The frontend is the
> one running the main loop. Since historically things have worked this
> way, I don't see the crime in telling the frontend that an
> updateScreen at too fast of an interval (whatever that interval is
> decided to be) may be ignored by the backend, and so it's safest to
> send another update call or to wait a little before sending it. That's
> really the only change I would want.

I don't think this is going to work, see my example above. And anyway, what would that "interval" you talk about be? Would it be the same for all backends and engines? How do you deal with the 29.95Hz vs. 30Hz clash that *will* happen?

For now, the best way for the PSP backend is to use the approach I suggested (either by adding "checks" to extra OSystem calls, or using a thread/timer). 


Bye,
Max