nvwave: Don't close the audio device unless there hasn't been any audio played for 10 seconds.

[jcsteh]

Link to issue number:

Fixes #5172. Fixes #10721 completely.

Summary of the issue:

1. As described in make the audio device still active for better response #5172, some audio drivers/hardware take a long time to open the device and/or truncate the start/end of the audio.
2. As described in OneCore performance #10721 (comment), calling WaveOutOpen in the OneCore synth callback mysteriously blocks (and thus lags) for ~100 ms. Because we close the audio device on idle, we can trigger this problem. Although oneCore synth: Fix lag between utterances introduced in NVDA 2019.3. #11023 mostly fixes this, it's impossible (or at least very difficult) to resolve this completely from within the OneCore driver.
3. Aside from all of this, closing and opening the audio device for rapid short utterances (e.g. rapid movement with the cursor keys or typing) doesn't seem particularly optimal. It's difficult to measure this, but I'd say mitigating this is likely to make audio performance faster/smoother.

Description of how this pull request fixes the issue:

When WavePlayer.idle() is called and closeWhenIdle is set, instead of closing the device immediately after the audio finishes, set a timer. If audio is played before the timer elapses, stop the timer. Close the device when the timer expires.

Testing performed:

Tested as per the STR in #10721 (comment).
It's impossible to know when the device is closed, but I placed a beep in WavePlayer._close, had NVDA speak an utterance, then waited 10 seconds and observed that the beep played. I observed that the beep didn't play unless there was a 10 second period of silence.
Anecdotally, response feels a bit snappier with eSpeak when moving rapidly, but this could be placebo effect; I haven't measured this.

Known issues with pull request:

None.

Change log entry:

[Brian1Gaff]

There do seem to be a lot of machines on sale that exhibit this loss of the first bit of speech, You almost need a kind of time warp system so the bit that you know will get lost is played after the sound device has struggled to its feet, then a little bit of raciness as it catches up. I 'd have thought Microsoft ought to get the vendors to allow always on sound for desktop machines by default. Brian bglists@blueyonder.co.uk Sent via blueyonder. Please address personal E-mail to:- briang1@blueyonder.co.uk, putting 'Brian Gaff' in the display name field. Newsgroup monitored: alt.comp.blind-users

[jcsteh]

We're going to use the timer again with the same callback, so the only thing setting it to None does is mean we have to recreate it when we next need it, which seems wasteful. Reusing a timer is perfectly valid. Unless I'm missing a concern you have?

[AppVeyorBot]

• PR introduces Flake8 errors
• Build (for testing PR)

See test results for failed build of commit 16e67864eb

[AppVeyorBot]

• System test failure
• Build (for testing PR)

See test results for failed build of commit aa38fb321c

[LeonarddeR]

We're going to use the timer again with the same callback, so the only thing setting it to None does is mean we have to recreate it when we next need it, which seems wasteful. Reusing a timer is perfectly valid. Unless I'm missing a concern you have?

Thanks for clarifying.

[JulienCochuyt]

Wouldn't this function be a candidate for inclusion in core.py beside callLater` you added there as of a59e007?

[jcsteh]

This doesn't notify the watchdog, etc., so it's not safe for general usage. Normally, you'd use queueHandler.queueFunction to run something in the main thread, but I didn't want to deal with the overhead of the core pump, etc. for this case; it really should run ASAP.

[JulienCochuyt]

Why not use here a regular threading.Timer or even wx.Timer?
Note wx.PyTimer seems to have gone away of wxPython documentation as of the Phoenix migration for some reason (maybe a bad one, no clue)
Sorry if I'm missing the point here.

[jcsteh]

threading.Timer would spawn another thread and execute the function there. That's pretty wasteful, since we already have an event loop in our main thread. It also increases the potential for race conditions.
wx.Timer doesn't allow you to pass in a callback, so you have to subclass it. That makes things a lot less readable.

[Neurrone]

Any chance the default of 10 seconds could be user configurable? For example, there are situations that I might want to set a longer timer and make that trade-off with battery power.

[k-kolev1985]

Will this functionality have an option to enable/disable it? Also, as mentioned above, a configurable timeout would also be welcome. Thanks in advance!

[Adriani90]

There is no need for optional setting here, this happens in the background and is standard for any screen reader, even voice over plays silence for about 6 seconds but the user does not notice it.
I understand that people fear that this would have significant impact on batery, but this is not the case here. The impact is neglegible.

[Adriani90]

A timing interval is also not needed, this would only overload the gui with additional settings which are not standard for any screen reader aat all. I understand that every user wants to have full control over every small setting in the screen reader, but this is not good from a design perspective. But in the end it is Jamie's decision and the decision of NV Access to implement it in whichever way.

[XLTechie]

@k-kolev1985, what is the use case for disabling this? @Neurrone, why would you want to change the timing? Where is the advantage in making it longer? Are you perhaps trying to duplicate the function of the Bluetooth Audio add-on by extending this?

[k-kolev1985]

