Bugzilla – Bug 533
Multiple problems with Delay effect
Last modified: 2018-08-20 11:51:26 UTC
Created attachment 263 [details] replacement for delay.ny The excessive wording in the interface is inconsistent with built in effects (this is not the only bundled plug-in with this issue, but best to clear them one at a time) The level of each echo is much lower than the level set by the user (the manual has been temporarily adjusted so as to be vague rather than wrong about this). The pitch change not only changes the pitch but also the speed of the echoes, causing the echoes to drift out of synch with the original audio. Missing license information in the comments (though brief license declaration is present in the GUI). Audio becomes stretched if the sample rate is less than 44.1 kHz File attached: The attached file is a complete rewrite (the original code is too broken to fix) of the Delay effect with the same features as the original effect, but without the above bugs. Limitations of this fix: The pitch shift now uses the Nyquist pitch shift so it resolves the time stretch issue. Unfortunately the Nyquist pitch shift effect is not particularly brilliant and when applied to short percussive sounds (such as a click track) it introduces a a short echo of its own (independent of the delay effect). This is a limitation of the pitch shift function in Nyquist and is not fixable within the Nyquist plug-in code. Other changes from the original effect: Where it may be useful and not dangerous I've allowed values higher than the slider values to be used by text input. The slider values are still the 'recommended' ranges for most purposes. The effect no longer uses Roger Dannenberg's delay algorithm as it is not suitable when using pitch change (the sound quality would be much worse).
As there is some question about how best to resolve the issue of pitch shifting, I've started a forum topic here: http://forum.audacityteam.org/viewtopic.php?f=42&t=66308 When there is consensus about an appropriate replacement for the Delay effect I'll post it here.
Comment on attachment 263 [details] replacement for delay.ny Appropriate solution for pitch shift issue is not yet agreed.
Created attachment 302 [details] updated delay.ny (In reply to comment #2) The "pitch shift" used in the original version was a simple resampling method. This changes both the pitch and speed, thus the delay times set by the user are not the delay times they get (other than the first sample of each echo. Until someone writes a high quality pitch change function for Nyquist there is no satisfactory solution to this problem, so this feature has been disabled. The code contains two "pitch change" features that are commented out. To enable these features requires one semicolon being removed from the start of lines 20 and 21. This is documented in the plug-in comment text. If enabled, the default "pitch change per echo" option is: "Pitch/Tempo" This is equivalent to the resampling method used in the original effect and changes both the pitch and tempo of each echo. This is improved from the original version but it still has the inherent problem of tempo change. The other option is "LQ Pitch Shift" (Low Quality Pitch Shift). This option uses Nyquists pitch shift function that changes the pitch without changing the tempo. Unfortunately the sound quality of this pitch shift is quite poor and introduces unwanted reverberation to the sound that causes the delay time to be inaccurate. After lengthy discussion on the QA list there has been no consensus to support either of these pitch shift methods in the shipped version of the delay effect, though a version with these features enabled can be made available on the wiki. If there is strong user feedback requesting pitch shift to be enabled in the shipped version, then it is a very simple matter to re-enable these features. All other bugs and issues are believed to be fixed in this version (but please test). Other Changes: The default delay time has been reduced from 0.5 to 0.3 seconds. This is probably a more useful default setting (and makes testing easier). New feature: Option: "Allow duration to change", default "Yes". In the original version of the delay effect, the duration of the selected audio would automatically be extended to allow space for the echoes. The disadvantage of this is that if the effect is applied to a selection within a track, then all subsequent audio in the track will be time shifted. This would not be acceptable in a multi-track project as synchronization between tracks is lost. Setting this option to "No" constrains the duration of the selected audio so that the output duration does not increase. If this option is selected users may want to make their selection long enough to allow for the echoes to fade out. Complete .NY file attached.
Created attachment 303 [details] patch file for delay.ny
(In reply to comment #3) Thanks, Steve. I did not find any audio bugs. I like the option: "Allow duration to change", default "Yes". What happened to "Reverb" and "Damping"? To come for 2.0.3? > After lengthy discussion on the QA list there has been no consensus to support > either of these pitch shift methods in the shipped version of the delay effect Well in a one to one dialogue you wanted to remove pitch and I wanted to keep it. I haven't changed my mind, in fact I think it would be respectful of David's wishes. If we do keep pitch, I think it should be labelled "Pitch change effect", and "Maximum delay time" is still wrong. Might just as well call it "Delay Time" (and no reason not to if we comment Pitch Change out). There is a typo "precusssive" at line 32. While we are at consistency issues, should we insist that Nyquist plug-ins shipped with Audacity have "action" the same as "name" (like Easy Vocoder does)?
(In reply to comment #5) > should we insist that Nyquist plug-ins > shipped with Audacity have "action" the same as "name" (like Easy Vocoder > does)? Are you referring to line 6 of "updated delay.ny 2012-07-25 11:44 EDT" ;action "Applying Delay Effect..." I'm not sure why you are raising this here and now. Does this have anything to do with bug 533? >> After lengthy discussion on the QA list there has been no consensus to support >> either of these pitch shift methods in the shipped version of the delay effect >Well in a one to one dialogue you wanted to remove pitch and I wanted to keep > it. I'm referring to this lengthy dialogue on the QA list: http://sourceforge.net/mailarchive/message.php?msg_id=29559515 As only Gale and myself contributed to that discussion I can only assume that other people on the QA list have no opinion on the subject. > in fact I think it would be respectful of > David's wishes. I have the utmost respect for David Sky and his contributions to Audacity, but that is not the issue. The issue is that if David or anyone else submitted the old Delay effect for inclusion in Audacity today, it would be rejected, not only because of the bugs but because it does not do what it says. It is not entirely clear what David's intentions were regarding "pitch shift" but I think that it is likely that he intended it to be a "pitch shift" effect. However it is not a "pitch shift" effect. It is a "Change Speed" effect, which like the "Change Speed" effect in Audacity, changes both pitch and tempo. When David wrote the code, the Nyquist pitch shift function may not have been available as it is one of the more recent additions to Audacity Nyquist. Thus changing pitch and tempo may have been the closest that he could get to a "pitch shift" effect. The problem that is inherent with using a "change speed" effect within the delay is that it is then *impossible* to deliver the delay time that is set by the user. Not only does the effect not do what it says, it *cannot* do what it says. This has already been discussed on the QA list, which I understand is the correct forum for discussion. > What happened to "Reverb" and "Damping"? To come for 2.0.3? They are new features that I suggested could be included in a future version. My primary concern now is to fix the know issues in the currently shipped version, preferably in time for the 2.0.2 release. The new option "Allow duration to change" (default "Yes") is a new feature, but addresses a know issue in the currently shipped version. > There is a typo "precusssive" at line 32. Fixed.
Created attachment 304 [details] typo correction
Created attachment 305 [details] typo correction
(In reply to comment #6) > ;action "Applying Delay Effect..." > > I'm not sure why you are raising this here and now. Does this have anything > to do with bug 533? It does, because you changed "Performing" here to "Applying". I was hoping we could agree to match "action" and "name" for sake of consistency with built-in effects, as and when we update plug-ins (so again, relevant here). But as you wish to discuss it I started a -quality thread about it. > I'm referring to this lengthy dialogue on the QA list: > http://sourceforge.net/mailarchive/message.php?msg_id=29559515 > As only Gale and myself contributed to that discussion I can only assume that > other people on the QA list have no opinion on the subject. Which still means there is no consensus. If no consensus to change, normally that means there is no change. If you call the pitch change "method" an "effect" then I think we can live with the zaniness of both "methods" rather than have the aggravation of explaining that user can turn pitch effects on by opening the .ny file. I see no need to introduce a regression into the effect by removing a feature where there are no complaints made about the feature. > if David or anyone else submitted the old Delay effect for inclusion in > Audacity today, it would be rejected, not only because of the bugs but > because it does not do what it says. You fixed the bugs (thanks). If the "pitch effect" is described as "Pitch/Tempo" as we have done, then it does what it says. And to most ears the pitch change is far more noticeable than the speed change (for most use cases on reasonably short selections). We have no alternative I know of to provide a delay with pitch shift, which is what David wanted. > The problem that is inherent with using a "change speed" effect within the > delay is that it is then *impossible* to deliver the delay time that is set > by the user. As indeed with the LQ Pitch change. You did not respond about "Maximum Delay Time". What does it mean, given LQ Pitch Shift produces more delay than the "Maximum"? It seems more sensible to me to call it "Delay Time" and accept that the pitch effects modify the set delay time slightly.
(In reply to comment #9) ;action text left as "Applying Delay Effect..." which I think we agree is acceptable. > If you call the pitch change "method" an > "effect" then I think we can live with the zaniness of both "methods" I can live with that. Control labels are now: Delay type: choice: regular (default), bouncing ball, reverse bouncing ball. Delay level per echo (dB): Delay time (seconds): Pitch change effect: choice: Pitch/Tempo (default), LQ Pitch Shift Pitch change per echo (semitones): Number of echoes: Allow duration to change: choice: Yes (default), No > You did not respond about "Maximum Delay > Time". What does it mean? As per the alpha documentation: "The time between echoes. For the Bouncing Ball types of delay, this sets the maximum delay time between bounces." http://manual.audacityteam.org/man/Delay More detailed description: Disregarding issues with pitch change: "Bouncing Ball" type delay: the first echo has the longest delay period and the subsequent delay periods get shorter. "Reverse Bouncing Ball": the echoes slow down and the final echo has the longest delay period. The "Delay time (seconds)" control sets the longest of these delay periods. If "LQ Pitch Shift" is used, the delay periods may be slightly inaccurate. If "Pitch/Tempo" the delay times will progressively shift earlier (if pitch is shifted up, the delayed audio plays faster) or later (if pitch is shifted down the delayed audio plays slower).
Created attachment 306 [details] replacement delay.ny with requested changes
Created attachment 307 [details] patch file for latest delay.ny
Thanks, Steve. Marked as "patch_ready".
Committed.
RESOLVED-FIXED. No-one has commented or complained.
