Hi! On Sun, Nov 06, 2016 at 04:15:51PM +0100, Tim Janik wrote: > 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.
Ok, so here is my first batch of answers. My stwbeast.git repo now has a branch wip/soundfont which is based on your branch, and includes all fixes that I implemented following your suggestions. As a general remark, the problem is that when the code was written (2009), much of the code had to be implemented in C, because bse C++ migration hadn't progressed far. So the general challenge now is of course to modernize the areas that really need it, without taking it to the extreme of throwing away all code and reimplementing everything in C++ from scratch. Note that in a few cases, I didn't fix problems yet, or I thought further discussion is necessary. However, new wip/soundfont should be closer to mergeable than any previous submission. > > 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 Ok. > > 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.) Ok. > + 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. This is not the UI label; _("Synth Input") is the property_group. > + 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 To be honest, I have no idea how to make BseStorage allocated with new+delete. So far it appears to be a GObject derived from BseObject, no idea which steps need to be done to use C++ new+delete. >From my competence level with how the object system works, the only thing that I could offer to implement would be a workaround: add a an embedded struct BseStorage { ... struct Data { } *data; }; allocate Data with new+delete, and put stuff that requires C++ initialization like std::vector<> or std::string into the data member. > b) add a vector<BlobP> Sounds good. > c) make BlobP a: typedef std::shared_ptr<Blob> BlobP; Sounds good too. > 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; } I doubt that the Rapicorn::Blob is what we need. If you look at the actual blob code we ship here, the blob is a file referenced by the bse file. There are two cases: 1) non-embedded: so basically then the blob is just a reference to "/foo/bar/bazz.sf2", the bse file doesn't contain any soundfont data - in this case the fluid synth engine can directly open/load the original sf2 2) embedded: then the blob stored within the bse file contains the whole sf2 file, can be houndreds of MB, then if you open that bse file, a temporary copy of the data is written to a temp file. the fluid synth engine then can load "/bse-user-1234-..." So in any case, we pass the filename (temporary or actual) to the fluid synth engine for loading; the blob is - if you want to call it that way - a file within a file; which allows us to make bse files self contained, without fluid synth API being capable of loading embedded soundfonts directly from the bse file. > e) If BSE really needs it's own Blob, it also needs proper unit tests for its > API. I don't agree on this point. We're not talking about a generic data structure here, which should have unit tests. We're talking about a specific API part of BseStorage. Its not Bse::Blob, but Bse::Storage::Blob - if you want to rename it to something that better reflects the purpose, ok. Of course if you're saying you want to have unit tests for every part of BseStorage, then ok, we'd also have unit tests for Bse::Storage::Blob. However, I don't think we can afford the developer time it costs for unit testing every single aspect of every single API we have... > @@ -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*? SoundFonts are huge (sometimes 400M or more). Not all systems clean /tmp reguarily. So we want to make sure that we don't leak temp files. Even if beast crashes or the user kills the beast process or the system was rebooted and the system doesn't clean /tmp on reboot. So it is wise to look at everything that we may have left from earlier beast processes that are now dead on startup. Therefore bse_storage_blob_clean_files() scans /tmp for old temp files, looks if the PID is still running, and if not removes the temp file. So we shouldn't accumulate more and more stale temp sf2 files. > 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. Done. > > > +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. Ok, I refactored the code, so this is std::vector now. > +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. Moved. > +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... Presets simply hide program|bank settings. To for instance if the user wants a church organ, then he can say so at the ui. The track will have a pointer to a preset which says "church organ". When the project is saved|loaded, these preset objects are saved similar to the bsewave objects. Finally, during playback, the program and bank integers from the preset determine which sound gets selected, so church organ may be program 20, bank 1 or something. So the user sees the name whereas the fluid engine sees the numbers. There is no need to edit these, as the soundfont already determines which presets exist and which numbers the presets have. > + 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. To be honest, this may be the "correct" solution, however I may not be able to implement it, because my knowledge of bse internals is not good enough. This is hardly standard procedure where I could lookup how to do it from some other code snippet. > 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.) As far as I know, once per project is fine. BseSoundFontRepo exists as a child to the project. The important thing is that we want to allow the user to use 10 instruments from the same soundfont in one project, without forcing us to load the same soundfont 10 times. Therefore the fluid state is in BseSoundFontRepo. However, if the user opens two projects that use the same soundfont, we will load it twice, which is acceptable I think. And yes, we explicitely instantiate our own fluidsynth per BseSoundFontRepo: static void bse_sound_font_repo_init (BseSoundFontRepo *sfrepo) { sfrepo->fluid_settings = new_fluid_settings(); sfrepo->fluid_synth = new_fluid_synth (sfrepo->fluid_settings); ... so different projects don't interfere and don't need to share one mutex. > 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)); Right, that sounds reasonable - so I migrated the lock/unlock calls to the guard stuff. > 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. All right, I replaced it. > +/// 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 Cu... Stefan -- Stefan Westerfeld, http://space.twc.de/~stefan _______________________________________________ beast mailing list [email protected] https://mail.gnome.org/mailman/listinfo/beast
