Long missing LADSPA port upgrade routine

[zonkmachine]

Summary (edited):

The storage into a dedicated DOM element was introduced ten years ago with commit e99efd5 which was intended to close #401. The commit only changed the save and load routines in src/core/LadspaControl.cpp but did not introduce any upgrades in DataFile.

---

Original post

The demo project shorties/Greshz-CoolSnip in master is different from stable-1.2 .

There is a calf flanger on the first track which doesn't seem to have upgraded correctly from an earlier version. Calf has been been bumped from Calf 0.18 -> Calf 0.90 since stable-1.2 .

The sound is glitchy and mostly seem to mute. Not unlike what you'd get with NaN related issues. It shouldn't carry on to other tracks in the mix since we remove NaN and infinite values but unfortunately it seem to not work in this case. When you bypass the flanger on track one both tracks are playing like they should apart from the missing FX.

[tresf]

Calf has been been bumped from Calf 0.18 -> Calf 0.90 since stable-1.2 .

@zonkmachine just for clarification, when you say "since stable-1.2", I assume you're referring to builds from master branch only.

The Calf Flanger FX plugin is definitely the culprit, likely as a result of the Calf upgrade from Calf 0.18 (stable-1.2) to Calf 0.90 (master) in #3987.

Unfortunately, I can't seem to find out why this happens. At a glance, the ports that have been added from 0.18 to 0.90 seem to have been done so in a compatible fashion, leading me to believe that the data is just not making it over at all.

For example, when I add a new Calf Flanger using 0.90 to an empty project, ports 04, 05, and 06 have identical values to the "upgraded" Greshz-CoolSnip project file.

@JohannesLorenz any ideas?

Before	After

Before

<ladspacontrols ports="8">
   <port04 data="0.1"/>
   <port05 data="4.0837498"/>
   <port06 data="0.44977501"/>
   <port07 data="0.2376"/>
   <port08 data="0"/>
   <port09 data="0"/>
   <port010 data="1"/>
   <port011 data="1"/>
</ladspacontrols>

After

<ladspacontrols ports="12">
   <port04>
      <data id="3214821" scale_type="log" value="0.31622776"/>
   </port04>
   <port05>
      <data id="5635070" scale_type="log" value="3.1622777"/>
   </port05>
   <port06>
      <data id="7328967" scale_type="log" value="0.066874027"/>
   </port06>
   <port07 data="0.99000001"/>
   <port08 data="90"/>
   <port09 data="0"/>
   <port010 data="1"/>
   <port011 data="1"/>
   <port012 data="1"/>
   <port013 data="1"/>
   <port014 data="1"/>
   <port023 data="1"/>
</ladspacontrols>

[zonkmachine]

I assume you're referring to builds from master branch only.

No. I meant if I open a project made in latest stable compared to one in master.

There are two issues here. The wrong values and some bad data in the signal. The bad data is from the Feedback. Turn it down and the interference with the drums goes away. The settings on the wonky version seem to be the default settings for the flanger if I add one in a new project/track. If i manually adjust the master version to the old settings the tracks are the same.

[zonkmachine]

when I add a new Calf Flanger using 0.90 to an empty project, ports 04, 05, and 06 have identical values to the "upgraded" Greshz-CoolSnip project file.

They use name 'value' instead of 'data' in the project file.

Example:

<port06 data="0.44977501"/>

vs.

   <port06>
      < ... value="0.066874027"/>
   </port06>

[tresf]

They use name [data->]value instead of datai n the project file.

Right, but isn't that us doing that?

lmms/src/core/AutomatableModel.cpp

Line 113 in 1746300

me.setAttribute( "scale_type", m_scaleType == Logarithmic ? "log" : "linear" );

[zonkmachine]

I try and recreate issues by saving projects in 1.2.1/1.2.2 and opening them on master. They all work fine.
I open the 1.2.2 version in master. Works.
The only thing that stands out in the data is that it is the file in ../shorties that has been unchanged the longest, 5 years, and should be identical on stable-1.2 and master. https://github.com/LMMS/lmms/tree/master/data/projects/shorties

