LMMS / lmms

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

StereoEnhancer changes the amplitude of the sound #847

Open Umcaruje opened 10 years ago

Umcaruje commented 10 years ago

The stereo enhancer alters the volume of the left or the right channel when stereo seperating. This is a pretty big bug, considering a lot of users use stereo widening in their productions. A test LMMS project: http://lmms.sourceforge.net/lsp/index.php?action=show&file=5552 Listen to the file: http://goo.gl/hQtyl4 How the file looks in audacity: the bug

diizy commented 10 years ago

On 06/12/2014 10:46 PM, Umcaruje wrote:

The stereo enhancer alters the volume of the left or the right channel when stereo seperating. This is a pretty big bug, considering a lot of users use stereo widening in their productions.

Well, seems to me that that's not a bug... it's just how stereo widening works. The same thing happens with other stereo-widening effects (chorus etc.). If you want the volume to stay constant, use a limiter...

StakeoutPunch commented 10 years ago

That is not a standard practice, it is just another way LMMS tries to achieve things in a weird way.

A better solution is to delay one side by a user defined amount of milliseconds, and it keeps volume the same.

I was tempted to do a LMGTFY on how to enhance stereo, but I try not to be an ass. The issue is not the change in volume, it is how the actual plugin is attempting to increase stereo width. Adding a mono limiter would render what the plugin is doing useless if it is only adjusting the volume. Furthermore, the plugin isn't making the track sound wider, it makes the stereo image sound biased to the left after being cranked past a certain amount.

diizy commented 10 years ago

On 06/13/2014 12:12 AM, StakeoutPunch wrote:

That is not a standard practice, it is just another way LMMS tries to achieve things in a weird way.

A better solution is to delay one side by a user defined amount of milliseconds, and it keeps volume the same.

I was tempted to do a LMGTFY on how to enhance stereo, but I try not to be an ass.

Well, I was tempted to say you fail at not being an ass, but I didn't so we can just leave it at that... ;)

The issue is not the change in volume, it is how the actual plugin is attempting to increase stereo width. Adding a mono limiter would render what the plugin is doing useless if it is only adjusting the volume. Furthermore, the plugin isn't making the track sound wider, it makes the stereo image sound biased to the left after being cranked past a certain amount.

The stereoenhancer isn't only adjusting the volume. It actually is delaying one side by a user-defined amount, but it is also applying a kind of stereo matrix on the channels. A limiter would not cancel the effect.

Certainly, the stereoenhancer isn't perfect, but then no effect is... In any case my thinking on the matter is, that we should leave it as is, if for no other reason than backwards-compatibility. If we want an improved stereo widening effect, it can be done as a new effect plugin. If you have an idea for a better implementation of a widening algorithm, feel free to code it...

softrabbit commented 10 years ago

I'm only having a quick look while sipping some coffee here, but here's my impression of the code.

I wonder if this really does what's intended (include/DspEffectLibrary.h)?

inLeft += inRight * sinf( m_wideCoeff * .5 * toRad);
inRight -= inLeft * sinf( m_wideCoeff * .5 * toRad);  // I'd expect this to use the original inLeft

And in plugins/stereo_enhancer/stereo_enhancer.cpp:

Might be better to use the stereo matrix, it looks more correct. It has no delay,but that could be worked around by sending to a channel with a stereo matrix and a delay.

diizy commented 10 years ago

On 06/13/2014 10:07 AM, Raine M. Ekman wrote:

I'm only having a quick look while sipping some coffee here, but here's my impression of the code: I wonder if this really does what's intended (include/DspEffectLibrary.h)?

|inLeft += inRight * sinf( m_wideCoeff * .5 * toRad); inRight -= inLeft * sinf( m_wideCoeff * .5 * toRad); // I'd expect this to use the original inLeft|

||That's a good catch... it should probably use a temp variable there.

And in plugins/stereo_enhancer/stereo_enhancer.cpp:

  • Looks like the small delay used is in samples, not too good when sampling rates change
  • Is only one channel delayed? |sample_t s[2] = { _buf[f][0], m_delayBuffer[frameIndex][1] };|

Might be better to use the stereo matrix, it looks more correct. It has no delay,but that could be worked around by sending to a channel with a stereo matrix and a delay.

