Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AVPlayerItem addOutput:] Cannot attach an output that is already attached. #1818

Closed
pabloromeu opened this issue Nov 9, 2022 · 47 comments · Fixed by doublesymmetry/SwiftAudioEx#68 or #2176

Comments

@pabloromeu
Copy link

Describe the Bug
I am getting some kind of error on SwiftAudioEx.

[AVPlayerItem addOutput:] Cannot attach an output that is already attached.

Steps To Reproduce
TBH it did not happen to me on debug, but it seems that is happening to my users in production. I have reports from Firebase Crashlytics.

Environment Info:

System:
    OS: macOS 13.0
    CPU: (8) arm64 Apple M1
    Memory: 137.73 MB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.11.0 - /usr/local/bin/node
    Yarn: 1.23.0-20220130.1630 - /usr/local/bin/yarn
    npm: 8.19.2 - /opt/homebrew/bin/npm
    Watchman: 4.9.0 - /usr/local/bin/watchman
  Managers:
    CocoaPods: 1.11.3 - /opt/homebrew/lib/ruby/gems/3.0.0/bin/pod
  SDKs:
    iOS SDK:
      Platforms: DriverKit 22.1, iOS 16.1, macOS 13.0, tvOS 16.1, watchOS 9.1
    Android SDK:
      API Levels: 23, 26, 27, 28, 29, 30, 31, 33
      Build Tools: 27.0.3, 28.0.3, 29.0.2, 29.0.3, 30.0.2, 30.0.3, 31.0.0
      System Images: android-28 | Google Play Intel x86 Atom, android-30 | Google APIs ARM 64 v8a, android-30 | Google Play ARM 64 v8a
      Android NDK: 21.0.6113669
  IDEs:
    Android Studio: Dolphin 2021.3.1 Patch 1 Dolphin 2021.3.1 Patch 1
    Xcode: 14.1/14B47b - /usr/bin/xcodebuild
  Languages:
    Java: 1.8.0_241 - /usr/bin/javac
  npmPackages:
    @react-native-community/cli: Not Found
    react: 18.0.0 => 18.0.0 
    react-native: 0.69.6 => 0.69.6 
    react-native-macos: Not Found
  npmGlobalPackages:
    *react-native*: Not Found

Using react-native-track-player@^3.2.0

This happens on real devices, most of them on iOS 15.

How can you help

I have seen a PR on SwiftAudioEx that might fix this: doublesymmetry/SwiftAudioEx#30

@pabloromeu pabloromeu added the Bug label Nov 9, 2022
@bradfloodx
Copy link
Collaborator

Thoughts @jspizziri ?

@Spice-Z
Copy link
Contributor

Spice-Z commented Nov 11, 2022

I also meet this crash in production. (cannot reproduce in debug)
I hope doublesymmetry/SwiftAudioEx#30 will fix this 🙏🏼

@pabloromeu
Copy link
Author

I have been looking at this and I think it is related to the NativeModule on iOS

https://github.com/doublesymmetry/react-native-track-player/blob/main/ios/RNTrackPlayer/RNTrackPlayer.swift

@objc(RNTrackPlayer)
public class RNTrackPlayer: RCTEventEmitter, AudioSessionControllerDelegate {

