diff --git a/CHANGELOG.md b/CHANGELOG.md index 5aa1669c4a..6df7b8b9c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ 7.41 ----- - Improve support section to help the user to send Apple Watch logs +- Fixed a crash that could occur when playing some downloaded episodes (#901) 7.40 ----- diff --git a/PocketCastsTests/Tests/Analytics/AnalyticsCoordinatorTests.swift b/PocketCastsTests/Tests/Analytics/AnalyticsCoordinatorTests.swift new file mode 100644 index 0000000000..b7dad27061 --- /dev/null +++ b/PocketCastsTests/Tests/Analytics/AnalyticsCoordinatorTests.swift @@ -0,0 +1,46 @@ +import XCTest + +@testable import podcasts + +final class AnalyticsCoordinatorTests: XCTestCase { + func testFallsBackToPreviousSourceOnNilCurrentSource() { + let coordinator = AnalyticsCoordinator() + coordinator.currentSource = .carPlay + + // Resets the current source and sets the previous source + let previousSource = coordinator.currentAnalyticsSource + + // The current source is expected to be nil, so this should reset to the original source value + coordinator.fallbackToPreviousSourceIfNeeded() + + XCTAssertEqual(previousSource, coordinator.currentSource) + } + + func testFallsBackToPreviousSourceOnUnknownCurrentSource() { + let coordinator = AnalyticsCoordinator() + coordinator.currentSource = .carPlay + + // Resets the current source and sets the previous source + let _ = coordinator.currentAnalyticsSource + coordinator.currentSource = .unknown + + // The current source is expected to be nil, so this should reset to the original source value + coordinator.fallbackToPreviousSourceIfNeeded() + + XCTAssertEqual(coordinator.currentSource, .carPlay) + } + + func testFallbackDoesntHappenIfCurrentSourceIsSet() { + let coordinator = AnalyticsCoordinator() + coordinator.currentSource = .carPlay + + // Resets the current source and sets the previous source + let _ = coordinator.currentAnalyticsSource + coordinator.currentSource = .chooseFolder + + // The current source is set, this should do nothing + coordinator.fallbackToPreviousSourceIfNeeded() + + XCTAssertEqual(coordinator.currentSource, .chooseFolder) + } +} diff --git a/podcasts.xcodeproj/project.pbxproj b/podcasts.xcodeproj/project.pbxproj index 1f57a60445..558431c914 100644 --- a/podcasts.xcodeproj/project.pbxproj +++ b/podcasts.xcodeproj/project.pbxproj @@ -1480,6 +1480,7 @@ C7C4CAEE28AB0BF200CFC8CF /* TracksSubscriptionData.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7C4CAED28AB0BF200CFC8CF /* TracksSubscriptionData.swift */; }; C7C4CAF328ABFD0900CFC8CF /* Notifications.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7C4CAF228ABFD0900CFC8CF /* Notifications.swift */; }; C7CA0559293E8918000E41BD /* HolographicEffect.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7CA0558293E8918000E41BD /* HolographicEffect.swift */; }; + C7CDE14E2A4B41BF0081E7FF /* AnalyticsCoordinatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7CDE14D2A4B41BF0081E7FF /* AnalyticsCoordinatorTests.swift */; }; C7CE415A28CBCFC200AD063E /* Analytics.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7AF5790289D87CF0089E435 /* Analytics.swift */; }; C7CE415B28CBD00A00AD063E /* AnalyticsEvent.swift in Sources */ = {isa = PBXBuildFile; fileRef = C72CED2C289DA14F0017883A /* AnalyticsEvent.swift */; }; C7CE415C28CBD01F00AD063E /* String+Analytics.swift in Sources */ = {isa = PBXBuildFile; fileRef = C72CED2E289DA1650017883A /* String+Analytics.swift */; }; @@ -3166,6 +3167,7 @@ C7C4CAED28AB0BF200CFC8CF /* TracksSubscriptionData.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TracksSubscriptionData.swift; sourceTree = ""; }; C7C4CAF228ABFD0900CFC8CF /* Notifications.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Notifications.swift; sourceTree = ""; }; C7CA0558293E8918000E41BD /* HolographicEffect.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HolographicEffect.swift; sourceTree = ""; }; + C7CDE14D2A4B41BF0081E7FF /* AnalyticsCoordinatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AnalyticsCoordinatorTests.swift; sourceTree = ""; }; C7D6551328E5153200AD7174 /* Debounce.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Debounce.swift; sourceTree = ""; }; C7D813782A0D4B89007F715F /* AppIcon-Patron-Chrome-iphone@2x.png */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = "AppIcon-Patron-Chrome-iphone@2x.png"; sourceTree = ""; }; C7D813792A0D4B89007F715F /* AppIcon-Patron-Chrome-ipad-pro.png */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = "AppIcon-Patron-Chrome-ipad-pro.png"; sourceTree = ""; }; @@ -6797,6 +6799,7 @@ children = ( C7D854F228ADD98700877E87 /* AppLifecyleAnalyticsTests.swift */, 8BA55A0F28CA6843002BECC5 /* AnalyticsPlaybackHelperTests.swift */, + C7CDE14D2A4B41BF0081E7FF /* AnalyticsCoordinatorTests.swift */, ); path = Analytics; sourceTree = ""; @@ -8115,6 +8118,7 @@ buildActionMask = 2147483647; files = ( 8B317BA528906CAB00A26A13 /* TestingSceneDelegate.swift in Sources */, + C7CDE14E2A4B41BF0081E7FF /* AnalyticsCoordinatorTests.swift in Sources */, 8B2319432902CF170001C3DE /* EndOfYearManagerMock.swift in Sources */, 8B68C1D829421E3400CF25C5 /* FeatureFlagTests.swift in Sources */, 8B317BA328906C8100A26A13 /* TestingAppDelegate.swift in Sources */, diff --git a/podcasts/Analytics/AnalyticsDescribable+Modules.swift b/podcasts/Analytics/AnalyticsDescribable+Modules.swift index 3e82fa6a1a..c2ba85ce5e 100644 --- a/podcasts/Analytics/AnalyticsDescribable+Modules.swift +++ b/podcasts/Analytics/AnalyticsDescribable+Modules.swift @@ -183,3 +183,24 @@ extension SocialAuthProvider: AnalyticsDescribable { } } } + +// MARK: - Players +extension DefaultPlayer: AnalyticsDescribable { + var analyticsDescription: String { + "default" + } +} + +#if !os(watchOS) +extension EffectsPlayer: AnalyticsDescribable { + var analyticsDescription: String { + "effects" + } +} + +extension GoogleCastPlayer: AnalyticsDescribable { + var analyticsDescription: String { + "google_cast" + } +} +#endif diff --git a/podcasts/Analytics/AnalyticsEvent.swift b/podcasts/Analytics/AnalyticsEvent.swift index a5e5c4648e..24ed4607b1 100644 --- a/podcasts/Analytics/AnalyticsEvent.swift +++ b/podcasts/Analytics/AnalyticsEvent.swift @@ -215,6 +215,7 @@ enum AnalyticsEvent: String { case playbackPlay case playbackPause + case playbackFailed case playbackSkipBack case playbackSkipForward case playbackSeek diff --git a/podcasts/Analytics/Helpers/AnalyticsCoordinator.swift b/podcasts/Analytics/Helpers/AnalyticsCoordinator.swift index fe5f4d1865..dad9287051 100644 --- a/podcasts/Analytics/Helpers/AnalyticsCoordinator.swift +++ b/podcasts/Analytics/Helpers/AnalyticsCoordinator.swift @@ -41,7 +41,6 @@ enum AnalyticsSource: String, AnalyticsDescribable { case upNext = "up_next" case userEpisode = "user_episode" case videoPlayerSkipForwardLongPress = "video_player_skip_forward_long_press" - case playbackFailed = "playback_failed" case watch case unknown @@ -52,9 +51,22 @@ class AnalyticsCoordinator { /// Sometimes the playback source can't be inferred, just inform it here var currentSource: AnalyticsSource? + /// Keep track of the analytics source that was used for the last event before it was reset + var previousSource: AnalyticsSource? + + /// If the current source is not available, attempt to fallback to previous value before it was reset + func fallbackToPreviousSourceIfNeeded() { + guard currentSource == nil || currentSource == .unknown else { + return + } + + currentSource = previousSource + } + #if !os(watchOS) var currentAnalyticsSource: AnalyticsSource { - if let currentSource = currentSource { + if let currentSource { + previousSource = currentSource self.currentSource = nil return currentSource } diff --git a/podcasts/Analytics/Helpers/AnalyticsPlaybackHelper.swift b/podcasts/Analytics/Helpers/AnalyticsPlaybackHelper.swift index b5be70396c..7d4dc6641b 100644 --- a/podcasts/Analytics/Helpers/AnalyticsPlaybackHelper.swift +++ b/podcasts/Analytics/Helpers/AnalyticsPlaybackHelper.swift @@ -25,6 +25,10 @@ class AnalyticsPlaybackHelper: AnalyticsCoordinator { track(.playbackSkipForward) } + func playbackFailed(errorMessage: String, episodeUuid: String, player: PlaybackProtocol?) { + track(.playbackFailed, properties: [ "error": errorMessage, "episode_uuid": episodeUuid, "player": player ?? "unknown" ]) + } + func seek(from: TimeInterval, to: TimeInterval, duration: TimeInterval) { // Currently ignore a seek event that is triggered by a sync process // Using the skip buttons triggers a seek, ignore this as well diff --git a/podcasts/AudioReadTask.swift b/podcasts/AudioReadTask.swift index a3970b1ecb..5a88ccfa4a 100644 --- a/podcasts/AudioReadTask.swift +++ b/podcasts/AudioReadTask.swift @@ -372,6 +372,11 @@ class AudioReadTask { let totalSeconds = totalFrames / audioFile.fileFormat.sampleRate let percentSeek = time / totalSeconds + // Ignore any invalid values + guard percentSeek.isNumeric else { + return(0, false) + } + return (Int64(totalFrames * percentSeek), percentSeek >= 1) } } diff --git a/podcasts/DefaultPlayer.swift b/podcasts/DefaultPlayer.swift index 833df54923..b54eb9dcd9 100644 --- a/podcasts/DefaultPlayer.swift +++ b/podcasts/DefaultPlayer.swift @@ -228,7 +228,10 @@ class DefaultPlayer: PlaybackProtocol, Hashable { private func playerStatusDidChange() { if player?.currentItem?.status == .failed { - PlaybackManager.shared.playbackDidFail(logMessage: "AVPlayerItemStatusFailed on currentItem", userMessage: nil) + // Attempt to provide more specific information about the error if its available + // This only returns the domain and code to help normalize it across different languages + let message = (player?.currentItem?.error as? NSError).map { "Domain: \($0.domain) - Code: \($0.code)"} + PlaybackManager.shared.playbackDidFail(logMessage: message ?? "AVPlayerItemStatusFailed on currentItem", userMessage: nil) return } diff --git a/podcasts/EffectsPlayer.swift b/podcasts/EffectsPlayer.swift index df90b93e72..37e3636254 100644 --- a/podcasts/EffectsPlayer.swift +++ b/podcasts/EffectsPlayer.swift @@ -98,6 +98,15 @@ class EffectsPlayer: PlaybackProtocol, Hashable { // we haven't cached a frame count for this episode, do that now strongSelf.cachedFrameCount = strongSelf.audioFile!.length DataManager.sharedManager.saveFrameCount(episode: episode, frameCount: strongSelf.cachedFrameCount) + + // Throw an error if the frame count is 0 + // `cachedFrameCount` is used to calculate the duration of the episode and a 0 value + // will result in the episode immediately being marked as played or can cause Inf/NaN issues. + // + // For more info, see: https://github.com/Automattic/pocket-casts-ios/issues/900 + if strongSelf.cachedFrameCount == 0 { + throw PlaybackError.effectsPlayerFrameCountZero + } } } catch { objc_sync_exit(strongSelf.playerLock) diff --git a/podcasts/PlaybackManager.swift b/podcasts/PlaybackManager.swift index 22526151af..5afe7e2da2 100644 --- a/podcasts/PlaybackManager.swift +++ b/podcasts/PlaybackManager.swift @@ -814,7 +814,10 @@ class PlaybackManager: ServerPlaybackDelegate { func playbackDidFail(logMessage: String?, userMessage: String?) { FileLog.shared.addMessage("playbackDidFail: \(logMessage ?? "No error provided")") - AnalyticsPlaybackHelper.shared.currentSource = .playbackFailed + + // If there is no current analytics source, then use the previous one + // This helps prevent an `unknown` from being used if this is called right after another event, such as playbackPlay + analyticsPlaybackHelper.fallbackToPreviousSourceIfNeeded() guard let episode = currentEpisode() else { endPlayback() @@ -828,7 +831,11 @@ class PlaybackManager: ServerPlaybackDelegate { // - Is the duration actually reasonable? // if either of these is false, flag it as an error, otherwise we got close enough to the end if episode.playedUpTo < 1.minutes || episode.duration <= 0 || ((episode.playedUpTo + 3.minutes) < episode.duration) { - pause() + analyticsPlaybackHelper.playbackFailed(errorMessage: logMessage ?? "Unknown", + episodeUuid: episode.uuid, + player: player) + + pause(userInitiated: false) NotificationCenter.postOnMainThread(notification: Constants.Notifications.playbackPaused) if episode.downloaded(pathFinder: DownloadManager.shared) { @@ -841,7 +848,6 @@ class PlaybackManager: ServerPlaybackDelegate { } NotificationCenter.postOnMainThread(notification: Constants.Notifications.playbackFailed) - return } diff --git a/podcasts/PlaybackProtocol.swift b/podcasts/PlaybackProtocol.swift index 29e60b24b0..1029103897 100644 --- a/podcasts/PlaybackProtocol.swift +++ b/podcasts/PlaybackProtocol.swift @@ -33,7 +33,19 @@ import PocketCastsDataModel func internalPlayerForVideoPlayback() -> AVPlayer? } -enum PlaybackError: Error { +enum PlaybackError: LocalizedError { case unableToOpenFile + case effectsPlayerFrameCountZero case errorDuringPlayback + + var errorDescription: String? { + switch self { + case .unableToOpenFile: + return "PlaybackError: unableToOpenFile" + case .effectsPlayerFrameCountZero: + return "EffectsPlayer frameCount was 0 while opening the file" + case .errorDuringPlayback: + return "PlaybackError: errorDuringPlayback" + } + } }