Yes, only one channel is delayed. That is probably a primitive way to make the phases out of sync.

As for the delay amount... this is true that it doesn't take sample rate in account, and that's something that could be fixed. Backwards-compat aside, there's very rarely a situation where the user wants the sound to be changed with changing samplerate. If this were the master branch, the effect could be modified to use my new RingBuffer class, which is written in a way that you can use it with a millisecond amount, and the class takes care of samplerate changes automatically (in theory, I haven't had the chance to test it in action yet).

So it might be better to leave fixing this for 1.2, because we'll have better tools available there...

tresf commented 9 years ago

Per @Umcaruje, milestoning and tagging so this gets some attention prior to the release of 1.2.

Not sure how practical this is, but we can bump it if needed.

-Tres

Umcaruje commented 9 years ago

So after some talk over at IRC, @curlymorphic and myself agreed that this should be fixed by adding a new plugin that would apply the Haas Effect. The production of the new plugin shouldn't be too complex to do, since the delay effect has some code that can be used.

The plugin could be called Haassan (That's my idea at least).

Sti2nd commented 9 years ago

So that means you aren't fixing anything?

Umcaruje commented 9 years ago

Well changing the effect would break backwards compatibility.

Sti2nd commented 9 years ago

Hmm, and it could be debated wether this is a bug or not, so ok, makes a little sense. You don't feel it will be annoying to always add two plugins just to use the stereo enhancer?

Umcaruje commented 9 years ago

?

The new plugin would be a standalone, no need to add the old one...

Sti2nd commented 9 years ago

Ooh, cause that is another way of achieving stereo effect. Ok, so basically a completely new effect that has nothing to do with the old one. Now I get it. I thought you were going to clone the old one and in addition apply the haas effect and then call it "Stereoenhancer New", only that you wanted to call it Haassan.

curlymorphic commented 9 years ago

@diizy wrote

If we want an improved stereo widening effect, it can be done as a new effect plugin. If you have an idea for a better implementation of a widening algorithm, feel free to code it...

This is why I suggested implementing a new plugin.

Sti2nd commented 9 years ago

Cool. Uros used the word fix. Sorry for not understanding, probably the best thing to do. :+1:

tresf commented 9 years ago

Yeah it is technically an enhancement then, but symantecs.... :)

We do need to consolidate/hide/renove some of our plugins for 2.0 so perhaps the old version can be part of a cleanup task. I'm also interested in how much of a change this is in code. Perhaps we entertain a config option for old behavior and use compat code to turn it on/off.

curlymorphic commented 9 years ago

Perhaps we entertain a config option for old behavior and use compat code to turn it on/off.

I am confused, Looking into the code revels the bug in this reported by @softrabbit has already been fixed .

https://github.com/LMMS/lmms/blob/master/include/DspEffectLibrary.h#L336

            const float toRad = F_PI / 180;
            const sample_t tmp = inLeft;
            inLeft += inRight * sinf( m_wideCoeff * ( .5 * toRad ) );
            inRight -= tmp * sinf( m_wideCoeff * ( .5 * toRad ) );

Currently The stereoEnhancer uses 2 techniques, The Haas Effect, and Polar Exploration. https://www.soundonsound.com/sos/sep10/articles/pt-0910.htm#Top.

@Umcaruje does this solve your use case, or do you still require a Hass only effect?

karmux commented 9 years ago

@curlymorphic, @Umcaruje, When I export this project to both ogg and wav then Audacity shows totally different soundwaves. http://www.upload.ee/image/4617598/lmms-stereo-ogg-vs-wav.png (ogg on top and wav on bottom)

Wallacoloo commented 9 years ago

Could this possibly just be due to how ogg encodes stereo information?

If possible, you should try exporting as wav, and then re-encoding that as an ogg using an external program (audacity if possible, or maybe ffmpeg). If the ogg looks similar to that generated directly by lmms, then this is likely not an issue with lmms. On Apr 5, 2015 8:25 AM, "Karmo Rosental" notifications@github.com wrote:

When I export this project to both ogg and wav then Audacity shows totally different soundwaves. http://www.upload.ee/image/4617598/lmms-stereo-ogg-vs-wav.png