[zonkmachine]

happened to test this file over #6771, couldn't say if it got fixed but am getting somewhat of an output. Might be worth checking deeper.

Tested it and it's precisely the same.

[michaelgregorius]

Ok, I think I now have an idea what has happened.

Let's first take a look at the relevant section of the file "Greshz-CoolSnip.mmpz" when it's dumped using the version from the 1.2.2 branch/tag:

<ladspacontrols port04="0.099" port05="4.08375" port06="0.449775" port07="0.2376" port08="0" port09="0" ports="8" port010="1" port011="1"/>

And here's how the relevant section looks like when dumped using the version from the master branch which has changed the last time during pull request #6239:

<ladspacontrols ports="12">
  <port04>
    <data id="3839390" value="0.316228" scale_type="log"/>
  </port04>
  <port05>
    <data id="6753889" value="3.16228" scale_type="log"/>
  </port05>
  <port06>
    <data id="3028459" value="0.066874" scale_type="log"/>
  </port06>
  <port07 data="0.99"/>
  <port08 data="90"/>
  <port09 data="0"/>
  <port010 data="1"/>
  <port011 data="1"/>
  <port012 data="1"/>
  <port013 data="1"/>
  <port014 data="1"/>
  <port023 data="1"/>
</ladspacontrols>

As we can see the values are already wrong/different in the current file.

I have then added some code that dumps the content of the data file after each upgrade step. When loading the version of the file from the 1.2.2 tag I have noticed that the line with ladspacontrols never changes during the updates. It always stays at:

<ladspacontrols port07="0.2376" port06="0.449775" port09="0" port010="1" port04="0.099" port011="1" port08="0" ports="8" port05="4.08375"/>

So after the upgrade it still has the correct values but the wrong file format. This means that the actual values will not be found when loading the file with a current version. Therefore the plugin stays at the default values.

So I guess that @yssh466 has opened the file while working on #6239 and saved it as a new version. Because the update code was already broken at that time the new version of the file was stored with the default values of the Flanger and this is why the values are different.

What's interesting is that the upgrade seems to work when loading the file with LMMS 1.2.2 so the upgrade code must have been broken in the meantime.

[Rossmaxx]

From michael's explanation, it seems the file is broken. Is it fine to copy a 1.2 copy of the file and re save it manually?

[zonkmachine]

Is it fine to copy a 1.2 copy of the file and re save it manually?

Have you tried it on current master?

[Rossmaxx]

Have you tried it on current master?

Not yet.

[michaelgregorius]

From michael's explanation, it seems the file is broken. Is it fine to copy a 1.2 copy of the file and re save it manually?

Unfortunately, I think that this will not work. And even if it worked, saving the file again would not fix the broken update routine. I have just loaded old versions of the file in a current version. They can be found here and here. They all open in a broken state.

So if users have an older version of the file it will still open in a broken state. And it might also affect other files.

[bratpeki]

The bad data is from the Feedback

Confirmed. If it's set below 0.90 the demo works just fine.

[zonkmachine]

What sets this project apart from the others is that it was not part of a forced save on the 1.2 branch in #2813. The reason for the forced save was to not have the projects emit a warning for being an old version on a new release. It was left out because it was deemed unfit for further use and was marked for deletion. It was however forgotten and remains in the project. It should still upgrade to 1.3 shouldn't it? If you open it up in 1.2 and save it it will now open in 1.3 but shouldn't the upgrade work anyway?

[Rossmaxx]

Now that the project in question has been nuked, close?

[zonkmachine]

No. The upgrade routine needs to be tested some more. It seems like you currently need to save an earlier project in 1.2.2 and then it works in 1.3. It should upgrade from an earlier version without an intermediate save. Open a version from 1.1.3. It should work out of the box.

[zonkmachine]

