Soundfont patch comments

[Tim Janik <timj gnu org>]

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.