— Reply to this email directly or view it on GitHub https://github.com/LMMS/lmms/issues/847#issuecomment-89791017.

rubiefawn commented 9 years ago

The StereoEnhancer changes the amplitude of my sound IN LMMS, and during export too.

On Sun, Apr 5, 2015 at 12:40 PM, Colin Wallace notifications@github.com wrote:

Could this possibly just be due to how ogg encodes stereo information?

If possible, you should try exporting as wav, and then re-encoding that as an ogg using an external program (audacity if possible, or maybe ffmpeg). If the ogg looks similar to that generated directly by lmms, then this is likely not an issue with lmms.

On Apr 5, 2015 8:25 AM, "Karmo Rosental" notifications@github.com wrote:

When I export this project to both ogg and wav then Audacity shows totally different soundwaves. http://www.upload.ee/image/4617598/lmms-stereo-ogg-vs-wav.png

— Reply to this email directly or view it on GitHub https://github.com/LMMS/lmms/issues/847#issuecomment-89791017.

— Reply to this email directly or view it on GitHub https://github.com/LMMS/lmms/issues/847#issuecomment-89830696.

karmux commented 9 years ago

@Wallacoloo they also sound differently. When I convert wav to ogg with Audacity then this ogg sounds and looks exactly like wav. It means that LMMS exports differently sounding ogg and wav out of this project. I did some testing about this issue and I couldn't reproduce it with other plugins that I tested. Should be only StereoEnhancer that makes LMMS to export drastically different wav and ogg sounds.

curlymorphic commented 9 years ago

@diizy wrote

If we want an improved stereo widening effect, it can be done as a new effect plugin. If you have an idea for a better implementation of a widening algorithm, feel free to code it...

This is the reason I dont want to touch the original plugin.

Im unsure how to proceed, do I change the current plugin, or write a seperate one, using the same techiques ( the processes used are pretty standard ) that we can address these issues on?

I dont really like the thought of adding loads of hacky if(oldVersion) .... else .....

@tresf @diizy @Lukas-W @ anyoneIForgotPleaseAddThem

what are your thoughs

tresf commented 9 years ago

@curlymorphic then please write a new one. I just want the team to agree that we don't need two of every plugin. Eventually we need a way of deprecating stuff so we don't end up with a bunch of old unused code. :+1:

Wallacoloo commented 9 years ago

Edit: ignore the following - I somehow thought karmux's comment was the start of the thread.

The original problem in this thread is simply that the wave and ogg export formats are visibly/audibly different. It probably doesn't have anything to do with the stereo expansion effect at all, but with whatever code encodes the ogg. On Apr 6, 2015 9:03 AM, "Tres Finocchiaro" notifications@github.com wrote:

@curlymorphic https://github.com/curlymorphic then please write a new one. I just want the team to agree that we don't need two of every plugin. Eventually we need a way of deprecating stuff so we don't end up with a bunch of old unused code. [image: :+1:]

— Reply to this email directly or view it on GitHub https://github.com/LMMS/lmms/issues/847#issuecomment-90125456.

tresf commented 9 years ago

@Wallacoloo yeah, or perhaps a bug in the default export resolution settings of OGG vs WAV per "... not all of the parameters above apply for all formats".

curlymorphic commented 9 years ago

@curlymorpic scrolls to top of page to check title, StereoEnhancer changes the amplitude of the sound

Have some wires been crossed?

curlymorphic commented 9 years ago

@Umcaruje are you still up for designing the ui for this?

Umcaruje commented 9 years ago

Yes I am :3 Just tell me how many knobs.

P.S. is the name gonna be 'Haassan'?

curlymorphic commented 9 years ago

Just tell me how many knobs.

2 knobs please. one for hass delay time, and one for polar exploration amount.

P.S. is the name gonna be 'Haassan'?

fine by me :)

softrabbit commented 9 years ago

Just in case @curlymorphic decides to copy the code for the next generation of the plugin...

This bit seems wrong. Didn't catch it earlier, but now I'm thinking both lines should have a -= operator. Better late than never, I suppose. https://github.com/LMMS/lmms/blob/master/include/DspEffectLibrary.h#L338

inLeft += inRight * sinf( m_wideCoeff * ( .5 * toRad ) );
inRight -= tmp * sinf( m_wideCoeff * ( .5 * toRad ) );

