Fix to fatal iOS 13 memory leak when using TextToSpeach.SpeekAsync #1112 #1153
Conversation
* Implement vertical accuracy in GeoLocation API (xamarin#1103) * Location: add property 'VerticalAccuracy' (xamarin#1099) * vertical accuracy is only available in Android API level 26 and above (xamarin#1099) * update Samples app to show vertical accuracy on GeolocationPage * add missing documentation bits for Location.VerticalAccuracy * .gitattributes: get better diff context for C# code (xamarin#1115) * Location.VerticalAccuracy: add runtime check for Android version (xamarin#1099) (xamarin#1116) * the compile-time check is not enough * it crashed on old Android versions (<8.0) * xamarinGH-1102 Fix launcher on older devices (xamarin#1120) * Update Launcher.ios.tvos.cs * OpenUrlAsync was introduced in iOS 10 not 12 https://docs.microsoft.com/en-us/dotnet/api/uikit.uiapplication.openurlasync?view=xamarin-ios-sdk-12 * Fix trailing whitespace * Really? fix whitespace * Really! Fix whitespace * Fixes xamarin#1129 - set empty required declarations on UWP (xamarin#1133) * Fixes xamarin#1129 - set empty required declarations on UWP * Update Permissions.uwp.cs * Update Xamarin.Essentials.csproj (xamarin#1136) * xamarinGH-1142 AccessBackgroundLocation only when compile & running Q (xamarin#1143) * AccessBackgroundLocation only when compile & running Q * put if compile check around permission * xamarinGH-1121 If VC is null use TraitCollection (xamarin#1144) * Fixes xamarin#1123 (xamarin#1126) * Update PlacemarkExtensions.xml (xamarin#1132) Co-authored-by: Janus Weil <janus@gcc.gnu.org> Co-authored-by: James Montemagno <james.montemagno@gmail.com> Co-authored-by: Michael <Michael@ZPF.fr>
|
|
|
@praeclarum are you able to look at this PR? You have a lot of experience with AVSpeechSynth. I remember that in my plugin we moved to creating a new one because if routes changed then it would get messed up: jamesmontemagno/TextToSpeechPlugin#59 |
|
@jamesmontemagno You are likely right ... this is just the kind of thing I suspected could be of concern. My feelings won't be hurt if someone else has a way to fix issue #1112 and prevent jamesmontemagno/TextToSpeechPlugin#59. I am guessing @nexxuno was on the right track with jamesmontemagno/TextToSpeechPlugin@77ab3e9. |
|
I have always used a singleton synthesizer object. I wonder if that bug that got filed and your PR was just because of a transient iOS bug or maybe the audio session was misconfigured. This change seems right to me. There are other changes outside the scope of the synthesizer, do you want me to review those? |
|
@praeclarum nope, just the synthesizer one. Would be good to just test the audio route change. perhaps it was an iOS thing back in the day and no longer an issue. |
|
@jamesmontemagno & @praeclarum What would a change-of-audio-route test look like? Start with using device speakers and then switch to Bluetooth speakers? |
|
@baskren Yep that would be a change. Also settings changes can cause it, backgrounding, switching audio session configuration (global properties in AVFoundation), phase of moon... Apple makes no mention of needing to handle it in their docs though. |
|
I'll be able to test this on Monday. |
|
@baskren: yes I had problem with backgrounding and route changes for example. (ps. At a glance I'm seeing a possible bug in semaphore handling when looking at my commit cited above) |
|
@jamesmontemagno, @praeclarum, @nexxuno I've successfully tested the commit on production devices running iOS 13.3.1 (iPad Air 2) and 12.4 (iPad 6th gen). |
|
@nexxuno can you describe and point out the code more? |
Hello, do you need a comment about my old commits or something else? I remember that my first fix involved recreating the synth object each time the route changed (while disposing the old one) but then you suggested to create a new one each time SpeechUtterance was called and then disposing it at the end of it's usage. But I don't know if it's relevant anymore 2 years later... Everything about that fix was about a functional bug (audio problems when switching to background processing) while this looks about memory issues. |
|
Thanks @nexxuno yeah, just wanted to make sure we checked to make sure we didn't regress at all from what yours fixed. |
jamesmontemagno
added
the
awaiting-review
Description of Change
In TextToSpeech.ios.tvos.watchos.cs:
TextToSpeach.SpeakAsynccall to a single static lazy instance of AVSpeechSynthesizer.TextToSpeach.SpeakAsynccall) instance of AVSpeechUtterance in ausingstatement, to implicitly invoke adispose()upon it.TESTS
To test, use the TextToSpeach sample in the existing Xamarin.Essentials.Sample.iOS app with Xamarin.Profiler or Instruments. If there is a way to automate this, I'm all ears.
SAMPLES
The TextToSpeach sample in the existing Xamarin.Essentials.Sample.iOS app is capable of demonstrating this issue. However, the demo project in issue #1112 is more concise.
UPDATED DOCUMENTATION
Nothing
publicwas effected by this change.Bugs Fixed
API Changes
none
Behavioral Changes
none
PR Checklist