Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Envelope points are multiplied when using Filter Curve EQ or Graphic EQ #1476

Closed
crsib opened this issue Aug 13, 2021 · 11 comments · Fixed by #1502
Closed

Envelope points are multiplied when using Filter Curve EQ or Graphic EQ #1476

crsib opened this issue Aug 13, 2021 · 11 comments · Fixed by #1502
Labels
P1 Highest level priority (ship blocker / must fix)
Milestone

Comments

@crsib
Copy link
Contributor

crsib commented Aug 13, 2021

Describe the bug
Using Filter Curve EQ will duplicate envelope points if they were present.

To Reproduce
Steps to reproduce the behavior:

  1. Get some audio
  2. Use envelope tool, generate some points
  3. Apply Filter Curve EQ to the selection, that is smaller than the clip
  4. Observe

Expected behavior
Filter Curve EQ should not change the envelope points.

Screenshots
image

See https://forum.audacityteam.org/viewtopic.php?p=432231#p432231

@crsib crsib added P1 Highest level priority (ship blocker / must fix) Bug severity: High labels Aug 13, 2021
@crsib crsib added this to the Audacity 3.1 milestone Aug 13, 2021
@crsib crsib added this to To do (Dev) in Release 3.1 via automation Aug 13, 2021
@petersampsonaudacity
Copy link

I do not see this on w10 with 3.0.3 or 3/1/0/3.0.4 alpha audacity-win-3.0.4-alpha-20210813+4058470-64bit

@crsib
Copy link
Contributor Author

crsib commented Aug 13, 2021

I've clarified STR a bit after checking - it works only if you select the part of the clip

Audacity 3.0.3 Win64

First time:
https://crsib-screenshots.s3.amazonaws.com/202108131713-y0w32.png

Second application, different selection
https://crsib-screenshots.s3.amazonaws.com/202108131714-uzh79.png

@crsib
Copy link
Contributor Author

crsib commented Aug 13, 2021

The potentially problematic code dates back to Aug 2020: 58adb94

@petersampsonaudacity
Copy link

OK with revised STR

Before EQ
image

After EQ
image

The disjointed envelope does not look right to me

@crsib
Copy link
Contributor Author

crsib commented Aug 13, 2021

Oh, you should check the result of third application then LOL

image

@petersampsonaudacity
Copy link

I get similar results when I use Graphic EQ as well

@crsib
Copy link
Contributor Author

crsib commented Aug 13, 2021

I don't really know how many effects are affected

@petersampsonaudacity
Copy link

I can get it to crash by applying the Filter curve EQ several times (using Ctrl+R) - it first goes to a flat line and then the next application causes the crash.

ET has just phoned-home with the error report.

@petersampsonaudacity
Copy link

petersampsonaudacity commented Aug 13, 2021

I don't really know how many effects are affected

Change Pitch, Change Tempo and Change speed also seem to be affected

And Paulstretch

And Notch Filter and Tremolo

But many other effects are not affected e.g. Amplify, fade In, Wahwah and others

@crsib
Copy link
Contributor Author

crsib commented Aug 13, 2021

Oh I've managed to crash it too: #1477

And ET's home says that it is a number 2 crash!

@Paul-Licameli Paul-Licameli changed the title Envelope points are multiplied when using Filter Curve EQ Envelope points are multiplied when using Filter Curve EQ or Graphic EQ Aug 13, 2021
@crsib
Copy link
Contributor Author

crsib commented Aug 13, 2021

Bug narrowed down to Track Clear-And-Paste usage

This was referenced Aug 18, 2021
@AnitaBats AnitaBats removed this from To do (Dev) in Release 3.1 Aug 23, 2021
@AnitaBats AnitaBats added this to To do in Sprint 4 - Release 3.1 via automation Aug 23, 2021
@AnitaBats AnitaBats moved this from To do to Review in progress in Sprint 4 - Release 3.1 Aug 23, 2021
@AnitaBats AnitaBats moved this from Review in progress to Ready for QA in Sprint 4 - Release 3.1 Aug 23, 2021
@Penikov Penikov moved this from Ready for QA to In QA in Sprint 4 - Release 3.1 Aug 26, 2021
@Penikov Penikov moved this from In QA to Done in Sprint 4 - Release 3.1 Aug 26, 2021
@Penikov Penikov closed this as completed Aug 26, 2021
Sprint 4 - Release 3.1 automation moved this from Done to Ready for QA Aug 26, 2021
@Penikov Penikov moved this from Ready for QA to Done in Sprint 4 - Release 3.1 Aug 26, 2021
crsib added a commit that referenced this issue Nov 18, 2021
This PR resolves performance issues when reading large project files. For the first time, those issues arose due to the #1476.

However, after the #1476 was fixed, the problem remained. It hit the 3.1.0 release sharply because there are easy ways to generate lots of `<waveblock>` tags without duplicating the actual data. For example - splitting a clip causes two full copies of block sequences.

Before this PR reading, the project was performed in the following steps:

1. `dict` and `doc` fields were read as a single blob into the memory.
2. Memory was copied to the `wxMemoryBuffer` object.
3. `wxMemoryBuffer` object was passed into the binary XML decoder, which generated a text representation of the XML file.
4. XML file was parsed using Expat, and the project structure was created using Expat handlers.

This approach had several significant problems:

1. A memory spike of at least `2 * size(doc || dict)`. The largest project that was used for testing had a blob of over 400 megabytes.
2. At least `3 * size(doc || dict)` of bytes copied.
3. Numeric values are first converted to string representation then parsed back. The original implementation was slow in both cases; the numeric to string conversion was improved in 3.1.1 and 3.1.2, but that was not enough.
4. Parsing is performed twice: once for the binary form, then for the text one.

These issues are solved by the following changes:

1. A new class is introduced that allows storing the real attribute value. It works both with text and binary forms.
2. Data is read incrementally directly from the SQLite in chunks of 32Kb.
3. Binary parser now accepts XMLTagHandler directly, allowing effective reconstruction of the project directly from the binary XML.

With all the changes, ~40x improvement was measured in terms of time, memory spike is now negligible.

For example, for the 400Mb project mentioned before the results are:

* 95 seconds on 3.1.0 (and a whooping peak around 4 Gb of RAM, while only 1.4 is needed!),
* 45 seconds on 3.1.1,
* ~2.5 seconds with this build.

Results were measured on Ryzen 5800x with a fast PCIe SSD. On slower machine it took over 3 minutes to open that project.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Highest level priority (ship blocker / must fix)
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants