Bugzilla – Bug 433
Truncate Silence: Relabel UX to clarify Min/Max and allow Min > Max saving
Last modified: 2018-08-20 11:45:19 UTC
If Min silence duration is set to less than Max silence duration, the Min silence duration is not remembered correctly. If the effect is applied with Min > Max and the effect is applied, any repeat of the effect will change the Min silence duration to the same value as the Max silence duration.
(In reply to comment #0) > If Min silence duration is set to less than Max silence duration, the Min > silence duration is not remembered correctly. Not true, I think, e.g Min 50, Max 100, Min remembered correctly as 50. > If the effect is applied with Min > Max and the effect is applied, any > repeat of the effect will change the Min silence duration to the same > value as the Max silence duration. Min cannot by definition be > Max (user error), so with Min 100, Max 10 as in your steps to repro, silence is truncated to 10 ms and the settings "Min 10, Max 10" when relaunching the effect reflect that. I guess the argument is "which of Min or Max does Audacity correct in case of user error?" The Manual says Truncate Silence will never shorten silent audio to less than than the Min length, so this isn't being observed, but the Manual assumes logical figures are being entered. I would argue Audacity is doing the right thing with "Min 100, Max 10". Novices should be either entering Min = Max, or ignoring Min and entering Max with compression of 1, then they get the resultant silence they asked for. I wouldn't want them to get 100 ms after entering "Min 100, Max 10". If advanced users are entering Min > Max, they can figure their mistake for themselves. Demoted to P5 Review and I suggest resolving this invalid. If you want an Enhancement P5, I think it should be that if user changes Max or Min so that Min > Max, add to the dynamic text "Min duration cannot be less than Max duration".
Can Audacity pop a dialog when Min > Max and/or refuse the operation?
(In reply to comment #1) > Min cannot by definition be > Max (user error) Looking at what the effect does, that is not correct. I think the control description is unclear, the documentation is misleading / incorrect. Min can (validly) be greater than Max, and is a useful feature that should be retained. "Min silence duration" appears to be "The minimum duration of silence in the unprocessed audio that is allowed". (InitialAllowedSilentMs) This is different to the manual description: "Specifies the shortest allowable resulting silence" which is incorrect as some settings will produce resulting silence that is less than Min. "Max silence duration" appears to be as described in the manual: "the longest allowable resulting silence." (LongestAllowedSilentMs) I can see that TruncSilence.cpp sets "Initial" to "Longest" if "Initial > Longest" (lines 61 and 62) but I don't see why it is useful to do this. Example: If the audio contains silences of 500 ms, and silence of 2 seconds at the end, the end silence can be trimmed to 50 ms without affecting the 500 ms silences by setting Min to 1000 and Max to 50. This would mean that silence less than 1000 ms (Min) is allowed (not truncated), and that silence greater than Min is truncated to "Max" duration (50 ms). This is very useful for trimming long duration leading / trailing silence from a recording. There is also something wrong about the "Silence compression" setting documentation: (http://manual.audacityteam.org/man/Truncate_Silence) Example 1: Mono track (32-bit float 44.1kHz) contains 2000 ms silence. Min = 500, Max = 1000, compression = 4:1 After processing, the silence is 875 ms (allowed Min silence + compressed remainder) 500 + ((2000 - 500)/4) = 875 Example 2: Mono track (32-bit float 44.1kHz) contains 2000 ms silence. Min = 500, Max = 600, compression = 4:1 After processing, the silence is 600 ms (= Max) According to the manual, Max > Min, so the silence should still be 875 ms. However, this does not happen because the resulting silence (875) would be greater than "Max", which is the "longest allowable resulting silence".
(In reply to comment #3) Thanks, Steve. I had a feeling there was more behind this than the initially stated bug. > Looking at what the effect does, that is not correct. I think the control > description is unclear, the documentation is misleading / incorrect. > Min can (validly) be greater than Max, and is a useful feature that should be > retained. "Min silence duration" appears to be "The minimum duration of > silence in the unprocessed audio that is allowed". (InitialAllowedSilentMs) I agree that's what it "appears" to be. John's text in the Manual (which no-one had really queried) actually said "silent passages shorter than this length will not be altered" but preceded it with "Specifies the shortest allowable resulting silence." The Manual also said for Min "If the selection to be truncated begins with silence, that silence will be truncated to this minimum value." That no longer seems to be true whether Min > Max or not so I deleted that sentence and tried to tidy up the description of min and the "Silence compression" section. Please see if you are happy with it (http://manual.audacityteam.org/man/Truncate_Silence). Also found an apparent bug (created at bug 434) where a silence may not be truncated if it is exactly the min length. > If the audio contains silences of 500 ms, and silence of 2 seconds at the > end, the end silence can be trimmed to 50 ms without affecting the 500 ms > silences by setting Min to 1000 and Max to 50. This would mean that silence > less than 1000 ms (Min) is allowed (not truncated), and that silence greater > than Min is truncated to "Max" duration(50 ms). This is very useful for > trimming long duration leading / trailing silence from a recording. I would be fine if you gave that in the Manual as another real world example. --- As this "bug" stands per its actual subject, I would still argue it's not a bug in the strict sense (TruncSilence.cpp sets "Initial" to "Longest" if "Initial > Longest" so it is doing what is intended). These seem to be James's lines so perhaps he will comment. I can't think of any great advantage in it except perhaps if you accidentally set min > max and didn't get all your silences truncated as you hoped, just running the effect again would fix it. That leaves "unfixed" your comment about wording of "Min silence duration" in the interface. I agree it's ambiguous and invites the trap that was fallen into, though most novices will hopefully ignore it. There may be resistance to changing it, especially now. If it was a Nyquist effect I would have "Ignore silence less than:". I don't think having "allowable" or "allowed" in the text helps all that much as without making clear it's input it would just make me think of the allowed output length. Is there some term for a "length threshold"?
> (TruncSilence.cpp sets "Initial" to "Longest" if "Initial > > Longest" so it is doing what is intended). These seem to be James's lines so > perhaps he will comment. I'm not attached to these lines. I'm not convinced of the utility of min>max, but am prepared to be, with the very slightest of nudge. Far more important to me is that what the effect does is confusing. If I've understood right 'Minimum' is the minimum on which the effect acts. 'Maximum' is the maximum of a silence on which the effect has acted. The resulting length of a silence after it has been acted on will be the minimum of the 'maximum parameter' and the original length divided by compression factor. I do not want to make significant change to the dialog at this stage. There is an argument for a UX change that would provide a clearer way to select compression factor only or max length only. That's for later. At this stage I don't mind whether we allow min>max or not, as long as the manual is clear about the behavior.
(In reply to comment #4) > As this "bug" stands per its actual subject, I would still argue it's not a > bug in the strict sense (TruncSilence.cpp sets "Initial" to "Longest" > if "Initial > Longest" so it is doing what is intended). The counter argument is that it is a bug because CTRL+R "Repeat Truncate Silence" is intended to repeat the last effect, but in this case the settings are changed before it is applied again. > That leaves "unfixed" your comment about wording of "Min silence duration" > in the interface. I agree it's ambiguous and invites the trap that was > fallen into, though most novices will hopefully ignore it. There may be > resistance to changing it, especially now. If it was a Nyquist effect I > would have "Ignore silence less than:" I've suggested a reworking of the Truncate Silence dialogue on the wiki discussion page: http://manual.audacityteam.org/man/Talk:Truncate_Silence
(In reply to comment #5) > I'm not convinced of the utility of min>max, > but am prepared to be, with the very slightest of nudge. Here's an example from the forum this week: http://forum.audacityteam.org/viewtopic.php?f=12&t=58665&p=149251
Bug subject changed to "Truncate Silence: Relabel UX to clarify Min/Max and allow Min > Max saving" as agreed on http://manual.audacityteam.org/man/Talk:Truncate_Silence .
Created attachment 186 [details] remove Min > Max test
Created attachment 315 [details] Relabel UX to clarify Min/Max and allow Min > Max saving * Interface redesigned to clarify functions of controls. * Minimum 'detected silence' duration may be greater than the maximum 'truncated silence' duration. * Space above buttons increased by one line to allow for warning messages. * -80 dB setting enabled. http://bugzilla.audacityteam.org/show_bug.cgi?id=434#c8 * includes the proposed fix for bug 434 comment 10 http://bugzilla.audacityteam.org/show_bug.cgi?id=434#c10
(In reply to comment #10) Only discussing the interface here, not the "fudge fix" for accuracy - if (r->end - r->start < mTruncInitialAllowedSilentMs / 1000.0) + if (r->end - r->start < (mTruncInitialAllowedSilentMs / 1000.0) - 0.0001) in this patch but which is I believe already committed as http://code.google.com/p/audacity/source/detail?r=11938 . Interface changes all look good to me (and detection now works at -80 dB). I would merely have changed "Duration must be at least 1 millisecond" to "Durations must be at least 1 millisecond" as I think "Cannot truncate to less than 1 millisecond" is a bit confusing if the value that is incorrect is "ignore silence less than". But I will leave Steve to decide if he agrees or not. I assume we want a new patch minus the fudge fix, with or without my change above, and then I can mark it "patch_ready".
(In reply to comment #11) > [...] > I assume we want a new patch minus the fudge fix, with or without my change > above, and then I can mark it "patch_ready". > If the wording doesn't change, there's no need for a new patch. Patching against the already-changed code should just be a no-op.
(In reply to comment #12) > Patching against the already-changed code should just be a no-op. Steve's fudge fix had fewer parentheses than the version you committed, so I assumed would conflict if left in the patch?
(In reply to comment #13) >> Patching against the already-changed code should just be a no-op. > Steve's fudge fix had fewer parentheses than the version you committed, so I > assumed would conflict if left in the patch? Ah, out-of-date patch. Thanks, but I wouldn't let anything like that slip, either, so no need for extra work from him unless the wordings will change. I never commit a patch without checking what it actually changes. Parentheses rule! ;-)
No input from Steve, so let's leave wording as it is. Marked "patch_ready".
(In reply to comment #15) Martyn has been doing some work on this. He has a better solution for the error messages (line 942 of TruncSilence.cpp). I'm not sure what other changes he has,so I'll temporarily move this back from "patch_ready" to "patch" to allow for Martyn's input.
(In reply to comment #16) Steve wrote on 09Sep2012 > Martyn has been doing some work on this. > He has a better solution for the error messages (line 942 of TruncSilence.cpp). > I'm not sure what other changes he has,so I'll temporarily move this back from > "patch_ready" to "patch" to allow for Martyn's input. Any news on this? The relabelling and allowing Min > Max saving would be really helpful.
(In reply to comment #17) I've restarted working on Truncate Silence. My first patch is for bug 434 (marked patch_ready). I'm now working on the GUI layout which I think is required for Min>Max to make sense for users.
Created attachment 443 [details] Upgrade Truncate Silence interface (In reply to comment #17) Gale wrote: > Any news on this? The relabelling and allowing Min > Max saving would be really helpful. The previous "Relabel UX to clarify Min/Max and allow Min > Max saving" is stale and fails to apply to svn head, so marked obsolete. This patch is a reworking of the Truncate Silence user interface. * It removes the Min < Max limitation and adds a comment to indicate why it has been removed. * The warning messages are now specific to the user error rather than one general message for all. * Labels and layout have been revised so as to provide a clearer indication of what the effect does and how to use it. NOTE: This patch clashes with the patch for bug 434. I suggest applying this patch first and the applying the (much smaller) patch for bug 434 manually.
Created attachment 444 [details] Upgrade Truncate Silence interface Minor update to previous patch.
Created attachment 445 [details] Upgrade Truncate Silence interface Minor update.
I note we've changed from "all warnings displayed on one error occurring" to "one warning displays at a time, however many errors there are" (see http://bugzilla.audacityteam.org/show_bug.cgi?id=683#c6 ). I think the words "Detection" and "Truncation" could confuse more than they clarify but don't object strongly to them. I think "For silence longer than" is very useful, so why hide it until you change "Ignore silence less than"? "For silence longer than" should be there all the time or not at all. Why the 1) and 2) in the "Truncation" cluster? How do you describe "1)"? If I understand it, it could be described as "Compress part of silence longer than <min>". If we are going to describe it rather than say "Compression ratio" I wonder if it should mention "waveform", otherwise the seemingly precise description makes it look like it's excess of "Truncate to" over "Ignore". Better, having "For silence longer than" permanently displayed would help avoid this confusion of what the "excess" is. "2)" IMO ought to be "Truncate to maximum of:". Should we have more user-friendly defaults e.g. Min 20, Compression 1:1, Max 100? Or are most people using this actually "speeding up their cadences"?
(In reply to comment #22) Gale wrote: > I note we've changed from "all warnings displayed on one error occurring" to > "one warning displays at a time, however many errors there are Now that we have improved validators available, this may change again in the near future. Having one error at a time is a step towards using the improved validators, so if the messages are "good enough for now" then I suggest that we go with it. I don't want to change the validators in this patch because I'm sure that the developers would prefer it as a separate patch. Also it will be easier to test if it is a separate patch. Gale wrote: > Should we have more user-friendly defaults e.g. Min 20, Compression 1:1, > Max 100? Definitely worth considering, but I'd prefer to defer that discussion until after the GUI changes are implemented. It will be a lot easier to consider default settings when it is clearer what the settings do. I'll respond to the other points later (I need to apply the patch).
(In reply to comment #22) Gale wrote: > I think "For silence longer than" is very useful, so why hide it until you > change "Ignore silence less than"? You must be seeing something different to what I am seeing. On Linux, when first opening Truncate Silence with default preferences, the message reads "For silence longer than 200 milliseconds". Gale wrote: > Why the 1) and 2) in the "Truncation" cluster? The order is important. It does 1) first, and then does 2) Gale wrote: > How do you describe "1)"? If I understand it, it could be described as > "Compress part of silence longer than <min>". It should read as: For silence longer than xx milliseconds: 1) Compress excess silence x:1 I don't know why you don't see the first line. Gale wrote: > "2)" IMO ought to be "Truncate to maximum of:". No, it is correct as it is. "2)" occurs after "1)" However much detected silence there is after "1)", it will be truncated to xx milliseconds. Obviously if after "1)" it is already less than xx milliseconds then it won't be truncated by "2)". If it is more than xx miliseconds then it will be truncated to xx milliseconds.
(In reply to comment #24) Steve wrote: >> Gale wrote: > > I think "For silence longer than" is very useful, so why hide it until you > > change "Ignore silence less than"? > You must be seeing something different to what I am seeing. > On Linux, when first opening Truncate Silence with default preferences, > the message reads "For silence longer than 200 milliseconds". I'm glad we agree that's important. On initialised .cfg on Windows 7 and 8.1, I don't see "For silence longer than 200 milliseconds" on first opening the effect. It is necessary to change the value (which then generates the message for the changed value) and then change back to 200 (which generates the message for 200 ms). Here it is on Win 8.1 after opening the effect: http://gaclrecords.org.uk/images/truncate_silence_new_gui.png . Same issue if I apply the effect and reopen it - no message. Steve wrote: >>Gale wrote: >> Why the 1) and 2) in the "Truncation" cluster? > The order is important. It does 1) first, and then does 2) I didn't even interpret the numbers as an order of action. Does it carry out 1) for unity ratio? Steve wrote: >>Gale wrote: >> "2)" IMO ought to be "Truncate to maximum of:". >No, it is correct as it is. The previous label was "Max silence duration" defined in the Manual as "Specifies the longest allowable resulting silence". > Obviously if after "1)" it is already less than xx milliseconds then it won't > be truncated by "2)". If it is more than xx miliseconds then it will be > truncated to xx milliseconds. Truncated by? If compression applies I may not get the value stated in the box e.g.: Silence in waveform and "Ignore..." = 1033 ms. "Compress..." 5:1. "Truncate to" 300 ms. Resulting silence is of course 233 ms. So not "truncated to 300 ms" unless you happen to already know what the effect does?
> Here it is on Win 8.1 after opening the effect: Windows must be initialising differently from Linux, though there is no obvious reason why it should. This does not touch any platform specific code. I have a couple of ideas how the problem may be fixed, which I will send off list for you to try, but if neither solves the problem then it will need the attention of a Windows developer. > Does it carry out 1) for unity ratio? Strictly speaking, if we're to get into the internal mechanism, 1) and 2) is the order of the calculations. 1) is calculated first and then 2) is calculated using the result of 1). The "deletion" is then performed in one pass. > The previous label was "Max silence duration" defined in the Manual as > "Specifies the longest allowable resulting silence". That is not strictly correct and is one of the reasons for this patch. Example: Generate a 30 second tone and create a 1 second silence and a 2 second silence somewhere within the tone. Ignore silence: 1500 ms Compress excess silence: 4:1 Truncate to: 300 ms The result is that the 2 second silence is truncated to 300 ms, but the 1 second silence is not truncated at all (it is ignored). "longest allowable resulting silence" would imply that there should be no silences longer than 300 ms but that is clearly not the case. >> If it is more than xx miliseconds then it will be >> truncated to xx milliseconds. > Truncated by? How much it is truncated 'by' depends on the duration of the silence. If we truncate a 4 second silence to 2 seconds then it is truncated by 2 seconds so that the result is 2 seconds of silence. If we truncate a 10 second silence to 2 seconds then it is truncated by 8 seconds so that the result is 2 seconds of silence. > If compression applies I may not get the value stated in the box Absolutely. The order of "compression" and "truncation" is important. This is an ordered list: 1) Compress excess silence 5:1 2) Truncate to 300 milliseconds. There is a subtle distinction that I think can only be made clear in the manual by giving examples: 1) is applied to the "excess" silence (that is, silence over and beyond the "ignored" silence, 2) is applied to the entire silence that remains after 1) (not just the "excess" silence).
(In reply to comment #26) > Steve wrote: >> Gale wrote >> Does it carry out 1) for unity ratio? > Strictly speaking, if we're to get into the internal mechanism, 1) and 2) > is the order of the calculations. 1) is calculated first and then 2) is > calculated using the result of 1). The "deletion" is then performed in one > pass. I still vote for not adding that confusing complication (see below). >> The previous label was "Max silence duration" defined in the Manual as >> "Specifies the longest allowable resulting silence". > That is not strictly correct and is one of the reasons for this patch. > Example: > Generate a 30 second tone and create a 1 second silence and a 2 second silence > somewhere within the tone. > Ignore silence: 1500 ms > Compress excess silence: 4:1 > Truncate to: 300 ms > > The result is that the 2 second silence is truncated to 300 ms, but the 1 > second silence is not truncated at all (it is ignored). > "longest allowable resulting silence" would imply that there should be no > silences longer than 300 ms but that is clearly not the case. It means the longest allowable resulting silence in those silences that are modified, which is fairly clear if "For silence longer than 1500 milliseconds:" is displayed. * There is one silence "longer than 1500 milliseconds" which has been truncated. * The other silence is not truncated because user chose to ignore silences shorter than 1500 ms, so they can't expect the 1000 ms silence to be truncated. The Manual should be clarified but I think the correct implication is already there from its description of min. Also in your example, we ignore silence less than 1500 ms and act on silence "longer than 1500 milliseconds". Shouldn't that be "For silence of 1500 milliseconds or longer"? Otherwise user might ask what happens to a waveform silence of exactly 1500 ms (the answer being that it's truncated). > The order of "compression" and "truncation" is important. > This is an ordered list: > 1) Compress excess silence 5:1 > 2) Truncate to 300 milliseconds. > > There is a subtle distinction that I think can only be made clear in the > manual by giving examples: > 1) is applied to the "excess" silence (that is, silence over and beyond the > "ignored" silence, > 2) is applied to the entire silence that remains after 1) (not just the > "excess" silence). Isn't that clear in the Manual that Min is added back to the result of ((input silence length) - min))/compression)? Adding 1) and 2) in the GUI actually makes it look like 2) is applied to the "1)" (i.e. the "compressed excess silence" as you describe it in "Compress excess silence"). And I repeat: Silence in waveform and "Ignore..." = 1033 ms. "Compress..." 5:1. "Truncate to" 300 ms. Resulting silence is 233 ms, not 300 ms, because of the output constraint. The "Truncate to" phrase is clearly wrong. It implies all the silences operated on will be "truncated to" 300 ms, but they will be truncated to more than that if compression is high enough. Do we need the "excess" word creating problems with how to explain that "excess" concisely in GUI? I think it reads too much like an "excess" between the values in the min and max boxes. Is this any better? Threshold: [ ] Ignore silence less than:[1500] milliseconds For each silence of 1500 milliseconds or more: Compress beyond 1500 milliseconds: [ ] :1 Maximum truncated silence: [ ] milliseconds If we don't like a dynamic value after "Compress beyond", then "Compress silence beyond ignored length:" ?
Created attachment 453 [details] Upgrade Truncate Silence interface v2 Hopefully this fixes the problem of initialising mTruncationMessage ("For silence longer than %d milliseconds:") on Windows.
(In reply to comment #28) Steve wrote: > Hopefully this fixes the problem of initialising mTruncationMessage on Windows It does.
(In reply to comment #29) >> Hopefully this fixes the problem of initialising mTruncationMessage on Windows > It does. Excellent. Thanks for testing. As it has been tested on Windows and Linux and it is clearly better than what we currently have, I suggest that this is patch_ready, not withstanding your reservations about the control names. If this is committed then that will open up the discussion about control names to a broader user base, which I think will be useful as you (Gale) and I do not seem to be able to reach agreement on that. As the release if 2.0.6 has been deferred until after Christmas then there is plenty of time for thorough testing and polishing, which must be better than leaving it languishing here for another couple of years.
(In reply to comment #30) > I suggest that this is patch_ready, not withstanding your > reservations about the control names. I disagree unfortunately Steve. I don't feel I have had a sensible answer about removing max from the "Truncate to" label, which seems worse than the label we already have and which I expect to generate false bug reports (for the reason I stated). "For silences longer than" is also clearly incorrect from my understanding, although not as bad as omitting "maximum". I could live with your 1) and 2) and "excess" as more than good enough to mark it "ready". If you want to let me know off bugzilla why "Max" is incorrect (not the answer you already gave which seems irrelevant) and why "For silences of x or longer" is incorrect, I'll look forward to hearing from you. The only explanation I can see is that the Manual is currently significantly wrong about what the plug-in does, but testing the effect suggests that isn't the case.
(In reply to comment #30) It all looked 'better than before', so I committed the patch, with a few changes from me. Given the current timing, we have a chance to change it further. Martyn
(In reply to comment #5) James wrote: > There is an argument for a UX change that would provide a clearer way to > select compression factor only or max length only. That's for later. I heartily agree. Much of the confusion with this effect is due to dissimilarities between the "compression" process and the "truncation" process. I feel confident that this effect can be made much more user friendly and I am keen to do so for Audacity 2.0.7. For now I have marked this as "devel - fix made" and will be submitting at least one further patch (as a new bugzilla entry) to tweak the new UI.
(In reply to comment #33) > Steve wrote: > > James wrote: > > There is an argument for a UX change that would provide a clearer way to > > select compression factor only or max length only. That's for later. > I heartily agree. Much of the confusion with this effect is due to > dissimilarities between the "compression" process and the "truncation" process. > > I feel confident that this effect can be made much more user friendly and I am > keen to do so for Audacity 2.0.7. FWIW I support a separate option that disables compression entirely. Steve and I have already discussed offlist what the other options might be. > For now I have marked this as "devel - fix made" and will be submitting at > least one further patch (as a new bugzilla entry) to tweak the new UI. Seems to me that bug 693 which I have marked "ready" and which corrects the "For silences..." message is necessarily this bug, given that message was already part of the patch for this bug.
Superseded by complete interface revision in r13073.
