audacity / audacity

Audio Editor
https://wiki.audacityteam.org/wiki/For_Developers
Other
12.55k stars 2.27k forks source link

Stretched clip: amplify doesn't amplify enough #5170

Closed DavidBailes closed 1 year ago

DavidBailes commented 1 year ago

Bug description

Tested with: https://github.com/audacity/audacity/commit/c9ff9faaa6ad2e348c6640c039025874fa870ac0

Steps to reproduce

  1. Create a tone with an amplitude 0.5 and a length 30s
  2. Shrink the clip by about 0.5s (using time-stretching)
  3. Select part of the clip, and apply amplify with default settings.

Expected behavior

image

Actual behavior

image

If you apply amplify again, you can amplify by another ~0.5dB

Audacity Version

other

Operating system

All OSs

Additional context

This issue does not occur with the Normalize effect.

petersampsonaudacity commented 1 year ago

Might this be related to:

Shrinking or stretching a clip reduces its loudness #5160 ?

Paul-Licameli commented 1 year ago

Try it in branch #5138

Paul-Licameli commented 1 year ago

Might this be related to:

Shrinking or stretching a clip reduces its loudness #5160 ?

Not exactly, but... (next)

Paul-Licameli commented 1 year ago

Try it in branch #5138

Haven't tried but I predict it won't be fixed in that branch.

Amplify uses the peaks (not the RMS) to decide the default settings: see where it uses GetMinMax

And that applies to the input tracks before the rendering for stretch, even with the fixes in #5138

Whereas, I predict Normalize will not have this problem in that branch, though it too uses GetMinMax.

Paul-Licameli commented 1 year ago

It is known that if you render the stretching, RMS changes (we think it ought not to, but don't understand why)

But also, the waveform may look very different though you might not hear difference of timbre in sustained notes. Even with the same RMS, the difference between that and the peak might change a lot, and that has to do with keeping the magnitudes of the frequency components, but not always their phase.

Example: Try a square wave (peak == RMS); stretch it; then mix-and-render to a new track. Observe the dark and pale blue parts of the waveform.

Amplify is still detecting the peaks of the raw data, as they are before stretching. #5138 fixes other problems by rendering the stretch in the selected interval before computing the effect transformation. So I think there would still be a mismatch, when computing the default amplification shown in the dialog before the real processing.

DavidBailes commented 1 year ago

Try it in branch #5138

Haven't tried but I predict it won't be fixed in that branch.

Amplify uses the peaks (not the RMS) to decide the default settings: see where it uses GetMinMax

And that applies to the input tracks before the rendering for stretch, even with the fixes in #5138

Whereas, I predict Normalize will not have this problem in that branch, though it too uses GetMinMax.

Tried it with: https://github.com/audacity/audacity/commit/0e78a9ea3f5be1dea7f07e48bc71d9950acf59b7 Amplify isn't fixed, Normalize works, as before.

dozzzzer commented 1 year ago

No longer reproducible in the latest master.

petersampsonaudacity commented 1 year ago

@dozzzzer @DavidBailes @Paul-Licameli

Testing on W10 with latest 3.4.0 alpha master: audacity-win-3.4.0-alpha-20231009+8b226a3-x64-msvc2022

This still fails for me: image

dozzzzer commented 1 year ago

I've previously misinterpreted the STRs and made a trim instead of stretching. With stretching, though, it is still reproducible indeed. Thx, @petersampsonaudacity

petersampsonaudacity commented 1 year ago

@dozzzzer

the image shows clearer now that the % speed has been added to time-stretched clips

I updated the STRs for clarity

Flagging @saintmatthieu

petersampsonaudacity commented 1 year ago

@dozzer @LWinterberg @saintmatthieu @DavidBailes

This begs a question: is it intended (designed) behavior that applying an effect to a clip automatically renders its time-stretching? Is it desired behavior? Should the % stretchiness be removed form a clip that has an effect applied to it?

And also I note, as a corollary, that if you amplify part of an un-stretched 100% clip then you do NOT get splits created. But if you amplify part of a time-stretched clip than the clip has splits generated. Is this intentional/designed behavior?

If these are desired/designed behaviors then I can add some notes re. this in the manual - else bug/bugs can be raised.

LWinterberg commented 1 year ago

The rendering is intended and desired. Otherwise, you'd get misbehaving effects. Maybe not in the case of amplify, but anything that has a time component in it - delay, compressors, etc.

saintmatthieu commented 1 year ago

Hello! We are aware that the scaling step in the stretch-rendering is imperfect. When slowing down, it scales it up too much, and vice-versa when accelerating (or the other way round). I was actually looking into this this very morning.

petersampsonaudacity commented 1 year ago

It's not just using effects that gets you auto-rendering - so too do generators, Analyzers and some Tools.

saintmatthieu commented 1 year ago

Yes, I just pushed a PR to fix that, #5350

petersampsonaudacity commented 1 year ago

@saintmatthieu that PR looks to fix the Analyzers - but what about the generators, are they still affected with auto-rendering?

petersampsonaudacity commented 1 year ago

OK so I have added a bold, in your face, Alert div to the Effects page: https://alphamanual.audacityteam.org/man/Index_of_Effects,_Generators_and_Analyzers

And added (gentler) advice divs to all the effects pages. This is an example: https://alphamanual.audacityteam.org/man/Amplify

Clips pages have also been updated: a) https://alphamanual.audacityteam.org/man/Audacity_Tracks_and_Clips b) https://alphamanual.audacityteam.org/man/Edit_Menu:_Clip_Boundaries

