LMMS / lmms

Cross-platform music production software
https://lmms.io
GNU General Public License v2.0
8.18k stars 1.01k forks source link

[Feature development] A native oscilloscope plugin #7460

Open bratpeki opened 3 months ago

bratpeki commented 3 months ago

Hello!

I am currently developing an oscilloscope for LMMS.

Initially, it is meant to be similar to the oscilloscope available at the top of the main window toolbar, except that it will be applicable to any effect stack.

Then, I would like to be able to change the time window window of the oscilloscope, so that it can be used a time-variable waveform display, which is useful for mixing.

Then, I would implement pitch detection capabilities, so the oscilloscope can keep periodic signals steady.

This issue has been made as per the statement made in the LMMS README: "Before coding a new big feature, please always file an issue..."

Please do keep in mind that, despite having contributed and written LMMS code before, this is my first time making a plugin. I would still like to develop it myself, with suggestions and help from interested and experienced developers, since the experience would make contributing in the future easier.

Thoughts?

bratpeki commented 3 months ago

screencap-2024-08-18-14-05-16

Currently, the scope is grabbing the same data from Engine as the main window scope. I'll work toward it grabbing the frames of the current track, not the entire song.

michaelgregorius commented 3 months ago

@bratpeki, work on an oscilloscope was started by @IanCaio some time ago. His branch is rather old but some months ago I merged it with `master´, fixed the build and pushed it into my repository so that does not get lost. You can find it here: https://github.com/michaelgregorius/lmms/tree/plugin/WaveAnalyzer

The current implementation has some performance problems. Perhaps you'd like to take a look at these. See also the discussion that started here: https://github.com/LMMS/lmms/pull/7155#issuecomment-2016641041.

If you are interested in this we could also push the branch to LMMS's main repository so that it can be worked on "officially" and collaboratively.

@IanCaio's original branch can be found here: https://github.com/IanCaio/lmms/tree/plugin/WaveAnalyzer

michaelgregorius commented 3 months ago

I have just merged my current branch with master again and fixed the build once more (sampleFrame vs. SampeFrame).

The current implementation looks like this:

WaveAnalyzer

It looks like I should have rather written that it has "serious performance problems". :sweat_smile:

A general thing that might be worth working on is the efficient rendering of audio buffers as this might also be causing the slow updates in SlicerT which are described here: https://github.com/LMMS/lmms/pull/7453#issuecomment-2295180687

michaelgregorius commented 3 months ago

Also linking https://github.com/LMMS/lmms/issues/1358 here which is an old request for an oscilloscope and vector scope. The vector scope was implemented in https://github.com/LMMS/lmms/pull/5328 so this issue would deal with the oscilloscope part of the request. If it leads to a pull request then this PR could close https://github.com/LMMS/lmms/issues/1358.

bratpeki commented 3 months ago

Thank you so much for writing! I'll be checking this out, then! Since you mentioned so many different Git branches, could you link me to the one I should get and build in order to begin testing and code review?

michaelgregorius commented 3 months ago

You're welcome, @bratpeki!

The branch to check out for now is https://github.com/michaelgregorius/lmms/tree/plugin/WaveAnalyzer because it is based on the current master.

michaelgregorius commented 3 months ago

@bratpeki, I forgot to push the changes with regards to sampleFrame vs. SampleFrame. I have now pushed them so that the branch should build without problems.

bratpeki commented 3 months ago

I am immediately noticing this break in the waveform. Happens on all three modes. Will investigate further.

https://github.com/user-attachments/assets/6f64eeae-8ab9-4fc9-b950-d09902865639

bratpeki commented 3 months ago

I'll be taking the plugin on. I have added it to my own fork and will be looking at the code and working on it! Also, official and collaborative work on this sounds great, since it'll ensure the quality of it! Should I close this issue and open a new issue/PR where we can communicate about the plugin?

bratpeki commented 3 months ago

Also, to note, since this thought came to me, I might not end up using the original codebase, since it's usually easier to implement something yourself than to work on someone else's code, and a waveform display is just a ring buffer and a linedraw call, but I will be taking inspiration from the plugin's UI design. This is, again, just a maybe!

michaelgregorius commented 3 months ago

[...] Should I close this issue and open a new issue/PR where we can communicate about the plugin?

You can leave this issue open and open a additional PR which you can configure such that it automatically closes this issue when it gets merged. The PR can then be used to discuss WIP code if necessary.

bratpeki commented 3 months ago

Alright, I'll do so!

bratpeki commented 3 months ago

I've decided I'll open a PR as soon as the issue stated above is resolved. I have found that the break happens every 256 samples, and am investigating the drawing and buffer fill code. Lost has brought it to my attention that a ring buffer class is now in LMMS, so that might be of use if the buffer handling code would be more readable.

Afterwards, I probably won't have more work other than making the window resizable and maybe adding pitch detection so that we can use the plugin as a proper oscilloscope. And testing, but I believe moving to a proper PR will yield a few testers, which is going to be great!

bratpeki commented 3 months ago

After adding this change to WaveAnalyzer.cpp:

@@ -98,6 +108,14 @@ bool WaveAnalyzerEffect::processAudioBuffer(SampleFrame *buffer, const fpp_t fra
                                m_controls.m_clippedRight.setAutomatedValue(true);
                        }
                }
+
+               printf("buffer\n==============================\n");
+               for ( int i = 0; i <= lastBufferIndex; i++ )
+               {
+                       printf("%f, ", m_controls.m_ampBufferL[i]);
+               }
+               printf("\n==============================\n");
+
                if (frameCount > 0)
                {
                        avgLeft = sqrt(avgLeft / frameCount);

I've managed to collect sample buffers which I could plot using Octave:

s1 s2 s3

If you recall, the signal above was a pure sine. That's exactly what we're getting from the amplitude frame array. Meaning that this is definitely a drawing issue. I'll investigate the appropriate code ASAP.

Rossmaxx commented 3 months ago

Lost has brought it to my attention that a ring buffer class is now in LMMS, so that might be of use if the buffer handling code would be more readable.

Don't use that, I am thinking of removing it, so if you continue with that class, you might end up reworking on it.

Ideally, RingBuffer should just be a vector.

bratpeki commented 3 months ago

If I were to use it, it would be for populating the amplitude buffer.

Given the fact that the amplitude buffers are properly functioning with the current static array implementation (see above), there's no need for the ring buffer. 👍

michaelgregorius commented 3 months ago

Lost has brought it to my attention that a ring buffer class is now in LMMS, so that might be of use if the buffer handling code would be more readable.

Don't use that, I am thinking of removing it, so if you continue with that class, you might end up reworking on it.

Ideally, RingBuffer should just be a vector.

@Rossmaxx , aren't there several implementations of ring buffers in the code base and it was only one that was intended to be removed? I guess they won't get all removed so there should be one that can be used by @bratpeki.

Rossmaxx commented 3 months ago

aren't there several implementations of ring buffers in the code base and it was only one that was intended to be removed?

I meant the one used by multitap echo. The other ring buffer is not usable for this purpose either as that one has some weird functions which I don't understand. But if you want, you can use the other one, named LocklessRingBuffer

bratpeki commented 3 months ago

I'll keep the LocklessRingBuffer in mind then, although, as I stated, I think it will not be necessary. A ring buffer would be used for storing sample frames. However, sample frames are already being stored properly (see the plots above), so this is obviously a graphical issue.

bratpeki commented 3 months ago

I'll use this message as a board of information I collect about the problem:

Saker has informed me that "all of LMMS's audio buffers use a size of 256 sample frames. Larger buffer sizes are handled using a FIFO mechanism that the audio device reads from".

bratpeki commented 3 months ago

Got it!

It's in void WaveAnalyzerWaveform::updatePoints(int count). I made the following addition to the end:

@@ -336,6 +336,18 @@ void WaveAnalyzerWaveform::updatePoints(int count)
                        return;
        }

+       printf("====== 1 \n");
+       for (int i = 0; i < totalPixels; i++ ) {
+               printf("%d, ", m_pointsL[i].x());
+       }
+       printf("\n");
+       printf("====== 2 \n");
+       for (int i = 0; i < totalPixels; i++ ) {
+               printf("%d, ", m_pointsL[i].y());
+       }
+       printf("\n");
+       printf("\n");
+
        repaint();
 }

And plotted the lines in Octave again!

screencap-2024-08-27-19-33-27 screencap-2024-08-27-19-33-31 screencap-2024-08-27-19-33-36 screencap-2024-08-27-19-33-43

However, upon closer inspection of the first plot, the change happens between points 243 an 244, even though the specified with of the widget is 500:

screencap-2024-08-27-19-46-53 screencap-2024-08-27-19-46-57

Every single one of the plots experiences this issue.

I'll investigate further!

bratpeki commented 3 months ago

For such a diff:

@@ -258,6 +258,20 @@ void WaveAnalyzerWaveform::updatePoints(int count)
        int currentFrame = totalFrames - count;
        int currentPixel = totalPixels - (count / framesPerPixel);

+       printf("totalPixels: %d\n", totalPixels);
+       printf("totalFrames: %d\n", totalFrames);
+       printf("lastFrame: %d\n", lastFrame);
+       printf("framesPerPixel: %d\n", framesPerPixel);
+
+       printf("\n");
+
+       printf("baseY: %d\n", baseY);
+       printf("ySpace: %d\n", ySpace);
+       printf("currentFrame: %d\n", currentFrame);
+       printf("currentPixel: %d\n", currentPixel);
+
+       printf("\n");
+
        // Raw mode will get the value of the immediate frame
        // while Peaks/Troughs will get the peaks and troughs of the range
        switch(m_controls->m_drawingMode.value())

the output is:

totalPixels: 500
totalFrames: 512
lastFrame: 511
framesPerPixel: 1

baseY: 100
ySpace: 70
currentFrame: 256
currentPixel: 244

Ideally, currentPixel should be 250, seeing as how we have 256 frames being drawn at a time, and 512 in total. My guess is that framesPerPixel draws the specified number of frames per pixel, resulting in the drawing starting from 500-256/framesPerPixel=244.

bratpeki commented 2 months ago

I have found that the code does a lot of things I find unnecessary.

Namely, I would make the wave analyzer have one mode, and have the option of pausing the wave and a single int knob for the number of frames.

I would hide these options and have them appear with a double click, much like the compressor, or keep them to the side of the oscillator view.

I have started my own plugin development back, taking inspiration from the one by Ian. The development has been wonderfully hasteful with the help of @LostRobotMusic!

Will keep you updated.