I'm saying both channels should have some of the opposite channel mixed in with reverse polarity, not like this (looks more like a butchered MS encoder).

curlymorphic commented 9 years ago

I have a prototype demonstrating 3 different algorithms, It is also dual band.

I would really like some feedback back please., on what algorithms we should use, and if the multiband is a good idea.

Here is a release with a windows32 installer, The plugin to use is called 'Haassan'

https://github.com/curlymorphic/lmms/releases/tag/HassanPrototype1

tresf commented 9 years ago

I would really like some feedback back please., on what algorithms we should use, and if the multiband is a good idea.

Sorry if this is implied, but can you please direct this comment at someone you feel can help? I say this because the Windows installer won't necessarily attract many developers that know algorithms (are you asking for testers to give feedback too or am I over thinking this?). The clearer and more direct you can be with your request for feedback, the better feedback I think you'll receive.

Umcaruje commented 9 years ago

I'll test it out later today. On 8 Apr 2015 16:04, "Dave" notifications@github.com wrote:

I have a prototype demonstrating 3 different algorithms, It is also dual band.

I would really like some feedback back please., on what algorithms we should use, and if the multiband is a good idea.

Here is a release with a windows32 installer, The plugin to use is called 'Haassan'

https://github.com/curlymorphic/lmms/releases/tag/HassanPrototype1

— Reply to this email directly or view it on GitHub https://github.com/LMMS/lmms/issues/847#issuecomment-90926320.

curlymorphic commented 9 years ago

Sorry I didnt explain properly. I was looking for users to try out the 3 different types of widening, to see If there are any preferences as to what method should be used.

@Umcaruje @tresf @softrabbit @Sti2nd @unfa @grejppi

I would really appreciate your views on the audio produced using the 3 different methods provided in the 'Haassan' pluging

https://github.com/curlymorphic/lmms/releases/tag/HassanPrototype1

rubiefawn commented 9 years ago

Doesn't Ableton have a stereo widener in Utility? I think it widens sound by using phase cancellation to remove sounds that occur in both left and right.

On Wed, Apr 8, 2015 at 12:37 PM, Dave notifications@github.com wrote:

Sorry I didnt explain properly. I was looking for users to try out the 3 different types of widening, to see If there are any preferences as to what method should be used.

@Umcaruje https://github.com/Umcaruje @tresf https://github.com/tresf @softrabbit https://github.com/softrabbit @Sti2nd https://github.com/Sti2nd @unfa https://github.com/unfa @grejppi https://github.com/grejppi

I would really appreciate your views on the audio produced using the 3 different methods provided in the 'Haassan' pluging

https://github.com/curlymorphic/lmms/releases/tag/HassanPrototype1

— Reply to this email directly or view it on GitHub https://github.com/LMMS/lmms/issues/847#issuecomment-90998685.

StakeoutPunch commented 9 years ago

That method would result in no sound if you had a completely mono sound source...

curlymorphic commented 9 years ago

Out of the 3 types of separation offered, what ones(s) do we want to use?

Do you feel the multiband feature is useful?

Should we have a "led button" to select what effects are active, or should we leave them all running in series, as is the current case?

There was some feedback given on irc, @Umcaruje @softrabbit can you please give your thoughts here.

Once this is determined @Umcaruje can work on the ui.

My votes are to use the hassan, and width effects, and drop the polar. I would also like to keep the multiband feature.

The width effect can be used to narrow a signal, This i personally use injunction with with the multiband feature, to narrow the bottom end of a signal, while leaving the rest to use the stereo field.

I am going to spend some time now, removing the buzzing from the hass control.

Umcaruje commented 8 years ago

Changed the milestone, as I don't believe this issue is crucial to 1.2, and I've also just learned that we include the 'Calf Stereo Tools' as a ladspa plugin, which can serve as a replacement.

IanCaio commented 3 years ago

I ran into this issue by chance looking for another one. Just adding a comment to link that issue to #5956 . I'm not sure it completely solves the issue but the output gain knob at least makes it more convenient to compensate the undesirable increase in the amplitude.

Rossmaxx commented 2 months ago

Since the talk 'was' about adding a new plugin in favour of the current plugin, I'm removing the milestone on this.