I am holding off on the Generators pending @saintmatthieu 's reply to my previous post

petersampsonaudacity commented 1 year ago

@Paul-Licameli Paul, does your latest push to master mean tha only some effects will now get auto-rendering with time-stretched clips? Don't compute interpolation of samples when no pitch shifting #991

If so can you please provide me a list of ones that aren't affected (or those that remain effected if that list is shorter.

Thanks

saintmatthieu commented 1 year ago

@saintmatthieu that PR looks to fix the Analyzers - but what about the generators, are they still affected with auto-rendering?

Generating over a selection [t0, t1] will

saintmatthieu commented 1 year ago

OK so I have added a bold, in your face, Alert div to the Effects page

That looks correct, insofar as by "effect" is meant processing effects. Analyzers aren't supposed to change the analyzed audio at all (which is what my PR intends to fix for Nyquist analyses). Maybe it's worthwhile clarifying.

saintmatthieu commented 1 year ago

OK so I have added a bold, in your face, Alert div to the Effects page: https://alphamanual.audacityteam.org/man/Index_of_Effects,_Generators_and_Analyzers

And added (gentler) advice divs to all the effects pages. This is an example: https://alphamanual.audacityteam.org/man/Amplify

Clips pages have also been updated: a) https://alphamanual.audacityteam.org/man/Audacity_Tracks_and_Clips b) https://alphamanual.audacityteam.org/man/Edit_Menu:_Clip_Boundaries

I am holding off on the Generators pending @saintmatthieu 's reply to my previous post

Thank you so much, Peter ! On the clip-boundary page, reading "Note that the whole clip must be selected for this command to be operable." made me think of the new behaviour that right-clicking the header of a clip selects it all. Not sure it's worthwhile mentioning in the docs?

saintmatthieu commented 1 year ago

@Paul-Licameli Paul, does your latest push to master mean tha only some effects will now get auto-rendering with time-stretched clips? Don't compute interpolation of samples when no pitch shifting #991

If so can you please provide me a list of ones that aren't affected (or those that remain effected if that list is shorter.

Thanks

I can answer that: this PR doesn't change the behaviour at all. It only is an optimization for the case the library is used for time-stretching only (as opposed to pitch shifting), sparing unnecessary computation.

petersampsonaudacity commented 1 year ago

