`fluid.ampfeature~` can break when it receives a `nan`
rconstanzo opened this issue · 18 comments
As outlined in this thread on the discourse when fluid.ampfeature~ receives a nan its own output breaks.
What was the expected result?
The envelope follower should get pulled in that direction but not ultimately fail.
What was the actual result?
The object stops working until audio is restarted.
What operating system were you using?
Mac
Operating system version
macOS 13.4.1
FluCoMa Version
1.0.6
now looking at this: the only question here is what to replace NaN with. traditionally it is by 0 but in this case I wonder if that is the best choice. Paging @AlexHarker for a strong opinion about this. then @weefuzzy for another. then @rconstanzo for another.
I'd check for NaN at line 45 of algo:public:envelop.hpp with something as dirty as std::isnan but hey ideas will abound I'm sure
I have no direct opinion as I know little about the implications. I imagine a zero would not be good here as that would be "loud", so maybe like -70 or -99 or something more in that range. But as I said, I have little clue of what the implications would be.
it is at the input, so it would be 0 as assuming NaN is silence
----------begin_max5_patcher----------
727.3ocuVErbaBCD8L9qfgyNtHA3X2SoeGcxjQFjcTJHwHIRbZl3u8JsBhcZ
fXECStHFsrnce693s5kYAQaD6opnveF96vffWlED.lrFBZ2GDUQ1mWRTfaQU
TkhriFM28NMcuFrG2Yolnyumw2cmjlqcmLdc5h34gKQ1U75EYlU7h3vaa+Dd
SEiWR0PDvGMJZzcVQsVYEPzDad3JzptP57S+bM0Eunnvasu40YyrKy8DZb5S
ly8CHKujk+mCeB7Riu1BLTZr8QxZ.kChOj23a4f3Sw1wIkSJJqnZoHDmEGdC
IWydjFh7.xwYvCzYvr+8zrAw7FBe2jhXEmTqtWnOXfwm.0jUPaEeMf3UvJNY
7HMoKlaEbsh8W.mHyAOTAXaofnurJPUSoloJYET44QZBx0aWG65swmfUEUa.
q4rcewUHLTNROGY27cJ8yk.TR5sDg6oDgOlrRhgfRk2Q4jMtiYv5Tz7IV.Xa
YCqXAopdKknajzCg2rknzlTpto1PdhOtuP7D2YQUJd5nGmXnyk23bkLNMWzv
gfkc11CxQAQYfZ5JzWSmomxb5WkH1p9bwk4KZDBFTZRRlzQHYS8DjlpMT4ay
JpDEzSyGaAlaHxfu+RxrEwgQbqD60PeNa4GTXsm1VR969YvixPOLf9khvuw.
LMbHH9PIFgJ0P5zrcG9TlAvIPNpQBZhl9h+NG99CSs87j+USK4GE8Elz.wBj
p9uaKBoq096QsRzHy6NutaqEdLkKnJMiSzLA+DmvNe5sv5cfx7HP16VM9Hsz
iHkNAAB6eb5ZoR6nd6+riCgI9TKee5UwJpELtV0M0B3qIwvMENc2HKJo9jZI
djZ3kqOI0fciL07hA9MwKR5mWfFGu3h9W1ogPpqejJUsNCgvnQ9f.RqUygsL
taKbanHI8QVm+NKDoQhTazGM2FCzy1uzcKFXjqj2vZwnAblPB5u1Aup514kf
L8rWm8O.+UZdeA
-----------end_max5_patcher-----------
test patch
it is at the input, so it would be 0 as assuming NaN is silence
Yeah that makes sense, I was (obviously) thinking output.
There is nothing dirty about std::isnan(). It is self documenting, is portable and should be reliable.
My more general disagreement is with fixing Nans by default although I do tend to do it to in cases where it might cause unrecoverable errors (e.g. a filter memory in some cases). Otherwise I consider it better practice to fix nans upstream. I dislike max fixing them in places where I think the user should already have avoided them (especially as this adds performance cost)
In this case it is at the input, as if a nan is sent in, the hpf will blow unrecoverable. so 0 would be a good replacement in that case?
My 2c: If we have to fix them (and avoiding exploding filters has something to be said for it) then 0 isn't the right choice here, because it's unpredictably saying that the input might have changed significantly even when we have no actual information to that effect.
I think I'd be more inclined to track the last 'valid' input sample and repeat that in the case of a NaN. For constant NaNs this will drag the HPF output quickly to 0 but won't explode it, for the odd NaN it shouldn't affect the detector too much.
With that said I do also tend towards the view that prevention is better than cure here: adding a clear or reset message would, IMO, be more virtuous (but non-trivial).
Agree that std::isnan is the correct test here. Nothing at all dirty about it: we should rely on standard library authors to have implemented this properly and portably.
----------begin_max5_patcher----------
1475.3ocyYk0baaCD9Y4eEL7I6Yb7PBRpi5lNou0WZ9CjzQCDIDERn.X..8Q
yD+au3f2hjhRT1oOX4Q6BfEe6I1U+3pY1anOg31V+l0mslM6GWMalljhvr7u
OydO7ovDHWuLadHME8h8sFVjr8XRBRn4ApHRyDETcxohiz6mt4qu20oX+oPQ
3NLIdMCEJLWCOevcN2Z46n+mqmS4+r9m7cAyDz8PANTsCWEsed0UpOtcjfHj
teOhHJtEBzSZgayPoIvPD25SvOwsdDK1YI2ivJAECSrvjzrx8jfInPZFQzB4
UpC2wpNVMf1.3sPgeOOi1Hvw7Mkx3BhaLgiXBKnEARrmDVVL.V7WpAg2R0mK
z3B3bgQRHkvEPhX5HIXHezky0Hw4BfDB5Qo3N.Hbb7Km..b6HJaHSgW9kd9c
ARb3p+BnVHl4nEOmhLaPceHvD6yBhBZbbBp55vf6QBDaMh.2jfpq+OOj5u73
1p.suGvu3y9QJVESeNvbSlPPIuhvzeHK5JMzlubrvbCjDaeI8XCSvgeah9r9
fiCQfimN3a0qqSaewknD4EhxNkRfcAzghNC.l.xEAZO2k+R.ZJCeIRi5OTs9
fkZSouIQZPPEPub3.EEiNEuRvgXvanJzAKL0BVYd1B33FKcj2suBQfu6CSzq
za4H.Zfw4bkpxwatW4VbhLk5cpTsH1iTlX2KVejg3ox59Hqc33coxSv5iaYn
umgHgOasLnymr4Oo7TdCkJ10jmx2UWi00Qq1l+VqohQjIlL1y83fzCXbJleB
4npenHVNxxg1LkEF8.hwwxRoU2K4i9SSqQdVssnzGekpOnk2VRBSLjBJIwPO
fK1OnjJjIglPhqLl9lZ+z7Biq5XnQHFICW1og1xjekz1.hrJOOU10fdyQ7z6
jZ9hCvtV3yp5dCN5Ld9A0ZqQZmiSngeCEUKUpTOlhHxNOj93x26Ja5wb+KYG
g1ByRDq2RIBN9e02BWURnN3uM+V1ISENzP3OYXoYpXIwLbDkntDMrFJxEhS5
LDX5NoNXzqf.S6XyRmKoZoGlxmuKx3afLkwJ+cSEVK4KIoIMYUtuDzVQN6TL
gzRKJno8yjISaLvd2PkL2OzYq4vWmQLbWqZZbMG9PSss.ljjGr173eBRvx9Y
QBrwD.bJYZd63NdHiljz.uFNOzAmHoedH5QbjXmVP0cFjKGmV3DYWZkivwHt
nIMALl2jBW7rQoWiT1l7330BzdY2yhVKPFgf4B9N5i77EV3nUWATMCh5w00y
70f9AMCFgTq71J9M7n+8+V5moiQi9i5Kpqzhk6tcvRuoL6qE39xbtzz0nWsm
3oSlVF5zY1S6Fr6IZWmTRpLz2k+Rp2orm0SvX8CvDbz0N2bu0WXegn9Cu852
gkwfjqwD2atQQ5GZdhpMX8AKIy6Uj+Yw9j2LWI8pEcukcgz0UtxyQdl1xFUy
ZOkBK2dMemrYxcTlo7NoN3I38ZjdaTERIOftvsWcgS25B+woKVYl9Tqmo2Bs
UU.Tu2pm3ZsLT76VMvoYrvBkZtwxpotWlJSfIk0C+bYrmkyHMBm5cXwHuC98
dGxIZn7+kwDAVL3a9A0a49UdNQu1CPAL+3P0E.9kMAEzSoLK92Yhqeu6MSzp
BFATmapD4ebq5YByr8aPUSJo2WYpbZZ7RktaecedkMPsyqUU5QzK+w6oIv7F
8fw2SirY9sITnnp4l1EnufiY64vDzKp2HNsd8AGWQj+5j4yOuvd8paVEvf61
YdyQ+AYb87qmxsmzspeLoZKhxhL8T1sJerhVMR4iJZU9jeQh1saI6NII6NBA
66VUd6rEjZfZGWRNW.I4OFL40i1D7FHZkw18h6BMZQew8g.KFqSj6TMsiI6f
5GTX5NQf2p.CvXjDXwkHDbLYYJhSmFlNgHvoAoQ406eIjzXb87dUBulOFGjC
c5M0haMCUkPZM6zVyM8vYl1+7RaOqT8bR6ZFopm8b0Ou5+Tvim6.
-----------end_max5_patcher-----------
Can't test this patch (no filter.butterworth~ here), but what you're describing sounds good, especially in that it keeps the envelope followers where they ought to be.
I would, personally, prefer something that treats the NaNs at the input as often it's not clear where an issue emerged (e.g. playlist~ randomly puking out a NaN sometimes for no good reason), so a clear or reset message will be of little use in a blown-up/frozen patch.
Ah, sorry – filter is a C74 package (muy useful, IMO). Can be installed by package manager.
Ah nice, didn't realize. Seems handy indeed! (should be native!).
I think I'd be more inclined to track the last 'valid' input sample and repeat that in the case of a NaN
ok I'll implement that and make a PR
as discussed in the CCL forum: I would use std::isfinite to test
should there be a sanitisation added
+1 for either last input sample over 0
new suggestion - freeze the filter (no update) - otherwise your assumption is that you are moving towards the last value (rather than "we just don't have any info to change the value with")
Just to clarify, is that freezing just the HPF or the detector as well? Can't quite feel my way to how this works.
OK - I've looked at the code now.
I guess what I am suggesting is that if a NaNs (or non finite value) comes in you don't update anything and just use the last outputs (so you don't run the HP filters or either slide filter). It's a philosophical position that a guess that the input has frozen (as in the current fix) is less well supported than saying "I don't have any useful data with which to update so I'll use the last output values" and await proper data. I think that the approach I'm advocating would involve saving two values (rather than one) and is slightly more involved to implement as a result.
However, another view is that if there are NaNs or other badness at the input then everything is already fairly broken, so once the output is non-NaN and not being subjected to assumptions of zero input then you've made a decent stab at not doing the most awful thing and that should be enough.