Soundfont patch comments

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Soundfont patch comments

Tim Janik-6
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
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Soundfont patch comments

Stefan Westerfeld
   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
> [hidden email]
> https://mail.gnome.org/mailman/listinfo/beast

   Cu... Stefan
--
Stefan Westerfeld, http://space.twc.de/~stefan
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Soundfont patch comments

Tim Janik-6
On 07.12.2016 14:10, Stefan Westerfeld wrote:
> 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.

Great, thanks for the work so far. I think we need to have a call to sort some
of the areas where I don't fully grasp the SF2 specifics and you asked for help
with BSE internals.
I'll try to mark those.

>> 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.

Wow, you're right indeed.
That just hammers home how useful the IDL files are compared to the old API.

>> 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.

Ok, I see, seems tricky indeed. Just the first thoughts I have about this:
* I wonder if it needs to be a GObject at all and couldn't be transformed into a
normal virtual C++ clas easily.
* If it needs to stay a BseObject, adding a struct Data with explicit ctor/dtor
as you suggest sounds like the best solution. You migth go for that either way,
because even if BseStorage can become a native C++ class you'll have done half
of the porting already.

>>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

Yes, those would go into object_init and object_finalize respecitvely, even
avoiding the extra allocation:
   struct Data
   {
   } data;
object_init:
        new (&data) Data();
object_finalize:
        data.~Data();

> 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

Note, Rapicorn::Blob can refer to ondisk files.

> 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.

Yes, about that...
I think we've previously discussed possibly moving BseStorage to a *directory*,
so its individual bits are composed of real files and we just need a longer
import/export period when loading/storing bse files.
Having that in place would certainly have helped with the implementation here,
but short of that, it's important to keep this in mind, so not too much effort
is wasted to maintain this in-file complexity.
I.e. if that seems to hard, efforts are better spent on the bse->directory move
instead and simply using external file handles for assets in the code.

>> 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...

Not for every existing bit, that's right. But we need some basic tests for new
stuff that gets added. We need to be able to assert some basic functionality in
between releases and after frequent code changes with all the migrations we have
going on. The only way to achieve that is adding basic unit tests when new
components are introduced (and designing for that) and adding tests for
regressions we encounter. Which btw means we also need a basic unit test for the
SF2 support itself. You could be starting with scripting that first and then
figure which parts of the new Blob are not indirectly tested and still need unit
test exposure.

>> @@ -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.

The whole thing still looks flakey to me. Thoughts:
* I really want to avoid messing with other programs data here, maybe moving
everything into tmp/beast-data-<UID>.XXXXXX/ can help with that.
* /tmp/ might very well be a *small* memfs mount, possibly residing in swap,
usually not bigger than the actual RAM size. Which means it might be the wrong
place to extract hundreds of MB.
* ~/ might bew NFS mounted or encrypted, which means it might be the wrong place
to extract hundreds of MB in.

Considering the above, the location needs to be user configurable. The XDG spec
has something in place for that alread [1]; i.e. we should pick
$XDG_CACHE_HOME/libbse/ - see Rapicorn::Path::cache_home().

[1] https://specifications.freedesktop.org/basedir-spec/latest

>> 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.

Couldn't the preset be stored as a simple string then?

>> 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.

Sure, I'll give you a hand with that. Regarding precedents, I think the
ContextMerger comes close, it creates more modules than BseObjects behind the
scenes to implement polyphony, so should be able to give us an idea how to wire
things up.

>> 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.

Keep in mind that loading it twice is a common case during editing.
I.e. the SF is loaded once for the project you're editing, and during note
preview in the piano roll, the current playback network (including SF) is
duplicated in a temporary project that plays the preview note.
Since that's a common case, it'd be good if there was a way to share big sound
fonts or fluidsynth contexts between two projects...

BTW, other DAWs allow only *one* project at a time to start/use the synthesis
engine, which is probably not too bad a limitation in practice. If we were to
move to that model, could we have a single global fluidsynth context that gets
shared between the active project and internal temporary assistant projects?

>    Cu... Stefan



--
Yours sincerely,
Tim Janik

https://testbit.eu/timj/
Free software author.
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Soundfont patch comments

Stefan Westerfeld
   HI

On Thu, Dec 08, 2016 at 12:21:23AM +0100, Tim Janik wrote:

> On 07.12.2016 14:10, Stefan Westerfeld wrote:
> > 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.
>
> Ok, I see, seems tricky indeed. Just the first thoughts I have about this:
> * I wonder if it needs to be a GObject at all and couldn't be transformed into a
> normal virtual C++ clas easily.
> * If it needs to stay a BseObject, adding a struct Data with explicit ctor/dtor
> as you suggest sounds like the best solution. You migth go for that either way,
> because even if BseStorage can become a native C++ class you'll have done half
> of the porting already.

Ok, I now moved C++ members to the data structure as proposed, with

> object_init:
> new (&data) Data();
> object_finalize:
> data.~Data();

This allowed me to use std::shared_ptr for Blobs now, so all manual C-style
reference counting is gone. If BseStorage gets proper constructors, for
instance if it is refactored to a native C++ class, Blobs should be extremely
easy to port to that.

> > 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
>
> Note, Rapicorn::Blob can refer to ondisk files.
>
> > 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.
>
> Yes, about that...
> I think we've previously discussed possibly moving BseStorage to a *directory*,
> so its individual bits are composed of real files and we just need a longer
> import/export period when loading/storing bse files.
> Having that in place would certainly have helped with the implementation here,
> but short of that, it's important to keep this in mind, so not too much effort
> is wasted to maintain this in-file complexity.
> I.e. if that seems to hard, efforts are better spent on the bse->directory move
> instead and simply using external file handles for assets in the code.