@XLTechie The ability to disable it is for battery saving in cases where this NVDA functionality is not needed (e.g. on portable computers which do not exhibit audio cut out behavior). But if really the impact of this functionality on the battery life is negligible, maybe adding the toggle for it it is not worth it.

As for the timeout period configurability. Some people may interrupt their work for longer periods of time but they prefer to still not experience initial speech cut outs when they resume their work.

[codeofdusk]

Any updates on this PR?

[codeofdusk]

Any updates on this? I suspect it'll help a lot with modern machines that aggressively close the sound card for power savings.

[Adriani90]

I get this error now in last alpha from time to time:

ERROR - unhandled exception (23:47:05.138) - MainThread (9068):
Traceback (most recent call last):
  File "wx\core.pyc", line 2196, in Notify
  File "nvwave.pyc", line 349, in _close
  File "nvwave.pyc", line 90, in _winmm_errcheck
OSError: [Errno 5] Das angegebene Gerätehandle ist ungültig.

It occurs quite often. Especially in Windows Explorer on Windows 10 2004.

I am using a Dell XPS13 with a Realtec sound card.

Any idea on how to fix this?

[jcsteh]

See #11482.

[feerrenrut]

@jcsteh Given #11482 I'm tempted to revert this PR and aim for 2020.4 instead. That will take some pressure off fixing this issue and resolving other queries that are popping up.

[jcsteh]

Note that #11482 has no user impact apart from the logged error, which won't be noticed in release. I'm not sure what other specific issues you're referring to, but anything related to audio glitches during speech already existed and is unrelated to this PR. That said, this is obviously your call to make.

[feerrenrut]

There is also #11490. There have been questions about whether silence should be sent to devices, questions about the impact on power usage and user configuration. In addition I'm not sure I have seen any confirmation that this fixes issues for anyone. Given the short time to release these problems aren't likely to be resolved.

[Mohamed00]

As the creator of both issues, I will say that even with the problems it has, I still find that it makes NVDA much more snappy on my machine, and things like rapidly moving through list items seem to be much faster.

[jcsteh]

For reference, there's also this tweet which suggests it greatly helps another user. Still, I totally understand the desire to mitigate risk; just providing another data point.

[codeofdusk]

I strongly vote to keep this PR despite the issues, as the new issues it introduces are (at least in my opinion) far less severe than those fixed by the patch.

However, I definitely think that it's worth considering in next release further improvements to this functionality, particularly playing silence instead of just keeping the device open, with something like #11481 to control the duration of silence.

[codeofdusk]

I think the changelog entry might need to be revised though, to focus more on speech performance after brief utterances more than truncation of audio (silence would fix the latter issue, holding open the device seems to only fix the former).

[nidza07]

Hello. Yes, I am the author of that tweet, and this PR literally fixes all performance issues I had on my machine with NVDA. Before, progress bar beeps would stutter a lot if played at the same time as speech, now this does not happen at all and the beeps are completely smooth, and as another user correctly mentioned the list navigation is a lot snappier than before. After updating to the alpha where this was merged, it felt like a completely new machine. So for me, the impact of this PR is huge, and I feel it is worth considering keeping this merged unless it causes major issues. I'm not sure if just playing silence is the right solution, since I already have a program doing this on my system for bluetooth devices, and it did not provide any performance improvements, unlike this PR. So thanks once again for your work, and hopefully the issues it has will be resolved.

[feerrenrut]

We have done some more testing with this PR, and agree it certainly improves performance significantly in the right circumstances. We will include it in the 2020.3 release, with the intention to fix or mitigate the related issues encountered. Thanks everybody for the extra information provided here.

[mltony]

I tested this PR in a recent alpha build and I can report that it also fixes the issue with periodic stuttering in Plantronics backbeat headphones. Playing silence didn't fix stuttering.
However, it doesn't seem to fix the issue of the beginning of each utterance being lost - this one can be fixed by playing silence, e.g. via my Bluetooth Audio add-on. I would like to offer my help porting this add-on into NVDA core. Please let me know if NVAccess would be interested in such kind of PR.

[Adriani90]

Maybe a combination of both approaches would be appropriate here? Von meinem iPhone gesendet

…

Am 27.08.2020 um 19:10 schrieb mltony ***@***.***>:  I tested this PR in a recent alpha build and I can report that it also fixes the issue with periodic stuttering in Plantronics backbeat headphones. Playing silence didn't fix stuttering. However, it doesn't seem to fix the issue of the beginning of each utterance being lost - this one can be fixed by playing silence, e.g. via my Bluetooth Audio add-on. I would like to offer my help porting this add-on into NVDA core. Please let me know if NVAccess would be interested in such kind of PR. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[ondrosik]

I will still use Bluetooth audio addon because I mostly got to a following situation. I sit in a radio studio prepared to read some text. I wait for the song outro (the song is playing from another computer). When It's my time, I press down arrow. NVDA will respond about one second later. I understand that make this configurable may introduce issues mentioned abowe. Maybe we should extend this to one minute but again, some users should come with different situations. I set the bluetooth audio to 10 minutes and simply ignore fact that this Lenovo laptop responds slovly after longer idle time.