Hi Stefan, first, thanks a lot for working on SF2 support, there's a lot of work involved and it's great you took that on.
As mentioned in my other email, full history has been pushed to wip/soundfont now, including all of your recent changes plus the history from 2010. Here's my first batch of comments and questions, by no means comprehensive, but it should be substantial enough to show what areas still need extra work. Where I'm referring to diff bits in the comments, you can easily find the relevant sections by searching the output of: git diff wip/soundfont^2..wip/soundfont -b > Old .proc files were replaced by proper bseapi.idl entries. Use present tense / imperative wording for commit messages http://git.kernel.org/cgit/git/git.git/tree/Documentation/SubmittingPatches > Merge branch 'wip/soundfont-merge' into master This is misleading, it's what you've done locally, but as far as the remote branch is concerned, that you're making public, you just merged master's fixes into the feature branch. (I've fixed this up in wip/soundfont already.) + bse_object_class_add_param (object_class, _("Synth Input"), + PROP_SOUND_FONT_PRESET, + bse_param_spec_object (("sound_font_preset"), _("Sound Font Preset"), + _("Sound font preset to be used as instrument"), + BSE_TYPE_SOUND_FONT_PRESET, + SFI_PARAM_STANDARD ":unprepared")); bse_object_class_add_param (object_class, _("Synth Input"), You must not use the same UI label for two properties of the same object. + g_object_notify ((GObject *) self, "sound_font_preset"); We use casts without intermediate whitespace. +BseStorageBlob *bse_storage_blob_ref (BseStorageBlob *self); Please attach the asterisk to the return type, not the function, like we do elsewhere for function return types. +void +bse_storage_blob_unref (BseStorageBlob *blob) +{ [...] + if (blob->is_temp_file) + { + unlink (blob->file_name); + /* FIXME: check error code and do what? */ I guess unlinking would most often fail if the temporaray file has been removed already (some unixes like linux support readin/writing files that have been unlinked already). So I suggest to ignore the return value and add a comment. On a more general note, I really dislike introducing new C structures, especially if they roll their own reference counting. I think what should be done here instead is: a) ensure BseStorage is properly C++ allocated with new+delete b) add a vector<BlobP> c) make BlobP a: typedef std::shared_ptr<Blob> BlobP; d) However, since we have a Blob in Rapicorn already, it should be investigated what can be done to use that instead, and then simply do namespace Bse { using Rapicorn::Blob; } e) If BSE really needs it's own Blob, it also needs proper unit tests for its API. @@ -108,6 +119,10 @@ bse_storage_class_init (BseStorageClass *klass) + bse_storage_blob_clean_files(); /* FIXME: maybe better placed in bsemain.c */ Why would we want to delete PID related temporary files on *startup*? On a more general note, I'd really like to see you fix the remaining FIXMEs on the branch before the final merge. +G_END_DECLS Please get rid of any extern "C" variants in the branch. +struct BseSoundFontRepo : BseSuper { [...] +class SoundFontRepoImpl : public SuperImpl, public virtual SoundFontRepoIface { Please try to move as many fields from BseSoundFontRepo into SoundFontRepoImpl as possible. That also means you get to use std::vector etc and should make the code significantly easier. + sfrepo->channel_values_left = (float **)g_realloc (sfrepo->channel_values_left, sizeof (float *) * channels_required); [...] + sfrepo->oscs = (BseSoundFontOsc **)g_realloc (sfrepo->oscs, sizeof (BseSoundFontOsc *) * (i + 1)); More whitespace issues in casts. Ideally this should be a std::vector anyway. +static void +bse_sound_font_repo_init (BseSoundFontRepo *sfrepo) +{ + new (&sfrepo->fluid_synth_mutex) Bse::Mutex(); Calling ctor/dtor manually is a strong indicator that a field should be moved into the corresponding C++ struct instead. +struct BseSoundFontPreset : BseItem { + int program; + int bank; +}; Hm, so we have BseSoundFontPreset objects that have program+bank, save and load those values but do not expose them as properties. I think that's unprecedented in BSE so far. But I'm not claiming I fully understand the Preset impl so far... + bse_param_spec_object (("sound_font_preset"), _("Sound Font Preset"), There are extraneous parenthesis here. bsesoundfontosc.cc: Good that you added an extended comment about how SF2 support works with the BSE engine, that helped a lot. Open issues I see with that: a) I'll probably rework the comment so it shows up in the Doxgen docs, not sure if that should go under SoundfontOsc or some more generic "Bse Engine" topic though. b) There's a basic design problem here wrg to locking of the SF2 osc modules. To recap, all SF2 engine modules lock fluidsynth, the first one calls process_fluid_L, the others block and stall concurrently processing CPUs. The reason this problem exists is that there's a n:1 dependency here (n*SoundfontOsc -> 1*fluidsynth) that's not reflected in the bse engine module tree, so the scheduler cannot accomodate for that. What needs to be done is: all n BseSoundfontOsc modules need to take 1 additional input channel (not shown at the UI), and that channel gets connected to 1 fluidsynth bse engine module that also has no user visible representation in the BseObject tree. This 1 fluidsynth module then calls process_fluid_L() and the bse engine scheduler ensures that all n dependant modules are called *after* the fluidsynth module has finieshed. c) What's unclear to me is wether there should be 1 fluidsynth module globally or per BseProject, is there something like a FluidsynthContext that's instantiated per project, or is there only one (implicit) context globally? That determines if there's need for one fluidsynth module per project or globally. (In case you wonder, simultaneous playback of multiple projects is required for sample preview and note editing, and will be required for virtual midi keyboard support.) d) Adjusting CPU affinity as mentioned in your comment will not solve the stallation problem. Affinity cannot be assigned per engine module, the engine scheduler can just schedule subtrees (dependency branches) per CPU, and a subtree may contain 0, 1 or n SoundFontOsc modules, where n is completely unrelated to number of available CPUs. + bse_sound_font_repo_lock_fluid_synth + bse_sound_font_repo_unlock_fluid_synth I'm quite unhappy with the API side of the locking here. The previous comment explains why the locking is needed with the current design, I wonder how much locking is still needed once a Bse engine fluidsynth module is in place. In case locking is still neded after that, I'd like to see it done with different API though. What I have in mind is something aikin towards: Bse::Mutex& bse_sound_font_repo_mutex (BseSoundFontRepo *sfrepo) { return sfrepo->mutex; } That way all lock/unlock pairs can be replaced with guards: std::lock_guard<Bse::Mutex> guard (bse_sound_font_repo_mutex (sfrepo)); Note 1, Bse::Mutex is a Rapicorn::Mutex and AFAICS that implementation has become obsolete with C++11, so it should become a typedef std::mutex Mutex; in the foreseeable future. Note 2, as with all other Bse* objects, we're migrating to the use of real C++ classes, which means new fields / data members should be added to the Bse::FooImpl classes instead of BseObject "derived" structs. So eventually, I expect that line to look like: std::lock_guard<std::mutex> guard (sfrepo.mutex()); +bse_sound_font_remove_item (BseContainer *container, + BseItem *item) +{ + BseSoundFont *sound_font = BSE_SOUND_FONT (container); + + if (g_type_is_a (BSE_OBJECT_TYPE (item), BSE_TYPE_SOUND_FONT_PRESET)) + sound_font->presets = g_list_remove (sound_font->presets, item); + else + g_warning ("BseSoundFontRepo: cannot hold non-sound-font-preset item type `%s'", + BSE_OBJECT_TYPE_NAME (item)); That check is pretty useless, just assert if item->parent = this or not. + std::list<EventHandler> event_handlers; Please use a std::vector here instead (and remove include<list>), unless you have several thausands of nodes and frequently need to keep external pointers to nodes around, vector is generally better than list. +/// Interface for sound fonts +interface SoundFont : Container { That comment is totally redundant, i.e. bad. Please add a short description about what a sound font is instead. Please go through the final diff yourself and address the FIXMEs you've left in place. Fix them, or ask here how to go about them. -- Yours sincerely, Tim Janik https://testbit.eu/timj/ Free software author. _______________________________________________ beast mailing list [email protected] https://mail.gnome.org/mailman/listinfo/beast