Here are two older versions that doesn't work in 1.3 but works in 1.2 . If you save it under 1.2 you can now open it in 1.3
v1.1.3 https://github.com/LMMS/lmms/blob/v1.1.3/data/projects/Shorties/Greshz-CoolSnip.mmpz
v1.2.2 https://github.com/LMMS/lmms/blob/stable-1.2/data/projects/shorties/Greshz-CoolSnip.mmpz

So:
1 - For some reason the upgrade routine only works sometimes.
2 - The project Greshz-CoolSnip was not part of a forced save in preparation of 1.2 release.
3 - Saving Greshz-CoolSnip from an earlier release (1.2 included) on 1.2 makes it now work again.

@tresf Got any clue?

[michaelgregorius]

I have built 1.2.2 and have compared the XML of the ladspacontrols element at the following stages:

• At the end of the upgrade routine in the 1.2.2 code
• In the file that's saved by 1.2.2
• At the end of the upgrade method DataFile::upgrade_1_2_0_rc3 in master (this should logically correspond to the end of 1.2.2's upgrade routine)

I have found that there are structural differences.

End of upgrade in 1.2.2

At the end of the upgrade routine the ports are represented as attributes:

<ladspacontrols ports="8" port011="1" port04="0.099" port05="4.08375" port08="0" port010="1" port09="0" port07="0.2376" port06="0.449775" />

File stored by 1.2.2

In the file that's saved by 1.2.2 the ports are represented as elements:

<ladspacontrols ports="8">
  <port04 data="0.1"/>
  <port05 data="4.0837498"/>
  <port06 data="0.44977501"/>
  <port07 data="0.2376"/>
  <port08 data="0"/>
  <port09 data="0"/>
  <port010 data="1"/>
  <port011 data="1"/>
</ladspacontrols>

End of DataFile::upgrade_1_2_0_rc3

During the upgrade routine in master the ports are also represented as attributes:

<ladspacontrols port04="0.099" port05="4.08375" port010="1" port09="0" ports="8" port011="1" port06="0.449775" port08="0" port07="0.2376" />

Conclusion

The difference between the representation at the end of upgrade in 1.2.2 and in the saved file is suspicious. The update does not bring the file into the state that the application writes when saving the file.

As was reported the file only seems to load correctly in master if it was saved by 1.2.2, i.e. if the ports are represented as elements.

So it seems that it was forgotten to add an upgrade routine to represent the ports as elements in 1.2.2. The upgrade code that was added later seems to make the assumption thought that the ports are represented as elements. Hence it only works if the file was saved with 1.2.2.

[zonkmachine]

I'm low key glancing at the PluginFactory stuff then #1719. It's a bit further back still and I'm currently failing to build stable-1.2 to bisect it. May try an older machine tomorrow.

[michaelgregorius]

The storage into a dedicated DOM element was introduced ten years ago with commit e99efd5 which was intended to close #401. The commit only changed the save and load routines in src/core/LadspaControl.cpp but did not introduce any upgrades in DataFile.

It's a bit of a question of how to fix this. I assume that people who have added upgrade routines after this commit might have assumed that the ports are stored in elements, especially if all they have seen or looked at might be files saved with the version of that commit or later. However, as already noted when an old file is loaded and it goes through the upgrade path it will never have its ports converted to elements and therefore all code that assumes this will fail.

A fix for this would need to add an upgrade that converts the port attributes to port elements. This upgrade would have to run before the first upgrade routine in the upgrade chain that assumes ports as elements.

This is how DataFile looked like at the point of the aforementioned commit: https://github.com/LMMS/lmms/blob/e99efd541a9dbd7b5656c769887c9d9ad94e4078/src/core/DataFile.cpp. The last version for which is tested is "0.4.0-rc2". Looking at the current version of DataFile the upgrade that follows this is DataFile::upgrade_1_0_99 . So to compensate for the missing update the new update would ideally have to be put in between these two updates (or at the start of the second one?). I currently have no idea how simple this would be without potentially messing with other updates.

[zonkmachine]

The storage into a dedicated DOM element was introduced ten years ago with commit e99efd5

Right and that was 10 years ago (v0.9.92). I thought that Greshz-CoolSnip had been touched and saved after this (8 years ago) but it was only the directory shorties that had changed name from Shorties.

[Spekular]

A fix for this would need to add an upgrade that converts the port attributes to port elements. This upgrade would have to run before the first upgrade routine in the upgrade chain that assumes ports as elements.

Unless things have changed drastically since I last worked on it, the upgrade system is built around the assumption that new routines are always added to the end (the version number of the project file is used as an index in a sequence of routines).

[michaelgregorius]

The CMakeLists.txt of the source code at commit e99efd5 shows that files saved around that time have written "0.9.91" as their "creatorversion". See here: https://github.com/LMMS/lmms/blob/e99efd541a9dbd7b5656c769887c9d9ad94e4078/CMakeLists.txt.

Commit e99efd5 did not touch the CMakeLists.txt. This means that some files that have "0.9.91" as their version might have been saved with the ports as attributes (before e99efd5) and others might have been saved with the ports as elements (starting with and after e99efd5).

So it seems like a new upgrade routine for version "0.9.91" needs to be added between "0.4.0-rc2" and "1.0.99-0" which checks if the ports are saved as elements or attributes:

• Elements: Do nothing.
• Attributes: Convert to elements.

[michaelgregorius]

If someone wants to fix this then please go ahead. You will mainly need to look at the differences in the LADSPA save code between commit 2981a59 and e99efd5 and then write an upgrade routine for the differences. That routine would need to be run between DataFile::upgrade_0_4_0_rc2 and DataFile::upgrade_1_0_99.

I gave it a try but then noticed that it is rather hard because the structure of the save files can be much more complex than the one shown in #5738 (comment), e.g. when there are automations. Automations have been stored as children of the ladspacontrols element in commit 2981a59. After the changes of e99efd5 they are stored as children of the port elements. This makes it necessary to somehow map them.

Another problem is that the aforementioned commits cannot be built anymore because they need Qt4 and no modern distribution provides the packages anymore. Having an old version would be nice to be able to create good test files though.

I am not even sure if it is worth it. Perhaps it is best to simply document that the files of projects with LADSPA controls that have been stored with versions before e99efd5 cannot be loaded anymore.

An alternative might be to write an "upgrade" routine which is placed between DataFile::upgrade_0_4_0_rc2 and DataFile::upgrade_1_0_99 and which simply checks if the file is a problematic one. The user could then be warned, e.g. via dialog or output to std::out.

[zonkmachine]

I am not even sure if it is worth it. Perhaps it is best to simply document that the files of projects with LADSPA controls that have been stored with versions before e99efd5 cannot be loaded anymore.

Not worth it.

1. It's really complex and stand a low chance of a 100% reliable fix.
2. There is a chance of an upgrade routine actually introducing new issues.
3. It's so old that any buggy projects (and there could be other issues introduced in this commit that we haven't found yet, couldn't it?) arising from this commit would have been fixed manually by their respective creators a long time ago.

[michaelgregorius]

Pull request #7267 introduces a warning message.

[michaelgregorius]

It piqued my curiosity that the file from e99efd5 can be loaded and saved with version 1.2 and that it can then be loaded successfully in the current master. Whereas it is not possible to directly load the file with the current master. Therefore I decided to do a git bisect to find out which commit is the critical one that broke that functionality.

The bisect identified commit ae0dd21 as the one that broke the functionality. The commit upgrades the Calf LADSPA plugins to version 0.90 and also introduces some changes to DataFile.cpp (see here).

The changes make adjustments for different Calf plugins but there are no adjustments for the Flanger. So it might be that it simply was forgotten and that this creates the problems reported in this issue.

@tresf, can you give some insights?

[tresf]

It piqued my curiosity that the file from e99efd5 can be loaded and saved with version 1.2 and that it can then be loaded successfully in the current master. Whereas it is not possible to directly load the file with the current master. Therefore I decided to do a git bisect to find out which commit is the critical one that broke that functionality.

The bisect identified commit ae0dd21 as the one that broke the functionality. The commit upgrades the Calf LADSPA plugins to version 0.90 and also introduces some changes to DataFile.cpp (see here).

The changes make adjustments for different Calf plugins but there are no adjustments for the Flanger. So it might be that it simply was forgotten and that this creates the problems reported in this issue.

@tresf, can you give some insights?

Not trying to pass the buck here, but I think my changes were very straightforward mappings whereas @JohannesLorenz 's were more dynamic looping/overhaul. His are towards the bottom of the commit history: https://github.com/LMMS/lmms/pull/3987/commits, notably 62e1d8f, 2574408.

[JohannesLorenz]

Sorry, I did not see this PR.

It is indeed possible that I forgot the Flanger in #3987 . But then, it would be just one effect to fix?

However, your warning message PR seems to be active for all "old" LADSPA effects (not even limited to CALF)? It would bring a warning message for every LMMS project which ever had LADSPA effects, insecuring users, even though only Flanger was affected?

[michaelgregorius]

The warning message would be active for all LADSPA effects that do not get upgraded correctly. So ideally it should not make the users insecure but rather warn them that their songs might sound broken or even noisy.

If the Flanger is the only plugin that was missed then it would only be one plugin that needs to be upgraded indeed. If you have the time it would be great if you could implement it because you have more experience in that area as you have implemented the other ones as well.

[JohannesLorenz]

I now checked all my files, and it seems almost all old files bring this warning:

grep 'ladspacontrols.*port0' -r . | cut -d: -f1 | sort -u | wc -l

However, most do sound good when I load them (there were several bugs in the past that did some corruption, so I am never surprised about any issues).

Example

<ladspacontrols port10="4.41" port00="4.41" port11="0.756525" port01="0.756525" link="1" port00link="1" ports="4" port01link="1"/>

Converted to

              <ladspacontrols link="1" ports="4">
                <port00 data="4.7999997" link="1"/>
                <port01 data="0.75652498" link="1"/>
                <port10 data="4.7999997"/>
                <port11 data="0.75652498"/>
              </ladspacontrols>

Here, port10 looks slightly wrong, but this is just the frequency, so that is acceptable. It might be because of the logscale updates.

Does this look like anything you expect @michaelgregorius ?

[michaelgregorius]

@JohannesLorenz, with the changes from #7267 the warnings are shown after some checks that are done as the very last "upgrade" step. If your old files show the warning then they likely still contain elements like the first one after going through the full upgrade chain, i.e.:

<ladspacontrols port10="4.41" port00="4.41" port11="0.756525" port01="0.756525" link="1" port00link="1" ports="4" port01link="1"/>

Can you check if this is really the case, e.g. by dumping the XML after the very last step? Please note that this does not mean to save the file after it has been loaded because the file will then likely be saved with the "new" structure with potentially different (default) values.

Alternatively the check routine might catch something that it should not interpret as a problem.

[michaelgregorius]

@JohannesLorenz, I also got your mail from some time ago. I think I will not be able to answer you via mail because the sender address is something like umleitung@web.de which I assume is not yours? I won't print it in fully here in case it is your real mail address though. 😅

If you want we can meet via https://meet.jit.si/. It's a simple way to setup meetings. We could either have a phone call or use the text chat there. Judging by your name and mail provider I guess that you might also speak German?

[michaelgregorius]

@JohannesLorenz, I have also just checked the file that you have sent me via mail. I have dumped it right before the check is performed and after all other upgrade routines have executed. At this stage there are still several ladspacontrols elements in the old format contained in the document, e.g.

<ladspacontrols ports="4" port10="17062.9" port11="10" port00link="1" port00="17062.9" port01="10" link="1" port01link="1"/>

So it is no false positive and the warning is okay.

If I open the affected plugins they open with their default values. I have tested this by opening the original plugin and then adding a new instance of the plugin and visually comparing their sliders. So if you have not kept all plugins at their default values in the original project then it should sound different than before the bug was introduced.

I have not checked fully but it seems that every type of LADSPA plugin in the file is affected.

[JohannesLorenz]

So if you have not kept all plugins at their default values in the original project then it should sound different than before the bug was introduced.

This might indeed be the reason. Back then I usually only used the default value - unless I modulated some parameters, but those should exactly be not affected (after the first modulation pattern starts). That explains that I do not hear the issue in my ~200 old songs (though, tbh, there are issues, but I think mostly are due to other compatibility break like the logscales patch or a linkable models issue).

The fact that this should be audible on master for many old projects and still (almost) no one complained tells me a complete fix might indeed not be worth it and the warnings message PR is sane.

And yes, you got my mail address right, you can reply there, but right now, I am happy with the proposed solution.

[JohannesLorenz]

Oh now I read it again and see that even automations are affected. And indeed, many automations in my projects were broken. It is really ugly, because there are mulitple bugs involved here, which might indeed suggest that the users should fix this themselves.

On the other hand, I wonder why not just once export all attributes into tags? Yes, this is not 100% accurate, because there were ports changed in between (e.g. #3987, making this update here "too late"), but it should be easy to implement (automations and linkabled models can be ignored IMO) and is still a lot better than loading the default values?

[michaelgregorius]

@JohannesLorenz, to be frank right now I do not want to do a deep dive into the different formats, the needed mappings, etc. I propose that we leave this issue open in case someone wants to fix it at one point. I have added you as a reviewer for pull request #7267 which introduces the warning dialog. This dialog might also be a good indicator for how often the problem really shows in practice because it might lead to feedback from users.

It seems that the buggy project has also been removed from the code base.

[JohannesLorenz]

Agreeing. The bug is still open, so we keep this issue open, too. The warning dialogue makes sense, too.

In the end, I might be the one writing a patch here, because I don't want to fix 200 old projects manually 😆

It seems that the buggy project has also been removed from the code base.

I propose to rename the title to "Old projects with LADSPA plugins are being loaded wrong". If anyone disagrees, please scream, or I will do this in the next days.

[michaelgregorius]

In the end, I might be the one writing a patch here, because I don't want to fix 200 old projects manually 😆

If you want to give it a try then the following comments in this issue might be helpful:

• Long missing LADSPA port upgrade routine #5738 (comment)
• Long missing LADSPA port upgrade routine #5738 (comment)
• Long missing LADSPA port upgrade routine #5738 (comment)

They describe two problems which might be responsible for bugs:

• The changes that have been made in the LADSPA save code between commit 2981a59 and e99efd5. The change was added without writing an update routine. Due to that some files with the version "0.9.91" load successfully and some do not.
• The upgrade of the Calf LADSPA plugins to version 0.90 with commit ae0dd21.

[michaelgregorius]

I have merged pull request #7267 which adds the warning dialog.

[michaelgregorius]

@JohannesLorenz, @tresf, is this still a milestone 1.3 bug now that there is at least the warning dialog? Or can we move it to the next milestone or backlog?

@JohannesLorenz, do you intend to attempt a full fix for milestone 1.3?

[tresf]

@tresf, is this still a milestone 1.3 bug now

I had to read back through the history of this bug to remember how it was all caused. From 1,000 ft, it seems like we missed something obvious if the upgrade routine worked and then it broke, but I'll try not to waste time speculating.

Nevermind, it's explained clearly here:

The storage into a dedicated DOM element was introduced ten years ago with commit e99efd5 which was intended to close #401. The commit only changed the save and load routines in src/core/LadspaControl.cpp but did not introduce any upgrades in DataFile.

@michaelgregorius I agree, the bug is esoteric enough to miss the 1.3 milestone now, removed from the 1.3 milestone. However I believe that the title no longer reflects the actual bug, so I'm also changing the title.