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

Improve performance and quality of Sample Track display, and add RMS graph. #5927

Merged
merged 29 commits into from
Apr 16, 2021

Conversation

LostRobotMusic
Copy link
Contributor

@LostRobotMusic LostRobotMusic commented Mar 2, 2021

Before:
Screenshot from 2021-03-01 17-24-24

After:
Screenshot from 2021-04-13 22-40-21

The top row shows the default coloring.

Alright, here's what changed.
Previously, the sample track display code simply skipped a certain number of frames and snagged a random one in the middle of the sample in order to get the next point in the line. This, of course, cannot possibly look any good without a super high point resolution. LMMS did have a super high point resolution due to that issue... and because of that, drawing the sample tracks took a ludicrously long amount of time, resulting in absolutely massive amounts of lag every time the user zooms in/out or resizes a sample track vertically. And, at the same time, quality would still be lower than what would be preferred when the user zooms in quite far. The waveform would also oftentimes fail to show very short peaks in the audio.

These new formulas I put together take every frame of audio into account. Each point on the line displays the maximum or minimum value of all the frames within that area. This'll accurately display all peaks, and allows us to have a significantly smaller point resolution while still having a significantly higher quality in general. These new changes probably run around 5x-10x as fast or so, so users should run into much less lag than before when using sample tracks.

On top of all this, I also added an Audacity-style RMS graph in the middle for bonus points, which should make LMMS look quite a bit fancier, and also give the user a decent general idea of the audio's loudness in that area relative to the peaks.

This PR is a WORK IN PROGRESS, do not merge. There are still a few bugs (e.g. selected and muted TCOs aren't shown with the right colors when a custom color isn't chosen), and the default colors might change a bit as well. It's ready now.

@LmmsBot
Copy link

LmmsBot commented Mar 2, 2021

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Windows

macOS

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://13456-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.120%2Bg3c6902b-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13456?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://13460-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.120%2Bg3c6902b8b-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13460?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://13459-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.120%2Bg3c6902b8b-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13459?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/y583mfhs4mkc9g7o/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38709062"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/ufnptrryh66350jm/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38709062"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://13457-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.120%2Bg3c6902b8b-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13457?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "1f5d535b2256763fc96326a984ce77b8665f7a5b"}

@Veratil Veratil changed the title Improve performance and quality of Sample Track display, and add RMS graph. [WIP] Improve performance and quality of Sample Track display, and add RMS graph. Mar 2, 2021
@Veratil
Copy link
Contributor

Veratil commented Mar 2, 2021

Out of curiosity... what's with the 11 commits about merging some of the first PRs?

@LostRobotMusic
Copy link
Contributor Author

@Veratil Me not knowing how to actually use Github properly, and making PRs from the LMMS master branch to the master branch on my fork in order to update it.
It shouldn't be a problem since we choose Squash and Merge anyway.

@thmueller64
Copy link
Contributor

Closes #5908 . Works very well and improves the performance a lot. Also takes into account panning, as opposed to before where the left and right channels simply have been plotted on top of each other. What do you think about drawing the peak/rms envelope, so the hull of the peak/rms values, instead of drawing lines connecting successive min-max-min-max-min values?

src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
@LostRobotMusic
Copy link
Contributor Author

Oh hey, I didn't realize 5908 exists. Cool.
Also, I completely forgot that the changes I'm making also impact AudioFileProcessor. I'm happy things worked out well for it as well, hahaha.

What do you think about drawing the peak/rms envelope, so the hull of the peak/rms values, instead of drawing lines connecting successive min-max-min-max-min values?

I feel like that isn't needed. In the future, LMMS will support significantly larger amounts of zoom, allowing us to see the waveform with very high detail... in which case filling between two lines would be quite odd (either wouldn't do anything at all or display incorrectly altogether), and will result in a rather significant increase in resources that would be required. We'd likely need to have different drawing logic for different amounts of zoom. We'll also need to fill in the area between the two lines drawn, which will require that we either draw a new line at every pixel in order to fill the space between (which would use far more resources than the current method), or maybe some Qt magic would be possible to fill in the area in between (?). In the end, as far as I know, the result would likely be visually identical, with not much of a benefit elsewhere.

Of course, if there are good reasons to switch to that method that I'm not taking into account, definitely let me know.

@thmueller64
Copy link
Contributor

