[Scummvm-devel] SDL backend modularization remarks

[Max Horn]

Hi Tony, hi ScummVM developers,

some remarks on Tony's project (which is about modularizing the SDL backend and providing OpenGL (ES) support) and recent commits... I was not really privy of the design discussion(s) with your mentors, so I am sorry if some of these are redundant remarks, but here they are anyway. Note: This is not meant to "shoot down" your project. I have my clear cut opinions on how the backend code should look and work, but I am always willing to listen to other points of views and reasons; but so far I heard none on this matter, and I feel it is important that we resolve this before you spend even more time on an approach that may need serious changes...

I am also not saying that you must implement *all* of the things I describe below, after all, some of them may go beyond your task. But I thought it's best to describe my vision for this in more generality, to help bring across my point of view :)


The first thing that I noticed is that you introduce lots of intermediate classes, and also heavy use of multiple inheritance and diamond shaped inheritance. That is a feature that has quite some pitfalls and makes things complicated, and I really question that design choice. I really would prefer to avoid this as much as possible. Luckily, it seems we can avoid it almost completely :).

Our current approach to modularize the backend code is based on component aggregation to model subsystems. The idea is that we provide a set of components (modules), and then an actual backend can easily pick and combine components as it likes to, even dynamically (in theory at least, we don't really need this ability right now). This is the point behind timer managers, event managers, file system factories etc.

IMO this makes it easier to combine submodules than the inheritance model you are now introducing, which open up all sorts of potential pitfalls for backend authors. Moreover, the new classes now introduce a duplication of functionality with the old component based approach; so if we go with it, it seems to me that we should at the same time drop the old component based stuff (but I am right now very skeptical that this is the right way to go). Let me clarify with concrete examples:

Timer code
==========

In the current code base, it is possible for a backend to use a custom "TimerManager" component. If desired, though, it can just use the default timer manager, and be done with it (almost all backends do that these days; we used to have a custom MorphoOS timer manager, and now there is a custom one for PSP). All it needs to do for that is to provide functionality for a timer callback. I.e. just instantiate DefaultTimerManager, hook it up with your timer callback, and you are done.

In the case of SDL timers, a natural way to provide an "SDL Timer Manager" hence would be to either subclass DefaultTimerManager, adding code to hook up the timer handler inside this subclass; or alternative provide the code to do this in another easily sharable location, such as a function or class that does it and which can be optionally compiled.

Instead, you now introduced BaseSubSys_Timer to model the "timer" module (while retaining the TimerManager component, and also the DefaultTimerManager). This seems to me to add no useful functionality, but *does* now require you to use virtual inheritance, and diamond shaped subclassing. Granted, this class (or rather its subclass for SDL) provides some extra functionality, but I will argue below why it can go.

My recommendation:
* Scrap BaseSubSys_Timer (side note: it currently contains no functionality anyway, and provides only init/deinit methods, so nothing lost)

* Scrap SdlSubSys_Timer

* Use the code from SdlSubSys_Timer::timerInit and timerDone() to make a new class SDLTimerManager, which subclasses DefaultTimerManager and has this in its constructor / destructor

* the various *feature* methods should be removed: they are (a) useless (contain no code), and (b) dangerous (as you overload them in several class in the middle of a diamond shaped inheritance. 

* The check for a custom _timer object in SdlSubSys_Timer::timerInit becomes redundant this way -- if a backends wants to use a custom timer, it won't use this TimerManager subclass *at all*

* The other functions in SdlSubSys_Timer are either trivial (getMillis, delayMillis), or not generic to all SDL ports (getTimeAndDate, which uses time() and localtime(), which do not exist on all SDL platforms).  The following alternatives all seem viable to me:
 - just live with the minor code duplication
 - for getMillis & delayMillis, we could add the code directly to the "base" SDL backend class (from which all SDL based backends derive, including the primary "real" SDL backend). The methods they call are present anyway, and if a subclass needs to customize them, it can just overload them; we just loose a few dozen bytes of binary size this way, nothing worse
 - we could add some or all of these to TimerManager. Drawback: This would force all backends to subclass TimerManager or DefaultTimerManager. Not sure this is worth it.

File code
=========

I can say similar things about some of the other "modules, e.g. the file module

* BaseSubSys_File can be scrapped

* SdlSubSys_File can be scrapped, because the following holds for its methods
 - fileInit only creates the _savefile object. Just put this into the base SDL backend's init() method:
	if (_savefile == 0) {
		_savefile = new DefaultSaveFileManager();
	}
  Then the "POSIX" subclass of the SDL backend simply overloads init with something like this
    void POSIX_SDL_OSystem::init() {
      	if (_savefile == 0) {
		_savefile = new POSIXSaveFileManager();
	}
        Base_SDL_OSystem::init();
    }

 - fileDone can be put into the destructor or "deinit" method of the "base" SDL backend class
 - the *feature* methods are useless (see above)
 - getFilesystemFactory & getSavefileManager can just be directly moved into the base SDL backend, together with the member variables _savefile and _fsFactory

  - addSysArchivesToSearchSet: add an empty implementation to the base SDL backend class; and move the code in SdlSubSys_File::addSysArchivesToSearchSet to the POSIX SDL backend class. The OS X specific one could be moved to an according class, too, of course.

 - make getDefaultConfigFileName() a pure virtual method of the base SDL backend; move createConfigReadStream & createConfigWriteStream to this


Mutex code
==========

It seems to me that SdlSubSys_Mutex (and by extension BaseSubSys_Mutex) can be scrapped directly. Just provide this functionality directly in the SDL base backend. As with getMillis(), this code is tiny and will compile on any SDL based backend; it just may not work correctly, but if a sub-backend overloads the mutex methods, then they never get called, so no harm done.

Also, having _graphicsMutex in there seems completely wrong to me, even if we retain SdlSubSys_Mutex. The graphics mutex is an implementation detail of the *graphics* subsystem, so it should be contained there, and the mutex should be used via the mutex subsystem (or, better, using the mutex code in common/mutex.h).


Feature methods
===============
As already mentioned, I think the hasFeature / setFeatureState / getFeatureState methods in the various intermediate classes are dangerous; moreover, I think they are difficult to use correctly. And in most (all?) cases useless, as they do nothing.

If a subsystem really needs to provide custom feature flags, we can find another way. E.g. you could simply add specific feature methods for these, e.g. hasGraphicsFeature(), and then let the actual OSystem::hasFeature() implementation invoke those, and bit-OR the results together. Likewise for setFeatureState, it could call through to setGraphicsFeatureState, if desired.


Audio code
==========

This could be achieved similarly to the TimerManager stuff: Just make a SDLMixerHandler class which contains the mixer specific code, like this:
  class SDLMixerHandler {
  public:
     SDLMixerHandler();
     ~SDLMixerHandler();

     init();
     deinit();
  protected:
     mixerProducerThreadEntry(...);
     mixCallback(...)

     Audio::MixerImpl *_mixer;
     ....
  }

  Then, when an SDL backend wants to use the "normal" audio code, it can just instantiate this component, and use it.
  Alternatively (maybe more elegant), subclass Audio::MixerImpl directly, and add the required init/deinit methods and code directly in there.

Note that in either way, the current code in setupMixer() should be reviewed; many backends use very similar but not completely identical versions of that; you may need to split this method further to allow useful overriding (no matter where the method(s) live in the end). Or not.

This only covers the mixer related code and leaves the Audio CD code. For many backends, it makes absolutely *no* sense to have this code! Indeed, the IMHO correct approach here would be the following: Turn AudioCDManager from a singleton into another backend component (similar to TimerManager and EventManager). Remove the whole Audio CD code from OSystem, instead add 
  virtual Common::AudioCDManager *get AudioCDManager() = 0;
to OSystem. Turn the current AudioCDManager into a new DefaultAudioCDManager which does *not* try to invoke the (now removed) Audio CD APIs in OSystem. Instead, backends which want to provide "real" audio CD support need to provide their own AudioCDManager subclass. AFAIK only three backends do this:

1) The SDL backend (and even here it only makes sense for a very few ports).
2) The Nintendo DS backend (has no real CD, but allows playing DVI ADPCM compressed WAVE files for audio tracks, as the regular compression based AudioCDManager code apparently is to resource intensive for the NDS?!)
3) The Dreamcast backend (can actually play CDs)

These could subclass DefaultAudioCDManager to restore the current functionality (or in the case of the NDS port, if it really cannot use the regular CD "emulation" code, it could directly subclass AudioCDManager).


Graphics & event code
=====================
I can't really comment on these yet, as there is no code there yet, only the empty BaseSubSys_Graphics and BaseSubSys_Events shells which don't even specify an API. However, I already now wonder if class BaseSubSys_Events could maybe be replaced by using / subclassing EventManager appropriately




Cheers,
Max