[TIMOB-26334] iOS: New AVPlayer-based Ti.Media.audioPlayer crashes on second setUrl() call

GitHub Issue	n/a
Type	Bug
Priority	Critical
Status	Closed
Resolution	Fixed
Resolution Date	2018-10-02T15:31:59.000+0000
Affected Version/s	Release 7.5.0
Fix Version/s	Release 7.5.0
Components	iOS
Labels	audioplayer, ios
Reporter	kosso
Assignee	Hans Knöchel
Created	2018-08-23T12:44:21.000+0000
Updated	2018-10-10T12:32:32.000+0000

Description

Having tested SDK mobilesdk-7.5.0.v20180821233900-osx to try the new (long-awaited) AVPlayer-based Ti.Media.audioPlayer I have discovered a crucial bug. After setting up a player and then using player.setUrl(URL) it works fine. If you then try to setUrl again, the app will crash. This is due to the notification observer for "timedmetadata" not being removed. To test:

    var test_player = Ti.Media.createAudioPlayer({ 
        allowBackground: true
    });

    var btn = Ti.UI.createButton({
        title:'set url'
    });
    btn.addEventListener('click', function(e){
        test_player.setUrl('URL TO MP3');
       // Will crash on second call.
    });

To fix the issue, please add one line to TiMediaAudioPlayerProxy.m at line 422: [[[self player] currentItem] removeObserver:self forKeyPath:@"timedMetadata"];

Attachments

File	Date	Size
sample.mp3	2018-08-24T07:04:53.000+0000	37596

Comments

kosso 2018-08-23

I also notice that the docs mention that AVPlayer is being used in 7.4.0. I could only find it in the continuous 7.5.0 builds. https://docs.appcelerator.com/platform/latest/#!/api/Titanium.Media.AudioPlayer The example provided in the docs works fine, since the url is never changed. Changing the url will crash the app since the timedmetadata observer is never removed. Another issue is that the NSNumber duration method doesn't return anything.

Hans Knöchel 2018-08-24

Thanks [~kosso]! The observers are a big mess, especially when we need to use so many of them, being a framework. We will address this in 7.5.0, where this will be introduced. Side note: 7.4.0 will be dedicated for iOS 12 and Xcode 9 support, aligning with Apples timelines. *EDIT*: I cannot reproduce the issue with the following test-code (sample.mp3 attached):

   var test_player = Ti.Media.createAudioPlayer({ 
       allowBackground: true
   });
   
   var win = Ti.UI.createWindow({
       backgroundColor: '#fff'
    });
    
    var btn = Ti.UI.createButton({
      title: 'Trigger'
    });
    
    btn.addEventListener('click', function() {
         // Will crash on second call.
         test_player.url = 'sample.mp3';
    });
    
    win.add(btn);
    win.open();

Hans Knöchel 2018-08-24

I am still not able to reproduce the crash related to "timedMetadata", but here is a fix for the duration and the listener to (very likely) fix the other one as well. It was simply missing from the observer-cleanup method and makes sense to be required. PR: https://github.com/appcelerator/titanium_mobile/pull/10285
kosso 2018-08-24

Hi, Here's a version of the TiMediaAudioPlayerProxy.m which I have got to work. https://gist.github.com/kosso/5fcd7ded25b0cc8ec932db633f995053 btw: I work for a radio/music company and have had to use my own (imperfect) Audio modules for some time now, since the Titanium audio player modules have been SO far apart in parity for many many years. This has been incredibly frustrating to see simple errors in status codes and missing methods. Regarding the timedmetadata error, maybe my test isn't the most complete code. But you only need to read the code to see the missing observer removal. (My current app is a 'playlist' music app.) I also see that Android is missing the seekToTime method and 'seek' complete event, despite that fact that it has the 'setTime()' method. Another hole in the parity. I also see that in iOS you automatically 'restart' the audio after the setUrl() method. Please do not ever do this. The start() method should only be used to start the audio. Also Android does not do it. It's not your fault Hans, but for years I find it incredible that software for a "mobile phone" - which is device initially naturally designed for audio - has such a gaping disparity when it comes to AUDIO APIs. :) It's been many years since I had to locally build the SDK (with scons) to patch up the holes left in the SDK, but I really really need working Audio APIs, so it looks like I'll be building again to solve my issues. :( If you have something you need me to test out, please let me know. Since I now have many years of experience with Titanium audio apps for news, radio, music and social applications. .
kosso 2018-08-24

iOS also throws a url-related error if you try to add the progress event listener before setting the url. (Which becomes painful when building a long music playlist player app, meaning listeners need to be constantly removed and added each time.)

kosso 2018-08-24

I also note that iOS is missing the 'time' property which Android has. I got this using :

   - (NSNumber *)time
   {
       _time = (int)(CMTimeGetSeconds([[[self player] currentItem] currentTime]) * 1000 );
       return NUMINT(_time);
   }

as well as defining in the header.

kosso 2018-08-24

I also noticed your fix for the duration. I see you returned a NUMDOUBLE. Wouldn't NUMINT be preferable when the value is always going to be integer milliseconds?
Hans Knöchel 2018-08-24

Thanks for the feedback [~kosso], really really valuable! I will rework on the audio-player to get it shaped. If you have a working solution (based on the core-sdk), please also feel free to submit a PR and we can go from there. That might be the easiest way to polish up the API via GitHub communication. Could you do that? Then I'd close my other PR above.
Samir Mohammed 2018-10-10

*Closing ticket.* Verified fix on SDK Version 7.5.0.v20181008124804. Not able to see the application crash using Ti.Media.audioPlayer on second setUrl() call. *Test Environment* APPC Studio: 5.1.0.201808080937 iPhone 6 (12.0) APPC CLI: 7.0.7-master.4 Operating System Name: Mac OS Mojave Operating System Version: 10.14 Node.js Version: 8.9.1 Xcode 10.0

JSON Source