Demo project Greshz-CoolSnip buggy in master

[zonkmachine]

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? https://github.com/LMMS/lmms/blob/174630087ef24899dfb52ef2e892c68a7fd7caf2/src/core/AutomatableModel.cpp#L113

[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

[Rossmaxx]

Looks like something fixed by #6424.

[zonkmachine]

Looks like something fixed by #6424.

Have you tested it?

[Rossmaxx]

Nope i haven't because my pc is currently broken. Will test as soon as i get it repaired.

[Rossmaxx]

Have you tested it?

sorry for the late reply. Tested it now. Unfortunately, the bug seems to have not been fixed by the pr i linked.

[Rossmaxx]

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.

[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 https://github.com/LMMS/lmms/pull/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 https://github.com/LMMS/lmms/pull/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 e99efd541a9 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 e99efd541a9 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 e99efd541a9 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 e99efd541a9) and others might have been saved with the ports as elements (starting with and after e99efd541a9).

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 2981a5994 and e99efd541a9 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 https://github.com/LMMS/lmms/issues/5738#issuecomment-2108680392, e.g. when there are automations. Automations have been stored as children of the ladspacontrols element in commit 2981a5994. After the changes of e99efd541a9 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 e99efd541a9 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 e99efd54 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 ae0dd21d 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.