    // MARK: - Attributes
    private var hasInitialized = false
    private let player = QueuedAudioPlayer() // THIS ONE
    private let audioSessionController = AudioSessionController.shared
    private var shouldEmitProgressEvent: Bool = false

The native modules can be loaded more than once, then QueuedAudioPlayer is instantiated more than once, and then it crashes. The PR from SwiftAudioEx may fix it, but I think might be a good taken to not instantiate QueuedAudioPlayer more than once (use a singleton with a "static")

@jspizziri jspizziri added Platform: iOS Unconfirmed Bugs that have not been confirmed by the core team. labels Dec 12, 2022
@baptistemon
Copy link

Hi Everyone, We have the same issue on our project not in production but in dev environment. Did someone manage to fix this issue?

@RamProg
Copy link

RamProg commented Feb 3, 2023

We are having this crash too.

@puckey
Copy link
Collaborator

puckey commented Feb 6, 2023

Can anyone provide steps that we can easily reproduce this crash? Otherwise it is impossible to verify a fix.

@pabloromeu
Copy link
Author

pabloromeu commented Feb 13, 2023

Can anyone provide steps that we can easily reproduce this crash? Otherwise it is impossible to verify a fix.

Hi @puckey, I think the best way of testing it is as I explained in my answer, is instantiating multiple times RNTrackPlayer and you will get the crash. And the easy fix is making the player a singleton, so you do not instantiate more than once. So, change this:

@objc(RNTrackPlayer)
public class RNTrackPlayer: RCTEventEmitter, AudioSessionControllerDelegate {

    // MARK: - Attributes
    private var hasInitialized = false
    private let player = QueuedAudioPlayer() // THIS ONE
    private let audioSessionController = AudioSessionController.shared
    private var shouldEmitProgressEvent: Bool = false

to this

@objc(RNTrackPlayer)
public class RNTrackPlayer: RCTEventEmitter, AudioSessionControllerDelegate {

    // MARK: - Attributes
    private var hasInitialized = false
    private static var player: QueuedAudioPlayer = {
    // here you can add some configuration or logging if needed
        return QueuedAudioPlayer() 
    }()
    private let audioSessionController = AudioSessionController.shared
    private var shouldEmitProgressEvent: Bool = false

@puckey
Copy link
Collaborator

puckey commented Feb 14, 2023

@pabloromeu thanks for the suggestion – this looks good.. I can make a pr with this fix. I just want to be able to reproduce the issue.. How would I go about instantiating RNTrackPlayer multiple times?

@mikeleruo
Copy link

@pabloromeu you would get:
Static member 'player' cannot be used on instance of type 'RNTrackPlayer' and then we need to replace player. with RNTrackPlayer.player ?

@puckey
Copy link
Collaborator

puckey commented Feb 18, 2023

@pabloromeu any chance you could give instructions on how to replicate this issue? tia

@pabloromeu
Copy link
Author

@mikeleruo yes, obviously. If you go for a singleton instance you need to use RNTrackPlayer.player instead of player

@puckey in my case, I was using RNTrackPlayer in a wrapper service on javascript side. For example:

import TrackPlayer, {
    Capability,
    State,
} from 'react-native-track-player';


export default class TrackPlayerService {
    constructor() {
        this._audioPlayer = TrackPlayer;
    }
}

So, each time I instantiate TrackPlayerService it will instantiate TrackPlayer, which will create a new QueuedAudioPlayer and lead to the error.

I got rid from the error by instantiating TrackPlayerService just once, hence the problem is solved, but we could solve it by creating the singleton I proposed.

@jspizziri
Copy link
Collaborator

jspizziri commented Feb 19, 2023

@pabloromeu , the documentation is (or at least used to be) very clear on not instantiating the track player multiple times for this very reason. While I agree implementing a singleton would make the library more ergonomic, I don't think this is a big, but rather confusion resulting in misuse and a subsequent feature request.

I'm going to relabel it as such.

@jspizziri jspizziri added Feature and removed Bug labels Feb 19, 2023
@puckey
Copy link
Collaborator

puckey commented Feb 20, 2023

@pabloromeu I don't believe your code wouldn't reinstantiate TrackPlayer / create a new QueuedAudioPlayer.. But if you want me to take a look at this, please make a fork of the example app demonstrating this.

@puckey
Copy link
Collaborator

puckey commented Feb 20, 2023

@pabloromeu just to be certain, have you tested this issue on the nightly version of react native track player?

@pabloromeu
Copy link
Author

No @puckey I did not. I just ensured -as @jspizziri suggested- that I do not import the native library in two different places or times. I created a singleton in the JavaScript side and that worked for me.

@jspizziri
Copy link
Collaborator

@pabloromeu i didn't say you couldn't import the library multiple times, I said you can't instantiate the player multiple times (which is what setupplayer does).

At this point I would say you need to provide a reproduction in code or my vote is to close this issue.

@bradfloodx
Copy link
Collaborator

Agreed. Without code to reproduce let’s close this. This doesn’t appear to be a bug but a misconfiguration. Thanks all!

@jspizziri jspizziri removed the Unconfirmed Bugs that have not been confirmed by the core team. label Feb 27, 2023
@jspizziri
Copy link
Collaborator

This issue is a feature request to prevent a common misconfiguration of RNTP. While it does make the library more ergonomic I don't consider it to be an urgent feature. I'm going to close it for now, if someone wants to contribute to add this quality-of-life improvement please let us know and we can help guide you on the implementation.

@kubatatami
Copy link

Hi, I have the same issue in my project. I am using TrackPlayer in singleton with the same trick as in your example(with getCurrentTrack and try-catch) to prevent double call of setupPlayer. Problem started when I updated RNTP from 2.x to 3.2.

@jspizziri
Copy link
Collaborator

@kubatatami if you create a reproduction in the example project by forking we can reopen. We won't reopen until there's a repro though.

@jspizziri
Copy link
Collaborator

@bradleyflood our current recommendation is how we do it in the example app. Have you wrapped setupPlayer in a try-catch like we do there?

@jspizziri
Copy link
Collaborator

I'm wondering if it's related to iOS pre-warming.

@bradleyflood have you verified that your particular issue is only happening on iOS 15+

@fivecar
Copy link
Contributor

fivecar commented May 27, 2023

Have you wrapped setupPlayer in a try-catch like we do there?

@jspizziri : Thanks for pointing out the sample's try/catch bit. Hadn't realized the Android background-start bug existed.

In that app, PlaybackService runs before setupPlayer, which is why I'm asking about the right order to call things. (I suspect @bradleyflood is curious for the same reason). The docs say both that the service registration has to come immediately after registerComponent, but also that no TrackPlayer functions may be called (safely) before setupPlayer. Yet the playback service no doubt needs TrackPlayer functions... which is why I had originally called setupPlayer within PlaybackService.

Is my suggestion in #1818 (comment) the right way to resolve this (i.e. once I add the try/catch/Android-sleep business)?

@jspizziri
Copy link
Collaborator

@fivecar setup the player as soon as possible

@bradfloodx
Copy link
Collaborator

@fivecar Also you aren't actually calling any TrackPlayer methods in the playback service, right? It's just registering callbacks - yes those callbacks will call TrackPlayer but none of those callbacks would be fired until later on, right? So there isn't really any race condition - I think we can assume the callbacks aren't called until later. So it could go first.

@fivecar
Copy link
Contributor

fivecar commented May 27, 2023

@fivecar Also you aren't actually calling any TrackPlayer methods in the playback service, right? It's just registering callbacks - yes those callbacks will call TrackPlayer but none of those callbacks would be fired until later on, right? So there isn't really any race condition - I think we can assume the callbacks aren't called until later. So it could go first.

@bradleyflood - Ah, interesting. I had taken the documentation literally when it says, "Make sure the setup method has completed before interacting with any other functions [emphasis mine] in TrackPlayer."

Combining yours and @jspizziri's suggestions together, it sounds like the approach I asked about in #1818 (comment) will work fine -- essentially calling setupPlayer "as soon as possible," and having the callback registration happen shortly thereafter.

Out of curiosity, one of my original questions remains open: does iOS ever call PlaybackService more than once? That is the current theory of why this bug happens, right? (i.e. specifically, this issue was thought to have happened only when you initiate setupPlayer more than once in your app; have we instead decided that enough people are initializing TrackPlayer correctly and still getting the error such that the theory should no longer be around incorrect initialization?)

@bradfloodx
Copy link
Collaborator

Good question @fivecar. I'm going to add a "Breadcrumb" to my playback service register so I can check that fact in Bugsnag reports.

Having said that - since I upgraded to "react-native-track-player": "^4.0.0-rc04" I haven't had any of these crashes anymore :) So V4 appears to solve it.

I'm running V4 RC in production for almost 2 weeks now and it seems all good so far.

@fivecar
Copy link
Contributor

fivecar commented May 30, 2023

Having said that - since I upgraded to "react-native-track-player": "^4.0.0-rc04" I haven't had any of these crashes anymore :) So V4 appears to solve it.

Great to hear. I'm a bit hesitant to upgrade to ^4.0.0 because I've patched a variety of issues to get RNTrackPlayer (and SwiftAudioEx) working, only some of the PRs of which the maintainers have agreed to take, and so I don't have the time right now to re-test and re-regress/port/patch the fixes they didn't take. But at some point, it'd be great to be able to move forward, especially with the types of timing-related hard-to-reproduce issues that arise at the architecture level.

@marcvictorpassarelli
Copy link

@brad-sf @fivecar @pabloromeu have you guys seen this issue recently? For me since we started using v3 it's appearing and it never went away. We have just updated from v3.2.0 to v4.0.0-rc06 and the issue seems to have increased a little bit.

Do you guys have any idea for me or what I could be doing wrong? We call setup player like this

image image

@fivecar
Copy link
Contributor

fivecar commented Sep 25, 2023

@marcvictorpassarelli : I upgraded my app to RC6, and am still seeing this crash. (@brad-sf -- just confirming that I've indeed seen this crash even beyond the RC4 you mentioned above).

I'm initializing the app exactly as the docs recommend (and in fact IIRC I updated the docs a bit, even).

@marcvictorpassarelli
Copy link

@fivecar I think in our app we're doing as it should too, only calling setupPlayer once in our App.jsx file. The worst part is that we cannot find out what are the steps to reproduce this crash as it seems to happen kind of randomly. I'll see if I spend some more time in this

@jspizziri
Copy link
Collaborator

@fivecar @marcvictorpassarelli please upgrade to the latest RC. You're likely not going to get a ton of help unless you're running the latest version.

@xcarpentier
Copy link

Hi,
Thanks for this lib awesome!!! 👍

Latest RC is the RC9 ATM, I will give it a try ASAP.

For more context on this topic see the issue occurred also in RC8, please see the stack trace from sentry:

NSInvalidArgumentException *** -[AVPlayerItem addOutput:] Cannot attach an output that is already attached or nil output
image

@puckey puckey reopened this Oct 17, 2023
@puckey
Copy link
Collaborator

puckey commented Oct 17, 2023

I am also seeing this crash in production – will spend some time to fix it in the coming days.

@dcvz
Copy link
Contributor

dcvz commented Oct 17, 2023

Hi, Thanks for this lib awesome!!! 👍

Latest RC is the RC9 ATM, I will give it a try ASAP.

For more context on this topic see the issue occurred also in RC8, please see the stack trace from sentry:

NSInvalidArgumentException *** -[AVPlayerItem addOutput:] Cannot attach an output that is already attached or nil output image

Thanks for the tracelog! That really pinpoints the place. So here’s where we add a metadata listener to a media item here which we remove here. I guess in some cases it tries to add the output before it’s been removed and because we only create one metadata output object that gets shared it fails. It shouldn’t because we make sure to call stopObservingCurrentItem before making a new listener, but perhaps that early return is hitting when we don’t expect it to.

@dcvz
Copy link
Contributor

dcvz commented Oct 17, 2023

Maybe better would not be to rely on those variables and see if we can just check if there are listeners/output to be removed and remove them.

@puckey
Copy link
Collaborator

puckey commented Oct 18, 2023

It could also be some kind of race condition caused by stopObservingAVPlayerItem's playerItemObserver.stopObservingCurrentItem() call – which occurs on recreateAVPlayer() which happens when there was a playback error.

@dcvz
Copy link
Contributor

dcvz commented Oct 20, 2023

Managed to reproduce in a unit test and made a fix in SwiftAudioEx. That'll be merged in RNTP via #2176 and will go out with v4 of the library.

@kubatatami
Copy link

Since I updated RNTP to 4.0.1 in my project I see increased amount of crashes:

Fatal Exception: NSInvalidArgumentException
0  CoreFoundation                 0xec870 __exceptionPreprocess
1  libobjc.A.dylib                0x2bc00 objc_exception_throw
2  AVFCore                        0x87c74 -[AVPlayerItem(AVPlayerItemOutputs) addOutput:]
3  quinn                          0x43f298 partial apply for closure #1 in AVPlayerItemObserver.startObserving(item:) + 76 (AVPlayerItemObserver.swift:76)
4  quinn                          0x34ea0 thunk for @escaping @callee_guaranteed () -> () (<compiler-generated>)
5  libdispatch.dylib              0x4300 _dispatch_client_callout
6  libdispatch.dylib              0x77b8 _dispatch_continuation_pop
7  libdispatch.dylib              0x1b5c0 _dispatch_source_latch_and_call
8  libdispatch.dylib              0x1a190 _dispatch_source_invoke
9  libdispatch.dylib              0x128a8 _dispatch_main_queue_drain
10 libdispatch.dylib              0x125b0 _dispatch_main_queue_callback_4CF
11 CoreFoundation                 0x3720c __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__
12 CoreFoundation                 0x33f18 __CFRunLoopRun
13 CoreFoundation                 0x33668 CFRunLoopRunSpecific
14 GraphicsServices               0x35ec GSEventRunModal
15 UIKitCore                      0x22c2b4 -[UIApplication _run]
16 UIKitCore                      0x22b8f0 UIApplicationMain
17 quinn                          0x5000 main + 8 (main.m:8)
18 ???                            0x1c3ba6dcc (Missing)

@Norcy
Copy link

Norcy commented Dec 1, 2023

Managed to reproduce in a unit test and made a fix in SwiftAudioEx. That'll be merged in RNTP via #2176 and will go out with v4 of the library.

Why revert the Swift-AudioEx version from 1.0.0-rc.11 to 1.0.0 at 4.0.0? This crash occurs also at latest version now.

@dcvz

https://github.com/doublesymmetry/react-native-track-player/commits/main/react-native-track-player.podspec


I hava tried the 1.0.0-rc.11, the crash still exists.

Steps To Reproduce: Calling TrackPlayer.next() time after time

2023-12-02 14:04:43.381818+0800 SFM[34322:7216160] *** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '*** -[AVPlayerItem addOutput:] Cannot attach an output that is already attached or nil output'
*** First throw call stack:
<_NSCallStackArray 0x280af87e0> [
	7619939508,
	7503152080,
	7881000876,
	4363523192,
	4346587964,
	4411678776,
	4411691444,
	4411784752,
	4411745784,
	4411744988,
	7620525096,
	7620400480,
	7620420588,
	8615609180,
	7657697112,
	7657696188,
	4343907612,
	8145964524
]
libc++abi: terminating due to uncaught exception of type NSException
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '*** -[AVPlayerItem addOutput:] Cannot attach an output that is already attached or nil output'
terminating due to uncaught exception of type NSException

@fivecar
Copy link
Contributor

fivecar commented Feb 23, 2024

I'm on v4.0.1 and am getting this crash in production. Should we reactivate this issue so that it's tracked, and close it once doublesymmetry/SwiftAudioEx#74 gets into a release of RNTP?

@fivecar
Copy link
Contributor

fivecar commented Mar 18, 2024

Ok - someone else continued this discussion in #2230 .

@9mzodiac
Copy link

This issue is also affecting my app, I hope it gets fixed soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment