lucianodato / noise-repellent

Lv2 suite of plugins for broadband noise reduction
GNU Lesser General Public License v3.0
455 stars 38 forks source link

Loading noise profile from Carla carxp file does not work #114

Open bergfried opened 1 year ago

bergfried commented 1 year ago

First of all, thanks for the great work!

I am using Carla on JACK 2 with Noise Repellent on Arch.

Noise Repellent works fine if Carla learned the noise profile during the very same session. That is, "Residual listen" clearly demonstrates the difference between noise and signal. However, now that it works, try saving the current Carla session in a carxp file and close Carla. The carxp file is an XML file and seems to store the noise profile as well.

Now, open Carla again and load the carxp file. "Residual listen" indicates that the noise profile stored in the carxp file is not respected. Note, however, that other parameters like "Reduction amount" ARE respected. Now, without changing anything, save the very same (seemingly broken) session under a different name using "File" -> "Save as". Both the old and the new carxp file are completely identical, indicating that the noise profile has not been discarded and/or not been overwritten during loading the carxp file.

Since Noise Repellent 0.1.5 is not affected, even when using the same Carla version, Carla is probably handling all the values as expected, indicating that Noise Repellent, for some reason, does not care about the noise profile presented by Carla.

Speculation: Maybe not all identifiers have been properly renamed while progressing from 0.1.5 to 0.2.0 or later.

Please keep up the good work.

lucianodato commented 1 year ago

Thanks for reporting! Will look into it soon!

bergfried commented 1 year ago

Thank you for the quick reply. I just want to point out issue lucianodato/libspecbleach#52 to you in case you come to the conclusion that fixing the bug here involves changes that are closely related to said other issue. (Of course, if it turns out that they are unrelated and can be fixed independently, please keep them separate.)

bergfried commented 1 year ago

Is there any progress regarding this issue? I consider it quite important because being able to always use the same noise profile is the main reason as to why someone would use this plugin in the first place (as opposed to, let's say, the adaptive variant).

Also, I just found out that Carla produces log lines whenever I open a carxp file with it. Here are the relevant lines:

[carla] lv2_rdf_new("https://github.com/lucianodato/noise-repellent#new") - got unknown port designation 'http://lv2plug.in/ns/lv2core#threshold'
[carla] lv2_rdf_new("https://github.com/lucianodato/noise-repellent#new") - got unknown port designation 'http://lv2plug.in/ns/lv2core#gain'

I hope this helps.

TekMiikka commented 1 year ago

I have (or had) the same issue. I actually didn't even know this plugin saves the learned noise profile before I saw this post. It happens on Raspian (Linux raspberrypi 5.15.76-v8+) + Carla 2.5.2 + lv2-dev 1.18.4.

I found the cause for it. On "LV2_State_Status restore"-function, the "fftsize" and "averagedblocks" get assigned to something else before they reach "specbleach_load_noise_profile"-function.

I didn't find a clear explanation why it happens, but from what I gathered, calling "retrieve" again changes the previous retrieved value to something high (maybe pointers to pointers? or just trash data?). I didn't try dereferencing them twice tho, because I just now thought of that.

Some links I researched: Here the value just gets reused every time Here on "State Methods" they use two variables, but they get assigned to somewhere else right away These last remarks I don't fully understand cause this is my first dwelve into lv2, but are they connected to this?

Anyways, I jerry rigged it with two extra constants and it works (nrepellent.c from line 402):

static LV2_State_Status restore(LV2_Handle instance,
                                LV2_State_Retrieve_Function retrieve,
                                LV2_State_Handle handle, uint32_t flags,
                                const LV2_Feature *const *features) {
  NoiseRepellentPlugin *self = (NoiseRepellentPlugin *)instance;

  size_t size = 0U;
  uint32_t type = 0U;
  uint32_t valflags = 0U;

  const uint32_t *fftsize = (const uint32_t *)retrieve(
      handle, self->state.property_noise_profile_size, &size, &type, &valflags);
  if (fftsize == NULL || type != self->uris.atom_Int) {
    return LV2_STATE_ERR_NO_PROPERTY;
  }
  const uint32_t permanent_fftsize = *fftsize;    // NEW

  const uint32_t *averagedblocks = (const uint32_t *)retrieve(
      handle, self->state.property_averaged_blocks, &size, &type, &valflags);
  if (averagedblocks == NULL || type != self->uris.atom_Int) {
    return LV2_STATE_ERR_NO_PROPERTY;
  }
  const uint32_t permanent_averagedblocks = *averagedblocks;    // NEW

  const void *saved_noise_profile_1 = retrieve(
      handle, self->state.property_noise_profile_1, &size, &type, &valflags);
  if (!saved_noise_profile_1 || size != noise_profile_get_size() ||
      type != self->uris.atom_Vector) {
    return LV2_STATE_ERR_NO_PROPERTY;
  }

  memcpy(self->noise_profile_1, (float *)LV2_ATOM_BODY(saved_noise_profile_1),
         sizeof(float) * self->profile_size);

  specbleach_load_noise_profile(self->lib_instance_1, self->noise_profile_1,
                                permanent_fftsize, permanent_averagedblocks);    // CHANGED

  if (strstr(self->plugin_uri, NOISEREPELLENT_STEREO_URI)) {
    const void *saved_noise_profile_2 = retrieve(
        handle, self->state.property_noise_profile_2, &size, &type, &valflags);
    if (!saved_noise_profile_2 || size != noise_profile_get_size() ||
        type != self->uris.atom_Vector) {
      return LV2_STATE_ERR_NO_PROPERTY;
    }

    memcpy(self->noise_profile_2, (float *)LV2_ATOM_BODY(saved_noise_profile_2),
           sizeof(float) * self->profile_size);

    specbleach_load_noise_profile(self->lib_instance_2, self->noise_profile_2,
                                  permanent_fftsize, permanent_averagedblocks);    // CHANGED
  }

  return LV2_STATE_SUCCESS;
}

I didn't wanna create a pull request cause I don't know if this is proper way to deal with it.

If you want to know more about the values, on one test run "fftsize" printed out as 1105 when first retrived and 128445920 after the "averagedblocks" retrieve. And "averagedblocks" printed out at first retrieve as 2354 and after the "saved_noise_profile_1" retrieve it printed out as 128444992.

bergfried commented 1 year ago

Thank you so much for fixing this issue, @TekMiikka. I simply replaced the entire restore function in plugins/nrepellent.c with what you have posted, and it works like a charm!

Let us hope that the issue will be fixed upstream so other users can enjoy it as well.

OracleToes commented 1 year ago

I tried your solution @TekMiikka and it seems like it almost worked. It definitely loaded something, but it only seemed to be doing something with one side of the audio. At least it's progress

SnipeXandrej commented 12 months ago

Here's @TekMiikka patch with my horrible hackiness applied...

I just made it to load the saved_noise_profile_1 on the noise_profile_2 thingy and now it seems to "work" when using the stereo version

diff --git a/plugins/nrepellent.c b/plugins/nrepellent.c
index d5743bf..ced680a 100644
--- a/plugins/nrepellent.c
+++ b/plugins/nrepellent.c
@@ -414,12 +414,14 @@ static LV2_State_Status restore(LV2_Handle instance,
   if (fftsize == NULL || type != self->uris.atom_Int) {
     return LV2_STATE_ERR_NO_PROPERTY;
   }
+  const uint32_t permanent_fftsize = *fftsize;

   const uint32_t *averagedblocks = (const uint32_t *)retrieve(
       handle, self->state.property_averaged_blocks, &size, &type, &valflags);
   if (averagedblocks == NULL || type != self->uris.atom_Int) {
     return LV2_STATE_ERR_NO_PROPERTY;
   }
+  const uint32_t permanent_averagedblocks = *averagedblocks;

   const void *saved_noise_profile_1 = retrieve(
       handle, self->state.property_noise_profile_1, &size, &type, &valflags);
@@ -432,21 +434,21 @@ static LV2_State_Status restore(LV2_Handle instance,
          sizeof(float) * self->profile_size);

   specbleach_load_noise_profile(self->lib_instance_1, self->noise_profile_1,
-                                *fftsize, *averagedblocks);
+                                permanent_fftsize, permanent_averagedblocks);

   if (strstr(self->plugin_uri, NOISEREPELLENT_STEREO_URI)) {
-    const void *saved_noise_profile_2 = retrieve(
-        handle, self->state.property_noise_profile_2, &size, &type, &valflags);
-    if (!saved_noise_profile_2 || size != noise_profile_get_size() ||
+    const void *saved_noise_profile_1 = retrieve(
+        handle, self->state.property_noise_profile_1, &size, &type, &valflags);
+    if (!saved_noise_profile_1 || size != noise_profile_get_size() ||
         type != self->uris.atom_Vector) {
       return LV2_STATE_ERR_NO_PROPERTY;
     }

-    memcpy(self->noise_profile_2, (float *)LV2_ATOM_BODY(saved_noise_profile_2),
+    memcpy(self->noise_profile_2, (float *)LV2_ATOM_BODY(saved_noise_profile_1),
            sizeof(float) * self->profile_size);

     specbleach_load_noise_profile(self->lib_instance_2, self->noise_profile_2,
-                                  *fftsize, *averagedblocks);
+                                  permanent_fftsize, permanent_averagedblocks);
   }

   return LV2_STATE_SUCCESS;