@saintmatthieu wowser I totally failed to realize, infer, "discover" that the auto-created clips either side of the selection get to be fully expandable via smart-trimming when you generate over a selection in a time-stretched clip (and this is partly because there is no visual cue for a smart-trimmed clip with hidden data - IIRC there is a bug logged for this on GitHub - and partly because, for me at least this is totally unexpected behavior)

And note very carefully that the same applies when you apply an effect to a selection in a time-stretched clip. Here to the new clips either side cab be smart-clip expanded to the full clip.

saintmatthieu commented 1 year ago

Visual indicators for stretching ... that's interesting. I sometimes unnecessarily suffered from not realizing there was hidden data, but never thought of improving on this. @LWinterberg any comments?

petersampsonaudacity commented 1 year ago

That looks correct, insofar as by "effect" is meant processing effects. Analyzers aren't supposed to change the analyzed audio at all (which is what my PR intends to fix for Nyquist analyses). Maybe it's worthwhile clarifying.

@saintmatthieu

Done: https://alphamanual.audacityteam.org/man/Index_of_Effects,_Generators_and_Analyzers

petersampsonaudacity commented 1 year ago

Visual indicators for stretching ... that's interesting. I sometimes unnecessarily suffered from not realizing there was hidden data, but never thought of improving on this.

@saintmatthieu - this is the bug that was logged #1912 But note that it was "shelved", closed as "not planned" by @LWinterberg on )2Jun23

petersampsonaudacity commented 1 year ago

On the clip-boundary page, reading "Note that the whole clip must be selected for this command to be operable." made me think of the new behaviour that right-clicking the header of a clip selects it all. Not sure it's worthwhile mentioning in the docs?

@saintmatthieu - thanks for the suggestion

I have actually added (in several places on that page)

an entire clip can be selected by left-clicking anywhere in its waveform.

I am more than somewhat disinclined to document the right-click in the Clip-handle drag-bar also making the whole clip selection, because: a) traditionally in apps it is left-click gestures that are used for selection b) right-clicking in apps is normally reserved for invoking context menus

Accordingly I, personally, think that using the right click to not just invoke the context menu but also change the user's carefully made selection that they want to apply the context menu to, thereby making some commands in that Clip-handle context menu totally useless i.e. Join Clips and Split Clip.

This to me is poor UX and thus I really have no wish to encourage its use by documenting it in the Manual.

UPDATE I have logged a UX/design issue on GitHub for this - see #5354

Paul-Licameli commented 1 year ago

This problem is analogous to the one fixed for Find Clipping at 2d95b986723ccf70f0c446a5fe31a833e2b34c53

Peak detection should be calculated from the as-stretched data when deciding the numbers to put by default in the dialog box.

I think a fix could be easily written but it might wastefully compute stretch twice. To find the peak, then to do the amplification.

dozzzzer commented 1 year ago

I no longer see the original issue testing against this PR

petersampsonaudacity commented 1 year ago

@saintmatthieu @Paul-Licameli @crsib

Testing on W10 with the latest Beta for 3.4.0: audacity-win-3.4.0-beta-20231017+51c3606-x64-msvc2022

I am still seeing this bug image

Paul-Licameli commented 1 year ago

There is a fix, not yet merged into beta

petersampsonaudacity commented 1 year ago

@Paul-Licameli - so is it going to be merged before 3.4.0 release please? I thinks its kind of important.

Paul-Licameli commented 1 year ago

@Paul-Licameli - so is it going to be merged before 3.4.0 release please? I thinks its kind of important.

Likely, today.

Paul-Licameli commented 1 year ago

@Paul-Licameli - so is it going to be merged before 3.4.0 release please? I thinks its kind of important.

Likely, today.

Done!

petersampsonaudacity commented 1 year ago

@Paul-Licameli yes, I saw - I'm waiting for the build to complete - Thanks Paul.

petersampsonaudacity commented 1 year ago

Testing on W10 with the very latest 3.4.0 Bata:

This now works fine: image

@Paul-Licameli I think you can close this :-)

Paul-Licameli commented 1 year ago

Merged to release branch, which was then merged into master.