Right. I believe the amount of work for allowing embedding is not too much right
now, but if bse ever uses directories, it would be simple to migrate to that.

> >> 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...
>
> Not for every existing bit, that's right. But we need some basic tests for new
> stuff that gets added. We need to be able to assert some basic functionality in
> between releases and after frequent code changes with all the migrations we have
> going on. The only way to achieve that is adding basic unit tests when new
> components are introduced (and designing for that) and adding tests for
> regressions we encounter. Which btw means we also need a basic unit test for the
> SF2 support itself. You could be starting with scripting that first and then
> figure which parts of the new Blob are not indirectly tested and still need unit
> test exposure.

Ok, so if we generally look at what we can test with a reasonable amount of work,
I suggest two tests:

1) add a .bse file which uses (external file) a sound font to audio tests
2) add a .bse file which includes (in bse file) a sound font to audio tests

Both are simple to add. I've added (1) to my wip/soundfont branch now. The
hardest part was stripping down fluid r3 so that it can be included, I've now
a very small

-rw------- 1 stefan stefan 84512 Dez  9 13:22 /home/stefan/src/stwbeast/tests/audio/minfluid.sf2

version which has only one preset, and thus should be ideal for testing. I've
included the (MIT) license info from debian.

In any case test (2) is very easy to add, I've just not done it yet in case
we still change something in the .bse file format. So with those we test

- loading by filename
- loading (via temp file) from embedded data
- playing/synthesis part

I'm sceptical whether there should be more tests. Although a test for saving
would be nice to have, there is no pre-existing code in bse for that. We don't
have any unit test for BseStorage, so it may be harder to add.

> >> @@ -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.
>
> The whole thing still looks flakey to me. Thoughts:
> * I really want to avoid messing with other programs data here, maybe moving
> everything into tmp/beast-data-<UID>.XXXXXX/ can help with that.
> * /tmp/ might very well be a *small* memfs mount, possibly residing in swap,
> usually not bigger than the actual RAM size. Which means it might be the wrong
> place to extract hundreds of MB.
> * ~/ might bew NFS mounted or encrypted, which means it might be the wrong place
> to extract hundreds of MB in.
>
> Considering the above, the location needs to be user configurable. The XDG spec
> has something in place for that alread [1]; i.e. we should pick
> $XDG_CACHE_HOME/libbse/ - see Rapicorn::Path::cache_home().
>
> [1] https://specifications.freedesktop.org/basedir-spec/latest

Ok - done, so it should usually go to ~/.cache/libbse/ now.

> >> 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.
>
> Couldn't the preset be stored as a simple string then?

If presets are objects, tracks and sound font oscs have one single property
"Sound Font Preset" which determines both, the active sound font and the preset
(as the preset object always knows to which sound font it belongs). This is the
way things work now.

If presets are strings, then each track and each sound font osc would need two
properties, "Sound Font" and "Sound Font Preset", as the string 'Trumpet' no
longer determines the sound font it belongs to, because multiple loaded
soundfonts could have this preset. The SF2 support could be refactored to work
in this two-propery-way, not sure if I'd consider this more elegant. It's quiet
a bit of work to change it, but you think it should be done this way, then it
should be changed now rather than later, because it will affect how the bse
files look.

> >> 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.
>
> Sure, I'll give you a hand with that. Regarding precedents, I think the
> ContextMerger comes close, it creates more modules than BseObjects behind the
> scenes to implement polyphony, so should be able to give us an idea how to wire
> things up.

Great.

> >> 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.
>
> Keep in mind that loading it twice is a common case during editing.
> I.e. the SF is loaded once for the project you're editing, and during note
> preview in the piano roll, the current playback network (including SF) is
> duplicated in a temporary project that plays the preview note.
> Since that's a common case, it'd be good if there was a way to share big sound
> fonts or fluidsynth contexts between two projects...
>
> BTW, other DAWs allow only *one* project at a time to start/use the synthesis
> engine, which is probably not too bad a limitation in practice. If we were to
> move to that model, could we have a single global fluidsynth context that gets
> shared between the active project and internal temporary assistant projects?

Sharing state between different projects is not easy to add. Right now, the
fluid state is initialized to a fixed number of channels / channel->sfont
mapping during project prepare. One channel is added for each track.

The number of channels/mapping is then considered static for the time the
project is playing. So sharing state between different projects, temporary or
non-temporary would be problematic, because if projects switch from playing to
non-playing dynamically, the total number of channels would have to be changed.
However the fluidsynth API forces us to do set the number of channels at
initialization time.

I think we'll just have to live with a factor of two here. Sounds acceptable
to me at least.

As a general remark, if we were not discussing fluid synth integration but a
project that contains a LV2 plugin, like ZynAddSubFX, duplication of the plugin
for temporary assistant projects would be a terrible idea, since the ZynAddSubFX
plugin has a GUI, and there is a 1:1 mapping between the UI and the plugin
instance. So maybe a long term strategy for BEAST should consider getting rid
of temporary assistant projects altogether, in order to be better able to
integrate components that contain complete instruments|effects as plugins, LV2,
VST or similar.

If you look at Bitwig Polysynth or VST instruments, there is always one
instance per instrument (track), and this one instance interacts with the user
via UI, renders audio data, processes midi events and so on. No temporary
assistant projects here. ZynAddSubFX VST|LV2 works in Ardour5, Qtractor, Bitwig
and other DAWs.  In the long run I'd like to be able to use it in BEAST tracks.

   Cu... Stefan
--
Stefan Westerfeld, http://space.twc.de/~stefan
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Loading...