We'd likely need to have different drawing logic for different amounts of zoom

That's how audacity does it. It switches from peak/rms to single samples connected directly when zoomed close enough.

We'll also need to fill in the area between the two lines drawn, which will require that we either draw a new line at every pixel in order to fill the space between (which would use far more resources than the current method), or maybe some Qt magic would be possible to fill in the area in between (?)

Yes, a closed path (which can be filled) consisting of all the max values from left to right plus all the min values in the opposite direction. But overall I think the drawing strategy of the PR is efficient enough and looks really good visually.

@LostRobotMusic LostRobotMusic changed the title [WIP] Improve performance and quality of Sample Track display, and add RMS graph. Improve performance and quality of Sample Track display, and add RMS graph. Mar 3, 2021
@LostRobotMusic
Copy link
Contributor Author

Alright, everything is finished now. This is no longer a WIP, it'll be ready for merge as soon as you all deem it as such.

src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
src/core/SampleBuffer.cpp Show resolved Hide resolved
src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
src/tracks/SampleTrack.cpp Outdated Show resolved Hide resolved
src/tracks/SampleTrack.cpp Outdated Show resolved Hide resolved
src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@IanCaio IanCaio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good, except for some small changes and minor code style issues.

src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
src/tracks/SampleTrack.cpp Outdated Show resolved Hide resolved
src/tracks/SampleTrack.cpp Outdated Show resolved Hide resolved
src/tracks/SampleTrack.cpp Outdated Show resolved Hide resolved
src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
@DomClark DomClark added the after-refactor PRs that will need to be rebased after the planned code refactor (#5592) label Mar 12, 2021
LostRobotMusic and others added 3 commits March 12, 2021 14:53
	- Fixes a small bug where the buffer was being drawn past the
end of the widget. That's because when dividing the number of frames by
the width, when it wasn't perfectly divisible the remaining frames where
still being considered for a pixel after the width. That required the
mysterious "+ 2" after the totalPoints to be used. Now those points are
ignored.
	- Adds doxygen comment before method.
	- Adds TODO comment regarding a commented statement using a
parameter that apparently is not used anymore.
	- Renames curFrame to curPixel.
	- Uses curPixel itself for the X calculation.
	- Fixes a bug where sometimes a higher amount of points was
allocated (because nbFrames/fpp would sometimes be higher than the
width) and those not-initialized extra points would result in a small
glitch in the sample drawing.
	- Fixes a bug where the the last frame wasn't being accounted on
the peak/trough calculation because "frame + i < last" was assuming last
was non inclusive, when it is inclusive ("frame + i <= last").
	- Reverses the change to the x calculation, so it's calculated
straight from curPixel if there are "width" number of points or
proportionally when it's not.
Copy link
Contributor

@IanCaio IanCaio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed some changes that fixes some raised issues. The code LGTM now.

There's one small behavior I didn't figure out the reason why (present on both master, Douglas version and mine) where zooming in a lot will make the wave being drawn slightly before the end, even with the proportion calculation. I'm not sure if this has to do with float approximation. This PR doesn't introduce it so I'm approving anyways.

src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
src/core/SampleBuffer.cpp Outdated Show resolved Hide resolved
@Veratil
Copy link
Contributor

Veratil commented Apr 14, 2021

Merging tomorrow unless anyone objects.

@LostRobotMusic
Copy link
Contributor Author

My original method of drawing the sample tracks left a noticeable line thickness difference between the top and bottom half of the waveforms, the latest two commits fix this.

@DomClark
Copy link
Member

The fpp variable is an integer, so isn't the exact number of frames per pixel. Previously this wasn't an issue, since the x coordinate of each point was calculated from the frame offset, but now the x coordinate is advanced by exactly one pixel each iteration, this introduces an error of up to one frame per pixel. I would suggest using a float or double for fpp and frame.

@Veratil
Copy link
Contributor

Veratil commented Apr 15, 2021

Giving one more day before merge after Dom's review just in case anything else pops up.

@Veratil Veratil merged commit ab41037 into LMMS:master Apr 16, 2021
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
…graph. (LMMS#5927)

* Buff Sample Track display performance and visuals, and add RMS display

Co-authored-by: IanCaio <iancaio_dev@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
after-refactor PRs that will need to be rebased after the planned code refactor (#5592)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants