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.

Sign up for GitHub

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

Jump to bottom

Fixes a crash that can happen when playing a corrupted file #901

Open
wants to merge 20 commits into
base: trunk
Choose a base branch
from
Open

Fixes a crash that can happen when playing a corrupted file #901

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
32deec2
Add a isNumeric check to prevent a crash
emilylaguna Jun 12, 2023
3f8007d
Update CHANGELOG.md
emilylaguna Jun 13, 2023
2c98d0c
Throw a playback error if the AVAudioFile.length returns 0 when loadi…
emilylaguna Jun 17, 2023
cf19c0e
Provide more context about throwing the effects player frame count error
emilylaguna Jun 21, 2023
16ce5b0
Add playbackFailed to the analytics events
emilylaguna Jun 21, 2023
9e3e908
Change PlaybackError to conform to LocalizedError
emilylaguna Jun 21, 2023
59f8601
Throw a more specific effectsPlayerFrameCountZero error from the effe…
emilylaguna Jun 21, 2023
0709508
Add a playbackFailed method to the AnalyticsPlaybackHelper
emilylaguna Jun 21, 2023
474236e
Set the userInitiated flag to false on the pause method when a playba…
emilylaguna Jun 21, 2023
614b02e
Track when a playback error occurs
emilylaguna Jun 21, 2023
446326f
Add more specific information if the DefaultPlayer fails to play
emilylaguna Jun 21, 2023
891822d
Remove the playback_failed analytics source since its been replaced b…
emilylaguna Jun 21, 2023
e0011ae
Keep track of the previous analytics source
emilylaguna Jun 21, 2023
01a115b
If there is no analytics source on playback fail, then revert to the …
emilylaguna Jun 21, 2023
ef3a51d
Add AnalyticsDescribable support to the players
emilylaguna Jun 21, 2023
1611c68
Pass the current player to the playbackFailed event
emilylaguna Jun 21, 2023
7e7730d
Move the analytics source fallback to the analytics coordinator
emilylaguna Jun 27, 2023
e04fc56
Add current source fallback tests
emilylaguna Jun 27, 2023
9ff7847
Fix lint
emilylaguna Jun 27, 2023
2bde5cc
Merge pull request #918 from Automattic/task/add-playback-failed-events
emilylaguna Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
-----
Expand Down
5 changes: 5 additions & 0 deletions podcasts/AudioReadTask.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
5 changes: 5 additions & 0 deletions podcasts/EffectsPlayer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,11 @@ 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)

// If the count is still 0 then way may not be able to read the file correctly, so throw an error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be we instead of way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🫣 oops, fixed here: cf19c0e

if strongSelf.cachedFrameCount == 0 {
throw PlaybackError.unableToOpenFile
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add tracking here to know how often this happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! I did this in a separate PR since there were some changes that were a bit more involved: #918

}
}
} catch {
objc_sync_exit(strongSelf.playerLock)
Expand Down