Bugzilla – Bug 683
Bass and Treble: Excessive amplification produces Nan's.
Last modified: 2018-08-20 11:45:29 UTC
Also reproduces if Treble is 3333 and Bass 0. Could be fixed by validation restricting input to "sensible" values.
Created attachment 448 [details] Limit max and min gain to reasonable range Disallow Bass or Treble settings greater than 100 dB or less than -100 dB.
Tested on Windows. Would it be better to have "Bass or Treble: Minimum -100 dB Maximum 100 dB" appear on a new line above "Enable level control" whenever the min or max bass or treble is exceeded? Then it doesn't look when having entered "222" in Bass that entering -222 in treble is legitimate (yes I know you see that -222 for treble is illegitimate after correcting the bass). Also it doesn't look like the limits apply to "Enable level control" when user may have already seen "Maximum 0 dB" for "Enable level control" (and similar confusion if that warning is already on when you enter "222" for bass). The extra space above "Enable level control" might make it look more convincing that that control is not in the cluster above.
(In reply to comment #2) Reading between the lines, I'm pleased that you find that the fix works. > Would it be better to have... There is an infinite variety of variations that we "could" use. I selected error messages that I believe are concise and sufficient to communicate to the user where the problem lies. I don't see a need to make the messages more verbose, but if you or one of the developers feel the need, please feel free to modify the patch before committing it.
(In reply to comment #3) Steve wrote: > I selected error messages that I believe are concise and sufficient to > communicate to the user where the problem lies. And I believe the messages don't fully do that in the cases I gave. They only show the problem after you've fixed the other problem. I was comparing with Truncate Silence which educates about all possible errors when you make one of the errors. Whichever method is preferred, shouldn't we be consistent?
(In reply to comment #4) > I was comparing with Truncate Silence which educates about all possible errors > when you make one of the errors. The error message in Truncate Silence was a "quick fix" (by Martyn ?) because at that time there was greying out of the OK button with no indication why. Personally I think it is better that there is an immediate warning as soon as the user error occurs, and for that warning to refer to the error that has been made. This error should persist until corrected. Considering the general case, an effect could have half a dozen controls and a user could put invalid data into one or more controls. If a simple and concise error message is shown as soon as the user enters invalid data, then I don't think they are likely to continue entering more invalid data. I think they are likely to correct the error before continuing. If on the other hand, a warning is shown about all possible input errors, then it is more likely to be TLDR. If a user does enter invalid data into multiple fields and then goes back to correct the errors: If there is one message for all errors, then there is no indication when one of the errors is corrected. If there is only a message about the first error, then when that is corrected the next error will be shown, and so on until all errors are fixed. Also, for "in-line" warnings (as opposed to pop-up warnings), there needs to be sufficient space in the interface for the warning to appear. Leaving space for a multi-line warning message looks highly inelegant. In this particular case, we could have separate messages for disallowed values in Bass and Treble, but personally I think that is more than necessary as the error is displayed as soon as the error is typed. If you feel strongly that we should do even more hand holding, then I can make a new patch that specifies exactly which control is being referred to. I think that it is worth considering that these errors are only possible by text input and most novice users use the sliders.
James added the warnings for Truncate Silence. I thought he made all the warnings show together for educational value. IIRC I had reservations but I've come to think that works. I even liked the extra line of space. For me this outweighed that you don't see when one of > 1 errors is corrected. If the messages change when there is > 1 error then ideally I think you should see all errors that are applicable at the time. If space is short I would expect you could combine similar messages. I see your Truncate Silence GUI changes have the same policy about messages so that "would" be consistent with Bass and Treble, but in Bass and Treble the warnings are alongside a control to which only one of the warnings applies. So if nothing else I feel that you should have (at least) the message(s) relating to the bass and treble sliders on a separate line from the level control checkbox, or make clear which error relates to what control. If the consensus is for showing maximum one error if there are multiple errors then I'm happy to accede. As I think we both like text messages (so there may be more of them in future), perhaps we should discuss the policy for them on -quality.
Created attachment 451 [details] Validate with wxFloatingPointValidator The new wxFloatingPointValidator makes it very hard to enter an invalid value. In-line warnings moved down to a new line just above the buttons so as to avoid the possible confusion raised in comment 6.
(In reply to comment #7) I tested it on Windows 7 running in German and English. I am much happier with the messages now they are on a separate line. I think this could probably be marked "patch_ready" but I have comments on the wxFloatingPointValidator which may apply more widely. Validation of typed and pasted input when running in German worked as I expected, accepting "," as decimal separator and rejecting "." as separator. There is a possible "problem" with how the validator handles the "wrong" pasted separator. Separators are displayed as pasted (so you can paste "36.99,89"). So if you paste the wrong single separator e.g. "9.6" when running in German it displays as 9.6 but will apply as +96 dB. If you paste "10.7" it will apply as +107 dB (without warning). Given we only accept one separator I think it would be sensible if possible to replace the wrong separator (typed or pasted) so that the "correct" one displays at once. When you paste a disallowed value such as "101" or type "16.6" then remove the separator, the reset is very quick. I found the displayed warning e.g. "Maximum Bass is 100 dB" when the value is on 100.0 dB (valid) rather confusing at first. Someone may think the decimal point in "100.0" is being complained about. We could say e.g. "Bass reset to maximum 100 dB" instead but I don't feel dogmatic about it. Is one decimal place enough? I don't know but I am just wondering about anyone using this for "scientific" purposes. Couple of typos noticed after patching the file: #include "../widgets/valnum.h" // Must comme after <wx/valtext.h> #define LEVEL_MIN -300 // Corresponds to -30 dN
(In reply to comment #8) > I think this could probably be marked "patch_ready" Agreed, but I'm submitting a new patch with a couple of minor changes as described below. > I have comments on the > wxFloatingPointValidator which may apply more widely. Yes I think they do apply more widely and need to be logged separately, but I'm unsure where. Perhaps a wiki "proposal" page about validation? > We could say e.g. "Bass reset to maximum 100 dB" instead Done. > Couple of typos noticed Corrected.
Created attachment 454 [details] Validate with wxFloatingPointValidator v2 Includes changes described in comment 9.
I marked the latest patch "ready". I assume it's your decision to limit to one decimal place in this effect, but I don't have any strong view on it. That only leaves the issue that separators are displayed as pasted, so it is confusing when they apply as validated. I think that should be a separate bugzilla bug. I suggested as a possible solution to that bug that the validator might replace the wrong separator (typed or pasted) so that the "correct" one always displays. However that is tied in with a policy decision on whether we accept multiple decimal separators, allow thousands separators and so on. Perhaps that should be part of a Wiki proposal but it is up to you to write that if you wish.
(In reply to comment #11) > I assume it's your decision to limit to one decimal place in this effect, but I > don't have any strong view on it. Yes I think that one decimal place is plenty for this effect. Even a difference of 1 dB is barely audible so hundredths would be far in excess of any practical use (even allowing for "scientific" use).
(In reply to comment #10) Steve, I started to apply this patch, but have some comments/questions about BassTreble.cpp patch: * Marked v2 on latest attachment, but it downloads as "BassAndTrebleValidation-03"... * +1 on your documentation about why "../widgets/valnum.h" is declared where it is. * It is more typical to declare min before max (e.g., TREBLE_MIN before TREBLE_MAX), as in all the existing declarations, so I started to rearrange your new constants. * Also started to make their names consistent with existing ones, i.e., parameter-first. * But then it became clear that there's no obvious semantic distinction between names of existing LEVEL_MAX and new MAX_LEVEL, especially because they have the same value. Names should as much as possible be self-evident and distinct, and follow conventions (at least local conventions in surrounding code), e.g. here, parameter-name first. Since you're already familiar with the code, Steve, I'll ask you to come up with better names, maybe for all four, i.e., existing and new. * All the existing #defines say the value is 10x the dB amount. But your new constants are in dB. You removed the comment that applied to all the existing declarations, rather than document why your new ones are on different scales -- so in your patch it's undocumented why either scale, for any of them. * "space to align warning messages" as a number of blanks/space-bars is not reliable cross-platform. Make it a number of pixels or a spacer, please. * Current line 418: ' ok->Enable(true); ' Is that necessary? Aren't they on by default? There are more problems after that, I think but patch is not ready, so I changed its status.
Created attachment 470 [details] Patch revised in response to Vaughan's comments This patch (BassAndTrebleValidation-04.patch) should be functionally the same as the previous patch, but with the code issues raised by Vaughan fixed.
(In reply to comment #14) Changed status back to patch_ready as the issues raised by Vaughan have been addressed.
(In reply to comment #15) Testing patch on Windows 7. Validation doesn't seem to work in below case (it seems that 337 dB is applied: 1 Set OS to English language, initialise audacity.cfg. 1 Start Audacity. 2 Generate > Tone 0.8 amplitude. 3 Copy text "33,7" (without quotes) and paste into Bass text box 4 Apply with other settings as default. Problem also occurs if you Undo, disable Level Control, Paste and apply. If you toggle level control before paste, validation works (reset to 100 dB). Same in previous version of patch.
(In reply to comment #16) I'm not able to reproduce that using BassAndTrebleValidation-04.patch (testing on Debian). Are you getting that only on Windows? What does it say in the Undo History window after applying Bass and Treble? Is it repeatable after a reboot?
(In reply to comment #17) > Are you getting that only on Windows? Not tried on Ubuntu, but you cannot paste in effect text boxes on Mac. > What does it say in the Undo History window after applying Bass and Treble? It confirms bass = 337.0 dB. > Is it repeatable after a reboot? Yes. When I said "If you toggle level control before paste, validation works (reset to 100 dB)" I meant, if you toggle the level control *after* paste, sorry. So, Paste, change the Level control, and the Bass gain value will change to 100.0.
(In reply to comment #18) > When I said "If you toggle level control before paste, validation works (reset > to 100 dB)" I meant, if you toggle the level control *after* paste, sorry. So, > Paste, change the Level control, and the Bass gain value will change to 100.0. Thanks, that helps. I still can't reproduce the issue, but that gives me an idea of what may be occurring. I've changed the status back the "patch" while I look into it.
Created attachment 488 [details] reworked validation I don't think that pasting invalid values can be prevented when using wxFloatingPointValidator, but user input can be validated before the effect is applied. This patch provides a second level of validation after wxFloatingPointValidator that comes into play on OK. Hopefully it makes it impossible to apply the effect with out of range user input. I've only tested on Linux.
(In reply to comment #20) There's still the (somewhat obscure) issue that (on Linux) 'Preview' can be actioned with invalid values pasted into text boxes, so patch not ready yet.
Created attachment 489 [details] reworked validation Tidied up the code a bit and included validation on Preview. Tested on Linux. Subject to this working cross-platform or changes to error message text, I think this is ready for code review.
(In reply to comment #22) > Subject to this working cross-platform or changes to error message text Doesn't work on Windows, Steve, sorry. If I paste in the "37,3" to "Treble" and OK, it applies 373 dB Treble. If I paste that in to "Treble" then toggle the Level control, the text in the box changes from "37,3" to "373.0" and it applies 373 dB. No static text appears at any time.
(In reply to comment #23) > Doesn't work on Windows, Steve, sorry. Sorry will have to repatch - I downloaded your patch#5 from comment 20 just before you withdrew it again.
(In reply to comment #24) Bug-683-BassAndTreble-06.diff behaves as per comment 23 on Windows, i.e. applies 373 dB.
(In reply to comment #25) An "initial commit" related to this bug was made at https://github.com/audacity/audacity/commit/797109d97febd9b22db335d6be53fe84f24bbd7c. Are further commits to be made?
(In reply to comment #26) The commit was: https://github.com/audacity/audacity/commit/a3ec006e3e6d08bef4a57d027fdfe5bed320a7ac Gale wrote: > Are further commits to be made? That depends on whether anyone replies to the comment that I added to the commit. If lines 382 to 386 are not required on any platform, they should be removed. If those lines are required (possibly on Windows), then the code should carry a comment to say that they are required. If it helps with testing, I could add an assert, then anyone testing a debug build would just need to try to trick the effect into applying > 100 dB (which will trigger the assert in a debug build).
I think that this bug can be "closed invalid" now as the tighter validation does not allow greater than +15 dB gain on any control.
Marked as Devel - Fix Made as Leland's changes to validation prevent the problem from occurring.
Testing on Mac El Capitan on 01a95c5-2.1.3-alpha-13-feb-16 and Testing on W10 on audacity-win-r4d154c4-2.1.3-alpha-18-feb-16 On both these platforms, using the steps to reproduce, it gets trapped ok by the validation error message - and as I assume that Steve's message above means that he tested on Linux then I think this bug is closable as RESOLVED.
Closed invalid as requested. I found no way to apply 3333 Bass. I tried: 1 Type or paste 3333 into Bass. Apply button is greyed out. 2 Uncheck "Enable". "Apply" button becomes enabled. 3 But pressing "Apply" shows the "not in range" error. This raises a separate issue. See http://audacity.238276.n2.nabble.com/Real-time-preview-treatment-of-invalid-values-td7575207.html.
